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

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

Merged
merged 4 commits into from
Nov 22, 2024

Conversation

JacPhe
Copy link
Contributor

@JacPhe JacPhe commented Nov 20, 2024

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.

…sub = model_sub.apply(epochs), making use of the regression coefficients from model_sub.
Copy link

welcome bot commented Nov 20, 2024

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

@scott-huberty
Copy link
Contributor

scott-huberty commented Nov 20, 2024

Thanks! @JacPhe Can you edit your top message and add "Fixes #12977 " Please? This is Github's magic way of linking this PR to an opened issue.

@wmvanvliet
Copy link
Contributor

wmvanvliet commented Nov 21, 2024

@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 doc/changes/devel/12978.other.rst with the line:

Fix a mistake in :ref:`_tut-artifact-regression` where the wrong regression coefficients were applied, by `Jac Phe (your name)`_

and then also add your name to doc/changes.inc.

This way, your contribution ends up in the "What's new" list for the next release.

@JacPhe JacPhe changed the title Changed epochs_clean_sub = model_plain.apply(epochs) to epochs_clean_… 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. Nov 21, 2024
@JacPhe JacPhe changed the title 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. 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 Nov 21, 2024
@JacPhe JacPhe requested a review from larsoner as a code owner November 21, 2024 12:44
@@ -0,0 +1 @@
Fix a mistake in :ref:`_tut-artifact-regression` where the wrong regression coefficients were applied, by :newcontrib:`Jacob Phelan`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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 )!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @JacPhe ! As far as I can tell I think the current CI error's are unrelated to the changes in this PR. Let's wait for the devs to take a look.

@wmvanvliet wmvanvliet merged commit 149453c into mne-tools:main Nov 22, 2024
28 checks passed
Copy link

welcome bot commented Nov 22, 2024

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

@wmvanvliet
Copy link
Contributor

Thanks @JacPhe!

larsoner added a commit to larsoner/mne-python that referenced this pull request Dec 6, 2024
* 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)
  ...
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.

Repairing artifacts with regression - Using the wrong regression coefficients
4 participants