Skip to content

Conversation

@larsoner
Copy link
Member

@larsoner larsoner commented Dec 3, 2024

Closes #12992
Closes #12994

Copy link
Member

@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.

Great!

Note that electrode names are not present in the .fif file so
they are here defined with the convention from VectorView
systems (EEG001, EEG002, etc.)

☝️ this needs to be adjusted in read_dig_fif

And what do you think of adding the recommendation that people save DigMontages with a -dig.fif[.gz] suffix?

I think if we are recommending it for raw, epochs, evoked, info, ... we may as well do it for Montages.

@sappelhoff
Copy link
Member

And one final suggestion (but feel free to disregard any of them, of course, because they are just "nice to haves"):

I think we should add read_dig_fif also to:

https://mne.tools/stable/generated/mne.channels.read_custom_montage.html

as this function is basically a wrapper that is triaging the different read_dig funcs

The function is a helper to read electrode positions you may have in various formats.

larsoner and others added 2 commits December 4, 2024 13:15
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@larsoner
Copy link
Member Author

larsoner commented Dec 4, 2024

@sappelhoff should be good now I think!

Copy link
Member

@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.

Thanks @larsoner!

one error with the testing dataset and a montage file that does not follow the new convention.

@larsoner
Copy link
Member Author

larsoner commented Dec 4, 2024

Argh yes, and it's likely that there will be one or two in examples so I'll push a fix for the test with [circle full]

@larsoner larsoner enabled auto-merge (squash) December 4, 2024 20:38
@larsoner larsoner merged commit 7071a0e into mne-tools:main Dec 4, 2024
24 checks passed
@larsoner larsoner deleted the rt branch December 4, 2024 21:53
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.

Allow for roundtrip between DigMontage.save() and mne.channels.read_dig_fif

2 participants