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

Add filter_zero_loss_peak argument to spikes_removal_tool and fix one bug. #1412

Merged

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Jan 30, 2017

It should be PR to next_minor, but I initially check out from next_patch... I can change it to next_minor if necessary.
It may also be a good opportunity to add some tests (not for the ui) for the spikes_removal_tool, I guess it should be possible to test the different method/functions without using the ui?

  • Add filter_zero_loss_peak argument
  • Add documentation
  • Add tests
  • Fix bug with update pointer position

@francisco-dlp
Copy link
Member

Wouldn't this be equivalent?:

s.isig[start:end].spikes_removal_tool()

@ericpre
Copy link
Member Author

ericpre commented Jan 31, 2017

Yes, it is! Thanks :) I will add it to the documentation!

Is there a clever way to set two intervals, that I would have miss? For example to ignore a zero-loss peak?

@francisco-dlp
Copy link
Member

You can pass a signal_mask to do that. That's simply a boolean matrix. Model1D has a sort of interactive signal mask editor (e.g. set_signal_range etc) that would be useful to turn into an independent function to create signal masks more conveniently.

@ericpre
Copy link
Member Author

ericpre commented Jan 31, 2017

Adding a get_signal_range method to Signal1D, that works in a similar manner than set_signal_range and returns a mask?

@francisco-dlp
Copy link
Member

Something like that. Currently Model1D has:

  • set_signal_range
  • remove_signal_range
  • reset_signal_range

They enable editing a mask interactively. Because the mask is stored in the model as an attribute for internal consumption there is no method to get the mask as you propose.

For the model it makes sense to store it internally, but does it make sense to store it internally for the signal? If not, it could be implemented as a Signal that includes the above methods e.g.

mask = s.get_signal_mask() # This returns a mask masking nothing by default
mask.set_signal_range() # It remembers its parent mask and, if no signal range is passed, it plots s for interactive selection
mask.set_signal_range(signal=s2) # Interactively edit the mask with a different signal
s.spikes_removal_tool(signal_mask=mask)

- Add link to the summary of the implementation of lazy signal to the dev_guide
- Display signal masked area with fill_between 0 and signal.
- close plot at the same time than closing the window
@ericpre ericpre force-pushed the add_energy_range_spikes_removal_tool branch from 1f35812 to ec73fd8 Compare March 19, 2017 22:31
@ericpre ericpre changed the title Add signal_start and signal_end arguments to spikes_removal_tool. Add filter_zero_loss_peak argument to spikes_removal_tool and fixes one bug. Mar 21, 2017
@ericpre ericpre changed the title Add filter_zero_loss_peak argument to spikes_removal_tool and fixes one bug. Add filter_zero_loss_peak argument to spikes_removal_tool and fix one bug. Mar 21, 2017
@ericpre
Copy link
Member Author

ericpre commented Mar 21, 2017

Ready for review.

@ericpre ericpre mentioned this pull request Mar 21, 2017
…ero_loss_peak_argument_spikes_removal_tool

# Conflicts:
#	doc/dev_guide.rst
#	hyperspy/_signals/signal1d.py
#	hyperspy/gui/egerton_quantification.py
…to signal_tools.py and remove unnecessary tests.
…ctive` to make it interactive or not.

- Allow the threshold to be estimated automatically using the `spikes_diagnosis` tool.
- Update documnentation.
@ericpre
Copy link
Member Author

ericpre commented Sep 10, 2019

Merged with upstream and resolved conflict.

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.

Minor doc change @ericpre otherwise looks good!

doc/user_guide/signal1d.rst Outdated Show resolved Hide resolved
…ero_loss_peak_argument_spikes_removal_tool

# Conflicts:
#	doc/dev_guide/lazy_computations.rst
#	doc/user_guide/signal1d.rst
@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #1412 into RELEASE_next_minor will increase coverage by 0.07%.
The diff coverage is 77.91%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           RELEASE_next_minor    #1412      +/-   ##
======================================================
+ Coverage               75.56%   75.63%   +0.07%     
======================================================
  Files                     202      202              
  Lines                   29544    29614      +70     
  Branches                 6439     6449      +10     
======================================================
+ Hits                    22324    22398      +74     
+ Misses                   5401     5390      -11     
- Partials                 1819     1826       +7     
Impacted Files Coverage Δ
hyperspy/signal.py 75.38% <ø> (ø)
hyperspy/_signals/signal1d.py 75.19% <71.42%> (+3.91%) ⬆️
hyperspy/signal_tools.py 65.64% <76.92%> (+0.08%) ⬆️
hyperspy/_signals/eels.py 75.48% <81.48%> (+0.13%) ⬆️
hyperspy/docstrings/signal.py 100.00% <100.00%> (ø)
hyperspy/docstrings/signal1d.py 100.00% <100.00%> (ø)
hyperspy/drawing/signal1d.py 75.07% <0.00%> (+0.61%) ⬆️

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 5c8ff8a...aa3558c. Read the comment docs.

@tjof2
Copy link
Contributor

tjof2 commented Jul 28, 2020

If you can resolve the conflicts this should be good to go?

…ero_loss_peak_argument_spikes_removal_tool

# Conflicts:
#	hyperspy/tests/signal/test_tools.py
@tjof2
Copy link
Contributor

tjof2 commented Jul 28, 2020

@francisco-dlp can you check this over again since you initially reviewed it?

@ericpre ericpre force-pushed the add_energy_range_spikes_removal_tool branch from 06010f7 to c9db02e Compare August 2, 2020 10:30
@ericpre ericpre force-pushed the add_energy_range_spikes_removal_tool branch from 563fc8a to 020863f Compare August 2, 2020 12:10
@francisco-dlp francisco-dlp modified the milestones: v1.6, v1.7 Aug 5, 2020
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.

Subject to resolving conflicts.

…ero_loss_peak_argument_spikes_removal_tool

# Conflicts:
#	doc/user_guide/signal1d.rst
#	hyperspy/_signals/eels.py
#	hyperspy/_signals/signal1d.py
#	hyperspy/tests/signal/test_eels.py
#	hyperspy/tests/signal/test_tools.py
@tjof2 tjof2 merged commit e4c6994 into hyperspy:RELEASE_next_minor Sep 9, 2020
@ericpre ericpre deleted the add_energy_range_spikes_removal_tool branch September 9, 2020 17:28
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