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

fixed test #187

Merged
merged 3 commits into from
Apr 25, 2023
Merged

fixed test #187

merged 3 commits into from
Apr 25, 2023

Conversation

Attolight-NTappy
Copy link
Contributor

Fixes the failing test in #185

@@ -48,11 +48,11 @@ def test__make_signal_mask(self):

def test_remove_spikes(self):
s = CLSpectrum(np.ones((2, 3, 30)))
np.random.seed(1)
np.random.seed(np.random.randint(666666))
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, np.random.randint is still using the legacy random generator: https://numpy.org/doc/stable/reference/random/generated/numpy.random.randint.html. Would it make sense to use the new generator?
The legacy generator is not deprecated but it will most likely be at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For what I understood the np.random.seed statement has no effect as the following add_gaussian_noise from hyperspy does not take it into account and applies its own seed using the generator object.

I updated the tests to remove this line

@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (ca4d57d) 100.00% compared to head (0c6d1a9) 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #187   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          554       554           
=========================================
  Hits           554       554           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ericpre
Copy link
Contributor

ericpre commented Mar 23, 2023

Does remove_spikes have any added functionalities in comparison to Signal1D.spikes_removal_tool? If not, is it worth keeping it?

@Attolight-NTappy
Copy link
Contributor Author

From what I see in the code, it mostly allows to choose whether it is done in place or not.

(I just discover now that the test is still flaky)

@jlaehne
Copy link
Contributor

jlaehne commented Mar 26, 2023

Does remove_spikes have any added functionalities in comparison to Signal1D.spikes_removal_tool? If not, is it worth keeping it?

Indeed, apart from the additional inplace option, which should be easy to contribute upstream, and the possibility to build a signal_mask from a luminescence_roi, it is only a wrapper function with different defaults (interactive=False).

@jordiferrero, you contributed this function, what was the original motivation behind it? If we are just changing defaults of an existing function, it does not really make sense to keep it up.

@jlaehne jlaehne added this to the v0.2.3 milestone Apr 25, 2023
@jlaehne jlaehne linked an issue Apr 25, 2023 that may be closed by this pull request
@jlaehne jlaehne merged commit 9d45dbf into LumiSpy:main Apr 25, 2023
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.

Fix test for remove_spikes
4 participants