Sample Model #56
Sample Model #56
Conversation
henrikjacobsenfys
added
[scope] enhancement
|
Code coverage is not 100%, but I think all the essentials are there, and I'd rather get feedback on the important things than polish tests that may need to change anyway :) Edit: apparently codecov is upset about the missing tests, so I added tests and discovered a minor bug in the process. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #56 +/- ##
===========================================
+ Coverage 95.02% 95.10% +0.07%
===========================================
Files 11 12 +1
Lines 663 837 +174
===========================================
+ Hits 630 796 +166
- Misses 33 41 +8
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:
|
| import scipp as sc | ||
| from easyscience.base_classes import ObjBase | ||
| from easyscience.variable import Parameter | ||
| from scipp import UnitError |
There was a problem hiding this comment.
the order of imports should be fixed and consistent (run ruff)
There was a problem hiding this comment.
I am running ruff, this is the order it decided on - perhaps I need to change some settings?
| Numeric = Union[float, int] | ||
|
|
||
|
|
||
| class SampleModel(ObjBase, MutableMapping): |
There was a problem hiding this comment.
Shouldn't you inherit from TheoreticalModelBase or at least from BaseObjectCollection?
There was a problem hiding this comment.
Probably? I'm not exactly sure
| self, | ||
| name: str = "MySampleModel", | ||
| temperature: Optional[Union[Numeric, None]] = None, | ||
| temperature_unit: Optional[str] = "K", |
There was a problem hiding this comment.
maybe you should accept scipp's Variable with unit as Temperature?
| def __init__( | ||
| self, | ||
| name: str = "MySampleModel", | ||
| temperature: Optional[Union[Numeric, None]] = None, |
There was a problem hiding this comment.
Optional[Union[Numeric, None]] is redundant. Should be Optional[Numeric]
| ########################################################## | ||
|
|
||
| @property | ||
| def temperature(self) -> Union[Parameter, None]: |
There was a problem hiding this comment.
Should be Optional[Parameter]
(in other typehints as well!)
| params = [] | ||
| for comp in self.components.values(): | ||
| params.extend(comp.get_parameters()) | ||
| return params |
There was a problem hiding this comment.
or just
from itertools import chain
# Create generator for temperature parameter
temp_params = (self._temperature,) if self._temperature is not None else ()
# Create generator for component parameters
comp_params = (param for comp in self.components.values()
for param in comp.get_parameters())
# Chain them together and return as list
return list(chain(temp_params, comp_params))(slight teaching aspect for itertools - a VERY important python module)
| if is_not_fixed and is_independent: | ||
| fit_parameters.append(parameter) | ||
|
|
||
| return fit_parameters |
There was a problem hiding this comment.
Again, not a bad loop (easy to see what's being done) but if you want to make it more "modular" you could use list comprehension, e.g.
def is_fit_parameter(param: Parameter) -> bool:
"""Check if a parameter can be used for fitting."""
return (not getattr(param, "fixed", False) and
getattr(param, "_independent", True))
return [param for param in self.get_parameters() if is_fit_parameter(param)]| ) | ||
|
|
||
| if self._temperature: | ||
| new_model.use_detailed_balance = self.use_detailed_balance |
There was a problem hiding this comment.
did you intend not to copy _normalize_detailed_balance?
| new_model = SampleModel( | ||
| name=name, | ||
| temperature=self._temperature.value if self._temperature else None, | ||
| ) |
There was a problem hiding this comment.
Whoops. Unit incoming.
| ############################################## | ||
|
|
||
| def __getitem__(self, key: str) -> ModelComponent: | ||
| """ |
There was a problem hiding this comment.
Shouldn't all those dunder methods be available in BaseObjectCollection already?
There was a problem hiding this comment.
Yes, but TheoreticalModelBase doesn't inherit from BaseObjectCollection, so I'm a bit confused - can we chat on Monday?
|
@rozyczko Would you have another look when you have time? I'm working on other things the rest of the day and am away tomorrow, so no rush. |
2 participants
Adding SampleModel which can contain ModelComponents and calculate the model at given x, with and without detailed balance.