Feature/unit system overhaul#215
Open
henrikjacobsenfys wants to merge 18 commits into
Open
Conversation
Incorporated docstring examples from develop while preserving the unit-system-overhaul API (x_unit/y_unit split). Kept HEAD repr format and API throughout; updated ExpressionComponent class docstring example from unit= to x_unit=. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Develop's cleanly-merged changes introduced self.unit (old API) in __repr__ methods of ResolutionModel, Convolution, NumericalConvolution, AnalyticalConvolution, and BackgroundModel. Updated all to use self.x_unit. Added missing __init__ docstrings to ResolutionModel and SampleModel. Updated test assertions to match HEAD repr format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
henrikjacobsenfys
added
[scope] enhancement
henrikjacobsenfys
left a comment
henrikjacobsenfys
left a comment
Member
Author
There was a problem hiding this comment.
Part one of review
| "\n", | ||
| "We create a `Polynomial` with two coefficients as the fit function. Two units must be set:\n", | ||
| "- `x_unit='1/angstrom'` — because `ParameterAnalysis` always uses Q as its x-axis.\n", | ||
| "- `y_unit='meV'` — because we are fitting `Gaussian area`, which has unit `meV` (area_unit = x_unit × y_unit = meV × dimensionless = meV for the Gaussian).\n", |
Member
Author
There was a problem hiding this comment.
This needs to be updated to match the style of the rest of the tutorial
| "With apologies for the lack of creativity, these all appear like straight lines. We can fit them individually or all together using `ParameterAnalysis`.\n", | ||
| "\n", | ||
| "For each parameter we want to fit, we create a fit function (here a `Polynomial`) and a `FitBinding` connecting the parameter name to the fit function. Two units must be set on each fit function:\n", | ||
| "- `x_unit='1/angstrom'` — because `ParameterAnalysis` always uses Q as its x-axis.\n", |
Member
Author
There was a problem hiding this comment.
Also needs updating
| analysis1d.refresh_convolver(energy=x) | ||
| # Make sure the convolver is up to date for this Q index. | ||
| # Wrap x in a sc.Variable so refresh_convolver carries the correct unit. | ||
| energy_sc = sc.array(dims=['energy'], values=x, unit=self.experiment.energy.unit) |
Member
Author
There was a problem hiding this comment.
It's silly to convert energy to numpy and back to scipp - needs updating here and probably else where
| energy : sc.Variable | None, default=None | ||
| Optional energy grid to use for calculation. If None, the energy grid from the | ||
| experiment is used. | ||
| convolver : object | None, default=None |
Member
Author
There was a problem hiding this comment.
it's not an "object", it's a ConvolutionBase
| raise TypeError('Q_index must be an integer or None.') | ||
|
|
||
| if Q_index < 0 or (self.Q is not None and Q_index >= len(self.Q)): | ||
| if Q_index < 0: |
Member
Author
There was a problem hiding this comment.
This can be written smarter
| # left half-width | ||
| if i == 0: # noqa: SIM108 | ||
| left = x[1] - x[0] if x.size > 1 else 0.5 | ||
| if x_vals.min() - EPSILON <= center <= x_vals.max() + EPSILON: |
Member
Author
There was a problem hiding this comment.
Add back comments explaining the calculation
| return model | ||
|
|
||
| def __repr__(self) -> str: | ||
| def convert_x_unit(self, new_x_unit: str | sc.Unit) -> None: |
Member
Author
There was a problem hiding this comment.
Maybe the bulk of all these convert_x_unit and convert_y_unit methods can be in ModelBase to simplify the code?
| f'unit={self.unit},\n' | ||
| f' area={self.area},\n' | ||
| f' center={self.center})' | ||
| f'DeltaFunction(name = {self.name}, display_name = {self.display_name}, ' |
Member
Author
There was a problem hiding this comment.
Why change from self.__class__.__name__ to hardcoded name?
| ------ | ||
| TypeError | ||
| If amplitude, center, or rate are not numbers or Parameters. | ||
| If *amplitude* or *rate* is not numeric or a :class:`Parameter`. |
Member
Author
There was a problem hiding this comment.
this method should not take Parameters as input?
Member
Author
There was a problem hiding this comment.
May need to check serialization to ensure it works when no longer allowing Parameter as input
| """ | ||
| Return all parameters. | ||
| if output == 'scipp': | ||
| import scipp as sc |
Member
Author
There was a problem hiding this comment.
imports should be at the top, here and elsewhere
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary:
feature/unit-system-overhaulThis PR does two things simultaneously: it lands a fundamental unit system redesign (the
branch's raison d'être), and it carries in a large body of new functionality from the
developbranch via a merge commit. The two are intertwined — the develop features needed to be updated
to speak the new unit API.
Part 1 — New functionality from
develop(merged in)These were already reviewed on the
developbranch. They arrive here becausefeature/unit-system-overhaulbranched from an older state ofdevelop, and the merge pulledeverything forward.
analysis/Analysis(2D multi-Q),Analysis1d(single-Q),AnalysisBase(shared fit/plot logic),FitBinding,ParameterAnalysisexperiment/Experiment— loads HDF5 data, rebins, masks NaNsbase_classes/EasyDynamicsBase,EasyDynamicsList,EasyDynamicsModelBase,NameMixinsample_model/diffusion_model/JumpTranslationalDiffusion,DeltaLorentzsample_model/components/ExpressionComponent,Exponentialsettings/ConvolutionSettings,DetailedBalanceSettingsPart 2 — Unit system overhaul (core branch work)
The design change
Every model class previously carried a single
unitfield representing the energy-axis unit.This is now split:
x_unit— the unit of the independent variable (energy axis, typically'meV')y_unit— the unit of the output / intensity (typically'dimensionless', but physicalwhen data is calibrated, e.g.
'1/meV')The consequence is that the area parameter of any peak function now has unit
x_unit * y_unit. Ify_unit='dimensionless', the area is in energy units as before. Ify_unit='1/meV', the area is dimensionless (the integral of a normalised peak), which is thephysically correct representation.
What changed in every class
unit=→x_unit=,y_unit='dimensionless'(new)unit→x_unit, plus newy_unitconvert_unit()→convert_x_unit()+convert_y_unit()evaluate(x)→evaluate(x, output='numpy')— passingoutput='scipp'returns asc.Variablecarryingy_unit, enabling unit-safe downstream computationHow unit handling during evaluate changed
Old behaviour (
_prepare_x_for_evaluate): if the caller passed a scippxwith adifferent compatible unit (e.g.
xineVwhen the component is inmeV), the method calledself.convert_unit(x.unit.name), permanently mutating the component and all itsparameters. A
UserWarningwas issued. If units were incompatible, aUnitErrorwas raised— but only after the mutation had already started.
New behaviour:
_prepare_x_for_evaluatevalidates compatibility and returns a 3-tuple(x_values, detected_unit, dim). It never touches the model. Individualevaluate()implementations call
_resolve_param_value(param, target_unit)to get a transiently convertedfloat value for the computation. The model's parameters and
x_unitare never changed by acall to
evaluate().Part 3 — All bugs fixed (with regression tests)
Bug 1 —
evaluate()permanently mutated model state when x had a different unitCommit:
8dfeb08a(overhaul units)File:
src/easydynamics/sample_model/components/model_component.pyWhat happened: Passing a scipp
xineVto a Gaussian initialised inmeVwould callself.convert_unit('eV'), permanently converting all parameters. Repeated calls withdifferent-unit inputs would keep re-converting in a random walk.
Fix:
_prepare_x_for_evaluatenow returns(values, detected_unit, dim)without touchingthe model.
_resolve_param_valuedoes a transient scipp scalar conversion at call time.Regression tests:
tests/unit/easydynamics/sample_model/components/test_model_component.py—test_prepare_x_for_evaluate_with_different_unit_no_warn: checks thatx_unitand parametervalues are unchanged after calling
_prepare_x_for_evaluatewith a compatible-but-differentunit
tests/unit/easydynamics/sample_model/components/test_model_component.py—test_resolve_param_value_converts_without_mutating: confirms the float result is correct(1.0 meV → 0.001 eV) and the parameter's
.valueand.unitare unchangedtests/unit/easydynamics/sample_model/components/test_model_component.py—test_evaluate_with_compatible_unit_gives_correct_result: end-to-end check — Gaussian in meVevaluated with scipp x in eV gives numerically identical output to an equivalent Gaussian in
eV
Bug 2 —
ComponentCollection.evaluate(output='scipp')raisedTypeErroron multi-component collectionsCommit:
3f59e3e2(Claude 7th review)File:
src/easydynamics/sample_model/component_collection.pyWhat happened:
sum(component.evaluate(x, output='scipp') for component in self)seedswith Python's integer
0. The first0 + sc.VariableraisesTypeErrorbecause scipp doesnot support adding integers to Variables.
Fix: Seed the sum from the first component:
first = next(gen); return sum(gen, first).Regression test:
tests/unit/easydynamics/sample_model/test_component_collection.py—test_evaluate_scipp_output_multi_component_does_not_raiseBug 3 —
evaluate(output='scipp')with a DataArray input produced wrong output dimension nameCommit:
3f59e3e2(Claude 7th review)File:
src/easydynamics/sample_model/components/model_component.pyWhat happened: A
sc.DataArraycan have a coord key (e.g.'energy') that differs fromthe coordinate Variable's internal dimension name (e.g.
'x'). The old code extracted thecoord key as
dim, but then immediately overwrote it with the Variable's.dims[0]whenprocessing the Variable. The returned
sc.Variablehad dimension'x'instead of'energy'.Fix: A
dim_from_dataarrayflag prevents the Variable branch from overwritingdimwhenthe DataArray branch already set it.
Regression test:
tests/unit/easydynamics/sample_model/components/test_model_component.py—test_evaluate_preserves_dataarray_coord_key_as_dimBug 4 —
_prepare_x_for_evaluatesorted x, desynchronising output values from input positionsCommit:
e90c9b1e(Claude 6th review)File:
src/easydynamics/sample_model/components/model_component.pyWhat happened: The function returned
np.sort(x_in). Whenevaluate(output='scipp')wrapped the result in a
sc.Variable, the values were in sorted order but thedimcoordinateof the caller's x was in its original order — a silent ordering mismatch.
Fix: Return
x_inunsorted. Callers that need a monotonic axis sort before passing x in.Bug 5 —
__repr__methods referenced the removedself.unit/self._unitattributeCommit:
47584394(Fix post-merge issues)Files:
src/easydynamics/convolution/analytical_convolution.pysrc/easydynamics/convolution/convolution.pysrc/easydynamics/convolution/numerical_convolution.pysrc/easydynamics/sample_model/background_model.pysrc/easydynamics/sample_model/resolution_model.pyWhat happened: The develop merge introduced
__repr__methods usingself.unit/self._unit. After the overhaul renamed these tox_unit, callingrepr()on any of theseobjects raised
AttributeError.Fix: All five
__repr__methods updated toself.x_unit.Regression tests: Existing
test_reprmethods in the corresponding test files, which allassert
'x_unit=meV'appears in the output.Bug 6 —
Analysis1d.calculate()mutatedself._convolver, corrupting the fit-time convolverCommit:
e90c9b1e(Claude 6th review)File:
src/easydynamics/analysis/analysis1d.pyWhat happened:
calculate(energy=custom_grid)wroteself._convolver = self._create_convolver(energy=custom_grid)and then setself._convolver_is_dirty = True. This destroyed the convolver that the subsequentfit()call would have used, forcing an unnecessary rebuild — and if
fit()was called before therebuild completed, it could use the plot-path energy grid.
Fix:
calculate()creates a localconvolverand passes it explicitly to_calculate(convolver=convolver).self._convolveris never touched bycalculate().Regression tests:
tests/unit/easydynamics/analysis/test_analysis1d.py—test_calculate_does_not_dirty_convolverBug 7 —
_on_Q_index_changedcrashed whenQ_indexwasNoneCommit:
e90c9b1e(Claude 6th review)File:
src/easydynamics/analysis/analysis1d.pyWhat happened: When
Q_indexwas cleared toNone, the callback still calledself.experiment.get_masked_energy(Q_index=None), raisingTypeErrorinside the experiment.Fix: Early return with
self._masked_energy = None; self._convolver_is_dirty = Truewhenself._Q_index is None.Regression test:
tests/unit/easydynamics/analysis/test_analysis1d.py—test_on_Q_index_changedBug 8 —
_verify_Q_indexpermitted any non-negative index whenself.QwasNoneCommit:
f02845e2(Claude 5th review)File:
src/easydynamics/analysis/analysis_base.pyWhat happened:
if Q_index < 0 or (self.Q is not None and Q_index >= len(self.Q))— whenself.QwasNonethe second condition was skipped, allowing any non-negative index to pass validation even with
no Q set.
Fix: Split into two separate guards:
if Q_index < 0: raise ...thenif self.Q is None or Q_index >= len(self.Q): raise ...Regression test:
tests/unit/easydynamics/analysis/test_analysis_base.py—_verify_Q_indextests withQ=NoneBug 9 —
plot_parameters(names=[])plotted all parameters instead of nothingCommit:
f02845e2(Claude 5th review)File:
src/easydynamics/analysis/analysis.pyWhat happened:
if not names:isTruefor an empty list, sonameswas replaced withall parameter names from the dataset. The caller's explicit empty list was silently ignored.
Fix:
if names is None:— only auto-populate whenNonewas passed, not when the callerpassed
[].Bug 10 —
plot_parametersmutated sharedParameterobjects while harmonising unitsCommit:
f02845e2(Claude 5th review)File:
src/easydynamics/analysis/analysis.pyWhat happened: When parameters from different Q indices had different units, the code called
p.convert_unit(units[name])directly on theParameterobject shared with the model. Thispermanently changed the unit of a live model parameter as a side-effect of plotting.
Fix:
p = copy(p)before callingconvert_unit, so only the local copy is mutated.Bug 11 —
ConvolutionSettingscopy in_create_analysis_listonly copied three fieldsCommit:
f02845e2(Claude 5th review)Files:
src/easydynamics/analysis/analysis.py,src/easydynamics/settings/convolution_settings.pyWhat happened: Each
Analysis1dgot its ownConvolutionSettingsbuilt by manuallycopying
upsample_factor,extension_factor, andsuppress_warnings. Any field added toConvolutionSettingsin the future would silently be omitted.Fix: Added
ConvolutionSettings.__copy__and changed the call tocopy(self.convolution_settings).Regression test:
tests/unit/easydynamics/settings/test_convolution_settings.py— copy testBug 12 —
ConvolutionSettings.extension_factor = Nonedid not invalidate the planCommit:
f02845e2(Claude 5th review)File:
src/easydynamics/settings/convolution_settings.pyWhat happened: The setter's numeric validation path raised
TypeErrorbefore reaching theplan-invalidation call.
Noneis a valid value meaning "auto-determine", but it was rejectedas non-numeric.
Fix: Added an early-return branch:
if factor is None: self._extension_factor = factor; self.convolution_plan_is_valid = False; return.Bug 13 —
fix_energy_offset/free_energy_offsetdid not update the template parameter when called withoutQ_indexCommit:
f02845e2(Claude 5th review)File:
src/easydynamics/sample_model/instrument_model.pyWhat happened: When
Q_index=None, the method iterated overself._energy_offsets(theper-Q list) but never updated
self._energy_offset(the template). New Q values added laterwould inherit the old fixed/free state.
Fix:
self._energy_offset.fixed = fixedadded before the loop.Bug 14 — Energy offset subtracted without unit conversion when offset unit ≠ energy grid unit
Commit:
e90c9b1e(Claude 6th review)File:
src/easydynamics/convolution/numerical_convolution.pyWhat happened:
self.energy_offset.valuewas subtracted directly from the energy grid evenwhen the offset was stored in a different unit (e.g. offset in
eV, grid inmeV). Thesubtraction was numerically wrong by a factor of 1000.
Fix: Convert the offset to the energy grid's unit via
sc.to_unitif they differ.Regression test:
tests/unit/easydynamics/convolution/test_numerical_convolution.py—test_convolution_energy_offset_different_unitBug 15 — Detailed balance correction applied to wrong energy when using energy offsets
Commit:
3f59e3e2(Claude 7th review)File:
src/easydynamics/convolution/numerical_convolution.pyWhat happened: The detailed balance factor was computed on
energy_dense - offset_value,but the sample evaluation used
energy_dense - energy_even_length_offset - offset_value. Theasymmetry means the detailed balance correction was applied on a shifted grid, introducing a
systematic error when the energy offset was non-zero.
Fix: Both computations now use the same energy expression:
energy_dense - energy_even_length_offset - offset_value.Regression test:
tests/unit/easydynamics/convolution/test_numerical_convolution.pyBug 16 —
plot_data_and_modelandresidualsinAnalysis1dincluded NaN data pointsCommit:
3f59e3e2(Claude 7th review)File:
src/easydynamics/analysis/analysis1d.pyWhat happened: The data slice
experiment.binned_data['Q', self.Q_index]includes energybins with NaN intensity (masked or empty bins from the experiment). The model array for the
same Q has finite values at all energy points. Including NaN data in
plot_data_and_modelproduces broken traces; including them in
residualsmakes the subtraction produce all-NaNresults.
Fix: Apply
experiment.get_finite_energy_mask(Q_index=self.Q_index)to the data slice inboth methods. A new
Experiment.get_finite_energy_maskhelper was also added.Regression tests:
tests/unit/easydynamics/analysis/test_analysis1d.py—test_plot_data_and_model_masks_nan_data,test_residuals_masks_nan_dataBug 17 —
ParameterAnalysis._get_q_values_and_weightsrejected NaN variances that should be silently filteredCommit:
e90c9b1e(Claude 6th review)File:
src/easydynamics/analysis/parameter_analysis.pyWhat happened: When a parameter was absent for some Q values (e.g. a free parameter only
at certain Q),
parameters_to_datasetfillsnp.nan. The old code treated~np.isfinite(variances)as an error and raisedValueError, preventing Q-trend analysis forany dataset with missing parameters.
Fix: Distinguish
nan_mask(absent data — silently filter rows) from othernon-finite/non-positive variances (true errors). Only rows where the variance is NaN are
dropped; everything else raises.
Regression tests:
tests/unit/easydynamics/analysis/test_parameter_analysis.py— NaN variance handling testsBug 18 —
ModelBase.convert_x_unitrollback calledconvert_x_unit(None)whenold_unitwasNoneCommit:
3f59e3e2(Claude 7th review)File:
src/easydynamics/sample_model/model_base.pyWhat happened: If the model was initialised with
x_unit=Noneand a conversion failed,the rollback attempted
component.convert_x_unit(None), which raisedTypeErrorinside therollback handler — hiding the original exception.
Fix: Guard
if old_unit is not None:around the rollback loop.Bug 19 —
ResolutionModel.from_sample_modeldirty-flag race condition discarded installed collectionsCommit:
3f59e3e2(Claude 7th review)File:
src/easydynamics/sample_model/resolution_model.pyWhat happened: EasyScience parameter callbacks triggered during
__init__orcopycouldset
_component_collections_is_dirty = Trueon the freshly createdResolutionModel. Thenext call to
get_component_collection()would then rebuild the collections from scratch,discarding the ones carefully installed by
from_sample_model. This was especially likely tohappen after
normalize_area()orfix_all_parameters()triggered parameter-change callbacks.Fix: Explicitly reset
_component_collections_is_dirty = Falseafter installingcollections, and again after the
normalize_area()/fix_all_parameters()calls.Bug 20 —
DeltaLorentz.calculate_widthsilently failed when called beforeQwas setCommit:
3f59e3e2(Claude 7th review)File:
src/easydynamics/sample_model/diffusion_model/delta_lorentz.pyWhat happened:
_lorentzian_width_listwas empty before Q was set. Callingcalculate_width()in that state raisedStopIterationor returned a wrong value depending onthe internal path.
Fix: Explicit
if not self._lorentzian_width_list: raise ValueError(...)with a clearmessage.
Regression test:
tests/unit/easydynamics/sample_model/diffusion_model/test_delta_lorentz.pyBug 21 —
_invalidate_plan_on_changetype annotation wasdict[str, object]instead ofset[str]Commit:
f02845e2(Claude 5th review)File:
src/easydynamics/convolution/convolution.pyWhat happened: The class variable was annotated as
ClassVar[dict[str, object]]but isactually a
set. The wrong annotation did not cause a runtime error but could mislead a readeror confuse a type checker.
Fix: Corrected to
ClassVar[set[str]].