ENH: group triaxial OPM topomaps by orientation#13866
ENH: group triaxial OPM topomaps by orientation#13866PragnyaKhandelwal wants to merge 44 commits into
Conversation
for more information, see https://pre-commit.ci
|
Hi @larsoner....Just a friendly ping on this final wrap-up for the OPM topomaps. I've included visual outputs in the PR description showing the new grouped views. Ready to review whenever you have a chance! |
|
Code looks reasonable at first look. But now I'm wondering whether or not this can be combined / refactored with the mag/grad code for Neuromag systems. The problem is similar there: there are three sensors per location, one radial (magnetometer) and two tangential (gradiometers) and we plot the mags in one plot and the RMS of the gradiometers in another. Do you think it's worth looking into that refactoring in this PR? Or would it be better to review + merge this PR as-is and then refactor afterward? |
|
Thanks @larsoner! I completely agree that unifying the OPM and Neuromag mag/grad logic is the right architectural direction since the underlying radial/tangential problem is so similar. |
|
Yeah that sounds reasonable to me! One last request, can you modify some example(s)/tutorial(s) in a way that shows this new functionality? |
…yaKhandelwal/mne-python into enh-opm-grouping-final-fix
…oad timeout in CI
|
Thanks @larsoner! I've added a new example demonstrating the grouped triaxial OPM topomaps using Well I initially tried using the Ready for final review and merge once these last CI checks turn green! |
Hmmm that's a problem, that's a real dataset and it should work on real data. Can you try running it with memory profiler for example to see if memory usage goes too high? It really shouldn't for evoked data... if you can push a commit that should work (or revert to one that should) I can also look A fake triaxial dataset I think is less useful for people. |
|
Agreed! Real data is definitely better for learning. To avoid stressing the CI with another heavy download, I just moved the visualization into the existing |
|
Ah, I see why the grouped view didn't trigger in the rendered docs! The Since |
|
This one is triaxial I think https://mne.tools/stable/auto_examples/datasets/kernel_phantom.html |
|
Alright, I migrated the example to the |
|
Thanks @larsoner! you're completely right that |
|
Failing tests of |
|
Hey @larsoner, checking back in after my exams! You make a completely fair point about the tangential components needing to be unsigned magnitudes to match Neuromag. Unifying this logic is definitely the right architectural move. Since my GSoC coding period officially kicks off this Monday (focusing heavily on the mffpy refactor), my bandwidth to tackle a major Neuromag/OPM plotting refactor is going to be a bit limited in the short term. > |
nordme
left a comment
There was a problem hiding this comment.
Hi @PragnyaKhandelwal !
First off, thanks for the PR! I have a comment about function design here from a future usability perspective. I think future OPM features are likely to need a generic helper function to merge tangential OPM channel data from triaxial systems. Currently, the _merge_ch_data helper function with the opm modality option uses the _merge_opm_data helper function, which does not do any merging at all, but simply discards the tangential data. Meanwhile, you've helpfully added code here for performing tangential merges, but only within the compute_opm_orientation_topomap_data function, which does not allow future authors to call the tangential channel merging logic for other tasks. I think other contributors coming along later will find this design confusing.
I suggest adding a generic helper function for combining tangential channels that the opm orientation function can call. Also, I suggest renaming _merge_opm_data to something like _restrict_opm_to_rad to distinguish it from your tangential merge function so that other contributors more easily understand the actual function behavior.
| assert tangential == ["OPM002", "OPM003", "OPM005", "OPM006"] | ||
|
|
||
|
|
||
| def test_should_use_opm_orientation_groups_only_for_triaxial(): |
There was a problem hiding this comment.
| def test_should_use_opm_orientation_groups_only_for_triaxial(): | |
| def test_should_use_opm_orientation_groups_for_tri_and_biaxial(): |
for more information, see https://pre-commit.ci
|
Hi @larsoner and @nordme, What this PR now includes: Tangential components now use unsigned RMS magnitude, aligning with the Neuromag grads Per-group colormaps and The
|
|
@harrisonritz did you want to look / comment / try this code? |


Reference issue (if any)
Closes #13781
What does this implement/fix?
Final fix for #13781: adds caller-facing grouped rendering for colocated triaxial OPM channels so orientation information is shown explicitly across visualization entry points.
Implemented:
Additional information
Manual visual check done for:
Evoked.plot_topomapgrouped radial/tangential mapsICA.plot_componentsgrouped radial/tangential titlesA local helper script was used for manual visual smoke-checking on synthetic triaxial OPM data. I can share it if needed.
Visual outputs: