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 14 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!
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?
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. Let me know if this looks good or if any further changes are to be made.
scott-huberty
left a comment
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.
|
Also @AasmaGupta please create a CircleCI account by signing in via GitHub . After you do this, the build_docs job will run. |
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Co-authored-by: Scott Huberty <52462026+scott-huberty@users.noreply.github.com>
for more information, see https://pre-commit.ci
|
Hi @scott-huberty, I’ve integrated the remaining suggestions, updated the error message and test accordingly, and set up CircleCI so the docs build now runs. Please let me know if anything else needs adjustment :) |
mne/channels/tests/test_layout.py
Outdated
| match="Duplicate EEG electrode positions detected due to mixed 10-20", | ||
| ): |
There was a problem hiding this comment.
Hey @AasmaGupta I think you still need to update this match= so that it reflects the newly revised error message!! e.g. "The following electrodes are aliases for the same physical location"..
There was a problem hiding this comment.
Thanks @scott-huberty for pointing that out — I’ve updated the test to match the revised error message. Let me know if anything else needs a fix!
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.