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

Improve documentation for PyPREP/MatPREP highpass differences #110

Merged
merged 5 commits into from
Jan 16, 2022

Conversation

a-hurst
Copy link
Collaborator

@a-hurst a-hurst commented Jan 16, 2022

PR Description

Closes #107. This PR updates the docs to contain a better explanation of the differences between PyPREP's and MatPREP's trend removal filters (and why the MNE method is technically preferable).

Wasn't sure if this warranted an addition in the whats_new.rst, but if it does I can definitely add a line!

Merge Checklist

  • the PR has been reviewed and all comments are resolved
  • all CI checks pass
  • (if applicable): the PR description includes the phrase closes #<issue-number> to automatically close an issue
  • (if applicable): bug fixes, new features, or API changes are documented in whats_new.rst

Copy link
Owner

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

perfect :-) Thanks a lot!

@a-hurst
Copy link
Collaborator Author

a-hurst commented Jan 16, 2022

Er, looks like MNE needs an additional module to download test datasets now? Should I add that to this PR as well?

@sappelhoff
Copy link
Owner

ah right 🤔 yes please - we can just add it here: https://github.com/sappelhoff/pyprep/blob/master/requirements-dev.txt

@sappelhoff
Copy link
Owner

although pooch may soon be installed automatically when we install mne, see: mne-tools/mne-python#10199

but no harm in adding it now to merge this PR, and then removing it again some time in the future when we don't need to declare it explicitly anymore.

@a-hurst
Copy link
Collaborator Author

a-hurst commented Jan 16, 2022

Whoops, looks like matplotlib's changed its API on us since last update too. I'll try to tackle that as well.

requirements-dev.txt Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #110 (301c458) into master (12047bf) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #110   +/-   ##
=======================================
  Coverage   99.04%   99.04%           
=======================================
  Files           7        7           
  Lines         733      733           
=======================================
  Hits          726      726           
  Misses          7        7           

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 12047bf...301c458. Read the comment docs.

@sappelhoff sappelhoff merged commit 3e78d7e into master Jan 16, 2022
@sappelhoff sappelhoff deleted the filtfilt_docs_update branch January 16, 2022 17:30
@sappelhoff
Copy link
Owner

Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

filt vs filtfilt (MATLAB implementation)
3 participants