[BUG] IndexError in set_montage() for MEG+EEG recordings when digitization is skipped#13700
[BUG] IndexError in set_montage() for MEG+EEG recordings when digitization is skipped#13700Famous077 wants to merge 7 commits intomne-tools:mainfrom
IndexError in set_montage() for MEG+EEG recordings when digitization is skipped#13700Conversation
Famous077
requested review from
agramfort,
drammock,
larsoner and
mscheltienne
as code owners
for more information, see https://pre-commit.ci
Famous077
changed the title
|
@mscheltienne Could you please take a look when you have time? I’d really value your perspective and any suggestions for improvement. |
Famous077
changed the title
IndexError in set_montage() for MEG+EEG recordings when digitization is skipped
mscheltienne
left a comment
mscheltienne
left a comment
There was a problem hiding this comment.
Thanks for taking the time to tackle this old issue! Looks great, a couple of small comments below.
doc/changes/dev/12011.bugfix.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Fix bug where :func:`mne.channels.DigMontage.set_montage` raised an :exc:`IndexError` on MEG+EEG recordings when EEG reference positions were set to the placeholder value ``[1, 0, 0]`` (i.e., digitization was not performed), by :gh:`Famous077`. No newline at end of file | |||
There was a problem hiding this comment.
For the contribution, the :gh: role is legacy element before we used the automatic changelog generation. If you never contributed to MNE, you should use the :newcontrib: role, if you already contributed then simply a link to your author field (which should be added to doc/changes/names.inc).
mne/channels/tests/test_montage.py
Outdated
| import numpy as np | ||
|
|
||
| from mne import EpochsArray, create_info | ||
| from mne.channels import make_standard_montage |
There was a problem hiding this comment.
Imports should be moved at the module-level.
mne/channels/montage.py
Outdated
| if not info["dig"]: | ||
| raise RuntimeError( | ||
| "Cannot determine EEG reference digitization coordinate " | ||
| "frame: info['dig'] is empty. This may happen when a " | ||
| "MEG+EEG recording has EEG reference positions set to a " | ||
| "placeholder value [1, 0, 0] (digitization was not " | ||
| "performed)." | ||
| ) |
There was a problem hiding this comment.
Since you added not np.array_equal(ref_pos[0], [1.0, 0.0, 0.0]), custom_eeg_ref_dig is now False thus this warning is never triggered and is dead-code.
|
Hi @mscheltienne, Thank you for the thorough review and kind words. I have addressed all the feedbacks: -Renamed the changelog file from 12011.bugfix.rst to 13700.bugfix.rst to match the PR number. Please let me know if there is anything else that needs to be changed. Thank you again for your time and guidance. |
2 participants
Reference issue (if any)
Fixed issue #12011
What does this implement/fix?
Calling
set_montage()on a MEG+EEG recording where digitization wasskipped raised an unhelpful
IndexError: pop from empty list.MEG readers write the sentinel value
[1, 0, 0]into the EEG referenceposition (
ch["loc"][3:6]) when no digitization was performed. Thisvalue incorrectly passed all three conditions guarding
custom_eeg_ref_dig = True,which then caused
info["dig"].pop()to crash on an empty list.Two changes in
mne/channels/montage.py:custom_eeg_ref_digcondition to explicitly reject the[1, 0, 0]sentinel (root cause fix).info["dig"].pop()that raises a clearRuntimeErrorifinfo["dig"]is empty, rather than crashingwith an unrelated
IndexError.Additional information
Added regression test
test_set_montage_meg_eeg_no_digitizationinmne/channels/tests/test_montage.pythat reproduces the exact scenariofrom the issue. All 60 existing montage tests continue to pass.