Skip to content

Conversation

@SimoneAriens
Copy link
Collaborator

after discussions we decided we need to save fewer params for the preprocessing of the impression marks

@SimoneAriens SimoneAriens marked this pull request as ready for review January 15, 2026 16:39
@github-actions
Copy link

Diff Coverage

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

  • packages/scratch-core/src/conversion/preprocess_impression/impression.py (100%)
  • packages/scratch-core/src/conversion/preprocess_impression/resample.py (100%)

Summary

  • Total: 6 lines
  • Missing: 0 lines
  • Coverage: 100%

@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Health
. 95% 88%
comparators 100% 100%
container_models 99% 100%
conversion 96% 89%
conversion.leveling 100% 100%
conversion.leveling.solver 100% 75%
conversion.preprocess_impression 99% 92%
extractors 95% 75%
parsers 98% 67%
parsers.patches 89% 60%
preprocessors 100% 100%
processors 100% 100%
renders 98% 50%
utils 91% 75%
Summary 97% (1136 / 1170) 84% (131 / 156)

Minimum allowed line rate is 50%

Copy link
Member

@laurensWe laurensWe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mark.meta_data.update(
_build_preprocessing_metadata(params, mark.center, is_resampled)
)
mark.meta_data.update(**asdict(params))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of a noob question; why can we just use .update here, but use the model_copy(update=...) in utils.py update_mark_* functions?

assert center_x == pytest.approx(50e-6)
assert center_y == pytest.approx(50e-6)
center_x, center_y = filtered.center
assert center_x == pytest.approx(40.5)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have these values changed? It feels that removing parameters from a meta data field should not change the overall outcome.
Either way: the comment on line 543 is now wrong

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants