Replace `level_map` and deprecate `reference_point` and `center` parameters by cfs-data · Pull Request #92 · NetherlandsForensicInstitute/scratch

cfs-data · 2026-01-21T13:34:47Z

Changes:

Our implementation of level_map() is replaced by a built-in function from surfalize
It seems reference_point has no influence on end-to-end tests so this parameter has been removed in this branch.
It seems center property of Mark had no influence on end-to-end tests, so this property has been removed as well

snregales · 2026-01-28T14:33:07Z

@Raytesnel will PR impact us?

Raytesnel · 2026-01-28T14:48:08Z

@Raytesnel will PR impact us?

ja

…e/scratch into replace-map-level

snregales · 2026-02-19T07:46:04Z

Is this PR still applicable?

…e/scratch into replace-map-level

github-actions · 2026-05-18T13:17:27Z

Diff Coverage

Diff: origin/main..HEAD, staged and unstaged changes

packages/scratch-core/src/conversion/leveling/init.py (100%)
packages/scratch-core/src/conversion/leveling/core.py (86.7%): Missing lines 21,41
packages/scratch-core/src/conversion/leveling/data_types.py (100%)
packages/scratch-core/src/conversion/preprocess_impression/preprocess_impression.py (100%)
packages/scratch-core/src/conversion/preprocess_impression/tilt.py (100%)
packages/scratch-core/src/conversion/preprocess_impression/utils.py (100%)
packages/scratch-core/src/mutations/filter.py (100%)

Summary

Total: 46 lines
Missing: 2 lines
Coverage: 95%

packages/scratch-core/src/conversion/leveling/core.py

  17 
  18 def get_polynomial_degree(terms: SurfaceTerms) -> int:
  19     """Get the highest polynomial degree present in the given surface terms."""
  20     if terms == SurfaceTerms.NONE:
! 21         raise ValueError(f"No degree defined for {terms}")
  22     return max(_DEGREE_MAP[term] for term in terms)
  23 
  24 
  25 def level_map(scan_image: ScanImage, terms: SurfaceTerms) -> LevelingResult:

  37             leveled_map=scan_image.data,
  38             fitted_surface=np.full_like(scan_image.data, 0.0),
  39         )
  40     if scan_image.valid_mask.sum() < 3:
! 41         raise ValueError("At least 3 values are needed for the least squares solver.")
  42 
  43     surface = Surface(
  44         height_data=scan_image.data,
  45         step_x=scan_image.scale_x,

github-actions · 2026-05-18T13:17:29Z

Package	Line Rate	Branch Rate	Health
.	96%	92%	✔
computations	94%	67%	✔
container_models	99%	100%	✔
conversion	96%	89%	✔
conversion.export	99%	93%	✔
conversion.filter	97%	89%	✔
conversion.leveling	94%	67%	✔
conversion.plots	99%	88%	✔
conversion.preprocess_impression	98%	88%	✔
conversion.preprocess_striation	90%	62%	✔
conversion.profile_correlator	96%	82%	✔
conversion.surface_comparison	99%	89%	✔
conversion.surface_comparison.cell_registration	100%	90%	✔
conversion.surface_comparison.cmc_consensus	91%	65%	✔
extractors	97%	75%	✔
mutations	100%	100%	✔
parsers	97%	50%	✔
parsers.patches	89%	60%	✔
preprocessors	100%	100%	✔
processors	100%	83%	✔
renders	99%	50%	✔
utils	71%	100%	➖
Summary	97% (3276 / 3361)	85% (351 / 412)	✔

Minimum allowed line rate is 50%

vergep

In this PR, you do two things:

You remove the center functionality, which I think is really nice! It is redundant for surface corrections indeed.
You use surfalize to lavel_map. This is a bit trickier in my opinion. If I understand it correctly, the surfalize detrend_polynomial operation to degree 1 equals SurfaceTerms=Plane, and to degree 2 equals SurfaceTerms = Plane + Sphere. But SurfaceTerms uses a lot of 'subterms', that are not covered by surfalize. Still all the tests pass. That means: I think we don't need all those subterms, but it would be good to check this explicitly. If that is the case, the code can be simplified even further, where we only have two SurfaceTerms: Plane, Plane + Sphere. That would lead to a major deletion of code lines as put forward in this PR, which is really nice! However, if we do need the subterms, we have to include a lot of the original code back in i'm afraid.

vergep · 2026-05-19T11:14:19Z

+    """Get the highest polynomial degree present in the given surface terms."""
+    if terms == SurfaceTerms.NONE:
+        raise ValueError(f"No degree defined for {terms}")
+    return max(_DEGREE_MAP[term] for term in terms)


I think this conversion is tricky. SurfaceTerms.Plane is the only case for which degree is 1. And SurfaceTerms.Sphere is the only case for which degree is 2. When i look at the definition of the designmatrix in detrend_polynomial.

vergep · 2026-05-19T11:18:36Z

+            leveled_map=scan_image.data,
+            fitted_surface=np.full_like(scan_image.data, 0.0),
+        )
+    if scan_image.valid_mask.sum() < 3:


