-
Notifications
You must be signed in to change notification settings - Fork 1
Allow using SampleModels as resolution #196
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: develop
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,8 +6,10 @@ | |
| from easydynamics.sample_model.component_collection import ComponentCollection | ||
| from easydynamics.sample_model.components import DeltaFunction | ||
| from easydynamics.sample_model.components import Polynomial | ||
| from easydynamics.sample_model.components.exponential import Exponential | ||
| from easydynamics.sample_model.components.model_component import ModelComponent | ||
| from easydynamics.sample_model.model_base import ModelBase | ||
| from easydynamics.sample_model.sample_model import SampleModel | ||
| from easydynamics.utils.utils import Q_type | ||
|
|
||
|
|
||
|
|
@@ -70,9 +72,72 @@ def append_component(self, component: ModelComponent | ComponentCollection) -> N | |
| components = component if isinstance(component, ComponentCollection) else (component,) | ||
|
|
||
| for comp in components: | ||
| if isinstance(comp, (DeltaFunction, Polynomial)): | ||
| if isinstance(comp, (DeltaFunction, Polynomial, Exponential)): | ||
| raise TypeError( | ||
| f'Component in ResolutionModel cannot be a {comp.__class__.__name__}' | ||
| ) | ||
|
|
||
| super().append_component(component) | ||
|
|
||
| @classmethod | ||
| def from_sample_model( | ||
| cls, | ||
| sample_model: SampleModel, | ||
| normalize_area: bool = True, | ||
| fix_parameters: bool = True, | ||
| ) -> 'ResolutionModel': | ||
| """ | ||
| Create a ResolutionModel from a SampleModel. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| sample_model : SampleModel | ||
| SampleModel to create the ResolutionModel from. | ||
| normalize_area : bool, default=True | ||
| Whether to normalize the components in the ResolutionModel to have area 1. | ||
| fix_parameters : bool, default=True | ||
| Whether to fix the parameters in the ResolutionModel. | ||
|
|
||
| Returns | ||
| ------- | ||
| 'ResolutionModel' | ||
| ResolutionModel created from the SampleModel. | ||
|
|
||
| Raises | ||
| ------ | ||
| TypeError | ||
| If sample_model is not a SampleModel, or if normalize_area or fix_parameters are not | ||
| bool. | ||
| """ | ||
| if not isinstance(sample_model, SampleModel): | ||
| raise TypeError( | ||
| f'sample_model must be an instance of SampleModel. Got {type(sample_model)}.' | ||
| ) | ||
|
|
||
| if not isinstance(normalize_area, bool): | ||
| raise TypeError('normalize_area must be True or False.') | ||
|
|
||
| if not isinstance(fix_parameters, bool): | ||
| raise TypeError('fix_parameters must be True or False.') | ||
|
|
||
| resolution_model = cls( | ||
| display_name=sample_model.display_name, | ||
| unit=sample_model.unit, | ||
| components=sample_model.components, | ||
| Q=sample_model.Q, | ||
| ) | ||
|
Comment on lines
+125
to
+130
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.'
)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a test that this isn't the case :) It does use the append_component from |
||
|
|
||
| from copy import copy | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a top-level import like elsewhere. No need to import it every time you call this method |
||
|
|
||
| if sample_model.Q is not None: | ||
| for index in range(len(sample_model.Q)): | ||
| resolution_model._component_collections[index] = copy( | ||
| sample_model.get_component_collection(Q_index=index) | ||
| ) | ||
| if normalize_area: | ||
| resolution_model.normalize_area() | ||
|
|
||
| if fix_parameters: | ||
| resolution_model.fix_all_parameters() | ||
|
|
||
| return resolution_model | ||
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.
are these still needed?