Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ROI fix and improvement #2809

Merged
merged 9 commits into from
Oct 7, 2021
Merged

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Aug 23, 2021

Description of the change

When a right hand side axis is added to a signal plot, this axes will receive the matplotlib event instead of the left hand side axis. This doesn't play well with the ROI, because by default, matplotlib send event to the last added axis and most of the time, the right hand side axis is added last, and this breaks matplotlib events connection, because we connect the ROI with the matplotlib events of the left hand side axis

Progress of the PR

  • Add support for passing string instead of string of tuple to index axes_manager.
  • Fix receiving matplotlib event to the left hand side axis by changing respective z-order of the matplotlib axis.
  • Completely remove right_axis instead of making it invisible when closing it.
  • add an changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
  • Check formatting changelog entry in the readthedocs doc build of this PR (link in github checks)
  • add tests,
  • ready for review.

Minimal example of the bug fix or the new feature

import hyperspy.api as hs

s = hs.datasets.artificial_data.get_core_loss_eels_line_scan_signal(True)
s.remove_background()

roi = hs.roi.SpanROI(left=450, right=600)
# before this PR, the roi can't be changed interactively
roi.add_widget(s, axes='Electron energy loss')

s_ti = s.isig[roi].integrate1D(axis="Electron energy loss")

@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

Merging #2809 (eb278b0) into RELEASE_next_patch (69e8a23) will decrease coverage by 1.69%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           RELEASE_next_patch    #2809      +/-   ##
======================================================
- Coverage               77.37%   75.67%   -1.70%     
======================================================
  Files                     202      202              
  Lines                   30108    30118      +10     
  Branches                 6579     6580       +1     
======================================================
- Hits                    23295    22793     -502     
- Misses                   5066     5587     +521     
+ Partials                 1747     1738       -9     
Impacted Files Coverage Δ
hyperspy/drawing/signal1d.py 77.47% <100.00%> (+0.06%) ⬆️
hyperspy/roi.py 82.33% <100.00%> (+0.26%) ⬆️
hyperspy/misc/machine_learning/import_sklearn.py 50.00% <0.00%> (-50.00%) ⬇️
hyperspy/learn/mva.py 45.00% <0.00%> (-37.93%) ⬇️
hyperspy/utils/peakfinders2D.py 55.86% <0.00%> (-35.20%) ⬇️
hyperspy/misc/axis_tools.py 55.55% <0.00%> (-22.23%) ⬇️
hyperspy/misc/eels/eelsdb.py 60.86% <0.00%> (-4.35%) ⬇️
hyperspy/learn/svd_pca.py 92.10% <0.00%> (-3.95%) ⬇️
hyperspy/_signals/lazy.py 88.10% <0.00%> (-2.58%) ⬇️
hyperspy/signal.py 74.04% <0.00%> (-2.17%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69e8a23...eb278b0. Read the comment docs.

hyperspy/drawing/signal1d.py Outdated Show resolved Hide resolved
hyperspy/roi.py Outdated Show resolved Hide resolved
hyperspy/roi.py Outdated Show resolved Hide resolved
hyperspy/roi.py Outdated Show resolved Hide resolved
hyperspy/roi.py Outdated Show resolved Hide resolved
hyperspy/roi.py Outdated Show resolved Hide resolved
…f list or tuple in `_parse_axes`. Raise ValueError if length of provided list/tuple doesn't match ROI dimensionality.
@ericpre
Copy link
Member Author

ericpre commented Oct 4, 2021

Thanks @jlaehne, I improved the dosctrings and consistency in the _parse_axes method.

@jlaehne
Copy link
Contributor

jlaehne commented Oct 6, 2021

LGTM

@jlaehne
Copy link
Contributor

jlaehne commented Oct 7, 2021

I corrected a few typos in the docstrings. Also realized that there are three __call__ functions where the docstring refers to the possibility to pass a tuple to the axis argument. Shouldn't we also add the following line to all of them: * Anything that can index the provided ``axes_manager``.

@ericpre
Copy link
Member Author

ericpre commented Oct 7, 2021

I corrected a few typos in the docstrings. Also realized that there are three __call__ functions where the docstring refers to the possibility to pass a tuple to the axis argument. Shouldn't we also add the following line to all of them: * Anything that can index the providedaxes_manager.

Yes, indeed! I consolidated the docstring of the methods using _parse_axes.

@jlaehne
Copy link
Contributor

jlaehne commented Oct 7, 2021

I corrected a few typos in the docstrings. Also realized that there are three __call__ functions where the docstring refers to the possibility to pass a tuple to the axis argument. Shouldn't we also add the following line to all of them: * Anything that can index the providedaxes_manager.

Yes, indeed! I consolidated the docstring of the methods using _parse_axes.

There is one more occurence of the axis-docstring on line 999 (CircleROI class)

@ericpre
Copy link
Member Author

ericpre commented Oct 7, 2021

I corrected a few typos in the docstrings. Also realized that there are three __call__ functions where the docstring refers to the possibility to pass a tuple to the axis argument. Shouldn't we also add the following line to all of them: * Anything that can index the providedaxes_manager.

Yes, indeed! I consolidated the docstring of the methods using _parse_axes.

There is one more occurence of the axis-docstring on line 999 (CircleROI class)

Done, I have removed the docstring completely so that it is inherited from the parent class.

@jlaehne jlaehne merged commit 1694111 into hyperspy:RELEASE_next_patch Oct 7, 2021
@ericpre ericpre deleted the ROI_fixes branch October 7, 2021 14:51
@ericpre ericpre added this to the v1.6.5 milestone Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants