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

[API] Remove noisy.py module from pyprep #39

Merged
merged 6 commits into from
Sep 4, 2020
Merged

[API] Remove noisy.py module from pyprep #39

merged 6 commits into from
Sep 4, 2020

Conversation

sappelhoff
Copy link
Owner

closes #11

@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2020

Codecov Report

Merging #39 into master will decrease coverage by 0.73%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
- Coverage   97.74%   97.01%   -0.74%     
==========================================
  Files           6        5       -1     
  Lines         710      502     -208     
==========================================
- Hits          694      487     -207     
+ Misses         16       15       -1     
Impacted Files Coverage Δ
pyprep/find_noisy_channels.py 97.16% <100.00%> (-0.07%) ⬇️

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 9517fe6...4238c7e. Read the comment docs.

@sappelhoff sappelhoff marked this pull request as ready for review September 3, 2020 16:59
@sappelhoff sappelhoff added the API changing the interace of pyprep label Sep 3, 2020
@yjmantilla
Copy link
Collaborator

yjmantilla commented Sep 3, 2020

so sad, noisy.py will be remembered for generations to come.

I do find some of the methods of noisy.py more readable than the current ones. Nevertheless the output of the current implementation should be similar to matlab's prep; I think the John Hopkins team did those tests but Im not sure.

Some of the differences between noisy.py and find_noisy_channels.py I have spotted are:

  • obviusly find_noisy_channels is missing the FASTER's epoch methods.
  • adding bad_by_SNR , bad_by_dropout attributes and methods.
  • find_all_bads function is a bit more obscure since in the current version some of the bad_by_Xmethod are filled because another bad_by_Ymethod calls it internally. I guess that optimizes a bit because no recalculation is done.
  • This avoidant of recalculation makes some functions dependent on the state of the class (as in "was the data filtered before by some other class method?" ). In general this mutability of the state make things a bit harder to keep track of. For example the original detrend bug was precisely because of this.
  • by_nan and by_flat were joined into by_nan_flat -> probably because original matlab implementation had it like that
  • some parameters were put in the exact number as matlab (ie deviation threshold in 5 rather than 3.29 like noisy.py)
  • Some of the methods are a bit more convoluted (ie hf_noise, correlation criteria), I guess because they tried to stay really close as was originally codified in matlab
  • ransac is the same apart from the changes I did in the lesser_ram_ransac PR lesser ram ransac #32

Copy link
Collaborator

@yjmantilla yjmantilla left a comment

Choose a reason for hiding this comment

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

All seems ok, although why is the code coverage of the project declining?

EDIT: Seems like the misses are:

  • EEG with sample rate beyond 100 in find_bad_by_hfnoise
  • if drop[i, j] == 1 in find_bad_by_correlation (not sure what that means though)
  • raise raise IOError("Too few channels ...") in find_bad_by_ransac

although that doesn't really account for the decline since these werent tested anyway; maybe it is because if one deletes a large chunk of code that was covered then the ratio of lines-covered/total-lines changes and the proportion of the updated coverage is lower.

@sappelhoff
Copy link
Owner Author

so sad, noisy.py will be remembered for generations to come.

😆

I do find some of the methods of noisy.py more readable than the current ones.

me too (but I am biased 🙂 ), but we can improve the current methods -> first prune the scope of the repo down to something we can maintain -> then add tests -> then refactor / improve

This avoidant of recalculation makes some functions dependent on the state of the class (as in "was the data filtered before by some other class method?" ). In general this mutability of the state make things a bit harder to keep track of. For example the original detrend bug was precisely because of this.

Yes, I noticed that as well --> this is not very robust and will lead to bugs

maybe it is because if one deletes a large chunk of code that was covered then the ratio of lines-covered/total-lines changes and the proportion of the updated coverage is lower.

yes, that's it!

@sappelhoff sappelhoff merged commit da8e3a6 into master Sep 4, 2020
@sappelhoff sappelhoff deleted the depracate branch September 4, 2020 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API changing the interace of pyprep
Projects
None yet
Development

Successfully merging this pull request may close these issues.

noisy.py module should be removed
3 participants