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

ENH: add interpolate_to method #13044

Merged
merged 47 commits into from
Feb 10, 2025
Merged

Conversation

antoinecollas
Copy link
Contributor

@antoinecollas antoinecollas commented Dec 30, 2024

Reference issue (if any)

Closes #12486

What does this implement/fix?

Implements interpolate_to next to interpolate_bads to interpolate EEG data to a given montage

Additional information

Interpolating channels using this implementation has shown to be effective in

Mellot, A., Collas, A., Chevallier, S., Engemann, D. and Gramfort, A., 2024. Physics-informed and Unsupervised Riemannian Domain Adaptation for Machine Learning on Heterogeneous EEG Datasets. EUSIPCO 2024

Copy link

welcome bot commented Dec 30, 2024

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

@antoinecollas antoinecollas marked this pull request as ready for review December 31, 2024 07:45
@antoinecollas antoinecollas changed the title WIP: add interpolate_to method ENH: add interpolate_to method Dec 31, 2024
@contsili
Copy link

contsili commented Feb 7, 2025

Perfect @antoinecollas I can take it from here!

@larsoner I see three options:

  1. Work on Antoine 's branch and push to this PR (I need Antoine to give me push access for that)
  2. Make my own branch based on Antoine's branch and make a new PR.
  3. You merge Antoine's PR with the main MNE branch, then I make my own branch based on main branch and make a new PR.

@drammock
Copy link
Member

drammock commented Feb 7, 2025

I see three options:

option 4: you add @antoinecollas's fork as a new remote, create a branch based off this antoinecollas/interpolate_to branch, push your changes to your own MNE-Python fork on GitHub, then open a PR from your fork to Antoine's fork. That method (often called "PR into your PR") doesn't require Antoine to grant you any special permissions, but it does require that they review & merge your "sub-PR" in order for your commits to show up in this PR.

$ # from within your MNE-Python folder:
$ git remote add antoinecollas [email protected]:antoinecollas/mne-python.git
$ git fetch antoinecollas
$ git checkout -b interpolate_to antoinecollas/interpolate_to
$ # make some changes, make some commits
$ git push -u origin interpolate_to

then on GitHub, open a PR with antoinecollas/mne-python as base repository, interpolate_to as base, contsili/mne-python as head repository, and interpolate_to as head. (see steps 4 & 5 here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork)

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

I think this is actually pretty close to being done, just a few little ideas for improvements and more complete testing. So maybe worth waiting a few days to get this in then continue work with extending it?

mne/channels/channels.py Outdated Show resolved Hide resolved
mne/channels/channels.py Outdated Show resolved Hide resolved
mne/channels/channels.py Outdated Show resolved Hide resolved
mne/channels/channels.py Outdated Show resolved Hide resolved
mne/channels/channels.py Outdated Show resolved Hide resolved
mne/channels/channels.py Show resolved Hide resolved
mne/channels/channels.py Outdated Show resolved Hide resolved
mne/channels/tests/test_interpolation.py Show resolved Hide resolved
@antoinecollas
Copy link
Contributor Author

Thanks for the review, @larsoner. I have taken it into account.

@larsoner
Copy link
Member

Does the CI error make sense to you?

E   AssertionError: 2 nesting errors:
E   Line 997:  ("mne/channels/channels.py", "        from .._fiff.proj import _has_eeg_average_ref_proj", "hierarchy: must not nest _fiff"),
E   Line 1003: ("mne/channels/channels.py", "        from . import DigMontage", "non-explicit relative import"),

if not then we should maybe improve the error messages

@antoinecollas
Copy link
Contributor Author

not super clear...
I changed the imports, is it better now?

@larsoner
Copy link
Member

not super clear...
I changed the imports, is it better now?

Must have been clear enough, because you did fix it 😆 !

I'll do one last review and mark for merge-when-green assuming it looks okay 👍

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Just some tiny tweaks I'll commit, thanks in advance @antoinecollas !

mne/channels/tests/test_interpolation.py Outdated Show resolved Hide resolved
mne/channels/tests/test_interpolation.py Outdated Show resolved Hide resolved
mne/channels/tests/test_interpolation.py Outdated Show resolved Hide resolved
mne/channels/tests/test_interpolation.py Outdated Show resolved Hide resolved
@larsoner larsoner enabled auto-merge (squash) February 10, 2025 17:51
@larsoner larsoner added this to the 1.10 milestone Feb 10, 2025
@larsoner larsoner disabled auto-merge February 10, 2025 19:02
@larsoner larsoner merged commit 7b316cb into mne-tools:main Feb 10, 2025
30 checks passed
Copy link

welcome bot commented Feb 10, 2025

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

@agramfort
Copy link
Member

congrats @antoinecollas for nailing this one !

larsoner added a commit to larsoner/mne-python that referenced this pull request Feb 11, 2025
* upstream/main:
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13110)
  ENH: add interpolate_to method (mne-tools#13044)
  add overwrite and verbose params to info.save (mne-tools#13107)
  Add support for n-dimensional arrays in `_tfr_from_mt` (mne-tools#13104)
  Skip first "New Segment" BrainVision marker (mne-tools#13100)
  MAINT: Use statsmodels pre and fix CircleCI (mne-tools#13106)
  Take units (m or mm) into account when showing fieldmaps on top of brains (mne-tools#13101)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13099)
  MAINT: Update code credit (mne-tools#13093)
  Fix EEGLAB import (nodatchans) (mne-tools#13097)
  MAINT: Fix CircleCI [circle deploy] (mne-tools#13089)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13088)
  Fix signature of some more _close() methods [circle deploy] (mne-tools#13087)
  Fix _close() on MNEAnnotationsFigure and MNESelectionFigure [circle deploy] (mne-tools#13086)
  BUG: Fix bug with Mesa 3D detection (mne-tools#13082)
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.

Add method for channel interpolation to predefined channel positions/montage
6 participants