From 293d54030863eed8cdf50201f0eae39afc7f473b Mon Sep 17 00:00:00 2001 From: henrikjacobsenfys Date: Tue, 7 Apr 2026 15:23:31 +0200 Subject: [PATCH 1/3] Allow transposing axes when using Experiment.plot_data --- docs/docs/tutorials/tutorial1_brownian.ipynb | 6 +- pixi.lock | 4 +- pyproject.toml | 3 +- src/easydynamics/experiment/experiment.py | 116 +++++++++++------- .../experiment/test_experiment.py | 33 ++++- 5 files changed, 111 insertions(+), 51 deletions(-) diff --git a/docs/docs/tutorials/tutorial1_brownian.ipynb b/docs/docs/tutorials/tutorial1_brownian.ipynb index 5562adde..b09546b7 100644 --- a/docs/docs/tutorials/tutorial1_brownian.ipynb +++ b/docs/docs/tutorials/tutorial1_brownian.ipynb @@ -76,7 +76,9 @@ "source": [ "We can visualize the data in multiple ways, relying on plopp: https://scipp.github.io/plopp/\n", "\n", - "We here show two ways to look at the data: as a 2d colormap with intensity as function of `Q` and `energy`, and as a slicer with intensity as function of `energy` for various `Q`." + "We here show two ways to look at the data: as a 2d colormap with intensity as function of `Q` and `energy`, and as a slicer with intensity as function of `energy` for various `Q`.\n", + "\n", + "If you want $Q$ on the x axis, then set `transpose_axes=True`" ] }, { @@ -86,7 +88,7 @@ "metadata": {}, "outputs": [], "source": [ - "vanadium_experiment.plot_data(slicer=False)" + "vanadium_experiment.plot_data(slicer=False, transpose_axes=False)" ] }, { diff --git a/pixi.lock b/pixi.lock index 1665d1a6..44c70390 100644 --- a/pixi.lock +++ b/pixi.lock @@ -4071,8 +4071,8 @@ packages: requires_python: '>=3.5' - pypi: ./ name: easydynamics - version: 0.4.0+devdirty10 - sha256: bd1d44f7263fe45e52e8b62d2740c303be86c7bcc89e3cbec95ec663568953b1 + version: 0.4.0+devdirty2 + sha256: aa4f4851802d4abba9dca0112aa3c99739e1c43425b48d91030f822baa577257 requires_dist: - darkdetect - easyscience diff --git a/pyproject.toml b/pyproject.toml index 49f35979..954af4af 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -256,8 +256,7 @@ select = [ # Ignore specific rules globally ignore = [ 'COM812', # https://docs.astral.sh/ruff/rules/missing-trailing-comma/ - # The following is replaced by 'D'/[tool.ruff.lint.pydocstyle] and [tool.pydoclint] - 'DOC', # https://docs.astral.sh/ruff/rules/#pydoclint-doc + # The following is replaced by 'D'/[tool.ruff.lint.pydocstyle] and [tool.pydoclint] 'DOC', # https://docs.astral.sh/ruff/rules/#pydoclint-doc # Disable, as [tool.format_docstring] split one-line docstrings into the canonical multi-line layout 'D200', # https://docs.astral.sh/ruff/rules/unnecessary-multiline-docstring/ ] diff --git a/src/easydynamics/experiment/experiment.py b/src/easydynamics/experiment/experiment.py index 383f6c06..66ee0bfc 100644 --- a/src/easydynamics/experiment/experiment.py +++ b/src/easydynamics/experiment/experiment.py @@ -23,7 +23,7 @@ class Experiment(NewBase): def __init__( self, - display_name: str | None = 'MyExperiment', + display_name: str | None = "MyExperiment", unique_name: str | None = None, data: sc.DataArray | str | None = None, ) -> None: @@ -32,7 +32,7 @@ def __init__( Parameters ---------- - display_name : str | None, default='MyExperiment' + display_name : str | None, default="MyExperiment" Display name of the experiment. unique_name : str | None, default=None Unique name of the experiment. If None, a unique name will be generated. None. @@ -59,7 +59,7 @@ def __init__( self._data = data else: raise TypeError( - f'Data must be a sc.DataArray or a filename string, not {type(data).__name__}' + f"Data must be a sc.DataArray or a filename string, not {type(data).__name__}" ) self._binned_data = ( @@ -98,7 +98,7 @@ def data(self, value: sc.DataArray) -> None: If the value is not a sc.DataArray. """ if not isinstance(value, sc.DataArray): - raise TypeError(f'Data must be a sc.DataArray, not {type(value).__name__}') + raise TypeError(f"Data must be a sc.DataArray, not {type(value).__name__}") self._validate_coordinates(value) self._data = value self._binned_data = ( @@ -134,7 +134,9 @@ def binned_data(self, _value: sc.DataArray) -> None: AttributeError Always, since binned_data is read-only. """ - raise AttributeError('binned_data is a read-only property. Use rebin() to rebin the data') + raise AttributeError( + "binned_data is a read-only property. Use rebin() to rebin the data" + ) @property def Q(self) -> sc.Variable | None: @@ -148,7 +150,7 @@ def Q(self) -> sc.Variable | None: """ if self._binned_data is None: return None - return self._binned_data.coords['Q'] + return self._binned_data.coords["Q"] @Q.setter def Q(self, _value: sc.Variable) -> None: @@ -167,7 +169,7 @@ def Q(self, _value: sc.Variable) -> None: AttributeError Always, since Q is read-only. """ - raise AttributeError('Q is a read-only property derived from the data.') + raise AttributeError("Q is a read-only property derived from the data.") @property def energy(self) -> sc.Variable | None: @@ -181,7 +183,7 @@ def energy(self) -> sc.Variable | None: """ if self._binned_data is None: return None - return self._binned_data.coords['energy'] + return self._binned_data.coords["energy"] @energy.setter def energy(self, _value: sc.Variable) -> None: @@ -200,7 +202,7 @@ def energy(self, _value: sc.Variable) -> None: AttributeError Always, since energy is read-only. """ - raise AttributeError('energy is a read-only property derived from the data.') + raise AttributeError("energy is a read-only property derived from the data.") def get_masked_energy(self, Q_index: int) -> sc.Variable | None: """ @@ -230,12 +232,12 @@ def get_masked_energy(self, Q_index: int) -> sc.Variable | None: or Q_index < 0 or (self.Q is not None and Q_index >= len(self.Q)) ): - raise IndexError('Q_index must be a valid index for the Q values.') + raise IndexError("Q_index must be a valid index for the Q values.") - energy = self._binned_data.coords['energy'] + energy = self._binned_data.coords["energy"] _, _, _, mask = self._extract_x_y_weights_only_finite(Q_index=Q_index) - mask_var = sc.array(dims=['energy'], values=mask) + mask_var = sc.array(dims=["energy"], values=mask) return energy[mask_var] ########### @@ -260,19 +262,19 @@ def load_hdf5(self, filename: str, display_name: str | None = None) -> None: data is not a sc.DataArray. """ if not isinstance(filename, str): - raise TypeError(f'Filename must be a string, not {type(filename).__name__}') + raise TypeError(f"Filename must be a string, not {type(filename).__name__}") if display_name is not None: if not isinstance(display_name, str): raise TypeError( - f'Display name must be a string, not {type(display_name).__name__}' + f"Display name must be a string, not {type(display_name).__name__}" ) self.display_name = display_name loaded_data = sc_load_hdf5(filename) if not isinstance(loaded_data, sc.DataArray): raise TypeError( - f'Loaded data must be a sc.DataArray, not {type(loaded_data).__name__}' + f"Loaded data must be a sc.DataArray, not {type(loaded_data).__name__}" ) self._validate_coordinates(loaded_data) self.data = loaded_data @@ -296,13 +298,13 @@ def save_hdf5(self, filename: str | None = None) -> None: """ if filename is None: - filename = f'{self.unique_name}.h5' + filename = f"{self.unique_name}.h5" if not isinstance(filename, str): - raise TypeError(f'Filename must be a string, not {type(filename).__name__}') + raise TypeError(f"Filename must be a string, not {type(filename).__name__}") if self._data is None: - raise ValueError('No data to save.') + raise ValueError("No data to save.") path = Path(filename) path.parent.mkdir(exist_ok=True, parents=True) @@ -335,31 +337,33 @@ def rebin(self, dimensions: dict[str, int | sc.Variable]) -> None: if not isinstance(dimensions, dict): raise TypeError( - 'dimensions must be a dictionary mapping dimension names ' - 'to number of bins or bin values as sc.Variable.' + "dimensions must be a dictionary mapping dimension names " + "to number of bins or bin values as sc.Variable." ) if self._data is None: - raise ValueError('No data to rebin. Please load data first.') + raise ValueError("No data to rebin. Please load data first.") binned_data = self._data.copy() dim_copy = dimensions.copy() for dim, value in dim_copy.items(): if not isinstance(dim, str): raise TypeError( - f'Dimension keys must be strings. Got {type(dim)} for {dim} instead.' + f"Dimension keys must be strings. Got {type(dim)} for {dim} instead." ) if dim not in self._data.dims: raise KeyError( f"Dimension '{dim}' not a valid dimension for rebinning. " - f'Should be one of {self._data.dims}.' + f"Should be one of {self._data.dims}." ) - if isinstance(value, float) and value.is_integer(): # I allow eg. 2.0 as well as 2 + if ( + isinstance(value, float) and value.is_integer() + ): # I allow eg. 2.0 as well as 2 value = int(value) # This line can be removed when scipp resize support # resizing with coordinates dimensions[dim] = value if not (isinstance(value, (int, sc.Variable))): raise TypeError( - f'Dimension values must be integers or sc.Variable. ' + f"Dimension values must be integers or sc.Variable. " f"Got {type(value)} for dimension '{dim}' instead." ) binned_data = binned_data.bin({dim: value}) @@ -372,7 +376,12 @@ def rebin(self, dimensions: dict[str, int | sc.Variable]) -> None: # other methods ########### - def plot_data(self, slicer: bool = False, **kwargs: dict) -> None: + def plot_data( + self, + slicer: bool = False, + transpose_axes: bool = False, + **kwargs: dict, + ) -> None: """ Plot the dataset using plopp: https://scipp.github.io/plopp/. @@ -380,6 +389,9 @@ def plot_data(self, slicer: bool = False, **kwargs: dict) -> None: ---------- slicer : bool, default=False If True, use plopp's slicer instead of plot. + transpose_axes : bool, default=False + If True, transpose the data to have dimensions in the order (energy, Q) before + plotting. **kwargs : dict Additional keyword arguments to pass to plopp. @@ -389,20 +401,34 @@ def plot_data(self, slicer: bool = False, **kwargs: dict) -> None: If there is no data to plot. RuntimeError If not in a Jupyter notebook environment. + TypeError + If slicer or transpose_axes are not True or False. """ if self._binned_data is None: - raise ValueError('No data to plot. Please load data first.') + raise ValueError("No data to plot. Please load data first.") if not _in_notebook(): - raise RuntimeError('plot_data() can only be used in a Jupyter notebook environment.') + raise RuntimeError( + "plot_data() can only be used in a Jupyter notebook environment." + ) + + if not isinstance(slicer, bool): + raise TypeError( + f"slicer must be True or False, not {type(slicer).__name__}" + ) + + if not isinstance(transpose_axes, bool): + raise TypeError( + f"transpose_axes must be True or False, not {type(transpose_axes).__name__}" + ) plot_kwargs_defaults = { - 'title': self.display_name, + "title": self.display_name, } if slicer: - plot_kwargs_defaults['keep'] = 'energy' + plot_kwargs_defaults["keep"] = "energy" # Overwrite defaults with any user-provided kwargs plot_kwargs_defaults.update(kwargs) @@ -412,11 +438,15 @@ def plot_data(self, slicer: bool = False, **kwargs: dict) -> None: **plot_kwargs_defaults, ) for widget in fig.bottom_bar[0].controls.values(): - widget.slider_toggler.value = '-o-' + widget.slider_toggler.value = "-o-" else: + if transpose_axes: + data_to_plot = self.binned_data.transpose(dims=["energy", "Q"]) + else: + data_to_plot = self.binned_data.transpose(dims=["Q", "energy"]) fig = pp.plot( - self._binned_data.transpose(dims=['energy', 'Q']), + data_to_plot, **plot_kwargs_defaults, ) return fig @@ -443,9 +473,9 @@ def _validate_coordinates(data: sc.DataArray) -> None: If required coordinates are missing. """ if not isinstance(data, sc.DataArray): - raise TypeError('Data must be a sc.DataArray.') + raise TypeError("Data must be a sc.DataArray.") - required_coords = ['Q', 'energy'] + required_coords = ["Q", "energy"] for coord in required_coords: if coord not in data.coords: raise ValueError(f"Data is missing required coordinate: '{coord}'") @@ -471,7 +501,9 @@ def _convert_to_bin_centers(self, data: sc.DataArray) -> sc.DataArray: data = data.assign_coords({dim: sc.midpoints(coord)}) return data - def _extract_x_y_var(self, Q_index: int) -> tuple[np.ndarray, np.ndarray, np.ndarray]: + def _extract_x_y_var( + self, Q_index: int + ) -> tuple[np.ndarray, np.ndarray, np.ndarray]: """ Extract the x, y, and weights arrays from the experiment for the given Q index. @@ -485,8 +517,8 @@ def _extract_x_y_var(self, Q_index: int) -> tuple[np.ndarray, np.ndarray, np.nda tuple[np.ndarray, np.ndarray, np.ndarray] The x, y, and variances arrays extracted from the experiment for the given Q index. """ - data = self.binned_data['Q', Q_index] - x = data.coords['energy'].values + data = self.binned_data["Q", Q_index] + x = data.coords["energy"].values y = data.values var = data.variances return x, y, var @@ -530,7 +562,7 @@ def _extract_x_y_weights_only_finite( var = var[mask] if np.any(var == 0): - raise ValueError('Cannot compute weights: some variances are zero.') + raise ValueError("Cannot compute weights: some variances are zero.") weights = 1.0 / var**0.5 @@ -550,18 +582,18 @@ def __repr__(self) -> str: A string representation of the Experiment object. """ - return f'Experiment `{self.unique_name}` with data: {self._data}' + return f"Experiment `{self.unique_name}` with data: {self._data}" - def __copy__(self) -> 'Experiment': + def __copy__(self) -> "Experiment": """ Return a copy of the object. Returns ------- - 'Experiment' + "Experiment" A copy of the Experiment object. """ - temp = self.to_dict(skip=['unique_name']) + temp = self.to_dict(skip=["unique_name"]) new_obj = self.__class__.from_dict(temp) new_obj.data = self.data.copy() if self.data is not None else None return new_obj diff --git a/tests/unit/easydynamics/experiment/test_experiment.py b/tests/unit/easydynamics/experiment/test_experiment.py index 4568580e..8430d13f 100644 --- a/tests/unit/easydynamics/experiment/test_experiment.py +++ b/tests/unit/easydynamics/experiment/test_experiment.py @@ -332,7 +332,15 @@ def test_get_masked_energy_invalid_Q_index_raises(self, experiment_with_data, Q_ # test plotting ############## - def test_plot_data_success(self, experiment): + @pytest.mark.parametrize( + 'transpose_axes, expected_dims', + [ + (False, ['Q', 'energy']), + (True, ['energy', 'Q']), + ], + ids=['no_transpose', 'transpose'], + ) + def test_plot_data_success(self, experiment, transpose_axes, expected_dims): "Test plotting data successfully when in notebook environment" # WHEN with ( @@ -343,12 +351,12 @@ def test_plot_data_success(self, experiment): mock_plot.return_value = mock_fig # THEN - result = experiment.plot_data() + result = experiment.plot_data(transpose_axes=transpose_axes) # EXPECT mock_plot.assert_called_once() args, kwargs = mock_plot.call_args - assert sc.identical(args[0], experiment.data.transpose()) + assert sc.identical(args[0], experiment.data.transpose(dims=expected_dims)) assert kwargs['title'] == f'{experiment.display_name}' assert result == mock_fig @@ -395,6 +403,25 @@ def test_plot_data_not_in_notebook_raises(self, experiment): ): experiment.plot_data() + def test_plot_data_invalid_slicer_type_raises(self, experiment): + "Test plotting data raises TypeError when slicer argument is invalid" + # WHEN THEN EXPECT + + with ( + patch(f'{Experiment.__module__}._in_notebook', return_value=True), + pytest.raises(TypeError, match='slicer must be True or False'), + ): + experiment.plot_data(slicer='not_a_boolean') + + def test_plot_data_invalid_transpose_type_raises(self, experiment): + "Test plotting data raises TypeError when transpose argument is invalid" + # WHEN THEN EXPECT + with ( + patch(f'{Experiment.__module__}._in_notebook', return_value=True), + pytest.raises(TypeError, match='transpose_axes must be True or False'), + ): + experiment.plot_data(transpose_axes='not_a_boolean') + ############## # test private methods ############## From f1e8d93d3c7532e86b162ca8117893a2f2db3af9 Mon Sep 17 00:00:00 2001 From: henrikjacobsenfys Date: Tue, 7 Apr 2026 15:25:24 +0200 Subject: [PATCH 2/3] formatting --- src/easydynamics/experiment/experiment.py | 94 ++++++++++------------- 1 file changed, 42 insertions(+), 52 deletions(-) diff --git a/src/easydynamics/experiment/experiment.py b/src/easydynamics/experiment/experiment.py index 66ee0bfc..db714942 100644 --- a/src/easydynamics/experiment/experiment.py +++ b/src/easydynamics/experiment/experiment.py @@ -23,7 +23,7 @@ class Experiment(NewBase): def __init__( self, - display_name: str | None = "MyExperiment", + display_name: str | None = 'MyExperiment', unique_name: str | None = None, data: sc.DataArray | str | None = None, ) -> None: @@ -59,7 +59,7 @@ def __init__( self._data = data else: raise TypeError( - f"Data must be a sc.DataArray or a filename string, not {type(data).__name__}" + f'Data must be a sc.DataArray or a filename string, not {type(data).__name__}' ) self._binned_data = ( @@ -98,7 +98,7 @@ def data(self, value: sc.DataArray) -> None: If the value is not a sc.DataArray. """ if not isinstance(value, sc.DataArray): - raise TypeError(f"Data must be a sc.DataArray, not {type(value).__name__}") + raise TypeError(f'Data must be a sc.DataArray, not {type(value).__name__}') self._validate_coordinates(value) self._data = value self._binned_data = ( @@ -134,9 +134,7 @@ def binned_data(self, _value: sc.DataArray) -> None: AttributeError Always, since binned_data is read-only. """ - raise AttributeError( - "binned_data is a read-only property. Use rebin() to rebin the data" - ) + raise AttributeError('binned_data is a read-only property. Use rebin() to rebin the data') @property def Q(self) -> sc.Variable | None: @@ -150,7 +148,7 @@ def Q(self) -> sc.Variable | None: """ if self._binned_data is None: return None - return self._binned_data.coords["Q"] + return self._binned_data.coords['Q'] @Q.setter def Q(self, _value: sc.Variable) -> None: @@ -169,7 +167,7 @@ def Q(self, _value: sc.Variable) -> None: AttributeError Always, since Q is read-only. """ - raise AttributeError("Q is a read-only property derived from the data.") + raise AttributeError('Q is a read-only property derived from the data.') @property def energy(self) -> sc.Variable | None: @@ -183,7 +181,7 @@ def energy(self) -> sc.Variable | None: """ if self._binned_data is None: return None - return self._binned_data.coords["energy"] + return self._binned_data.coords['energy'] @energy.setter def energy(self, _value: sc.Variable) -> None: @@ -202,7 +200,7 @@ def energy(self, _value: sc.Variable) -> None: AttributeError Always, since energy is read-only. """ - raise AttributeError("energy is a read-only property derived from the data.") + raise AttributeError('energy is a read-only property derived from the data.') def get_masked_energy(self, Q_index: int) -> sc.Variable | None: """ @@ -232,12 +230,12 @@ def get_masked_energy(self, Q_index: int) -> sc.Variable | None: or Q_index < 0 or (self.Q is not None and Q_index >= len(self.Q)) ): - raise IndexError("Q_index must be a valid index for the Q values.") + raise IndexError('Q_index must be a valid index for the Q values.') - energy = self._binned_data.coords["energy"] + energy = self._binned_data.coords['energy'] _, _, _, mask = self._extract_x_y_weights_only_finite(Q_index=Q_index) - mask_var = sc.array(dims=["energy"], values=mask) + mask_var = sc.array(dims=['energy'], values=mask) return energy[mask_var] ########### @@ -262,19 +260,19 @@ def load_hdf5(self, filename: str, display_name: str | None = None) -> None: data is not a sc.DataArray. """ if not isinstance(filename, str): - raise TypeError(f"Filename must be a string, not {type(filename).__name__}") + raise TypeError(f'Filename must be a string, not {type(filename).__name__}') if display_name is not None: if not isinstance(display_name, str): raise TypeError( - f"Display name must be a string, not {type(display_name).__name__}" + f'Display name must be a string, not {type(display_name).__name__}' ) self.display_name = display_name loaded_data = sc_load_hdf5(filename) if not isinstance(loaded_data, sc.DataArray): raise TypeError( - f"Loaded data must be a sc.DataArray, not {type(loaded_data).__name__}" + f'Loaded data must be a sc.DataArray, not {type(loaded_data).__name__}' ) self._validate_coordinates(loaded_data) self.data = loaded_data @@ -298,13 +296,13 @@ def save_hdf5(self, filename: str | None = None) -> None: """ if filename is None: - filename = f"{self.unique_name}.h5" + filename = f'{self.unique_name}.h5' if not isinstance(filename, str): - raise TypeError(f"Filename must be a string, not {type(filename).__name__}") + raise TypeError(f'Filename must be a string, not {type(filename).__name__}') if self._data is None: - raise ValueError("No data to save.") + raise ValueError('No data to save.') path = Path(filename) path.parent.mkdir(exist_ok=True, parents=True) @@ -337,33 +335,31 @@ def rebin(self, dimensions: dict[str, int | sc.Variable]) -> None: if not isinstance(dimensions, dict): raise TypeError( - "dimensions must be a dictionary mapping dimension names " - "to number of bins or bin values as sc.Variable." + 'dimensions must be a dictionary mapping dimension names ' + 'to number of bins or bin values as sc.Variable.' ) if self._data is None: - raise ValueError("No data to rebin. Please load data first.") + raise ValueError('No data to rebin. Please load data first.') binned_data = self._data.copy() dim_copy = dimensions.copy() for dim, value in dim_copy.items(): if not isinstance(dim, str): raise TypeError( - f"Dimension keys must be strings. Got {type(dim)} for {dim} instead." + f'Dimension keys must be strings. Got {type(dim)} for {dim} instead.' ) if dim not in self._data.dims: raise KeyError( f"Dimension '{dim}' not a valid dimension for rebinning. " - f"Should be one of {self._data.dims}." + f'Should be one of {self._data.dims}.' ) - if ( - isinstance(value, float) and value.is_integer() - ): # I allow eg. 2.0 as well as 2 + if isinstance(value, float) and value.is_integer(): # I allow eg. 2.0 as well as 2 value = int(value) # This line can be removed when scipp resize support # resizing with coordinates dimensions[dim] = value if not (isinstance(value, (int, sc.Variable))): raise TypeError( - f"Dimension values must be integers or sc.Variable. " + f'Dimension values must be integers or sc.Variable. ' f"Got {type(value)} for dimension '{dim}' instead." ) binned_data = binned_data.bin({dim: value}) @@ -406,29 +402,25 @@ def plot_data( """ if self._binned_data is None: - raise ValueError("No data to plot. Please load data first.") + raise ValueError('No data to plot. Please load data first.') if not _in_notebook(): - raise RuntimeError( - "plot_data() can only be used in a Jupyter notebook environment." - ) + raise RuntimeError('plot_data() can only be used in a Jupyter notebook environment.') if not isinstance(slicer, bool): - raise TypeError( - f"slicer must be True or False, not {type(slicer).__name__}" - ) + raise TypeError(f'slicer must be True or False, not {type(slicer).__name__}') if not isinstance(transpose_axes, bool): raise TypeError( - f"transpose_axes must be True or False, not {type(transpose_axes).__name__}" + f'transpose_axes must be True or False, not {type(transpose_axes).__name__}' ) plot_kwargs_defaults = { - "title": self.display_name, + 'title': self.display_name, } if slicer: - plot_kwargs_defaults["keep"] = "energy" + plot_kwargs_defaults['keep'] = 'energy' # Overwrite defaults with any user-provided kwargs plot_kwargs_defaults.update(kwargs) @@ -438,13 +430,13 @@ def plot_data( **plot_kwargs_defaults, ) for widget in fig.bottom_bar[0].controls.values(): - widget.slider_toggler.value = "-o-" + widget.slider_toggler.value = '-o-' else: if transpose_axes: - data_to_plot = self.binned_data.transpose(dims=["energy", "Q"]) + data_to_plot = self.binned_data.transpose(dims=['energy', 'Q']) else: - data_to_plot = self.binned_data.transpose(dims=["Q", "energy"]) + data_to_plot = self.binned_data.transpose(dims=['Q', 'energy']) fig = pp.plot( data_to_plot, **plot_kwargs_defaults, @@ -473,9 +465,9 @@ def _validate_coordinates(data: sc.DataArray) -> None: If required coordinates are missing. """ if not isinstance(data, sc.DataArray): - raise TypeError("Data must be a sc.DataArray.") + raise TypeError('Data must be a sc.DataArray.') - required_coords = ["Q", "energy"] + required_coords = ['Q', 'energy'] for coord in required_coords: if coord not in data.coords: raise ValueError(f"Data is missing required coordinate: '{coord}'") @@ -501,9 +493,7 @@ def _convert_to_bin_centers(self, data: sc.DataArray) -> sc.DataArray: data = data.assign_coords({dim: sc.midpoints(coord)}) return data - def _extract_x_y_var( - self, Q_index: int - ) -> tuple[np.ndarray, np.ndarray, np.ndarray]: + def _extract_x_y_var(self, Q_index: int) -> tuple[np.ndarray, np.ndarray, np.ndarray]: """ Extract the x, y, and weights arrays from the experiment for the given Q index. @@ -517,8 +507,8 @@ def _extract_x_y_var( tuple[np.ndarray, np.ndarray, np.ndarray] The x, y, and variances arrays extracted from the experiment for the given Q index. """ - data = self.binned_data["Q", Q_index] - x = data.coords["energy"].values + data = self.binned_data['Q', Q_index] + x = data.coords['energy'].values y = data.values var = data.variances return x, y, var @@ -562,7 +552,7 @@ def _extract_x_y_weights_only_finite( var = var[mask] if np.any(var == 0): - raise ValueError("Cannot compute weights: some variances are zero.") + raise ValueError('Cannot compute weights: some variances are zero.') weights = 1.0 / var**0.5 @@ -582,9 +572,9 @@ def __repr__(self) -> str: A string representation of the Experiment object. """ - return f"Experiment `{self.unique_name}` with data: {self._data}" + return f'Experiment `{self.unique_name}` with data: {self._data}' - def __copy__(self) -> "Experiment": + def __copy__(self) -> 'Experiment': """ Return a copy of the object. @@ -593,7 +583,7 @@ def __copy__(self) -> "Experiment": "Experiment" A copy of the Experiment object. """ - temp = self.to_dict(skip=["unique_name"]) + temp = self.to_dict(skip=['unique_name']) new_obj = self.__class__.from_dict(temp) new_obj.data = self.data.copy() if self.data is not None else None return new_obj From 51958c295a34136a199b7b586ac252903189526d Mon Sep 17 00:00:00 2001 From: henrikjacobsenfys Date: Tue, 7 Apr 2026 20:37:24 +0200 Subject: [PATCH 3/3] PR comments --- src/easydynamics/experiment/experiment.py | 30 +++++++++++-------- .../easydynamics/analysis/test_analysis.py | 14 ++++++--- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/easydynamics/experiment/experiment.py b/src/easydynamics/experiment/experiment.py index db714942..0d305400 100644 --- a/src/easydynamics/experiment/experiment.py +++ b/src/easydynamics/experiment/experiment.py @@ -8,6 +8,7 @@ import plopp as pp import scipp as sc from easyscience.base_classes.new_base import NewBase +from plopp.backends.matplotlib.figure import InteractiveFigure from scipp.io import load_hdf5 as sc_load_hdf5 from scipp.io import save_hdf5 as sc_save_hdf5 @@ -32,7 +33,7 @@ def __init__( Parameters ---------- - display_name : str | None, default="MyExperiment" + display_name : str | None, default='MyExperiment' Display name of the experiment. unique_name : str | None, default=None Unique name of the experiment. If None, a unique name will be generated. None. @@ -146,9 +147,9 @@ def Q(self) -> sc.Variable | None: sc.Variable | None The Q values from the dataset, or None if no data is loaded. """ - if self._binned_data is None: + if self.binned_data is None: return None - return self._binned_data.coords['Q'] + return self.binned_data.coords['Q'] @Q.setter def Q(self, _value: sc.Variable) -> None: @@ -179,9 +180,9 @@ def energy(self) -> sc.Variable | None: sc.Variable | None The energy values from the dataset, or None if no data is loaded. """ - if self._binned_data is None: + if self.binned_data is None: return None - return self._binned_data.coords['energy'] + return self.binned_data.coords['energy'] @energy.setter def energy(self, _value: sc.Variable) -> None: @@ -222,7 +223,7 @@ def get_masked_energy(self, Q_index: int) -> sc.Variable | None: sc.Variable | None The masked energy values from the dataset, or None if no data is loaded. """ - if self._binned_data is None: + if self.binned_data is None: return None if ( @@ -232,7 +233,7 @@ def get_masked_energy(self, Q_index: int) -> sc.Variable | None: ): raise IndexError('Q_index must be a valid index for the Q values.') - energy = self._binned_data.coords['energy'] + energy = self.binned_data.coords['energy'] _, _, _, mask = self._extract_x_y_weights_only_finite(Q_index=Q_index) mask_var = sc.array(dims=['energy'], values=mask) @@ -377,7 +378,7 @@ def plot_data( slicer: bool = False, transpose_axes: bool = False, **kwargs: dict, - ) -> None: + ) -> InteractiveFigure: """ Plot the dataset using plopp: https://scipp.github.io/plopp/. @@ -387,10 +388,15 @@ def plot_data( If True, use plopp's slicer instead of plot. transpose_axes : bool, default=False If True, transpose the data to have dimensions in the order (energy, Q) before - plotting. + plotting, so that energy is on the x-axis. This only applies when slicer=False. **kwargs : dict Additional keyword arguments to pass to plopp. + Returns + ------- + InteractiveFigure + A plot of the data and model. + Raises ------ ValueError @@ -401,7 +407,7 @@ def plot_data( If slicer or transpose_axes are not True or False. """ - if self._binned_data is None: + if self.binned_data is None: raise ValueError('No data to plot. Please load data first.') if not _in_notebook(): @@ -426,7 +432,7 @@ def plot_data( plot_kwargs_defaults.update(kwargs) if slicer: fig = pp.slicer( - self._binned_data, + self.binned_data, **plot_kwargs_defaults, ) for widget in fig.bottom_bar[0].controls.values(): @@ -580,7 +586,7 @@ def __copy__(self) -> 'Experiment': Returns ------- - "Experiment" + 'Experiment' A copy of the Experiment object. """ temp = self.to_dict(skip=['unique_name']) diff --git a/tests/unit/easydynamics/analysis/test_analysis.py b/tests/unit/easydynamics/analysis/test_analysis.py index 291e1811..25274a55 100644 --- a/tests/unit/easydynamics/analysis/test_analysis.py +++ b/tests/unit/easydynamics/analysis/test_analysis.py @@ -260,6 +260,9 @@ def test_plot_data_and_model_defaults(self, analysis): fake_fig.bottom_bar = [MagicMock()] fake_fig.bottom_bar[0].controls = {'test': fake_widget} + fake_data = MagicMock() + fake_data.coords = {'Q': 'Q_VALUES', 'energy': 'ENERGY_VALUES'} + analysis._create_model_array = MagicMock(return_value='MODEL') with ( patch('plopp.slicer', return_value=fake_fig) as mock_slicer, @@ -270,7 +273,7 @@ def test_plot_data_and_model_defaults(self, analysis): ) as mock_binned, patch('easydynamics.analysis.analysis._in_notebook', return_value=True), ): - mock_binned.return_value = 'DATA' + mock_binned.return_value = fake_data # THEN fig = analysis.plot_data_and_model(plot_components=False) @@ -284,7 +287,7 @@ def test_plot_data_and_model_defaults(self, analysis): assert 'Data' in data_passed assert 'Model' in data_passed - assert data_passed['Data'] == 'DATA' + assert data_passed['Data'] == fake_data assert data_passed['Model'] == 'MODEL' # Check the default kwargs @@ -310,6 +313,9 @@ def test_plot_data_and_model_plot_components_true(self, analysis): fake_fig.bottom_bar = [MagicMock()] fake_fig.bottom_bar[0].controls = {'test': fake_widget} + fake_data = MagicMock() + fake_data.coords = {'Q': 'Q_VALUES', 'energy': 'ENERGY_VALUES'} + analysis._create_model_array = MagicMock(return_value='MODEL') analysis._create_components_dataset = MagicMock(return_value={'Gaussian': 'GAUSS'}) with ( @@ -321,7 +327,7 @@ def test_plot_data_and_model_plot_components_true(self, analysis): ) as mock_binned, patch('easydynamics.analysis.analysis._in_notebook', return_value=True), ): - mock_binned.return_value = 'DATA' + mock_binned.return_value = fake_data # THEN fig = analysis.plot_data_and_model(plot_components=True) @@ -335,7 +341,7 @@ def test_plot_data_and_model_plot_components_true(self, analysis): assert 'Data' in data_passed assert 'Model' in data_passed - assert data_passed['Data'] == 'DATA' + assert data_passed['Data'] == fake_data assert data_passed['Model'] == 'MODEL' # Check the default kwargs assert kwargs['title'] == 'TestAnalysis'