From 6e8acb3e6e9e7435ba547c050e658cf527527eac Mon Sep 17 00:00:00 2001 From: henrikjacobsenfys Date: Thu, 18 Jun 2026 20:29:45 +0200 Subject: [PATCH 1/5] code cleanup --- src/easydynamics/analysis/analysis.py | 60 +++----------------- src/easydynamics/analysis/analysis1d.py | 44 ++------------- src/easydynamics/analysis/analysis_base.py | 66 +++++++++++++++++----- 3 files changed, 66 insertions(+), 104 deletions(-) diff --git a/src/easydynamics/analysis/analysis.py b/src/easydynamics/analysis/analysis.py index c7d01ef8..07931a83 100644 --- a/src/easydynamics/analysis/analysis.py +++ b/src/easydynamics/analysis/analysis.py @@ -279,14 +279,9 @@ def plot_data_and_model( 'No Q values available for plotting. Please check the experiment data.' ) - if not isinstance(plot_components, bool): - raise TypeError('plot_components must be True or False.') - - if not isinstance(add_background, bool): - raise TypeError('add_background must be True or False.') - - if not isinstance(plot_residuals, bool): - raise TypeError('plot_residuals must be True or False.') + self._verify_bool(plot_components, 'plot_components') + self._verify_bool(add_background, 'add_background') + self._verify_bool(plot_residuals, 'plot_residuals') if energy is None: energy = self.energy @@ -300,39 +295,8 @@ def plot_data_and_model( include_residuals=plot_residuals, ) - plot_kwargs_defaults = { - 'title': self.display_name, - 'linestyle': {}, - 'marker': {}, - 'color': {}, - 'markerfacecolor': {}, - 'keep': 'energy', - } - - for key in data_and_model: - if key == 'Data': - plot_kwargs_defaults['linestyle'][key] = 'none' - plot_kwargs_defaults['marker'][key] = 'o' - plot_kwargs_defaults['color'][key] = 'black' - plot_kwargs_defaults['markerfacecolor'][key] = 'none' - - elif key == 'Model': - plot_kwargs_defaults['linestyle'][key] = '-' - plot_kwargs_defaults['marker'][key] = None - plot_kwargs_defaults['color'][key] = 'red' - plot_kwargs_defaults['markerfacecolor'][key] = 'none' - - elif key == 'Residuals': - plot_kwargs_defaults['linestyle'][key] = 'none' - plot_kwargs_defaults['marker'][key] = 'o' - plot_kwargs_defaults['color'][key] = 'blue' - plot_kwargs_defaults['markerfacecolor'][key] = 'none' - - else: - plot_kwargs_defaults['linestyle'][key] = '--' - plot_kwargs_defaults['marker'][key] = None - - # Overwrite defaults with any user-provided kwargs + plot_kwargs_defaults = self._build_plot_style_defaults(data_and_model) + plot_kwargs_defaults['keep'] = 'energy' plot_kwargs_defaults.update(kwargs) if plot_residuals: @@ -401,14 +365,9 @@ def data_and_model_to_datagroup( 'No Q values available for creating DataGroup. Please check the experiment data.' ) - if not isinstance(add_background, bool): - raise TypeError('add_background must be True or False.') - - if not isinstance(include_components, bool): - raise TypeError('include_components must be True or False.') - - if not isinstance(include_residuals, bool): - raise TypeError('include_residuals must be True or False.') + self._verify_bool(add_background, 'add_background') + self._verify_bool(include_components, 'include_components') + self._verify_bool(include_residuals, 'include_residuals') energy = self._verify_energy(energy) @@ -809,8 +768,7 @@ def _create_components_dataset( sc.Dataset A scipp Dataset where each entry is a component of the model, with dimensions "Q". """ - if not isinstance(add_background, bool): - raise TypeError('add_background must be True or False.') + self._verify_bool(add_background, 'add_background') if energy is None: energy = self.energy diff --git a/src/easydynamics/analysis/analysis1d.py b/src/easydynamics/analysis/analysis1d.py index 480b5fd0..66ca8e00 100644 --- a/src/easydynamics/analysis/analysis1d.py +++ b/src/easydynamics/analysis/analysis1d.py @@ -309,38 +309,7 @@ def plot_data_and_model( include_residuals=plot_residuals, ) - plot_kwargs_defaults = { - 'title': self.display_name, - 'linestyle': {}, - 'marker': {}, - 'color': {}, - 'markerfacecolor': {}, - } - - for key in data_and_model: - if key == 'Data': - plot_kwargs_defaults['linestyle'][key] = 'none' - plot_kwargs_defaults['marker'][key] = 'o' - plot_kwargs_defaults['color'][key] = 'black' - plot_kwargs_defaults['markerfacecolor'][key] = 'none' - - elif key == 'Model': - plot_kwargs_defaults['linestyle'][key] = '-' - plot_kwargs_defaults['marker'][key] = None - plot_kwargs_defaults['color'][key] = 'red' - plot_kwargs_defaults['markerfacecolor'][key] = 'none' - - elif key == 'Residuals': - plot_kwargs_defaults['linestyle'][key] = 'none' - plot_kwargs_defaults['marker'][key] = 'o' - plot_kwargs_defaults['color'][key] = 'blue' - plot_kwargs_defaults['markerfacecolor'][key] = 'none' - - else: - plot_kwargs_defaults['linestyle'][key] = '--' - plot_kwargs_defaults['marker'][key] = None - - # Overwrite defaults with any user-provided kwargs + plot_kwargs_defaults = self._build_plot_style_defaults(data_and_model) plot_kwargs_defaults.update(kwargs) if plot_residuals: @@ -409,14 +378,9 @@ def data_and_model_to_datagroup( 'No Q values available for creating DataGroup. Please check the experiment data.' ) - if not isinstance(add_background, bool): - raise TypeError('add_background must be True or False.') - - if not isinstance(include_components, bool): - raise TypeError('include_components must be True or False.') - - if not isinstance(include_residuals, bool): - raise TypeError('include_residuals must be True or False.') + self._verify_bool(add_background, 'add_background') + self._verify_bool(include_components, 'include_components') + self._verify_bool(include_residuals, 'include_residuals') if self.Q_index is None: raise ValueError('Q_index must be set to create DataGroup.') diff --git a/src/easydynamics/analysis/analysis_base.py b/src/easydynamics/analysis/analysis_base.py index ac30a9ea..9ef46397 100644 --- a/src/easydynamics/analysis/analysis_base.py +++ b/src/easydynamics/analysis/analysis_base.py @@ -1,6 +1,8 @@ # SPDX-FileCopyrightText: 2026 EasyScience contributors # SPDX-License-Identifier: BSD-3-Clause +from collections.abc import Iterable + import numpy as np import scipp as sc from easyscience.variable import Parameter @@ -266,7 +268,7 @@ def energy(self) -> sc.Variable | None: Returns ------- sc.Variable | None - The energy values from the associated. + The energy values from the associated Experiment, if available, and None if not. """ return self.experiment.energy @@ -460,17 +462,8 @@ def get_parameters_near_bounds( If rtol or atol is negative. """ - if not isinstance(rtol, (int, float)): - raise TypeError(f'rtol must be a float. Got {type(rtol)}.') - - if rtol < 0: - raise ValueError(f'rtol must be non-negative. Got {rtol}.') - - if not isinstance(atol, (int, float)): - raise TypeError(f'atol must be a float. Got {type(atol)}.') - - if atol < 0: - raise ValueError(f'atol must be non-negative. Got {atol}.') + self._verify_nonneg_float(rtol, 'rtol') + self._verify_nonneg_float(atol, 'atol') parameters = self.get_all_parameters() at_bounds = [] @@ -573,6 +566,53 @@ def _verify_energy(self, energy: sc.Variable | None) -> sc.Variable | None: raise TypeError(f'Energy must be a sc.Variable or None. Got {type(energy)}.') return energy + @staticmethod + def _verify_bool(value: object, name: str) -> None: + """Raise TypeError if value is not a bool.""" + if not isinstance(value, bool): + raise TypeError(f'{name} must be True or False.') + + @staticmethod + def _verify_nonneg_float(value: object, name: str) -> None: + """Raise TypeError/ValueError if value is not a non-negative number.""" + if not isinstance(value, (int, float)): + raise TypeError(f'{name} must be a float. Got {type(value)}.') + if value < 0: + raise ValueError(f'{name} must be non-negative. Got {value}.') + + def _build_plot_style_defaults(self, keys: Iterable[str]) -> dict: + """Build default plot style kwargs for the given DataGroup keys.""" + linestyle: dict = {} + marker: dict = {} + color: dict = {} + markerfacecolor: dict = {} + for key in keys: + if key == 'Data': + linestyle[key] = 'none' + marker[key] = 'o' + color[key] = 'black' + markerfacecolor[key] = 'none' + elif key == 'Model': + linestyle[key] = '-' + marker[key] = None + color[key] = 'red' + markerfacecolor[key] = 'none' + elif key == 'Residuals': + linestyle[key] = 'none' + marker[key] = 'o' + color[key] = 'blue' + markerfacecolor[key] = 'none' + else: + linestyle[key] = '--' + marker[key] = None + return { + 'title': self.display_name, + 'linestyle': linestyle, + 'marker': marker, + 'color': color, + 'markerfacecolor': markerfacecolor, + } + ############# # Dunder methods ############# @@ -587,6 +627,6 @@ def __repr__(self) -> str: A string representation of the Analysis. """ return ( - f' {self.__class__.__name__} (display_name={self.display_name}, ' + f'{self.__class__.__name__} (display_name={self.display_name}, ' f'unique_name={self.unique_name})' ) From f8c21b751bcbe74766a2621f32a9fbada64c9301 Mon Sep 17 00:00:00 2001 From: henrikjacobsenfys Date: Thu, 18 Jun 2026 20:51:36 +0200 Subject: [PATCH 2/5] simplify analysis1d --- src/easydynamics/analysis/analysis1d.py | 309 ++++-------------- .../easydynamics/analysis/test_analysis1d.py | 287 +++------------- 2 files changed, 108 insertions(+), 488 deletions(-) diff --git a/src/easydynamics/analysis/analysis1d.py b/src/easydynamics/analysis/analysis1d.py index 66ca8e00..9b0bf203 100644 --- a/src/easydynamics/analysis/analysis1d.py +++ b/src/easydynamics/analysis/analysis1d.py @@ -171,11 +171,17 @@ def _calculate(self, energy: sc.Variable | None = None) -> np.ndarray: The calculated model prediction. """ - sample_intensity = self._evaluate_sample(energy=energy) - - background_intensity = self._evaluate_background(energy=energy) - - return sample_intensity + background_intensity + Q_index = self._require_Q_index() + sample = self._evaluate_with_convolution( + self.sample_model.get_component_collection(Q_index), + energy, + convolver=self._convolver, + ) + background = self._evaluate_direct( + self.instrument_model.background_model.get_component_collection(Q_index), + energy, + ) + return sample + background def fit(self) -> FitResults: """ @@ -496,98 +502,62 @@ def _calculate_energy_with_offset( # Private methods: evaluation ############# - def _evaluate_components( + def _evaluate_with_convolution( self, components: ComponentCollection | ModelComponent, + energy: sc.Variable | None, convolver: Convolution | None = None, - convolve: bool = True, - energy: sc.Variable | None = None, - apply_detailed_balance: bool = False, ) -> np.ndarray: """ - Calculate the contribution of a set of components, optionally convolving with the - resolution. + Evaluate sample components, applying convolution and detailed balance as appropriate. - If convolve is True and a Convolution object is provided (for full model evaluation), we - use it to perform the convolution of the components with the resolution. If convolve is - True but no Convolution object is provided, create a new Convolution object for the given - components (for individual components). If convolve is False, evaluate the components - directly without convolution (for background). + Uses the pre-built convolver when provided (fit path, for performance). If no convolver + is given, creates a temporary one per call (plot path for individual components). Falls + back to direct evaluation with detailed balance if there is no resolution model. Parameters ---------- components : ComponentCollection | ModelComponent - The components to evaluate. + The sample components to evaluate. + energy : sc.Variable | None + Energy grid to use. If None, uses the masked energy from the experiment. convolver : Convolution | None, default=None - An optional Convolution object to use for convolution. If None, a new Convolution - object will be created if convolve is True. - convolve : bool, default=True - Whether to perform convolution with the resolution. - energy : sc.Variable | None, default=None - Optional energy grid to use for evaluation. If None, the energy grid from the - experiment is used. - apply_detailed_balance : bool, default=False - Whether to apply detailed balance correction. - + Pre-built Convolution to reuse. If None, a new one is created if needed. Returns ------- np.ndarray - The evaluated contribution of the components. + The evaluated sample contribution. """ - Q_index = self._require_Q_index() if energy is None: energy = self._masked_energy - energy_offset = self.instrument_model.get_energy_offset(Q_index) - energy_with_offset = self._calculate_energy_with_offset( - energy=energy, - energy_offset=energy_offset, - ) - - # If there are no components, return zero if isinstance(components, ComponentCollection) and components.is_empty: return np.zeros_like(energy.values) - # If a convolver is provided, we use it. This allows reusing the - # same convolver for multiple evaluations during fitting for - # performance reasons. if convolver is not None: return convolver.convolution() - # No convolution can happen for multiple reasons: - # Case 1: convolve=False, used for evaluating background components, where we don't want - # to convolve with the resolution. In this case, apply_detailed_balance is False, - # and we evaluate the components without DBF regardles of the settings - # Case 2: convolve=True but there is no resolution_model. In this case, - # apply_detailed_balance is True. We apply DBF if temperature is provided and - # the settings say to use detailed balance. - + energy_offset = self.instrument_model.get_energy_offset(Q_index) + energy_with_offset = self._calculate_energy_with_offset(energy, energy_offset) resolution = self.instrument_model.resolution_model.get_component_collection(Q_index) - if not convolve or resolution.is_empty: - result_no_convolution = components.evaluate(energy_with_offset) + + if resolution.is_empty: + result = components.evaluate(energy_with_offset) if ( - apply_detailed_balance - and self.temperature is not None + self.temperature is not None and self.detailed_balance_settings.use_detailed_balance ): - DBF = detailed_balance_factor( + result *= detailed_balance_factor( energy=energy_with_offset, temperature=self.temperature, divide_by_temperature=self.detailed_balance_settings.normalize_detailed_balance, energy_unit=self.unit, ) - result_no_convolution *= DBF - return result_no_convolution + return result - # If no convolver is provided, we create a new one. This is for - # evaluating individual components for plotting, where - # performance is not important. We already handled the case of - # background components above, so we know that this is for sample components, - # where detailed balance settings should be applied. - - conv = Convolution( + return Convolution( energy=energy, sample_components=components, resolution_components=resolution, @@ -595,77 +565,22 @@ def _evaluate_components( convolution_settings=self.convolution_settings, temperature=self.temperature, detailed_balance_settings=self.detailed_balance_settings, - ) - return conv.convolution() + ).convolution() - def _evaluate_sample( + def _evaluate_direct( self, - energy: sc.Variable | None = None, - ) -> np.ndarray: - """ - Evaluate the sample contribution for a given Q index. - - Assumes that self._convolver is up to date. - - Parameters - ---------- - energy : sc.Variable | None, default=None - Optional energy grid to use for evaluation. If None, the energy grid from the - experiment is used. - - Returns - ------- - np.ndarray - The evaluated sample contribution. - """ - Q_index = self._require_Q_index() - components = self.sample_model.get_component_collection(Q_index=Q_index) - return self._evaluate_components( - components=components, - convolver=self._convolver, - convolve=True, - energy=energy, - apply_detailed_balance=True, - ) - - def _evaluate_sample_component( - self, - component: ModelComponent, - energy: sc.Variable | None = None, + components: ComponentCollection | ModelComponent, + energy: sc.Variable | None, ) -> np.ndarray: """ - Evaluate a single sample component for the chosen Q index. + Evaluate background components directly — no convolution, no detailed balance factor. Parameters ---------- - component : ModelComponent - The sample component to evaluate. - energy : sc.Variable | None, default=None - Optional energy grid to use for evaluation. If None, the energy grid from the - experiment is used. - - Returns - ------- - np.ndarray - The evaluated sample component contribution. - """ - return self._evaluate_components( - components=component, - convolver=None, - convolve=True, - energy=energy, - apply_detailed_balance=True, - ) - - def _evaluate_background(self, energy: sc.Variable | None = None) -> np.ndarray: - """ - Evaluate the background contribution for the chosen Q index. - - Parameters - ---------- - energy : sc.Variable | None, default=None - Optional energy grid to use for evaluation. If None, the energy grid from the - experiment is used. + components : ComponentCollection | ModelComponent + The background components to evaluate. + energy : sc.Variable | None + Energy grid to use. If None, uses the masked energy from the experiment. Returns ------- @@ -673,46 +588,15 @@ def _evaluate_background(self, energy: sc.Variable | None = None) -> np.ndarray: The evaluated background contribution. """ Q_index = self._require_Q_index() - background_components = self.instrument_model.background_model.get_component_collection( - Q_index=Q_index - ) - return self._evaluate_components( - components=background_components, - convolver=None, - convolve=False, - energy=energy, - apply_detailed_balance=False, - ) - - def _evaluate_background_component( - self, - component: ModelComponent, - energy: sc.Variable | None = None, - ) -> np.ndarray: - """ - Evaluate a single background component for the chosen Q index. - - Parameters - ---------- - component : ModelComponent - The background component to evaluate. - energy : sc.Variable | None, default=None - Optional energy grid to use for evaluation. If None, the energy grid from the - experiment is used. + if energy is None: + energy = self._masked_energy - Returns - ------- - np.ndarray - The evaluated background component contribution. - """ + if isinstance(components, ComponentCollection) and components.is_empty: + return np.zeros_like(energy.values) - return self._evaluate_components( - components=component, - convolver=None, - convolve=False, - energy=energy, - apply_detailed_balance=False, - ) + energy_offset = self.instrument_model.get_energy_offset(Q_index) + energy_with_offset = self._calculate_energy_with_offset(energy, energy_offset) + return components.evaluate(energy_with_offset) def _create_convolver( self, @@ -762,66 +646,6 @@ def _create_convolver( # Private methods: create scipp arrays for plotting ############# - def _create_component_scipp_array( - self, - component: ModelComponent, - background: np.ndarray | None = None, - energy: sc.Variable | None = None, - ) -> sc.DataArray: - """ - Create a scipp DataArray for a single component. - - Adds the background if it is not None. - - Parameters - ---------- - component : ModelComponent - The component to evaluate. - background : np.ndarray | None, default=None - Optional background to add to the component. - energy : sc.Variable | None, default=None - Optional energy grid to use for evaluation. If None, the energy grid from the - experiment is used. - - Returns - ------- - sc.DataArray - The model calculation of the component. - """ - - values = self._evaluate_sample_component(component=component, energy=energy) - if background is not None: - values += background - return self._to_scipp_array(values=values, energy=energy) - - def _create_background_component_scipp_array( - self, - component: ModelComponent, - energy: sc.Variable | None = None, - ) -> sc.DataArray: - """ - Create a scipp DataArray for a single background component. - - Parameters - ---------- - component : ModelComponent - The component to evaluate. - energy : sc.Variable | None, default=None - Optional energy grid to use for evaluation. If None, the energy grid from the - experiment is used. - - Returns - ------- - sc.DataArray - The model calculation of the component. - """ - - values = self._evaluate_background_component( - component=component, - energy=energy, - ) - return self._to_scipp_array(values=values, energy=energy) - def _create_model_array(self, energy: sc.Variable | None = None) -> sc.DataArray: """ Create a scipp DataArray for the full sample model including background. @@ -866,44 +690,47 @@ def _create_components_dataset_single_Q( self, add_background: bool = True, energy: sc.Variable | None = None, - ) -> dict[str, sc.DataArray]: + ) -> sc.Dataset: """ Create sc.DataArrays for all sample and background components. Parameters ---------- add_background : bool, default=True - Whether to add background components. + Whether to add the background to each sample component. energy : sc.Variable | None, default=None Optional energy grid to use for evaluation. If None, the energy grid from the experiment is used. Returns ------- - dict[str, sc.DataArray] - A dictionary of component names to their corresponding sc.DataArrays. + sc.Dataset + A Dataset of component names to their corresponding sc.DataArrays. """ - scipp_arrays = {} - sample_components = self.sample_model.get_component_collection(Q_index=self.Q_index) + Q_index = self.Q_index + if energy is None: + energy = self._masked_energy background_components = self.instrument_model.background_model.get_component_collection( - Q_index=self.Q_index + Q_index=Q_index + ) + background_values = ( + self._evaluate_direct(background_components, energy) if add_background else None ) - if energy is None: - energy = self._masked_energy - - background = self._evaluate_background(energy=energy) if add_background else None + result: dict[str, sc.DataArray] = {} + for component in self.sample_model.get_component_collection(Q_index=Q_index): + values = self._evaluate_with_convolution(component, energy) + if background_values is not None: + values = values + background_values + result[component.display_name] = self._to_scipp_array(values, energy) - for component in sample_components: - scipp_arrays[component.display_name] = self._create_component_scipp_array( - component=component, background=background, energy=energy - ) for component in background_components: - scipp_arrays[component.display_name] = self._create_background_component_scipp_array( - component=component, energy=energy + result[component.display_name] = self._to_scipp_array( + self._evaluate_direct(component, energy), energy ) - return sc.Dataset(scipp_arrays) + + return sc.Dataset(result) def _to_scipp_array( self, diff --git a/tests/unit/easydynamics/analysis/test_analysis1d.py b/tests/unit/easydynamics/analysis/test_analysis1d.py index 9df8815e..c6e1f09b 100644 --- a/tests/unit/easydynamics/analysis/test_analysis1d.py +++ b/tests/unit/easydynamics/analysis/test_analysis1d.py @@ -121,15 +121,15 @@ def test__calculate_adds_sample_and_background(self, analysis1d): sample = np.array([1.0, 2.0, 3.0]) background = np.array([0.5, 0.5, 0.5]) - analysis1d._evaluate_sample = MagicMock(return_value=sample) - analysis1d._evaluate_background = MagicMock(return_value=background) + analysis1d._evaluate_with_convolution = MagicMock(return_value=sample) + analysis1d._evaluate_direct = MagicMock(return_value=background) result = analysis1d._calculate() np.testing.assert_array_equal(result, sample + background) - analysis1d._evaluate_sample.assert_called_once() - analysis1d._evaluate_background.assert_called_once() + analysis1d._evaluate_with_convolution.assert_called_once() + analysis1d._evaluate_direct.assert_called_once() def test_fit_raises_if_no_experiment(self, analysis1d): # WHEN THEN @@ -533,61 +533,59 @@ def test_calculate_energy_with_offset_raises_if_incompatible_units(self, analysi # Private methods: evaluation ############# - def test_evaluate_components_no_components(self, analysis1d): + def test_evaluate_direct_no_components(self, analysis1d): # WHEN components = ComponentCollection() # THEN - result = analysis1d._evaluate_components(components=components) + result = analysis1d._evaluate_direct(components=components, energy=None) # EXPECT assert isinstance(result, np.ndarray) assert result.shape == (len(analysis1d.experiment.energy),) assert np.all(result == pytest.approx(0.0)) - def test_evaluate_components_no_convolution(self, analysis1d): + def test_evaluate_direct(self, analysis1d): # WHEN components = Polynomial(coefficients=[1.0]) + # THEN - result = analysis1d._evaluate_components( - components=components, convolver=None, convolve=False - ) + result = analysis1d._evaluate_direct(components=components, energy=None) + # EXPECT assert np.array_equal(result, np.array([1.0, 1.0, 1.0])) - def test_evaluate_components_convolution(self, analysis1d): + def test_evaluate_with_convolution_uses_convolver(self, analysis1d): # WHEN components = Gaussian() convolver = MagicMock() convolver.convolution = MagicMock(return_value=np.array([1, 2, 3])) # THEN - result = analysis1d._evaluate_components( - components=components, convolver=convolver, convolve=True + result = analysis1d._evaluate_with_convolution( + components=components, energy=None, convolver=convolver ) # EXPECT convolver.convolution.assert_called_once() assert result is convolver.convolution.return_value - def test_evaluate_components_empty_resolution(self, analysis1d): + def test_evaluate_with_convolution_no_resolution(self, analysis1d): # WHEN components = MagicMock() components.evaluate = MagicMock(return_value=np.array([1.0, 2.0, 3.0])) # The default analysis1d has no resolution model components, so - # no convolution should be applied even if convolve=True + # evaluate is called directly even though we want convolution. # THEN - result = analysis1d._evaluate_components( - components=components, convolver=None, convolve=True - ) + result = analysis1d._evaluate_with_convolution(components=components, energy=None) # EXPECT components.evaluate.assert_called_once() assert np.array_equal(result, np.array([1.0, 2.0, 3.0])) - def test_evaluate_components_empty_resolution_DBF(self, analysis1d): + def test_evaluate_with_convolution_no_resolution_DBF(self, analysis1d): # WHEN components = MagicMock() components.evaluate = MagicMock(return_value=np.array([1.0, 2.0, 3.0])) @@ -596,19 +594,16 @@ def test_evaluate_components_empty_resolution_DBF(self, analysis1d): analysis1d.sample_model.temperature = 10 mock_dbf = np.array([10.0, 10.0, 10.0]) - # The default analysis1d has no resolution model components, so - # no convolution should be applied even if convolve=True + # The default analysis1d has no resolution model components with patch( 'easydynamics.analysis.analysis1d.detailed_balance_factor', return_value=mock_dbf, ) as dbf_mock: - # WHEN - result = analysis1d._evaluate_components( + # THEN + result = analysis1d._evaluate_with_convolution( components=components, - convolver=None, - convolve=True, - apply_detailed_balance=True, + energy=None, ) # EXPECT @@ -620,18 +615,13 @@ def test_evaluate_components_empty_resolution_DBF(self, analysis1d): assert np.array_equal(result, expected) def test_evaluate_with_resolution(self, analysis1d): - # WHEN (set up the resolution model and create a component to - # evaluate) + # WHEN (set up the resolution model and create a component to evaluate) analysis1d.instrument_model.resolution_model.components = Gaussian() components = Gaussian() with patch('easydynamics.analysis.analysis1d.Convolution') as MockConvolution: # THEN - analysis1d._evaluate_components( - components=components, - convolver=None, - convolve=True, - ) + analysis1d._evaluate_with_convolution(components=components, energy=None) # EXPECT # Ensure constructor called once @@ -661,98 +651,6 @@ def test_evaluate_with_resolution(self, analysis1d): # and check that convolution() was called MockConvolution.return_value.convolution.assert_called_once_with() - def test_evaluate_sample(self, analysis1d): - # WHEN - analysis1d.sample_model.get_component_collection = MagicMock() - analysis1d._evaluate_components = MagicMock() - - # THEN - analysis1d._evaluate_sample() - - # EXPECT - - # The correct component collection is requested with the correct - # Q_index - analysis1d.sample_model.get_component_collection.assert_called_once_with( - Q_index=analysis1d.Q_index - ) - - # The components are evaluated with the correct convolver and - # convolve=True - analysis1d._evaluate_components.assert_called_once_with( - components=analysis1d.sample_model.get_component_collection(), - convolver=analysis1d._convolver, - convolve=True, - energy=None, - apply_detailed_balance=True, - ) - - def test_evaluate_sample_component(self, analysis1d): - # WHEN - analysis1d._evaluate_components = MagicMock() - component = object() - - # THEN - analysis1d._evaluate_sample_component(component=component) - - # EXPECT - - # The components are evaluated with the correct convolver and - # convolve=True - analysis1d._evaluate_components.assert_called_once_with( - components=component, - convolver=None, - convolve=True, - energy=None, - apply_detailed_balance=True, - ) - - def test_evaluate_background(self, analysis1d): - # WHEN - analysis1d.instrument_model.background_model.get_component_collection = MagicMock() - analysis1d._evaluate_components = MagicMock() - - # THEN - analysis1d._evaluate_background() - - # EXPECT - - # The correct component collection is requested with the correct - # Q_index - analysis1d.instrument_model.background_model.get_component_collection.assert_called_once_with( - Q_index=analysis1d.Q_index - ) - - # The components are evaluated with the correct convolver and - # convolve=True - analysis1d._evaluate_components.assert_called_once_with( - components=analysis1d.instrument_model.background_model.get_component_collection(), - convolver=None, - convolve=False, - energy=None, - apply_detailed_balance=False, - ) - - def test_evaluate_background_component(self, analysis1d): - # WHEN - analysis1d._evaluate_components = MagicMock() - component = object() - - # THEN - analysis1d._evaluate_background_component(component=component) - - # EXPECT - - # The components are evaluated with the correct convolver and - # convolve=True - analysis1d._evaluate_components.assert_called_once_with( - components=component, - convolver=None, - convolve=False, - energy=None, - apply_detailed_balance=False, - ) - def test_create_convolver(self, analysis1d): # WHEN # Mock sample components @@ -843,88 +741,6 @@ def test_create_residuals_array_no_Q_index_raises(self, analysis1d): with pytest.raises(ValueError, match='Q_index must be set'): analysis1d._create_residuals_array() - @pytest.mark.parametrize( - 'background', - [ - None, - np.array([0.5, 0.5, 0.5]), - ], - ids=[ - 'No background', - 'With background', - ], - ) - def test_create_component_scipp_array(self, analysis1d, background): - """ - Test that _create_component_scipp_array correctly evaluates - the component, adds the background and calls _to_scipp_array - with the correct values. - """ - # WHEN - - # Mock the functions that will be called. - analysis1d._evaluate_sample_component = MagicMock(return_value=np.array([1.0, 2.0, 3.0])) - - analysis1d._to_scipp_array = MagicMock() - - component = object() - - # THEN - analysis1d._create_component_scipp_array(component=component, background=background) - - # EXPECT - analysis1d._evaluate_sample_component.assert_called_once_with( - component=component, energy=None - ) - - expected_values = np.array([1.0, 2.0, 3.0]) - if background is not None: - expected_values += background - - analysis1d._to_scipp_array.assert_called_once() - - # Extract the actual call - _, kwargs = analysis1d._to_scipp_array.call_args - - np.testing.assert_array_equal( - kwargs['values'], - expected_values, - ) - - def test_create_background_component_scipp_array(self, analysis1d): - """Test that _create_background_component_scipp_array correctly - evaluates the component, adds the background and calls - _to_scipp_array with the correct values.""" - - # WHEN - - # Mock the functions that will be called. - analysis1d._evaluate_background_component = MagicMock( - return_value=np.array([1.0, 2.0, 3.0]) - ) - analysis1d._to_scipp_array = MagicMock() - - component = object() - - # THEN - analysis1d._create_background_component_scipp_array(component=component) - - # EXPECT - analysis1d._evaluate_background_component.assert_called_once_with( - component=component, - energy=None, - ) - - analysis1d._to_scipp_array.assert_called_once() - - # Extract the actual call - _, kwargs = analysis1d._to_scipp_array.call_args - - np.testing.assert_array_equal( - kwargs['values'], - np.array([1.0, 2.0, 3.0]), - ) - def test_create_model_array(self, analysis1d): """Test that _create_model_array correctly evaluates the full model and calls _to_scipp_array with the @@ -996,65 +812,42 @@ def test_create_components_dataset_single_Q( return_value=background_collection ) - # ---- Background evaluation ---- + # ---- Evaluation mocks ---- background_value = np.array([11.0, 21.0, 31.0]) - analysis1d._evaluate_background = MagicMock(return_value=background_value) - - # ---- Return scipp DataArrays ---- - fake_sample_da = sc.DataArray(data=sc.array(dims=['energy'], values=[1.0, 2.0, 3.0])) + sample_value = np.array([1.0, 2.0, 3.0]) - analysis1d._create_component_scipp_array = MagicMock(return_value=fake_sample_da) + analysis1d._evaluate_direct = MagicMock(return_value=background_value) + analysis1d._evaluate_with_convolution = MagicMock(return_value=sample_value) - fake_background_da = sc.DataArray(data=sc.array(dims=['energy'], values=[4.0, 5.0, 6.0])) - - analysis1d._create_background_component_scipp_array = MagicMock( - return_value=fake_background_da - ) + # ---- Return scipp DataArrays ---- + fake_da = sc.DataArray(data=sc.array(dims=['energy'], values=[1.0, 2.0, 3.0])) + analysis1d._to_scipp_array = MagicMock(return_value=fake_da) # THEN dataset = analysis1d._create_components_dataset_single_Q(add_background=add_background) # EXPECT - # The correct component collections are requested with the - # correct Q_index + # The correct component collections are requested with the correct Q_index analysis1d.sample_model.get_component_collection.assert_called_once_with( Q_index=analysis1d.Q_index ) - analysis1d.instrument_model.background_model.get_component_collection.assert_called_once_with( Q_index=analysis1d.Q_index ) - # Background is evaluated if add_background=True, and not - # evaluated if False + # _evaluate_direct is called once for the background collection (for add_background), + # and once for the background component — if add_background=False only the component call. if add_background: - analysis1d._evaluate_background.assert_called_once() - expected_background = background_value - else: - analysis1d._evaluate_background.assert_not_called() - expected_background = None - - # The sample component scipp array is created with the correct - # component and background - analysis1d._create_component_scipp_array.assert_called_once() - _, kwargs = analysis1d._create_component_scipp_array.call_args - - assert kwargs['component'] is sample_component - - if expected_background is None: - assert kwargs['background'] is None + # First call: background_collection to get total background value + # Second call: background_component for its individual array + assert analysis1d._evaluate_direct.call_count == 2 else: - np.testing.assert_array_equal( - kwargs['background'], - expected_background, - ) + # Only called for the background component + assert analysis1d._evaluate_direct.call_count == 1 - # Background component creation - analysis1d._create_background_component_scipp_array.assert_called_once() - _, kwargs = analysis1d._create_background_component_scipp_array.call_args - assert kwargs['component'] is background_component - assert sc.identical(kwargs['energy'], analysis1d.energy) + # _evaluate_with_convolution called once for the sample component + analysis1d._evaluate_with_convolution.assert_called_once_with(sample_component, analysis1d._masked_energy) # Dataset content assert isinstance(dataset, sc.Dataset) From b23aa1b65af36e7a5a5fb23673124a88aed2d4a9 Mon Sep 17 00:00:00 2001 From: henrikjacobsenfys Date: Thu, 18 Jun 2026 21:27:24 +0200 Subject: [PATCH 3/5] Clean up analysis --- src/easydynamics/analysis/analysis.py | 24 +++++++++++-------- src/easydynamics/analysis/analysis1d.py | 4 ++++ src/easydynamics/analysis/analysis_base.py | 12 +--------- .../sample_model/component_collection.py | 24 +++++++++++++++++++ .../easydynamics/analysis/test_analysis.py | 11 ++++++++- .../analysis/test_analysis_base.py | 4 ++-- .../sample_model/test_component_collection.py | 22 +++++++++++++++++ 7 files changed, 77 insertions(+), 24 deletions(-) diff --git a/src/easydynamics/analysis/analysis.py b/src/easydynamics/analysis/analysis.py index 07931a83..61738d44 100644 --- a/src/easydynamics/analysis/analysis.py +++ b/src/easydynamics/analysis/analysis.py @@ -411,12 +411,12 @@ def parameters_to_dataset(self) -> sc.Dataset: ds = sc.Dataset(coords={'Q': self.Q}) - # Collect all parameter names - all_names = { + # Collect all parameter names in first-seen order + all_names = dict.fromkeys( param.name for analysis in self.analysis_list for param in analysis.get_all_parameters() - } + ) # Storage values = {name: [] for name in all_names} @@ -424,7 +424,15 @@ def parameters_to_dataset(self) -> sc.Dataset: units = {} for analysis in self.analysis_list: - pars = {p.name: p for p in analysis.get_all_parameters()} + all_params = analysis.get_all_parameters() + param_names = [p.name for p in all_params] + if len(param_names) != len(set(param_names)): + dups = sorted({n for n in param_names if param_names.count(n) > 1}) + raise ValueError( + f'Duplicate parameter names at Q_index {analysis.Q_index}: {dups}. ' + 'Rename components so all parameters have unique names.' + ) + pars = {p.name: p for p in all_params} for name in all_names: if name in pars: @@ -611,7 +619,7 @@ def _create_analysis_list(self) -> None: for Q_index in range(len(self.Q)): analysis = Analysis1d( display_name=f'{self.display_name}_Q{Q_index}', - unique_name=(f'{self.unique_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, @@ -641,8 +649,6 @@ def _fit_single_Q(self, Q_index: int) -> FitResults: The results of the fit for the specified Q index. """ - Q_index = self._verify_Q_index(Q_index) - return self.analysis_list[Q_index].fit() def _fit_all_Q_independently(self) -> list[FitResults]: @@ -679,9 +685,7 @@ def _fit_all_Q_simultaneously(self) -> FitResults: ws.append(weight) # Make sure the convolver is up to date for this Q index - analysis1d._convolver = analysis1d._create_convolver( # noqa: SLF001 - energy=x - ) + analysis1d.refresh_convolver(energy=x) mf = MultiFitter( fit_objects=self.analysis_list, diff --git a/src/easydynamics/analysis/analysis1d.py b/src/easydynamics/analysis/analysis1d.py index 9b0bf203..fc8ff00c 100644 --- a/src/easydynamics/analysis/analysis1d.py +++ b/src/easydynamics/analysis/analysis1d.py @@ -424,6 +424,10 @@ def free_energy_offset(self) -> None: """Free the energy offset parameter for the current Q index.""" self.instrument_model.free_energy_offset(Q_index=self._require_Q_index()) + def refresh_convolver(self, energy: sc.Variable | None = None) -> None: + """Refresh the pre-built Convolution object for the current Q index.""" + self._convolver = self._create_convolver(energy=energy) + ############# # Private methods: small utilities ############# diff --git a/src/easydynamics/analysis/analysis_base.py b/src/easydynamics/analysis/analysis_base.py index 9ef46397..df5aadfa 100644 --- a/src/easydynamics/analysis/analysis_base.py +++ b/src/easydynamics/analysis/analysis_base.py @@ -105,17 +105,7 @@ def __init__( 'convolution_settings must be an instance of ConvolutionSettings or None.' ) - if extra_parameters is not None: - if isinstance(extra_parameters, Parameter): - self._extra_parameters = [extra_parameters] - elif isinstance(extra_parameters, list) and all( - isinstance(p, Parameter) for p in extra_parameters - ): - self._extra_parameters = extra_parameters - else: - raise TypeError('extra_parameters must be a Parameter or a list of Parameters.') - else: - self._extra_parameters = [] + self.extra_parameters = extra_parameters if detailed_balance_settings is None: self._detailed_balance_settings = DetailedBalanceSettings() diff --git a/src/easydynamics/sample_model/component_collection.py b/src/easydynamics/sample_model/component_collection.py index f0bbd2d7..302baa3a 100644 --- a/src/easydynamics/sample_model/component_collection.py +++ b/src/easydynamics/sample_model/component_collection.py @@ -106,6 +106,8 @@ def __init__( unique_name=unique_name, ) + self._warn_if_duplicate_names() + # ------------------------------------------------------------------ # Properties # ------------------------------------------------------------------ @@ -196,6 +198,7 @@ def append_component(self, component: ModelComponent | ComponentCollection) -> N self.extend(component) else: self.append(component) + self._warn_if_duplicate_names() def list_component_names(self) -> list[str]: """ @@ -336,6 +339,27 @@ def free_all_parameters(self) -> None: for param in self.get_fittable_parameters(): param.fixed = False + # ------------------------------------------------------------------ + # Private methods + # ------------------------------------------------------------------ + + def _warn_if_duplicate_names(self) -> None: + """Warn if any two components share the same name.""" + names = [c.name for c in self] + seen: set[str] = set() + dups: set[str] = set() + for name in names: + if name in seen: + dups.add(name) + seen.add(name) + if dups: + warnings.warn( + f'Duplicate component names in ComponentCollection: {sorted(dups)}. ' + 'Components with the same name will produce duplicate parameter names.', + UserWarning, + stacklevel=3, + ) + # ------------------------------------------------------------------ # Dunder methods # ------------------------------------------------------------------ diff --git a/tests/unit/easydynamics/analysis/test_analysis.py b/tests/unit/easydynamics/analysis/test_analysis.py index 7e76bef2..d7cdb06c 100644 --- a/tests/unit/easydynamics/analysis/test_analysis.py +++ b/tests/unit/easydynamics/analysis/test_analysis.py @@ -532,6 +532,15 @@ def test_parameters_to_dataset_different_units(self, analysis): assert parameter_name in parameters_dataset assert 'Q' in parameters_dataset[parameter_name].dims + def test_parameters_to_dataset_raises_on_duplicate_names(self, analysis): + # Add a second Gaussian with the same parameter names as the first + analysis.sample_model.append_component( + Gaussian(name='GaussianName', display_name='Gaussian2', area=0.5) + ) + + with pytest.raises(ValueError, match='Duplicate parameter names'): + analysis.parameters_to_dataset() + @pytest.mark.parametrize( 'parameter_names', [ @@ -750,7 +759,7 @@ def test_fit_single_Q_invalid_Q_index(self, analysis): IndexError, match='must be a valid index', ): - analysis._fit_single_Q(Q_index=3) + analysis.fit(Q_index=3) def test_fit_all_Q_independently(self, analysis): # WHEN diff --git a/tests/unit/easydynamics/analysis/test_analysis_base.py b/tests/unit/easydynamics/analysis/test_analysis_base.py index 120e0af0..e9f0426a 100644 --- a/tests/unit/easydynamics/analysis/test_analysis_base.py +++ b/tests/unit/easydynamics/analysis/test_analysis_base.py @@ -140,12 +140,12 @@ def test_init_calls_on_experiment_changed(self): ( {'extra_parameters': 123}, TypeError, - 'extra_parameters must be a Parameter or a list of Parameters.', + 'extra_parameters must be a Parameter, a list of Parameters, or None.', ), ( {'extra_parameters': [123]}, TypeError, - 'extra_parameters must be a Parameter or a list of Parameters.', + 'extra_parameters must be a Parameter, a list of Parameters, or None.', ), ], ids=[ diff --git a/tests/unit/easydynamics/sample_model/test_component_collection.py b/tests/unit/easydynamics/sample_model/test_component_collection.py index 14835ec3..7ae15ed6 100644 --- a/tests/unit/easydynamics/sample_model/test_component_collection.py +++ b/tests/unit/easydynamics/sample_model/test_component_collection.py @@ -465,3 +465,25 @@ def test_copy(self, component_collection): assert param_copy.min == param_orig.min assert param_copy.max == param_orig.max assert param_copy.fixed == param_orig.fixed + + def test_warns_on_duplicate_names_at_init(self): + g1 = Gaussian(name='SameName', display_name='Display1', area=1.0) + g2 = Gaussian(name='SameName', display_name='Display2', area=2.0) + + with pytest.warns(UserWarning, match='Duplicate component names'): + ComponentCollection(components=[g1, g2]) + + def test_warns_on_duplicate_names_on_append(self): + g1 = Gaussian(name='SameName', display_name='Display1', area=1.0) + g2 = Gaussian(name='SameName', display_name='Display2', area=2.0) + collection = ComponentCollection(components=[g1]) + + with pytest.warns(UserWarning, match='Duplicate component names'): + collection.append_component(g2) + + def test_no_warning_with_unique_names(self, recwarn): + g1 = Gaussian(name='Name1', display_name='Display1', area=1.0) + g2 = Gaussian(name='Name2', display_name='Display2', area=2.0) + ComponentCollection(components=[g1, g2]) + user_warnings = [w for w in recwarn.list if issubclass(w.category, UserWarning)] + assert not user_warnings From 2af2ef961efd4e9b83bddddc54131f8dd47ed20b Mon Sep 17 00:00:00 2001 From: henrikjacobsenfys Date: Thu, 18 Jun 2026 21:38:21 +0200 Subject: [PATCH 4/5] improve docstrings --- src/easydynamics/analysis/analysis.py | 12 ++---------- src/easydynamics/analysis/analysis1d.py | 8 +++----- src/easydynamics/analysis/analysis_base.py | 7 ------- tests/unit/easydynamics/analysis/test_analysis1d.py | 4 +++- 4 files changed, 8 insertions(+), 23 deletions(-) diff --git a/src/easydynamics/analysis/analysis.py b/src/easydynamics/analysis/analysis.py index 61738d44..494939ac 100644 --- a/src/easydynamics/analysis/analysis.py +++ b/src/easydynamics/analysis/analysis.py @@ -249,8 +249,6 @@ def plot_data_and_model( values available for plotting. RuntimeError If not in a Jupyter notebook environment. - TypeError - If plot_components or add_background is not True or False. Returns ------- @@ -348,9 +346,6 @@ def data_and_model_to_datagroup( If there is no data to include in the DataGroup, or if there are no Q values available for creating the DataGroup. - TypeError - If add_background is not True or False. If include_components is not True or False. - Returns ------- sc.DataGroup @@ -401,6 +396,8 @@ def parameters_to_dataset(self) -> sc.Dataset: ------ UnitError If there are inconsistent units for the same parameter across different Q values. + ValueError + If duplicate parameter names exist for the same Q index. Returns ------- @@ -762,11 +759,6 @@ def _create_components_dataset( The energy values to use for calculating the components. If None, uses the energy from the experiment. - Raises - ------ - TypeError - If add_background is not True or False. - Returns ------- sc.Dataset diff --git a/src/easydynamics/analysis/analysis1d.py b/src/easydynamics/analysis/analysis1d.py index fc8ff00c..62c6cbf9 100644 --- a/src/easydynamics/analysis/analysis1d.py +++ b/src/easydynamics/analysis/analysis1d.py @@ -366,8 +366,6 @@ def data_and_model_to_datagroup( If no data is available in the experiment to include in the DataGroup. If no Q values are available in the experiment to create the DataGroup. If Q_index is not set to create the DataGroup. - TypeError - If add_background is not a boolean. If include_components is not a boolean. Returns ------- @@ -515,9 +513,9 @@ def _evaluate_with_convolution( """ Evaluate sample components, applying convolution and detailed balance as appropriate. - Uses the pre-built convolver when provided (fit path, for performance). If no convolver - is given, creates a temporary one per call (plot path for individual components). Falls - back to direct evaluation with detailed balance if there is no resolution model. + Uses the pre-built convolver when provided (fit path, for performance). If no convolver is + given, creates a temporary one per call (plot path for individual components). Falls back + to direct evaluation with detailed balance if there is no resolution model. Parameters ---------- diff --git a/src/easydynamics/analysis/analysis_base.py b/src/easydynamics/analysis/analysis_base.py index df5aadfa..060d417b 100644 --- a/src/easydynamics/analysis/analysis_base.py +++ b/src/easydynamics/analysis/analysis_base.py @@ -443,13 +443,6 @@ def get_parameters_near_bounds( ------- list[Parameter] A list of parameters that are near their bounds. - - Raises - ------ - TypeError - If rtol or atol is not a float. - ValueError - If rtol or atol is negative. """ self._verify_nonneg_float(rtol, 'rtol') diff --git a/tests/unit/easydynamics/analysis/test_analysis1d.py b/tests/unit/easydynamics/analysis/test_analysis1d.py index c6e1f09b..3340fc1d 100644 --- a/tests/unit/easydynamics/analysis/test_analysis1d.py +++ b/tests/unit/easydynamics/analysis/test_analysis1d.py @@ -847,7 +847,9 @@ def test_create_components_dataset_single_Q( assert analysis1d._evaluate_direct.call_count == 1 # _evaluate_with_convolution called once for the sample component - analysis1d._evaluate_with_convolution.assert_called_once_with(sample_component, analysis1d._masked_energy) + analysis1d._evaluate_with_convolution.assert_called_once_with( + sample_component, analysis1d._masked_energy + ) # Dataset content assert isinstance(dataset, sc.Dataset) From 08d7421756f06e7505733eb3349f5d020134b054 Mon Sep 17 00:00:00 2001 From: henrikjacobsenfys Date: Thu, 18 Jun 2026 22:15:12 +0200 Subject: [PATCH 5/5] Clean up convolutions --- .../convolution/analytical_convolution.py | 43 ------------- src/easydynamics/convolution/convolution.py | 16 +---- .../convolution/convolution_base.py | 19 ------ .../convolution/numerical_convolution.py | 64 +------------------ .../convolution/numerical_convolution_base.py | 42 ++---------- .../convolution/test_convolution.py | 34 ---------- .../convolution/test_convolution_base.py | 5 +- .../test_numerical_convolution_base.py | 12 +++- 8 files changed, 17 insertions(+), 218 deletions(-) diff --git a/src/easydynamics/convolution/analytical_convolution.py b/src/easydynamics/convolution/analytical_convolution.py index 5ab305cf..def65567 100644 --- a/src/easydynamics/convolution/analytical_convolution.py +++ b/src/easydynamics/convolution/analytical_convolution.py @@ -4,8 +4,6 @@ from typing import ClassVar import numpy as np -import scipp as sc -from easyscience.variable import Parameter from scipy.special import voigt_profile from easydynamics.convolution.convolution_base import ConvolutionBase @@ -15,7 +13,6 @@ from easydynamics.sample_model import Voigt from easydynamics.sample_model.component_collection import ComponentCollection from easydynamics.sample_model.components.model_component import ModelComponent -from easydynamics.utils.utils import Numeric class AnalyticalConvolution(ConvolutionBase): @@ -37,46 +34,6 @@ class AnalyticalConvolution(ConvolutionBase): ('Voigt', 'Voigt'): '_convolute_voigt_voigt', } - def __init__( - self, - energy: np.ndarray | sc.Variable, - unit: str | sc.Unit = 'meV', - sample_components: ComponentCollection | ModelComponent | None = None, - resolution_components: ComponentCollection | ModelComponent | None = None, - energy_offset: Numeric | Parameter = 0.0, - display_name: str | None = 'MyConvolution', - unique_name: str | None = None, - ) -> None: - """ - Initialize an AnalyticalConvolution. - - Parameters - ---------- - energy : np.ndarray | sc.Variable - 1D array of energy values where the convolution is evaluated. - unit : str | sc.Unit, default='meV' - The unit of the energy. - sample_components : ComponentCollection | ModelComponent | None, default=None - The sample model to be convolved. - resolution_components : ComponentCollection | ModelComponent | None, default=None - The resolution model to convolve with. - energy_offset : Numeric | Parameter, default=0.0 - An offset to shift the energy values by. - display_name : str | None, default='MyConvolution' - Display name of the model. - unique_name : str | None, default=None - Unique name of the model. If None, a unique name will be generated. - """ - super().__init__( - energy=energy, - unit=unit, - sample_components=sample_components, - resolution_components=resolution_components, - energy_offset=energy_offset, - display_name=display_name, - unique_name=unique_name, - ) - def convolution( self, ) -> np.ndarray: diff --git a/src/easydynamics/convolution/convolution.py b/src/easydynamics/convolution/convolution.py index 8a11e05f..e6339c5b 100644 --- a/src/easydynamics/convolution/convolution.py +++ b/src/easydynamics/convolution/convolution.py @@ -38,7 +38,6 @@ class Convolution(NumericalConvolutionBase): # When these attributes are changed, the convolution plan # needs to be rebuilt _invalidate_plan_on_change: ClassVar[dict[str, object]] = { - 'energy', '_energy', '_energy_grid', '_sample_components', @@ -180,8 +179,7 @@ def _check_if_pair_is_analytic( Raises ------ TypeError - If either component is not a ModelComponent, or if the resolution component is a - DeltaFunction. + If the resolution component is a DeltaFunction. Returns ------- @@ -189,18 +187,6 @@ def _check_if_pair_is_analytic( True if the component pair can be handled analytically, False otherwise. """ - if not isinstance(sample_component, ModelComponent): - raise TypeError( - f'`sample_component` is an instance of {type(sample_component).__name__}, \ - but must be a ModelComponent.' - ) - - if not isinstance(resolution_component, ModelComponent): - raise TypeError( - f'`resolution_component` is an instance of {type(resolution_component).__name__}, \ - but must be a ModelComponent.' - ) - if isinstance(resolution_component, DeltaFunction): raise TypeError( 'resolution components contains delta functions. This is not supported.' diff --git a/src/easydynamics/convolution/convolution_base.py b/src/easydynamics/convolution/convolution_base.py index dd98d5e9..c8516cb0 100644 --- a/src/easydynamics/convolution/convolution_base.py +++ b/src/easydynamics/convolution/convolution_base.py @@ -151,25 +151,6 @@ def energy_with_offset(self) -> sc.Variable: energy_with_offset.values = self.energy.values - self.energy_offset.value return energy_with_offset - @energy_with_offset.setter - def energy_with_offset(self, _value: sc.Variable) -> None: - """ - Energy with offset is a read-only property derived from energy and energy_offset. - - Parameters - ---------- - _value : sc.Variable - The value to set (ignored). - - Raises - ------ - AttributeError - Always raised since energy_with_offset is read-only. - """ - raise AttributeError( - 'Energy with offset is a read-only property derived from energy and energy_offset.' - ) - @property def energy(self) -> sc.Variable: """ diff --git a/src/easydynamics/convolution/numerical_convolution.py b/src/easydynamics/convolution/numerical_convolution.py index d39550cb..7deac048 100644 --- a/src/easydynamics/convolution/numerical_convolution.py +++ b/src/easydynamics/convolution/numerical_convolution.py @@ -2,17 +2,10 @@ # SPDX-License-Identifier: BSD-3-Clause import numpy as np -import scipp as sc -from easyscience.variable import Parameter from scipy.signal import fftconvolve from easydynamics.convolution.numerical_convolution_base import NumericalConvolutionBase -from easydynamics.sample_model.component_collection import ComponentCollection -from easydynamics.sample_model.components.model_component import ModelComponent -from easydynamics.settings.convolution_settings import ConvolutionSettings -from easydynamics.settings.detailed_balance_settings import DetailedBalanceSettings from easydynamics.utils.detailed_balance import detailed_balance_factor -from easydynamics.utils.utils import Numeric class NumericalConvolution(NumericalConvolutionBase): @@ -24,62 +17,6 @@ class NumericalConvolution(NumericalConvolutionBase): applied to the sample model. """ - def __init__( - self, - energy: np.ndarray | sc.Variable, - sample_components: ComponentCollection | ModelComponent, - resolution_components: ComponentCollection | ModelComponent, - energy_offset: Numeric | Parameter = 0.0, - convolution_settings: ConvolutionSettings | None = None, - temperature: Parameter | Numeric | None = None, - temperature_unit: str | sc.Unit = 'K', - detailed_balance_settings: DetailedBalanceSettings | None = None, - unit: str | sc.Unit = 'meV', - display_name: str | None = 'MyConvolution', - unique_name: str | None = None, - ) -> None: - """ - Initialize the NumericalConvolution object. - - Parameters - ---------- - energy : np.ndarray | sc.Variable - 1D array of energy values where the convolution is evaluated. - sample_components : ComponentCollection | ModelComponent - The sample model to be convolved. - resolution_components : ComponentCollection | ModelComponent - The resolution model to convolve with. - energy_offset : Numeric | Parameter, default=0.0 - An energy offset to apply to the energy values before convolution. - convolution_settings : ConvolutionSettings | None, default=None - The settings for the convolution. - temperature : Parameter | Numeric | None, default=None - The temperature to use for detailed balance correction. - temperature_unit : str | sc.Unit, default='K' - The unit of the temperature parameter. - detailed_balance_settings : DetailedBalanceSettings | None, default=None - The settings for detailed balance. If None, default settings will be used. - unit : str | sc.Unit, default='meV' - The unit of the energy. - display_name : str | None, default='MyConvolution' - Display name of the model. - unique_name : str | None, default=None - Unique name of the model. If None, a unique name will be generated. - """ - super().__init__( - energy=energy, - sample_components=sample_components, - resolution_components=resolution_components, - energy_offset=energy_offset, - convolution_settings=convolution_settings, - temperature=temperature, - temperature_unit=temperature_unit, - detailed_balance_settings=detailed_balance_settings, - unit=unit, - display_name=display_name, - unique_name=unique_name, - ) - def convolution( self, ) -> np.ndarray: @@ -96,6 +33,7 @@ def convolution( # settings before convolution. if not self.convolution_settings.convolution_plan_is_valid: self._energy_grid = self._create_energy_grid() + self.convolution_settings.convolution_plan_is_valid = True # Give warnings if peaks are very wide or very narrow if not self.convolution_settings.suppress_warnings: diff --git a/src/easydynamics/convolution/numerical_convolution_base.py b/src/easydynamics/convolution/numerical_convolution_base.py index 9a6e80cd..da17cae8 100644 --- a/src/easydynamics/convolution/numerical_convolution_base.py +++ b/src/easydynamics/convolution/numerical_convolution_base.py @@ -116,6 +116,7 @@ def __init__( # When upsample_factor>1, we evaluate on this grid and # interpolate back to the original values at the end self._energy_grid = self._create_energy_grid() + self._convolution_settings.convolution_plan_is_valid = True @property def convolution_settings(self) -> ConvolutionSettings: @@ -179,30 +180,13 @@ def upsample_factor(self) -> Numeric | None: @upsample_factor.setter def upsample_factor(self, factor: Numeric | None) -> None: """ - Set the upsample factor and recreate the dense grid. + Set the upsample factor. Parameters ---------- factor : Numeric | None The new upsample factor. - - Raises - ------ - TypeError - If factor is not a number or None. - ValueError - If factor is not greater than 1. """ - if factor is None: - self.convolution_settings.upsample_factor = factor - return - - if not isinstance(factor, Numeric): - raise TypeError('Upsample factor must be a numerical value or None.') - factor = float(factor) - if factor <= 1.0: - raise ValueError('Upsample factor must be greater than 1.') - self.convolution_settings.upsample_factor = factor @property @@ -224,7 +208,7 @@ def extension_factor(self) -> float: @extension_factor.setter def extension_factor(self, factor: Numeric) -> None: """ - Set the extension factor and recreate the dense grid. + Set the extension factor. The extension factor determines how much the energy range is extended on both sides before convolution. 0.2 means extending by 20% of the original energy span on each side. @@ -233,21 +217,8 @@ def extension_factor(self, factor: Numeric) -> None: ---------- factor : Numeric The new extension factor. - - Raises - ------ - TypeError - If factor is not a number. - ValueError - If factor is negative. """ - - if not isinstance(factor, Numeric): - raise TypeError('Extension factor must be a number.') - if factor < 0.0: - raise ValueError('Extension factor must be non-negative.') - - self.convolution_settings.extension_factor = float(factor) + self.convolution_settings.extension_factor = factor @property def temperature(self) -> Parameter | None: @@ -397,16 +368,13 @@ def _create_energy_grid( else: energy_dense_centered = energy_dense - energy_grid = EnergyGrid( + return EnergyGrid( energy_dense=energy_dense, energy_dense_centered=energy_dense_centered, energy_dense_step=energy_dense_step, energy_span_dense=energy_span_dense, energy_even_length_offset=energy_even_length_offset, ) - self._energy_grid = energy_grid - self.convolution_settings.convolution_plan_is_valid = True - return energy_grid def _check_width_thresholds( self, diff --git a/tests/unit/easydynamics/convolution/test_convolution.py b/tests/unit/easydynamics/convolution/test_convolution.py index 59191f7c..dbb4290d 100644 --- a/tests/unit/easydynamics/convolution/test_convolution.py +++ b/tests/unit/easydynamics/convolution/test_convolution.py @@ -391,40 +391,6 @@ def test_check_if_pair_is_analytic_raises_with_delta_in_resolution(self, default resolution_component=resolution_component, ) - @pytest.mark.parametrize( - 'sample_component,resolution_component', - [ - ( - 'NotAModelComponent', - Gaussian(name='G', area=1.0, center=0.0, width=0.1), - ), - ( - Gaussian(name='G', area=1.0, center=0.0, width=0.1), - 'NotAModelComponent', - ), - ], - ids=['invalid_sample_component', 'invalid_resolution_component'], - ) - def test_check_if_pair_is_analytic_raises_with_invalid_types( - self, default_convolution, sample_component, resolution_component - ): - """ - Test that _check_if_pair_is_analytic raises TypeError when given - invalid component types. - """ - # WHEN - conv = default_convolution - - # THEN EXPECT - with pytest.raises( - TypeError, - match='must be a ModelComponent', - ): - conv._check_if_pair_is_analytic( - sample_component=sample_component, - resolution_component=resolution_component, - ) - @pytest.mark.parametrize( 'analytical_component', [True, False], diff --git a/tests/unit/easydynamics/convolution/test_convolution_base.py b/tests/unit/easydynamics/convolution/test_convolution_base.py index a300890b..fe8ceef6 100644 --- a/tests/unit/easydynamics/convolution/test_convolution_base.py +++ b/tests/unit/easydynamics/convolution/test_convolution_base.py @@ -252,10 +252,7 @@ def test_energy_offset_setter_invalid_type_raises(self, convolution_base): def test_energy_with_offset_setter_raises(self, convolution_base): # WHEN THEN EXPECT - with pytest.raises( - AttributeError, - match='is a read-only property', - ): + with pytest.raises(AttributeError): convolution_base.energy_with_offset = 5 def test_sample_components_property(self, convolution_base): diff --git a/tests/unit/easydynamics/convolution/test_numerical_convolution_base.py b/tests/unit/easydynamics/convolution/test_numerical_convolution_base.py index a084b898..a40e4388 100644 --- a/tests/unit/easydynamics/convolution/test_numerical_convolution_base.py +++ b/tests/unit/easydynamics/convolution/test_numerical_convolution_base.py @@ -169,7 +169,9 @@ def test_energy_setter(self, default_numerical_convolution_base): # THEN # Force regeneration of energy grid - default_numerical_convolution_base._create_energy_grid() + default_numerical_convolution_base._energy_grid = ( + default_numerical_convolution_base._create_energy_grid() + ) # EXPECT assert default_numerical_convolution_base._energy_grid.energy_dense.shape[0] == round( @@ -204,7 +206,9 @@ def test_upsample_factor_setter( ) # Force regeneration of energy grid - default_numerical_convolution_base._create_energy_grid() + default_numerical_convolution_base._energy_grid = ( + default_numerical_convolution_base._create_energy_grid() + ) # EXPECT: correct factor + grid size assert default_numerical_convolution_base.upsample_factor == new_upsample_factor @@ -257,7 +261,9 @@ def test_extension_factor_setter(self, default_numerical_convolution_base): # THEN # Force regeneration of energy grid - default_numerical_convolution_base._create_energy_grid() + default_numerical_convolution_base._energy_grid = ( + default_numerical_convolution_base._create_energy_grid() + ) # EXPECT assert default_numerical_convolution_base.extension_factor == new_extension_factor