Conversation
* initial instrument model * first draft of analysis * add test of model base * small changes * tests * clear notebook * respond to PR comments * Update resolution_model docstring for clarity
henrikjacobsenfys
added
[scope] enhancement
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #100 +/- ##
==========================================
Coverage ? 98.12%
==========================================
Files ? 36
Lines ? 2292
Branches ? 375
==========================================
Hits ? 2249
Misses ? 23
Partials ? 20
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:
|
| from inspect import Parameter | ||
|
|
There was a problem hiding this comment.
This imports inspect.Parameter (a function signature parameter descriptor) instead of easyscience.variable.Parameter. The type hints on line 72 use this incorrectly imported Parameter class for extra_parameters, which will cause type confusion and potential runtime issues.
There was a problem hiding this comment.
Haha oops. I blame the tab button.
| weights = 1.0 / e | ||
| return x, y, weights |
There was a problem hiding this comment.
Are you sure the variances are never zero? Maybe make a check here?
if np.any(e == 0):
raise ValueError('Cannot compute weights: some variances are zero.')| analysis = Analysis1d( | ||
| display_name=f'{self.display_name}_Q{Q_index}', | ||
| unique_name=(f'{self.unique_name}_Q{Q_index}'), | ||
| experiment=self.experiment, | ||
| sample_model=self.sample_model, | ||
| instrument_model=self.instrument_model, | ||
| extra_parameters=self._extra_parameters, | ||
| Q_index=Q_index, | ||
| ) |
There was a problem hiding this comment.
The Analysis class creates Analysis1d objects in __init__ with references to experiment, sample_model, and instrument_model.
However, when these properties are changed via setters (inherited from AnalysisBase), the Analysis1d objects in _analysis_list are not regenerated.
This means that changing experiment won't update the data used by individual Q analyses and changing sample_model won't update the components used for fitting.
you could potentially override the setters in Analysis class or add regeneration logic:
def _on_experiment_changed(self) -> None:
super()._on_experiment_changed()
self._regenerate_analysis_list()There was a problem hiding this comment.
Thanks, this is really important! I'll save it for last since it requires some thinking.
There was a problem hiding this comment.
I think I fixed it. The logic behind updating sample models etc needs an overhaul. For now, I think what I have works, and then I'll have to think about how to do it properly. The issue is to find a balance between keeping everything up to date while not overwriting any changes users have made.
| Args: | ||
| energy (float): The energy value to calculate the model for. |
There was a problem hiding this comment.
I don't see any arguments in the method signature?
There was a problem hiding this comment.
Fixed docstring :)
| Args: | ||
| add_background (bool): Whether to add the background to the | ||
| model prediction when plotting individual components. | ||
|
|
||
| kwargs: Keyword arguments to pass to the plotting | ||
| function. |
There was a problem hiding this comment.
missing plot_components arg description
missing return value type (-> None)
There was a problem hiding this comment.
This has not been addressed yet
There was a problem hiding this comment.
This should be fixed now
There was a problem hiding this comment.
This is how it looks on my computer. I don't understand why it doesn't show up here
| **kwargs, | ||
| ) -> None: |
There was a problem hiding this comment.
Not -> None but plopp figure object
| ValueError: If the Q index is not valid. | ||
| """ |
There was a problem hiding this comment.
It's IndexError raised (line 318)
|
I'm working on fixing the docstrings for analysis.py, so don't worry too much about them :) |
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
This time more sinister issues found.
Also,
Current tests are OK, but some aspects are not covered:
Analysislifecycle rebuild behavior after replacing experiment/sample/instrumentConvolutionBase.convert_energy_unitwith non-zeroenergy_offsetConvolutionBasesetter path when assigning singleModelComponent
There was a problem hiding this comment.
This converts self.energy but not self.energy_offset.
energy_with_offset then subtracts a value in the old unit from energy in the new unit.
There was a problem hiding this comment.
Thanks, fixed!
| def sample_components(self, sample_components: ComponentCollection | ModelComponent) -> None: | ||
| """Set the sample model. | ||
| Args: |
There was a problem hiding this comment.
sample_components (and resolution_components) setters accept both ComponentCollection | ModelComponent but do not wrap ModelComponent into ComponentCollection.
Downstream, Convolution assumes .components exists on these attributes.
e.g.
conv.sample_components = Gaussian()
conv.convolution()
AttributeError: 'Gaussian' object has no attribute 'components'.Same for resolution_components.
You could just mirror __init__ behavior inside setters:
if input is ModelComponent, wrap as ComponentCollection([component]).
There was a problem hiding this comment.
Nice spot. I had encountered this problem elsewhere (hense the fix in the init), but did not realise I hadn't fixed the setters.
|
I think I fixed all the problems (and a few minor things that I found because I wanted to get high code coverage :) ) @rozyczko |
|
|
That's just me being silly. It now returns the figure |
2 participants
This implements the analysis class, consisting of a base class, analysis1d and analysis as discussed here: easyscience/dynamics#5
Furthermore, it implements energy_offset in Convolution, as this is required.
Finally, some small refactoring and bug fixes also found their way into this PR