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

Vacuum mask eels #2183

Merged
merged 16 commits into from Sep 11, 2020
Merged

Vacuum mask eels #2183

merged 16 commits into from Sep 11, 2020

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented May 22, 2019

Similarly to the way it can be currently done with EDSSpectrum, this PR add a vacuum_mask method to the EELSSpectrum signal.

Progress of the PR

  • Add vacuum_mask method to EELSSpectrum,
  • make the usage of mask more consistent across different method by accepting signal of bool or array of bool,
  • fix bug with navigation_mask in the quantification method of EDS signal, which was ignored.
  • update docstring (if appropriate),
  • add tests,
  • ready for review.

Minimal example of the bug fix or the new feature

ll = hs.datasets.artificial_data.get_low_loss_eels_line_scan_signal()*100
ll.inav[:4] *= 0
ll.vacuum_mask().plot()

…m_mask_eels

# Conflicts:
#	hyperspy/signal.py
Copy link
Contributor

@dnjohnstone dnjohnstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work for me insofar as I understand it should from the tests provided.

It seems to me that for consistency there should be a super().decomposition() for EELSSpectrum in the same way that there is for EDSTEMSpectrum with a flag to use this method to determine a vacuum mask for decomposition as default.

Other than this, and minor typos - looks good to me

hyperspy/_signals/eds_tem.py Outdated Show resolved Hide resolved
hyperspy/_signals/eels.py Outdated Show resolved Hide resolved
hyperspy/_signals/eels.py Outdated Show resolved Hide resolved
hyperspy/_signals/eels.py Outdated Show resolved Hide resolved
hyperspy/_signals/eels.py Outdated Show resolved Hide resolved
hyperspy/_signals/eels.py Outdated Show resolved Hide resolved
hyperspy/_signals/eels.py Outdated Show resolved Hide resolved
hyperspy/_signals/eels.py Show resolved Hide resolved
…m_mask_eels

# Conflicts:
#	hyperspy/signal.py
@dnjohnstone
Copy link
Contributor

@ericpre can you deal with the very minor comments I had so that this can be merged?

Copy link
Contributor

@tjof2 tjof2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the comments from @dnjohnstone , there's a couple of merge conflicts.

hyperspy/learn/mva.py Show resolved Hide resolved
@tjof2
Copy link
Contributor

tjof2 commented Sep 8, 2020

Requires the merge conflicts to be resolved plus a few of the above comments

…m_mask_eels

# Conflicts:
#	hyperspy/_signals/signal1d.py
#	hyperspy/learn/mva.py
#	hyperspy/tests/signal/test_eels.py
@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

Merging #2183 into RELEASE_next_minor will increase coverage by 0.03%.
The diff coverage is 79.16%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #2183      +/-   ##
======================================================
+ Coverage               75.85%   75.88%   +0.03%     
======================================================
  Files                     202      202              
  Lines                   29623    29650      +27     
  Branches                 6446     6454       +8     
======================================================
+ Hits                    22470    22501      +31     
+ Misses                   5336     5330       -6     
- Partials                 1817     1819       +2     
Impacted Files Coverage Δ
hyperspy/learn/mva.py 79.86% <25.00%> (-0.22%) ⬇️
hyperspy/signal.py 75.32% <50.00%> (-0.10%) ⬇️
hyperspy/misc/eds/utils.py 86.66% <80.00%> (+1.53%) ⬆️
hyperspy/_signals/eels.py 76.02% <90.90%> (+0.54%) ⬆️
hyperspy/_signals/eds_tem.py 81.00% <100.00%> (+1.28%) ⬆️
hyperspy/_signals/signal1d.py 76.14% <100.00%> (+0.74%) ⬆️

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 ad18000...8188c5b. Read the comment docs.

@tjof2
Copy link
Contributor

tjof2 commented Sep 11, 2020

Is that everything now @ericpre?

@ericpre
Copy link
Member Author

ericpre commented Sep 11, 2020

Yes, this is good now. Increasing coverage made me find a bug with the navigation mask of the quantification method!

@tjof2 tjof2 merged commit ebe06c0 into hyperspy:RELEASE_next_minor Sep 11, 2020
@ericpre ericpre deleted the vacuum_mask_eels branch September 11, 2020 18:06
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.

None yet

3 participants