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 9 commits intomne-tools:mainfrom
Conversation
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`. |
There was a problem hiding this comment.
@AasmaGupta can you please integrate this suggestion?
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?
mne/channels/tests/test_layout.py
Outdated
| with pytest.raises( | ||
| ValueError, | ||
| match="mixed 10-20 naming conventions", | ||
| match=r"mixed 10.?20 naming conventions", |
There was a problem hiding this comment.
What is the purpose of the regex here? The error message is hard coded, so can we simply just match the beginning of the message, e.g. match="Duplicate EEG electrode positions detected due to mixed 10-20"
There was a problem hiding this comment.
Yeah, that makes sense. I’ve updated the test to simply match the error message and pushed the change.
There was a problem hiding this comment.
@scott-huberty
Just a quick follow-up, I’ve updated the error message and adjusted the test accordingly. Please let me know if this looks good or if any further changes are to be made.
| duplicates_1020 = { | ||
| "T3": "T7", | ||
| "T4": "T8", | ||
| "T5": "P7", | ||
| "T6": "P8", | ||
| } | ||
|
|
||
| names = set(problematic_electrodes) | ||
| duplicate_pairs = [ | ||
| (old, new) | ||
| for old, new in duplicates_1020.items() | ||
| if old in names and new in names | ||
| ] | ||
|
|
||
| if duplicate_pairs: | ||
| raise ValueError( | ||
| "Duplicate EEG electrode positions detected due to mixed 10-20 " | ||
| "naming conventions.\n" | ||
| "You appear to have both legacy (T3/T4/T5/T6) and modern " | ||
| "(T7/T8/P7/P8) electrode names present. The modern convention " | ||
| "(T7/T8/P7/P8) is recommended.\n\n" | ||
| "Please drop the legacy channels before plotting, for example:\n" | ||
| " inst.drop_channels(['T3', 'T4', 'T5', 'T6'])" | ||
| ) |
There was a problem hiding this comment.
| duplicates_1020 = { | |
| "T3": "T7", | |
| "T4": "T8", | |
| "T5": "P7", | |
| "T6": "P8", | |
| } | |
| names = set(problematic_electrodes) | |
| duplicate_pairs = [ | |
| (old, new) | |
| for old, new in duplicates_1020.items() | |
| if old in names and new in names | |
| ] | |
| if duplicate_pairs: | |
| raise ValueError( | |
| "Duplicate EEG electrode positions detected due to mixed 10-20 " | |
| "naming conventions.\n" | |
| "You appear to have both legacy (T3/T4/T5/T6) and modern " | |
| "(T7/T8/P7/P8) electrode names present. The modern convention " | |
| "(T7/T8/P7/P8) is recommended.\n\n" | |
| "Please drop the legacy channels before plotting, for example:\n" | |
| " inst.drop_channels(['T3', 'T4', 'T5', 'T6'])" | |
| ) | |
| # Check for duplicate 10-20 channels that are aliases for the same position | |
| LEGACY_TO_MODERN_1020 = {"T3": "T7", "T4": "T8", "T5": "P7", "T6": "P8"} | |
| names = set(problematic_electrodes) | |
| conflicts = { | |
| old: new | |
| for old, new in LEGACY_TO_MODERN_1020.items() | |
| if old in names and new in names | |
| } | |
| if conflicts: | |
| overlap_info = ", ".join(f"{old}/{new}" for old, new in conflicts.items()) | |
| drop_list = ', '.join(repr(old) for old in conflicts) | |
| raise ValueError( | |
| "The following electrodes are aliases for the same physical location " | |
| f"(10-20 vs 10-10): {overlap_info}\n. To fix this call " | |
| f"`.drop_channels([{drop_list}])` on your Raw/Epochs/Evoked object." | |
| ) |
There was a problem hiding this comment.
A couple notes on that suggestion:
- The error message is now a bit shorter and provides an actionable fix. The
repr(old)just makes sure that e.g.'T3'is printed instead ofT3so that the suggestion is valid Python code. - I added a code comment at the beginning of this code block as a hint to future devs.
- condensed
duplicates_1020to a single line. IMO the new name is more descriptive. - Make sure to remember to update the
match=...In your test! Otherwise it will fail after incorporating this change.
scott-huberty
left a comment
There was a problem hiding this comment.
Sorry for the delay @AasmaGupta almost there! Once you integrate the remaining suggestions I think this will be good to merge.
| duplicates_1020 = { | ||
| "T3": "T7", | ||
| "T4": "T8", | ||
| "T5": "P7", | ||
| "T6": "P8", | ||
| } | ||
|
|
||
| names = set(problematic_electrodes) | ||
| duplicate_pairs = [ | ||
| (old, new) | ||
| for old, new in duplicates_1020.items() | ||
| if old in names and new in names | ||
| ] | ||
|
|
||
| if duplicate_pairs: | ||
| raise ValueError( | ||
| "Duplicate EEG electrode positions detected due to mixed 10-20 " | ||
| "naming conventions.\n" | ||
| "You appear to have both legacy (T3/T4/T5/T6) and modern " | ||
| "(T7/T8/P7/P8) electrode names present. The modern convention " | ||
| "(T7/T8/P7/P8) is recommended.\n\n" | ||
| "Please drop the legacy channels before plotting, for example:\n" | ||
| " inst.drop_channels(['T3', 'T4', 'T5', 'T6'])" | ||
| ) |
There was a problem hiding this comment.
A couple notes on that suggestion:
- The error message is now a bit shorter and provides an actionable fix. The
repr(old)just makes sure that e.g.'T3'is printed instead ofT3so that the suggestion is valid Python code. - I added a code comment at the beginning of this code block as a hint to future devs.
- condensed
duplicates_1020to a single line. IMO the new name is more descriptive. - Make sure to remember to update the
match=...In your test! Otherwise it will fail after incorporating this change.
|
Also @AasmaGupta please create a CircleCI account by signing in via GitHub . After you do this, the build_docs job will run. |
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.