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?
There was a problem hiding this comment.
Hmm, I don't think so? Then we could have a "restore" method for any attribute you can set, which is essentially an undo/redo stack. In any event, that is something which could be added in the future, it is not a critical feature atm. :)
| 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?
There was a problem hiding this comment.
It should, or be completely removed, I wrote this code before deciding exactly how secondary rebins should work, but I've kept the code there, commented out, in case I change my mind later.
I decided that when rebinning, you always rebin the "current" array, which I feel like is what most users would expect, if they see a 16x16 grid, and rebin by a factor of 2, they would expect to see an 8x8 grid, rather than a 64x64 grid (if the original data was a 128x128 grid).
| '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)?
There was a problem hiding this comment.
Nope, the colour-scale is the transmission values, which should ideally always be <= 1, but due to extinction and other effects can sometimes be slightly above 1. In my test data I have a few errenous pixels with values in the 1000's, so if the cmax is automatic, you can't see the real data at all. This can be overwritten by the user anyways. Maybe 3.0 isn't the best value, but only user-testing can really decide what the best default would be :)
|
|
||
| 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.
| 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
| @_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.
There was a problem hiding this comment.
There are no private properties in Python . . .
The _ prefix is just a convention to mark something as "private", users can access these without issue and also set them if they so desire. But that is what I am blocking here. It's mostly a help to myself as users shouldn't even be aware of the _data_array property :)
There was a problem hiding this comment.
I probably meant something which I didn't articulate properly. Of course I don't remember what now...
feel free to disregard
| 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
There was a problem hiding this comment.
Better yet, it should be a utility function.
| if TYPE_CHECKING: | ||
| pass |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #21 +/- ##
============================================
+ Coverage 57.14% 87.88% +30.74%
============================================
Files 2 8 +6
Lines 7 520 +513
Branches 0 84 +84
============================================
+ Hits 4 457 +453
- Misses 3 60 +57
- Partials 0 3 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
No description provided.