-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changed epochs_clean_sub = model_plain.apply(epochs) to epochs_clean_sub = model_sub.apply(epochs), making use of the regression coefficients from model_sub. #12977 #12978
Conversation
…sub = model_sub.apply(epochs), making use of the regression coefficients from model_sub.
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
@JacPhe you are correct. That is (soon to be "was") a mistake in the tutorial. Thanks for pointing it out to us and even submitting a PR to fix it. Before I merge this, could you also add a new file
and then also add your name to This way, your contribution ends up in the "What's new" list for the next release. |
doc/changes/devel/12978.other.rst
Outdated
@@ -0,0 +1 @@ | |||
Fix a mistake in :ref:`_tut-artifact-regression` where the wrong regression coefficients were applied, by :newcontrib:`Jacob Phelan`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix a mistake in :ref:`_tut-artifact-regression` where the wrong regression coefficients were applied, by :newcontrib:`Jacob Phelan`. | |
Fix a mistake in :ref:`tut-artifact-regression` where the wrong regression coefficients were applied, by :newcontrib:`Jacob Phelan`. |
I think this should fix the CI error's (as seen here )!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
Thanks @JacPhe! |
* upstream/main: (736 commits) ENH: Add round-trip channel name saving (mne-tools#13003) update governance (mne-tools#12896) pin selenium and stop filtering its warning (mne-tools#13000) DOC: fix return value doc of inst.get_montage() (mne-tools#12995) remove some ._filenames uses in favor of .filenames (mne-tools#12996) use eegbci.standardize instead of custom code (mne-tools#12997) Bump autofix-ci/action from dd55f44df8f7cdb7a6bf74c78677eb8acd40cd0a to ff86a557419858bb967097bfc916833f5647fa8c in the actions group (mne-tools#12999) MAINT: Update code credit (mne-tools#12998) MAINT: Unpin VTK (mne-tools#12991) [pre-commit.ci] pre-commit autoupdate (mne-tools#12988) Fix: update cnt.py to check missing annotation (mne-tools#12986) do not log about using set_montage AGAIN if it has never been used (mne-tools#12984) DOC: fix numpy docstr example Vectorizer() (mne-tools#12983) Changed epochs_clean_sub = model_plain.apply(epochs) to epochs_clean_sub = model_sub.apply(epochs), making use of the regression coefficients from model_sub. mne-tools#12977 (mne-tools#12978) DOC: add meegkit to related software suite (mne-tools#12976) DOC: specify tmax_raw param may be None, and what it means (mne-tools#12972) [pre-commit.ci] pre-commit autoupdate (mne-tools#12975) DOC: fix note in legacy pick_channels func (mne-tools#12973) Use `uint16_codec` argument (mne-tools#12971) Bump codecov/codecov-action from 4 to 5 in the actions group (mne-tools#12970) ...
Fixes #12977
Originally, the documentation has epochs_clean_sub = model_plain.apply(epochs), which uses model_plain. This contains regression coefficients for EOG, with some bleed in from adjacent EEG sensors. I believe that it should be using epochs_clean_sub = model_sub.apply(epochs), as this makes use of model_sub, which has the regression coefficients for EOG, with a subtraction of epochs.evoked. I think this makes sense because otherwise, model_sub is completely redundant.