-
Notifications
You must be signed in to change notification settings - Fork 0
changed the saved params for the impression preprocessing #90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Diff CoverageDiff: origin/main..HEAD, staged and unstaged changes
Summary
|
Minimum allowed line rate is |
laurensWe
left a comment
There was a problem hiding this 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)) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
after discussions we decided we need to save fewer params for the preprocessing of the impression marks