ENH: clearer error for mixed 10–20 EEG electrode names#13622
ENH: clearer error for mixed 10–20 EEG electrode names#13622AasmaGupta wants to merge 7 commits intomne-tools:mainfrom
Conversation
AasmaGupta
requested review from
agramfort,
dengemann,
jasmainak and
mscheltienne
as code owners
for more information, see https://pre-commit.ci
|
This PR adds a clearer error message for mixed 10–20 EEG electrode naming |
doc/changes/dev/13447.enhance.rst
Outdated
| @@ -0,0 +1,3 @@ | |||
| Improve the error message raised when mixed 10–20 EEG electrode naming | |||
There was a problem hiding this comment.
This file is mis-named and lacks a :newcontrib: entry, can you look at other examples in that directory? And also you'll need an entry in doc/changes/names.inc
There was a problem hiding this comment.
Yes, I looked into it and have made the new entry under :newcontib: and updated the file.
mne/channels/tests/test_layout.py
Outdated
|
|
||
| with pytest.raises( | ||
| ValueError, | ||
| match="10.?20", |
There was a problem hiding this comment.
I’ve updated the error message to be explicit and deterministic and replaced the .? with “mixed 10–20 naming conventions”. I hope that helps!
| @@ -0,0 +1 @@ | |||
| Improve the error message raised by :func:`mne.viz.plot_topomap` (via :func:`mne.channels.layout._auto_topomap_coords`) when mixed 10–20 EEG electrode naming conventions are detected, by :newcontrib:`Aasma Gupta`. No newline at end of file | |||
There was a problem hiding this comment.
We should only reference public functions in changelogs
| Improve the error message raised by :func:`mne.viz.plot_topomap` (via :func:`mne.channels.layout._auto_topomap_coords`) when mixed 10–20 EEG electrode naming conventions are detected, by :newcontrib:`Aasma Gupta`. | |
| Improve the error message raised by :func:`mne.viz.plot_topomap` when mixed 10–20 EEG electrode naming conventions are detected, by :newcontrib:`Aasma Gupta`. |
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
@britta-wstnr I think you opened the original issue... can you look and see if it does what you had in mind?
| raise ValueError( | ||
| "Duplicate EEG electrode positions detected due to mixed 10–20 " | ||
| "naming conventions.\n" | ||
| "You appear to have both old (T3/T4/T5/T6) and new (T7/T8/P7/P8) " | ||
| "electrode names present.\n\n" | ||
| "Please drop one set before plotting, for example:\n" | ||
| " inst.drop_channels(['T3', 'T4', 'T5', 'T6'])\n" | ||
| "or\n" | ||
| " inst.drop_channels(['T7', 'T8', 'P7', 'P8'])" | ||
| ) |
There was a problem hiding this comment.
@britta-wstnr suggested that we make a decisive suggestion here about which electrodes to keep (T3-T6 or T7-T8), which is probably a good idea. I don't use the 10-20 system much so don't have an opinion. Does anyone else have an opinion?
There was a problem hiding this comment.
I am not sure which set of electrodes should be preferred. I'll be glad to make changes according to your suggestion. Just let me know!
There was a problem hiding this comment.
@larsoner @scott-huberty If any changes need to be made or any other suggestions, I am glad to do the needful!
There was a problem hiding this comment.
@AasmaGupta can you do some research and make a suggestion with respect to which set of channels we should recommend users keep?
There was a problem hiding this comment.
From what I found,
it seems that the newer naming convention (T7/T8/P7/P8) aligns with the updated 10–10 system and is more commonly used in modern EEG datasets and software, while T3–T6 are legacy labels referring to the same positions, so we could recommend keeping the newer set. Let me know if that works, and I'll make the changes.
3 participants
Reference issue (if any)
Fixes #13447
What does this implement/fix?
This PR improves the error raised when plotting topomaps with duplicate EEG
electrode positions caused by mixed 10–20 naming conventions
(T3/T4/T5/T6 vs T7/T8/P7/P8).
Instead of a generic "overlapping positions" error, a clearer and more
actionable message is raised that explains the naming conflict and suggests
how to resolve it by dropping one set of channels.
Additional information
A regression test has been added to ensure the clearer error message is raised
when mixed 10–20 electrode naming conventions are present.