FIX: correct sensor name ordering in plot_topomap when using Info object#13686
FIX: correct sensor name ordering in plot_topomap when using Info object#13686Famous077 wants to merge 29 commits intomne-tools:mainfrom
Conversation
Famous077
requested review from
agramfort,
drammock and
larsoner
as code owners
for more information, see https://pre-commit.ci
|
Hi @agramfort , Would appreciate your review when you get a chance. Happy to update based on your feedbac.k. Thank you |
…mous077/mne-python into fix/plot-topomap-sensor-names
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
Can you add/update some test that would fail on main but pass on this PR?
Also can you please disclose AI usage for this PR?
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
@larsoner Update done. Can you review it again ? Waiting for you feedbacks. |
mne/viz/tests/test_topomap.py
Outdated
|
|
||
| # This must not raise and must display correct sensor names | ||
| fig, _ = plot_topomap(data, info, names=names, show=False) | ||
| assert fig is not None |
There was a problem hiding this comment.
If you want to go the extra mile, you could look at fig.axes[0].texts and .get_text() from the objects and make sure the names match the expected ones!
…p_info_names_ordering
for more information, see https://pre-commit.ci
|
Hi @larsoner, changes have been made according to your suggestions. let me know if anything else needs to be implement. Waiting for you feedback. |
|
Hi @larsoner, I have been working on the 4 failing CI jobs:
Please let me know if anything else needs to be addressed. Thank you |
|
@Famous077 The logs show only It is failing because maptlotlib raises a warning about too many figures being opened at once, and in our docs build, warnings are treated as errors. This comment about not needing to close figures only applies at the end of tests, but may be required between plotting calls within tests to prevent too many figures existing. Please revert all the |
mne/viz/tests/test_topomap.py
Outdated
| displayed_names = [t.get_text() for t in im.axes.texts] | ||
| for name in names: | ||
| assert name in displayed_names, f"{name!r} not found in {displayed_names}" |
There was a problem hiding this comment.
This doesn't check that name ordering is preserved, it just checks that the names are present.
There was a problem hiding this comment.
@ tsbinns I will update shortly.
…mous077/mne-python into fix/plot-topomap-sensor-names
for more information, see https://pre-commit.ci
…mous077/mne-python into fix/plot-topomap-sensor-names
|
Hi @drammock , My old changes only check that the names are present or not, but now it will check both presence and ordering. let me know if I did anything wrong. waiting for you feedback. |
mne/viz/topomap.py
Outdated
| if names is not None: | ||
| names = [names[p] for p in picks] |
There was a problem hiding this comment.
Since here picks is just list(range(len(data))) I don't think this is needed?
mne/viz/tests/test_topomap.py
Outdated
| info = create_info(ch_names=["Fp1", "Fp2", "Fz"], sfreq=1000.0, ch_types="eeg") | ||
| montage = make_standard_montage("standard_1020") | ||
| info.set_montage(montage) | ||
| data = np.array([1.0, 2.0, 3.0]) | ||
| names = ["Fp1", "Fp2", "Fz"] | ||
| im, _ = plot_topomap(data, info, names=names, show=False) | ||
| assert im is not None | ||
| # ADD this instead | ||
| displayed_names = [t.get_text() for t in im.axes.texts] | ||
| assert displayed_names == list(names), ( | ||
| f"Expected {list(names)}, got {displayed_names}" | ||
| ) |
There was a problem hiding this comment.
To actually test the bug for #12700, you need to use MEG data with paired gradiometers (like from the MEG sample data). The conditional:
if any(type_ in ch_type for type_ in ("planar", "grad")):
at line 1289 in mne/viz/topomap.py won't be triggered when using EEG data like this, so it doesn't check that the bug is actually fixed
…st to use MEG gradiometer data
|
Hi @larsoner , I've implemented the required changes as per the suggestions. Could you please review the PR when you get a chance and let me know if anything needs to be updated or improved. Thank you |
3 participants
Fixed issue #12700
What does this implement/fix?
--> When passing an Info object as pos to mne.viz.plot_topomap() with the
names argument, the sensor names were not reordered to match the
reordered channel positions. This caused a mismatch between displayed
names and their actual positions on the topomap.
The fix reorders the names array to match the picks ordering when:
Additional information
--> The data values were always correct , only the displayed sensor names
were mismatched. The fix is minimal and targeted, only affecting the
names reordering when pos is an Info object.