Allow energy as input to plotters#128
Conversation
henrikjacobsenfys
added
[scope] enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #128 +/- ##
==========================================
Coverage ? 97.65%
==========================================
Files ? 36
Lines ? 2385
Branches ? 403
==========================================
Hits ? 2329
Misses ? 32
Partials ? 24
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:
|
|
the title of the PR and the name of the branch seem pretty far off |
Yes, this is the "fixing a bug (NaNs in data give errors because the energy axis is inconsistent since it changes depending on where the NaNs are) by making an enhancement (give users control over the energy axis for plotting, and make the defaults sensible so they work both for fitting and plotting with default args)" that I mentioned :) |
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
minor issues only, otherwise looks solid.
| def _evaluate_sample(self, energy: sc.DataArray | None = None) -> np.ndarray: | ||
| """Evaluate the sample contribution for a given Q index. |
There was a problem hiding this comment.
Hasn't energy been typed as sc.Variable? (Also in the following docstring)
There was a problem hiding this comment.
I blame copilot!
| def _evaluate_background(self, energy: sc.DataArray | None = None) -> np.ndarray: | ||
| """Evaluate the background contribution for the chosen Q index. |
There was a problem hiding this comment.
Hasn't energy been typed as sc.Variable? (Also in the following docstring)
| self._convolver = self._create_convolver(energy=energy) | ||
|
|
There was a problem hiding this comment.
The convolver is now recreated on every call to calculate(), even when energy is None (most common case?). Previously it was cached and only updated on Q-index changes. Looks like performance regression?
There was a problem hiding this comment.
I think it's ok here, fit uses _calculate
| masked_energy = experiment_with_data.get_masked_energy(Q_index=Q_index) | ||
|
|
||
| # EXPECT | ||
| len(masked_energy) == 1 |
There was a problem hiding this comment.
I think you meant assert len(masked_energy) == 1?
There was a problem hiding this comment.
I blame.. ummm... myself?
| def get_masked_energy(self, Q_index: int) -> sc.Variable | None: | ||
| """Get the energy values from the dataset, applying the mask if | ||
| present. | ||
|
|
||
| Returns: | ||
| sc.Variable | None: The masked energy values from the | ||
| dataset, or None if no data is loaded. | ||
| """ |
There was a problem hiding this comment.
no description of args: Q_index
2 participants
Ensures that the correct energies are used when calculating the model for plotting. Also allows users to input a finer energy grid to make the plot prettier.
Closes #119