refactor: simplify sample merging and fix edge case when all samples are affected#558
refactor: simplify sample merging and fix edge case when all samples are affected#558alexander-held merged 5 commits intomasterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #558 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 2145 2132 -13
Branches 302 300 -2
=========================================
- Hits 2145 2132 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
hi @alexander-held ! I think this looks great and cleans up a lot of the codebase. I am wondering if we maybe want to include an integration-type test that makes sure things like visualisation are impacted by the sample merging and reordering, whie fit results are not? (or do we have these already and I am forgetting?) |
|
Thanks! An integration test is a good idea, I propose to add one in a follow-up and opened #559 to track. This already contains a lot of changes so hopefully doing it in a separate PR makes things a bit easier to keep track. |
|
cool i think this in general looks great. It would be good if we can maybe add some more info in the PR summary to explain the changes for future, but otherwise all is good! |
|
Good point, I extended the description in the body a bit. |
This is a follow-up to #505 which simplifies the sample merging machinery to better handle edge cases. Tests are updated accordingly. The docstrings now also highlight that this can be used to rename or reorder samples.
sample_update_mapis now required by bothLightConfigandLightModel, as other cases can be handled fine by the original model. It is still possible to usesample_update_map = {}if needed._merge_sample_yieldsnow only operates on 2d inputs, which simplifies the implementation. This works by flipping the order of when it is called inprediction. The merging logic from_update_samplesis integrated intoLightConfig.__init__and simplified.resolves #539
resolves #485 (already possible prior to this, but did not work due to #539 when reordering everything)