1 + degree ipv 3?

vergep · 2026-05-19T11:27:27Z

+        if self.terms == SurfaceTerms.NONE:
+            return True
+        # We need at least 3 values for the least squares solver
+        return scan_image.valid_mask.sum() < 3


1 + get_polynomial_degree(self.terms)?

vergep · 2026-05-19T11:39:52Z

-    assert not np.allclose(
-        result_centered.leveled_map, result_ref.leveled_map, equal_nan=True
+@pytest.mark.parametrize("terms", list(SurfaceTerms))
+def test_map_level_has_effect(scan_image_with_nans: ScanImage, terms: SurfaceTerms):


wat test je hier nou? of, in her geval van geen surface term er niks gebeurt is, en anders of er wel iets gebeurt is. Maar gebeurt er dan het goede?

cfs-data added 2 commits January 21, 2026 14:34

First commit

11e293a

Use match-case

736ff25

cfs-data changed the title ~~Replace level_map with built-in function~~ [Draft] Replace level_map with built-in function Jan 21, 2026

cfs-data marked this pull request as draft January 21, 2026 13:43

cfs-data changed the title ~~[Draft] Replace level_map with built-in function~~ [Draft] Replace level_map with built-in function and depecrate reference_point parameter Jan 21, 2026

cfs-data changed the title ~~[Draft] Replace level_map with built-in function and depecrate reference_point parameter~~ [Draft] Replace level_map and depecrate reference_point parameter Jan 21, 2026

cfs-data changed the title ~~[Draft] Replace level_map and depecrate reference_point parameter~~ [Draft] Replace level_map and deprecrate reference_point parameter Jan 21, 2026

cfs-data changed the title ~~[Draft] Replace level_map and deprecrate reference_point parameter~~ [Draft] Replace level_map and deprecate reference_point parameter Jan 21, 2026

cfs-data added 4 commits February 2, 2026 13:59

update from main

d843b17

update lock file

2857a20

remove ref in marks

7a492e5

Merge branch 'main' of https://github.com/NetherlandsForensicInstitut…

db5b27d

…e/scratch into replace-map-level

cfs-data requested a review from SimoneAriens February 9, 2026 15:29

cfs-data added 4 commits February 16, 2026 16:14

Merge branch 'main' of https://github.com/NetherlandsForensicInstitut…

51d41a3

…e/scratch into replace-map-level

fix tests

ad12d66

fixture

228c165

remove ref tests

6836149

cfs-data changed the title ~~[Draft] Replace level_map and deprecate reference_point parameter~~ Replace level_map and deprecate reference_point parameter Feb 16, 2026

cfs-data added 3 commits February 17, 2026 09:17

update docstring

e597db6

update number of required values

1b987f3

update number of required values

1351ef9

update branch

1269b1e

cfs-data changed the title ~~Replace level_map and deprecate reference_point parameter~~ [Draft] Replace level_map and deprecate reference_point parameter Mar 3, 2026

update branch

6fb609c

cfs-data changed the title ~~[Draft] Replace level_map and deprecate reference_point parameter~~ [Draft] Replace level_map and deprecate reference_point and Mark.center parameter Mar 16, 2026

cfs-data changed the title ~~[Draft] Replace level_map and deprecate reference_point and Mark.center parameter~~ [Draft] Replace level_map and deprecate reference_point parameter Mar 16, 2026

update branch

ab5d556

cfs-data added 3 commits March 17, 2026 15:03

update tests

4f4c5e7

Merge branch 'main' of https://github.com/NetherlandsForensicInstitut…

92f9966

…e/scratch into replace-map-level

Merge branch 'main' of https://github.com/NetherlandsForensicInstitut…

b1a9450

…e/scratch into replace-map-level

cfs-data changed the title ~~Replace level_map and deprecate reference_point parameter~~ [Draft] Replace level_map and deprecate reference_point parameter Mar 19, 2026

cfs-data added 3 commits March 22, 2026 08:01

update branch

c602db1

Merge branch 'main' of https://github.com/NetherlandsForensicInstitut…

badbae1

…e/scratch into replace-map-level

fix subclass api

67985ed

cfs-data changed the title ~~[Draft] Replace level_map and deprecate reference_point parameter~~ Replace level_map and deprecate reference_point parameter May 13, 2026

cfs-data marked this pull request as ready for review May 13, 2026 08:29

cfs-data requested a review from vergep May 13, 2026 08:43

cfs-data added 4 commits May 13, 2026 13:55

move predicate

d3db84e

add conditional

60a35bd

split conditional

16e4365

extra tests

405bf2a

SimoneAriens reviewed May 18, 2026

View reviewed changes

Comment thread packages/scratch-core/src/conversion/leveling/core.py Outdated

SimoneAriens reviewed May 18, 2026

View reviewed changes

Comment thread packages/scratch-core/src/conversion/preprocess_impression/preprocess_impression.py Outdated

SimoneAriens reviewed May 18, 2026

View reviewed changes

Comment thread packages/scratch-core/src/mutations/filter.py Outdated

SimoneAriens approved these changes May 18, 2026

View reviewed changes

cfs-data added 4 commits May 18, 2026 14:34

add terms to deg method

ed32335

remove test

9407ea2

remove parm

a2a9e39

deprecate mark center

a8767c3

cfs-data requested a review from SimoneAriens May 18, 2026 13:02

cfs-data changed the title ~~Replace level_map and deprecate reference_point parameter~~ Replace level_map and deprecate reference_point and center parameters May 18, 2026

shorter code

63ed369

vergep reviewed May 19, 2026

View reviewed changes

vergep requested changes May 19, 2026

View reviewed changes

SimoneAriens approved these changes May 20, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `level_map` and deprecate `reference_point` and `center` parameters#92

Replace `level_map` and deprecate `reference_point` and `center` parameters#92
cfs-data wants to merge 35 commits into
mainfrom
replace-map-level

cfs-data commented Jan 21, 2026 •

edited

Loading

Uh oh!

snregales commented Jan 28, 2026

Uh oh!

Raytesnel commented Jan 28, 2026

Uh oh!

snregales commented Feb 19, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

vergep left a comment •

edited

Loading

Uh oh!

vergep May 19, 2026

Uh oh!

vergep May 19, 2026

Uh oh!

vergep May 19, 2026

Uh oh!

vergep May 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

cfs-data commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snregales commented Jan 28, 2026

Uh oh!

Raytesnel commented Jan 28, 2026

Uh oh!

snregales commented Feb 19, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 18, 2026

Diff Coverage

Diff: origin/main..HEAD, staged and unstaged changes

Summary

packages/scratch-core/src/conversion/leveling/core.py

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

vergep left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vergep May 19, 2026

Choose a reason for hiding this comment

Uh oh!

vergep May 19, 2026

Choose a reason for hiding this comment

Uh oh!

vergep May 19, 2026

Choose a reason for hiding this comment

Uh oh!

vergep May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cfs-data commented Jan 21, 2026 •

edited

Loading

vergep left a comment •

edited

Loading