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 21 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?
doc/changes/dev/12700.bugfix.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Fix sensor name ordering in :func:`~mne.viz.plot_topomap` when passing an :class:`~mne.Info` object as ``pos`` with ``names`` argument, by `Famous077 <https://github.com/Famous077>`__. No newline at end of file | |||
There was a problem hiding this comment.
Don't need this file, the other is enough
There was a problem hiding this comment.
HI @larsoner , Thank you for the review. I have addressed all the feedbacks:
-Removed the duplicate changelog file 12700.bugfix.rst as suggested
-Updated 13686.bugfix.rst to use :newcontrib: format
-Added regression test test_plot_topomap_info_names_ordering that verifies correct sensor name ordering when passing an Info object as pos with a names argument
Please let me know if anything else needs to be changed.
for more information, see https://pre-commit.ci
mne/viz/tests/test_topomap.py
Outdated
| import numpy as np | ||
|
|
||
| from mne import create_info | ||
| from mne.channels import make_standard_montage | ||
| from mne.viz import plot_topomap |
There was a problem hiding this comment.
None of these should be nested (see how we don't nest things in other tests unless they are non-required dependencies)
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 | ||
| plt.close("all") |
There was a problem hiding this comment.
No need for this, we auto-close mpl figures at the end of all tests
| plt.close("all") |
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 |
3 participants
Reference issue (if any)
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.
: This PR was developed with the assistance of an AI coding assistant for guidance on code structure, docstring formatting, and implementation approach.