Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MRG, VIZ, FIX: sEEG picking in _prepare_topomap_plot() #8736

Merged
merged 6 commits into from Jan 14, 2021

Conversation

drammock
Copy link
Member

closes #8733

Not sure if this is a reasonable fix since I've never personally worked with sEEG, but based on general knowledge I'm assuming there's nothing special needed as far as combining sensor pairs? Can someone confirm?

cc @BarryLiu97. On this branch using your code from #8733 I get this figure:

Figure_1

@drammock drammock changed the title fix seeg pick in _prepare_topomap_plot() VIZ, FIX: sEEG picking in _prepare_topomap_plot() Jan 12, 2021
@drammock drammock changed the title VIZ, FIX: sEEG picking in _prepare_topomap_plot() MRG, VIZ, FIX: sEEG picking in _prepare_topomap_plot() Jan 12, 2021
@drammock
Copy link
Member Author

All green, ready for review/merge.

@zhengliuer
Copy link

Great!

@agramfort
Copy link
Member

given the extrapolation artifacts wouldn't it make sense to restrict the topomap to the convex hull of the sensors?

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

And yes in the example something like extrapolate='local' (I think this is what does the convex + padding?) would look better

Comment on lines 115 to 117
with warnings.catch_warnings(): # unit has changed from T/m to V
warnings.simplefilter('ignore')
evoked.set_channel_types(mapping)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically with these we just do:

Suggested change
with warnings.catch_warnings(): # unit has changed from T/m to V
warnings.simplefilter('ignore')
evoked.set_channel_types(mapping)
with pytest.warns(RuntimeWarning, match='unit changed')
evoked.set_channel_types(mapping)

Does the same (plus a bit more, checking that we got the expected one) with fewer lines/imports

@drammock
Copy link
Member Author

Here's what the sample code from #8733 generates now:

Figure_1

@agramfort
Copy link
Member

agramfort commented Jan 13, 2021 via email

@jasmainak
Copy link
Member

Late to the party, does plot_joint even make sense for sEEG data? What can you tell from this figure? I think you should just throw an error.

You need proper support for sEEG and a separate function altogether. Each "stick" plotted in a separate subplot.

@larsoner
Copy link
Member

Even if suboptimal I think it should work. We can create other optimized functions for sEEG visualization that people can can prefer as well, but I don't see a good reason to raise an error here.

@zhengliuer
Copy link

zhengliuer commented Jan 14, 2021

Late to the party, does plot_joint even make sense for sEEG data? What can you tell from this figure? I think you should just throw an error.

You need proper support for sEEG and a separate function altogether. Each "stick" plotted in a separate subplot.

This sample data is not very good for many reasons, like the experimental paradigm is not good enough. So it is normal to see nothing useful, but to sEEG, it is useful if the experimental paradigm is good.
And it should be very good if every shaft has its own subplot

@jasmainak
Copy link
Member

@BarryLiu97 do you have a screenshot or a figure from a publication to share which shows how topoplots are used in sEEG data analysis?

@zhengliuer
Copy link

@BarryLiu97 do you have a screenshot or a figure from a publication to share which shows how topoplots are used in sEEG data analysis?

Sadly, I haven't seen any publication using figures like plot_evoke_joint. But in sEEG, it may make sense if using brain regions as the standard to separate the subplots for the most important feature of sEEG is the ability to get the deep brain structures' information.

@larsoner larsoner merged commit 826ddd2 into mne-tools:master Jan 14, 2021
@larsoner
Copy link
Member

Thanks for the fixes @drammock !

cbrnr pushed a commit to cbrnr/mne-python that referenced this pull request Jan 15, 2021
* fix seeg pick in _prepare_topomap_plot()

* add test

* modernize adjacent test

* update what's new

* simplify test

* use local extrapolation for sEEG
@drammock drammock deleted the fix-seeg-evoked branch January 15, 2021 21:01
larsoner added a commit to vpeterson/mne-python that referenced this pull request Feb 25, 2021
* upstream/master: (66 commits)
  MRG, ENH: Add infant template downloader (mne-tools#8738)
  ENH: add reader for NeuroElectrics .nedf files (mne-tools#8734)
  DOC: improve glossary entry about fiducials (mne-tools#8763)
  MRG, ENH: Add Report.add_custom_css (mne-tools#8762)
  BUG, DOC: read_raw_egi didn't support pathlib.Path; update read_raw() docstring (mne-tools#8759)
  Add "dbs" as new channel type (mne-tools#8739)
  MRG, VIZ: Fix title position in plot_sensors (mne-tools#8752)
  MRG: Support for non-FIFF files in Report.parse_folder (mne-tools#8744)
  MRG, VIZ, FIX: sEEG picking in _prepare_topomap_plot() (mne-tools#8736)
  DOC: don't use single letter variable name in _compute_forward (mne-tools#8727)
  WIP: Fix search [skip github] [skip azp] (mne-tools#8742)
  WIP: Compare Beer-lambert to HOMER (mne-tools#8711)
  MRG: bump spyder version (mne-tools#8020)
  FIX anon with IO round trip (mne-tools#8731)
  fix set_bipolar_reference for Epochs (mne-tools#8728)
  WIP: Add width argument, reduce default (mne-tools#8725)
  ENH: Add toggle-all button to Report (mne-tools#8723)
  fix int/float conversion in nicolet header (mne-tools#8712)
  MRG, BUG: Fix Report.add_bem_to_section n_jobs != 1 (mne-tools#8713)
  MRG, DOC: Make "rank" options in docs more accessible (mne-tools#8707)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ValueError in mne.viz.plot_evoked_joint
5 participants