Conversation
damskii9992
added
[scope] significant
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
henrikjacobsenfys
left a comment
henrikjacobsenfys
left a comment
There was a problem hiding this comment.
Only minor nitpicks and questions.
It may be nice to provide an example notebook to see how it all works in practice.
| self._data_array.coords['y'] = y_positions | ||
| self._has_physical_coords = True | ||
|
|
||
| def delete_physical_coord_positions(self) -> None: |
There was a problem hiding this comment.
Would a restore_physical_coord_positions method make sense?
| if not isinstance(dimensions, dict): | ||
| raise TypeError('dimensions must be a dictionary mapping dimension names to rebin factors.') | ||
| if all(isinstance(value, Numeric) and value == 1 for value in dimensions.values()): # Reverts to original data | ||
| # self.revert_rebin() # If we want secondary rebins to work on the original data |
There was a problem hiding this comment.
Should presumably not be outcommented?
| return | ||
| sizes = dimensions.copy() | ||
| if 't' in dimensions: | ||
| raise ValueError("Rebinning of the time-of-flight ('t') dimension is yet not supported.") |
| 'title': self.display_name + title_suffix, | ||
| 'clabel': 'Transmission', | ||
| 'cmin': 0.0, | ||
| 'cmax': 3.0, |
There was a problem hiding this comment.
Should perhaps be something like 1.1*max(self._data_array.values)?
| f"'{coord_name}' coordinate must have a unit of {expected_dim_string}, such as ('{expected_unit}')." | ||
| ) from None # noqa: E501 | ||
|
|
||
| # Does this need to be moved somewhere else? Maybe Corelib? |
There was a problem hiding this comment.
I think it's fine to have it here, at least for now. Perhaps in a utils file so it doesn't get lost?
| assert measurement.regions_of_interest[0] is valid_roi | ||
| assert measurement.regions_of_interest[roi_name] is valid_roi | ||
|
|
||
| def test_regions_of_interest_removal_by_index(self, valid_data_array, valid_roi): |
There was a problem hiding this comment.
Here and in the following test: May be good to add at least another roi and check that only the requested roi is removed As it is, the test would pass if the removal removed all roi, the first in the list, the last in the list or a random one.
|
|
||
| monkeypatch.setattr(measurement, '_is_notebook', mock_is_notebook) | ||
| # Then Expect | ||
| measurement.plot(time_of_flight=time_of_flight) # Just ensure no exception is raised |
There was a problem hiding this comment.
Could you mock plot and check that it gets the right kwargs?
| # Then Expect | ||
| measurement.slicer_plot() # Just ensure no exception is raised | ||
|
|
||
| def test_spectrum_inspector_fails_outside_notebook(self, valid_data_array): |
There was a problem hiding this comment.
You mentioned that some tests don't work yet, perhaps this is one of them. In any case, it fails for me with the following error, just posting in case it's useful:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
src\easyimaging\measurement\measurement.py:524: in spectrum_inspector
return pp.inspector(self._data_array, dim='t', orientation='vertical', **inspector_kwargs_defaults)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
.pixi\envs\dev\Lib\site-packages\plopp\plotting\inspector.py:206: in inspector
f1d = linefigure(
.pixi\envs\dev\Lib\site-packages\plopp\graphics\figures.py:31: in linefigure
return backends.get(group='2d', name='figure')(view_maker, *nodes, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
.pixi\envs\dev\Lib\site-packages\plopp\backends\matplotlib\figure.py:195: in Figure
return InteractiveFigure(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
.pixi\envs\dev\Lib\site-packages\plopp\backends\matplotlib\figure.py:136: in __init__
self.__init_figure__(View, *args, **kwargs)
.pixi\envs\dev\Lib\site-packages\plopp\backends\matplotlib\figure.py:24: in __init_figure__
self.view = View(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
.pixi\envs\dev\Lib\site-packages\plopp\graphics\graphicalview.py:100: in __init__
self.canvas = canvas_maker(
.pixi\envs\dev\Lib\site-packages\plopp\backends\matplotlib\canvas.py:157: in __init__
self.fig = make_figure(figsize=(6.0, 4.0) if figsize is None else figsize)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
args = (), kwargs = {'figsize': (6.0, 4.0)}
fig = <Figure size 600x400 with 0 Axes>
def make_figure(*args, **kwargs) -> plt.Figure:
"""
Create a new figure.
If we use ``plt.figure()`` directly, the figures auto-show in the notebooks.
We want to display the figures when the figure repr is requested.
When using the static backend, we can return the ``plt.Figure`` (note the uppercase
F) directly.
When using the interactive backend, we need to do more work. The ``plt.Figure``
will not have a toolbar nor will it be interactive, as opposed to what
``plt.figure`` returns. To fix this, we need to create a manager for the figure
(see https://stackoverflow.com/a/75477367).
"""
fig = plt.Figure(*args, **kwargs)
if is_interactive_backend():
# Create a manager for the figure, which makes it interactive, as well as
# making it possible to show the figure from the terminal.
> plt._backend_mod.new_figure_manager_given_figure(1, fig)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E AttributeError: 'NoneType' object has no attribute 'new_figure_manager_given_figure'
.pixi\envs\dev\Lib\site-packages\plopp\backends\matplotlib\utils.py:71: AttributeError
| ): # noqa: E501 | ||
| measurement.spectrum_inspector() | ||
|
|
||
| def test_spectrum_inspector_runs_in_notebook_with_widget_backend(self, valid_data_array, monkeypatch, _use_ipympl): |
There was a problem hiding this comment.
_use_ipympl not found
There was a problem hiding this comment.
Do you need to have the same file in two different places in the project?
| else: | ||
| raise ValueError('Both x_range and y_range must be provided together or not at all.') | ||
|
|
||
| def set_pixel_coord_range(self, x_pixel_range: Sequence[int, int], y_pixel_range: Sequence[int, int]) -> None: |
There was a problem hiding this comment.
This and similar lines are quite long and therefore not so easy to read:
def set_pixel_coord_range(self, x_pixel_range: Sequence[int, int], y_pixel_range: Sequence[int, int]) -> None:Maybe it would be better to format them like this:
def set_pixel_coord_range(
self,
x_pixel_range: Sequence[int, int],
y_pixel_range: Sequence[int, int]
) -> None:Yes, this uses more lines, but it is much easier to read. It is faster to see what the arguments are, their types, and the return value.
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
A few issues, nothing blocking.
| requires-python = ">=3.11,<3.14" | ||
| dependencies = [ | ||
| "easyscience>=2.0.0", | ||
| #"easyscience@git+https://github.com/easyscience/corelib#egg=easylist2", |
There was a problem hiding this comment.
You rely on EasyList but use the release version. The current code doesn't work when installed
| Revert any rebinning applied to the measurement data, restoring it to its original resolution. | ||
| """ | ||
| if self._rebinned_data_array is not None: | ||
| del self._rebinned_data_array |
There was a problem hiding this comment.
This will call AttributeError if called before any rebinning (_rebinned_data_array is not defined!)
Consider guards
| [dependencies] | ||
| python = ">=3.11,<3.13" | ||
| python = ">=3.14.3,<3.15" | ||
|
|
There was a problem hiding this comment.
inconsistent python version between pixi.toml and pyproject.toml!
| Rebin the measurement image stack. This operation reduces the resolution of the data by combining adjacent pixels or time bins. | ||
| The rebinned dimensions must be evenly divisible by their specific rebin factor. | ||
|
|
||
| To revert to the original data, provide a dictionary with all rebin factors set to 1. |
There was a problem hiding this comment.
The above statement is not true, as the revert path is commented out.
| @staticmethod | ||
| def _validate_provided_coord( | ||
| data_array: sc.DataArray, | ||
| coord: sc.Variable | np.array, |
There was a problem hiding this comment.
np.array - should this not be np.ndarray?
np.array is a function, np.ndarray is a type.
| @_data_array.setter | ||
| def _data_array(self, value: sc.DataArray) -> None: | ||
| raise AttributeError('Cannot set _data_array, it is a read-only property.') |
There was a problem hiding this comment.
Pytohn will raise AttributeError even without the setter - this is a private property.
| plot_kwargs_defaults.update(kwargs) | ||
|
|
||
| if time_of_flight is None: | ||
| plot = self._data_array.mean('t').plot(**plot_kwargs_defaults) |
There was a problem hiding this comment.
But you just said in the docstring, that
If no time-of-flight is provided, the plot will sum over all time-of-flight values.
mean =/= sum ...
| def _is_notebook(self) -> bool: | ||
| """ | ||
| Check if the code is running in a Jupyter notebook environment. | ||
| """ | ||
| try: | ||
| shell = get_ipython().__class__.__name__ # pyright: ignore[reportUndefinedVariable] | ||
| if shell == 'ZMQInteractiveShell': | ||
| return True # Jupyter notebook or qtconsole | ||
| elif shell == 'TerminalInteractiveShell': | ||
| return False # Terminal running IPython | ||
| else: | ||
| return False # Other type (possibly other IDE) | ||
| except NameError: | ||
| return False # Probably standard Python interpreter |
There was a problem hiding this comment.
This should be a @staticmethod
| if TYPE_CHECKING: | ||
| pass |
| raise TypeError(f'{name} indice must be an integer.') | ||
| if value < 0: | ||
| raise ValueError(f'{name} indice must be non-negative.') |
4 participants
No description provided.