Allow using SampleModels as resolution#196
Conversation
henrikjacobsenfys
added
[scope] enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #196 +/- ##
===========================================
+ Coverage 98.13% 98.14% +0.01%
===========================================
Files 51 51
Lines 3542 3567 +25
Branches 652 661 +9
===========================================
+ Hits 3476 3501 +25
Misses 36 36
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
Good code. Minor comments added for consideration.
| Q=sample_model.Q, | ||
| ) | ||
|
|
||
| from copy import copy |
There was a problem hiding this comment.
This should be a top-level import like elsewhere. No need to import it every time you call this method
| "instrument_model.resolution_model.fix_all_parameters()\n", | ||
| "instrument_model.normalize_resolution()\n", |
| resolution_model = cls( | ||
| display_name=sample_model.display_name, | ||
| unit=sample_model.unit, | ||
| components=sample_model.components, | ||
| Q=sample_model.Q, | ||
| ) |
There was a problem hiding this comment.
cls(components=sample_model.components, Q=sample_model.Q, ...) invokes ModelBase._generate_component_collections, which calls the base append_component, not ResolutionModel.append_component.
This means the check for DeltaFunction, Polynomial, and Exponential is never done.
If someone passes a SampleModel that contains a DeltaFunction, it will silently find its way to the ResolutionModel. Consider adding an explicit check at the top of from_sample_model:
for comp in sample_model.components:
if isinstance(comp, (DeltaFunction, Polynomial, Exponential)):
raise TypeError(
f'SampleModel contains a {comp.__class__.__name__}, '
f'which is not allowed in a ResolutionModel.'
)There was a problem hiding this comment.
I added a test that this isn't the case :) It does use the append_component from ResolutionModel, not from the base class.
2 participants
Closes #193