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

Functions adapted to hyperspy-based methods #44

Merged
merged 32 commits into from
Feb 21, 2021

Conversation

jordiferrero
Copy link
Contributor

@jordiferrero jordiferrero commented Nov 11, 2020

Some functions currently in LumiSpy have created their own methods while there are similar functionalities in Hyperspy which can do the same.
In this PR, a better integration of Hyperspy-based methods is intended.
(Former #40)

TODO:

  • Cosmic ray removal function
  • Background subtraction methods
  • Create tests for each function

@jordiferrero jordiferrero added this to In progress in LumiSpy first release 0.1 Jan 15, 2021
@jordiferrero
Copy link
Contributor Author

jordiferrero commented Jan 24, 2021

I am struggling with an error I get when running the Tests in this function:

spikes_removal = SpikesRemoval(signal=signal, navigation_mask=navigation_mask, signal_mask=signal_mask,
                                       threshold=threshold, default_spike_width=default_spike_width,
>                                      add_noise=add_noise,)
E       TypeError: __init__() got an unexpected keyword argument 'threshold'

However, the SpikesRemoval class, inherited from hyperspy.signal_tools here, does indeed have a threshold argument. I don't know why it is raising this issue.

When I run the tests internally in my computer (py3.7 in PyCharm) all tests pass...

Can you @ericpre @jlaehne have a quick look if you see any obvious mistake? Thank you.

After resolving this issue, this PR is ready for review.

@ericpre
Copy link
Contributor

ericpre commented Jan 24, 2021

I am struggling with an error I get when running the Tests in this function:

spikes_removal = SpikesRemoval(signal=signal, navigation_mask=navigation_mask, signal_mask=signal_mask,
                                       threshold=threshold, default_spike_width=default_spike_width,
>                                      add_noise=add_noise,)
E       TypeError: __init__() got an unexpected keyword argument 'threshold'

However, the SpikesRemoval class, inherited from hyperspy.signal_tools here, does indeed have a threshold argument. I don't know why it is raising this issue.

When I run the tests internally in my computer (py3.7 in PyCharm) all tests pass...

Can you @ericpre @jlaehne have a quick look if you see any obvious mistake? Thank you.

After resolving this issue, this PR is ready for review.

This is not released and this is only on the RELEASE_next_minor branch.

lumispy/signals/cl_spectrum.py Outdated Show resolved Hide resolved
lumispy/signals/cl_spectrum.py Outdated Show resolved Hide resolved
@ericpre
Copy link
Contributor

ericpre commented Jan 24, 2021

If you check the py3.8/coverage which uses the non_uniform branch and is ahead of (include) RELEASE_next_minor, you will notice that there is no error, but the test fails.

@jordiferrero
Copy link
Contributor Author

Oh. I assumed it was released already :-) Since I am always in the non_uniform branch....

So how should I deal with this?
My guess it to add a check whether SpikesRemovalTool is released. If yes, then the function can be used. If not, then it is not available yet for the user until the hyperspy new version is released (I could leave my old simpler function for such cases as a temporal thing, but I would rather not).
Does this make sense @ericpre? Thanks for the help!

@jlaehne
Copy link
Contributor

jlaehne commented Jan 25, 2021

spikes_removal_tool is an old function, which is released: https://hyperspy.org/hyperspy-doc/current/api/hyperspy._signals.signal1d.html?highlight=spikes_removal_tool#hyperspy._signals.signal1d.Signal1D.spikes_removal_tool

However, the threshold parameter was only introduced in hyperspy/hyperspy#1412, which was merged last September. So you could maybe check for the availability of that parameter. As the use of lumispy is still limited without the non_uniform_axes, I would not bother providing alternative implementations.

@jordiferrero
Copy link
Contributor Author

All comments have been adressed and accepted.
This PR has been closed.
Thank you all for your help.

@jordiferrero jordiferrero merged commit 521ed73 into LumiSpy:master Feb 21, 2021
This was referenced Feb 22, 2021
@jlaehne jlaehne moved this from In progress to Done in LumiSpy first release 0.1 Feb 22, 2021
@jordiferrero jordiferrero deleted the hyperspy_based branch January 29, 2023 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants