[ENH, MRG] Add mne.viz.Brain.add_volume_labels #9540

alexrockhill · 2021-07-08T14:07:48Z

Here is a very, very rough draft of what I was thinking, I didn't want to go too far in a bad direction so if you have a second to look at it @larsoner, that would be very much appreciated.

Basically, the ROIs are a very different kind of surfaces than the SUBJECTS_DIR/SUBJECT/surf surfaces and so I think need their own class but I think inheritance works nicely; they are named very differently and don't all have hemispheres.

mne/source_space.py

mne/surface.py

mne/viz/_brain/_brain.py

mne/viz/_brain/surface.py

mne/surface.py

alexrockhill · 2021-07-15T06:08:36Z

Ok this is missing all the brain stuff but I was a bit too tired to figure it out tonight. Looks like it works as a minimal version though. I'll add some tests tomorrow or maybe I could get a little help finishing this one because the mne.viz.Brain class is quite complicated and I'm not sure how much I really need to dive in for something this simple. Maybe we can talk about it tomorrow. I'll go ahead and start on the GUI tomorrow too.

larsoner · 2021-07-15T19:12:47Z

Using:

import mne
subjects_dir = mne.datasets.sample.data_path() + '/subjects'
brains = list()
brain = mne.viz.Brain(
    'fsaverage', 'both', 'pial',
    alpha=0.1, cortex='low_contrast', subjects_dir=subjects_dir)
brain.show_view(dict(azimuth=90, elevation=90))
brain.add_aseg()
brain.enable_depth_peeling()

I get:

So I added some smoothing and now I get:

I'll push, then @alexrockhill I think if you add this to tutorials/inverse/60_visualize_stc.py to show that we can now add_aseg in addition to add_annotation then I think we're close to done here!

alexrockhill · 2021-07-15T19:19:16Z

Awesome! Looks great!!

Sure, I can do the last bit when you push your version.

larsoner · 2021-07-15T19:28:20Z

I also feel bad for the poor neglected cerebellum so I added it to the defaults:

larsoner · 2021-07-15T19:28:41Z

I'm done, feel free to push!

alexrockhill · 2021-07-15T19:32:39Z

Wow you put labels on each of the default lut indices, thanks!

larsoner · 2021-07-15T19:33:41Z

Yeah it was a quick copy-paste-trim from the FreeSurfer LUT file. vscode makes it really easy to edit N lines in an identical way :)

alexrockhill · 2021-07-15T22:23:13Z

Hmmm, some camera issues I think

alexrockhill · 2021-07-15T22:56:12Z

Hmmm the electrodes seem to be out-of-scale as well... it might be easier to mock up a quick add_sensors that doesn't support MEG but I don't want to make problems/get in the way of @GuillaumeFavelier

This is with

fig = mne.viz.plot_alignment(epochs.info.pick_channels(picks), trans,
                             'fsaverage', subjects_dir=subjects_dir,
                             surfaces=[])

brain = mne.viz.Brain('fsaverage', alpha=0.1, cortex='low_contrast',
                      subjects_dir=subjects_dir, figure=fig)
brain.add_aseg(aseg='aparc+aseg', rois=rois)

larsoner · 2021-07-15T23:59:48Z

You need units='m' in the brain constructor

larsoner · 2021-07-16T00:01:24Z

(plot_alignment only ever uses meters but brain defaults to millimeters for consistency with freesurfer and pysurfer)

larsoner · 2021-07-16T18:05:44Z

Last run CircleCI complained:

/home/circleci/project/mne/viz/_brain/_brain.py:docstring of mne.viz._brain._brain.Brain.add_aseg:13: WARNING: py:func reference target not found: mne.get_montage_rois
/home/circleci/project/mne/viz/_brain/_brain.py:docstring of mne.viz._brain._brain.Brain.add_aseg:13: WARNING: undefined label: freesurfer lookup table
/home/circleci/project/mne/viz/_brain/_brain.py:docstring of mne.viz._brain._brain.Brain.add_aseg:32: WARNING: py:func reference target not found: pyvista.BasePlotter.add_legend
changes/latest.inc:102: WARNING: py:func reference target not found: mne.viz.Brain.add_eeg_sensors

Not sure if you fixed those in the latest rebase or not...

alexrockhill · 2021-07-16T18:12:42Z

Last run CircleCI complained:

/home/circleci/project/mne/viz/_brain/_brain.py:docstring of mne.viz._brain._brain.Brain.add_aseg:13: WARNING: py:func reference target not found: mne.get_montage_rois
/home/circleci/project/mne/viz/_brain/_brain.py:docstring of mne.viz._brain._brain.Brain.add_aseg:13: WARNING: undefined label: freesurfer lookup table
/home/circleci/project/mne/viz/_brain/_brain.py:docstring of mne.viz._brain._brain.Brain.add_aseg:32: WARNING: py:func reference target not found: pyvista.BasePlotter.add_legend
changes/latest.inc:102: WARNING: py:func reference target not found: mne.viz.Brain.add_eeg_sensors

Not sure if you fixed those in the latest rebase or not...

That's going to depend on #9545 to merge first but other than that, yes taken care of

mne/viz/_brain/tests/test_brain.py

mne/viz/_brain/_brain.py

larsoner · 2021-07-19T13:48:53Z

@alexrockhill this needs a rebase or merge. I went to do it but I think you'll have ideas for how best to combine add_aseg with the other tutorial changes.

Also based on naming consistency I suppose this should be add_volume_labels

larsoner · 2021-07-19T15:49:40Z

@alexrockhill I pushed a commit that cleans up the smoothing and makes the marching cubes faster for labeled volumes following the VTK example https://kitware.github.io/vtk-examples/site/Cxx/Medical/GenerateModelsFromLabels/

larsoner · 2021-07-19T15:51:45Z

... so I think all we need here is a rebase or merge with upstream/main, and rename add_aseg to add_volume_labels then we should be good!

author Alex <aprockhill@mailbox.org> 1626322812 -0700 committer Alex <aprockhill@mailbox.org> 1626466592 -0700 add roi/subcortical plotting wip wip wip slighly working version FIX: Fix units and render ENH: Cerebellum wip add legend, fix seeg tut add legend kwargs, update visualize stc tut fix flake fix doc, fix copy error in tutorial fix func->meth Eric suggestions

larsoner · 2021-07-19T18:51:34Z

... just realized you asked about Brain. We should expose focalpoint in https://mne.tools/stable/generated/mne.viz.Brain.html#mne.viz.Brain.show_view

larsoner · 2021-07-19T18:52:40Z

And it would be nice to color the electrodes differently but that's only available in #9585 not mne.viz.plot_alignment.

In this case I would favor a PR to plot_alignment first that allows channel-by-channel color choosing first, as we'll want it there, too. Then later refactoring as we've talked about.

alexrockhill · 2021-07-19T18:55:28Z

... just realized you asked about Brain. We should expose focalpoint in https://mne.tools/stable/generated/mne.viz.Brain.html#mne.viz.Brain.show_view

Yeah I can open an issue for that too, I was able to just change the view angles for now.

larsoner · 2021-07-19T18:56:16Z

Yeah I can open an issue for that too, I was able to just change the view angles for now.

If it's as easy as adding focalpoint=None to the signature and passing it through to set_3d_view under the hood somewhere, then it can be part of this PR

alexrockhill · 2021-07-19T18:57:55Z

Interestingly, it ran locally for me with the insula label but that was for fsaverage and test_brain uses sample. I don't think it's an issue with the core functionality though.

Actually that's for aparc+aseg and fsaverage and the test is for aseg and sample.

larsoner · 2021-07-19T19:06:48Z

Interestingly, it ran locally for me with the insula label but that was for fsaverage and test_brain uses sample. I don't think it's an issue with the core functionality though.

I suspect it's a problem with the example arguments/usage. Can you fix it?

alexrockhill · 2021-07-19T19:16:43Z

Interestingly, it ran locally for me with the insula label but that was for fsaverage and test_brain uses sample. I don't think it's an issue with the core functionality though.

I suspect it's a problem with the example arguments/usage. Can you fix it?

Sure in a meeting, will fix in 30 minutes

larsoner · 2021-07-19T19:35:42Z

Sure in a meeting, will fix in 30 minutes

Sounds good, FYI two examples appear broken now:

https://app.circleci.com/pipelines/github/mne-tools/mne-python/9151/workflows/e75f8447-9f58-40e4-b333-38bd240c79b9/jobs/31313

  File "/home/circleci/project/tutorials/inverse/60_visualize_stc.py", line 163, in <module>
    brain.add_volume_labels(aseg='aparc.a2009s+aseg', labels=labels)
  File "/home/circleci/project/mne/viz/_brain/_brain.py", line 2391, in add_volume_labels
    warn(f'Value not found for label {repr(label)} in: '
  File "/home/circleci/project/mne/utils/_logging.py", line 358, in warn
    warnings.warn_explicit(
RuntimeWarning: Value not found for label 'ctx_lh_G_occipital_sup' in: /home/circleci/mne_data/MNE-sample-data/subjects/sample/mri/aparc.a2009s+aseg.mgz
...
  File "/home/circleci/project/tutorials/clinical/20_seeg.py", line 178, in <module>
    brain.add_volume_labels(aseg='aparc+aseg', labels=labels)
  File "/home/circleci/project/mne/viz/_brain/_brain.py", line 2391, in add_volume_labels
    warn(f'Value not found for label {repr(label)} in: '
  File "/home/circleci/project/mne/utils/_logging.py", line 358, in warn
    warnings.warn_explicit(
RuntimeWarning: Value not found for label 'ctx-lh-caudalmiddlefrontal' in: /home/circleci/mne_data/MNE-sample-data/subjects/fsaverage/mri/aparc+aseg.mgz

alexrockhill · 2021-07-19T20:19:55Z

I think some changes maybe to smoothing have broken it, it's not finding any labels not just insula. Looking into it

larsoner · 2021-07-19T20:33:05Z

I think some changes maybe to smoothing have broken i

The failure is in the add_volume_labels step, which should have nothing to do with ~~labels~~ smoothing, just with the contents of aparc+aseg.mgz and the FreeSurfer LUT. Or am I missing something?

alexrockhill · 2021-07-19T20:38:02Z

I think some changes maybe to smoothing have broken i

The failure is in the add_volume_labels step, which should have nothing to do with ~~labels~~ smoothing, just with the contents of aparc+aseg.mgz and the FreeSurfer LUT. Or am I missing something?

I'm going to try putting things back before your latest marching cubes change (only the first smooth), if that fixes things then we can do the discrete part in another PR.

larsoner · 2021-07-19T20:48:28Z

I'm going to try putting things back before your latest marching cubes change (only the first smooth), if that fixes things then we can do the discrete part in another PR.

Feel free, but please preserve the warning. I suspect things might have been failing silently before (because we just silently didn't bother trying to do marching cubes before my commits) and the warning makes it clear that something is wrong somewhere (we shouldn't in an example ask to plot a volume label that does not exist in the mgz)

larsoner · 2021-07-19T20:49:24Z

mne/viz/_brain/_brain.py

-        for label, color, (verts, triangles) in zip(labels, colors, surfs):
-            if len(verts) == 0:  # not in aseg vals
-                warn(f'Value not found for label {repr(label)} in: '
-                     f'{aseg_fname}')


This is the warning we need to preserve

mne/viz/_brain/_brain.py

alexrockhill · 2021-07-19T20:51:28Z

I'm going to try putting things back before your latest marching cubes change (only the first smooth), if that fixes things then we can do the discrete part in another PR.

Feel free, but please preserve the warning. I suspect things might have been failing silently before (because we just silently didn't bother trying to do marching cubes before my commits) and the warning makes it clear that something is wrong somewhere (we shouldn't in an example ask to plot a volume label that does not exist in the mgz)

Good call, it does look like what was broken was from the marching cubes changes though. I didn't really delve into it fully but when I reverted all the surfaces rendered properly

alexrockhill · 2021-07-19T21:05:53Z

So there was an issue with ctx-insula not being found and that was because the atlas was changed to aseg which has a subset of labels, not including the ctx ones

larsoner · 2021-07-19T21:06:31Z

Good call, it does look like what was broken was from the marching cubes changes though. I didn't really delve into it fully but when I reverted all the surfaces rendered properly

Did you verify this by locally testing with some PATTERN=... make html_dev-pattern-memory as suggested above (or even just running python -i tutorials/clinical/... to see if there is a warning)? You should get in the habit of doing this (example testing) as well as checking pytest -- a991d32 for example almost certainly broke pytest mne/tests/test_surface.py -k march because it reverted the array-like handling but not the tests I pushed for it. Try to get in the habit of testing these things locally before each push because:

It will make PR cycles shorter, and
We do end up with a backlog of CI runs in general

So pushing commits that uniformly keep CIs green is better than pushing commits without checking locally that things work first. The CIs are really meant to act as a sanity / completeness check of things that have been checked locally.

So there was an issue with ctx-insula not being found and that was because the atlas was changed to aseg which has a subset of labels, not including the ctx ones

Can the discrete support be restored, or is there also an issue with the smoothing there?

alexrockhill · 2021-07-19T21:09:18Z

Yeah sorry, got a bit hasty with the fix for the last PR being a priority.

I checked and running locally there are no warnings for the seeg tutorial with this version, all the CIs should come back green.

Can the discrete support be restored, or is there also an issue with the smoothing there?

There was an issue with the atlas in the tests but also a larger issue with the discrete method, I don't think it was working...

larsoner · 2021-07-19T21:41:06Z

There was an issue with the atlas in the tests but also a larger issue with the discrete method, I don't think it was working...

Indeed it failed to find some ROIs, if I force it to use VTK_DOUBLE it seems to work fine! But like you said I can open a separate PR for this once this one is merged.

larsoner · 2021-07-19T22:48:53Z

Not waiting for macOS on this one, thanks @alexrockhill !

alexrockhill · 2021-07-19T22:48:56Z

Sounds great, looks like it's good to go

alexrockhill changed the title ~~[ENH, WIP] Add Subcortical Surface Plotting~~ [ENH, WIP] Suggested API changes to allow subcortical and other ROI plotting Jul 8, 2021

larsoner reviewed Jul 8, 2021

View reviewed changes

alexrockhill force-pushed the subcort branch from 783651f to f33f077 Compare July 15, 2021 06:09

alexrockhill changed the title ~~[ENH, WIP] Suggested API changes to allow subcortical and other ROI plotting~~ [ENH, WIP] Add mne.viz.Brain.add_aseg to plot anatomical segmentation surfaces Jul 15, 2021

alexrockhill force-pushed the subcort branch from 4db3d19 to 975672c Compare July 16, 2021 04:03

alexrockhill changed the title ~~[ENH, WIP] Add mne.viz.Brain.add_aseg to plot anatomical segmentation surfaces~~ [ENH, MRG] Add mne.viz.Brain.add_aseg to plot anatomical segmentation surfaces Jul 16, 2021

alexrockhill force-pushed the subcort branch from 21d8860 to 60b2b73 Compare July 16, 2021 17:58

larsoner reviewed Jul 16, 2021

View reviewed changes

alexrockhill force-pushed the subcort branch from 0819a81 to 6fd678c Compare July 16, 2021 20:17

larsoner mentioned this pull request Jul 19, 2021

[MRG, ENH] Find aseg labels near montage #9545

Merged

larsoner changed the title ~~[ENH, MRG] Add mne.viz.Brain.add_aseg to plot anatomical segmentation surfaces~~ [ENH, MRG] Add mne.viz.Brain.add_volume_labels Jul 19, 2021

alexrockhill added 3 commits July 19, 2021 10:04

fix arg

3c8a5d9

merge changes

1fe41e8

fix flake

b1844db

fix tut

7603c4f

revert discrete changes

a991d32

larsoner reviewed Jul 19, 2021

View reviewed changes

mne/viz/_brain/_brain.py Outdated Show resolved Hide resolved

alexrockhill added 3 commits July 19, 2021 13:52

add back warning

6733f18

fix flake

392fe38

aseg doesn't have ctx, only aparc+aseg, fix

f4e4fe6

put back marching cubes test

f477e60

larsoner merged commit 3a4a048 into mne-tools:main Jul 19, 2021

alexrockhill deleted the subcort branch July 19, 2021 23:52

Uh oh!

[ENH, MRG] Add mne.viz.Brain.add_volume_labels #9540

[ENH, MRG] Add mne.viz.Brain.add_volume_labels #9540

Uh oh!

Conversation

alexrockhill commented Jul 8, 2021 • edited by larsoner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexrockhill commented Jul 15, 2021

Uh oh!

larsoner commented Jul 15, 2021

Uh oh!

alexrockhill commented Jul 15, 2021

Uh oh!

larsoner commented Jul 15, 2021

Uh oh!

larsoner commented Jul 15, 2021

Uh oh!

alexrockhill commented Jul 15, 2021

Uh oh!

larsoner commented Jul 15, 2021

Uh oh!

alexrockhill commented Jul 15, 2021

Uh oh!

alexrockhill commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Jul 15, 2021

Uh oh!

larsoner commented Jul 16, 2021

Uh oh!

larsoner commented Jul 16, 2021

Uh oh!

alexrockhill commented Jul 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larsoner commented Jul 19, 2021

Uh oh!

larsoner commented Jul 19, 2021

Uh oh!

larsoner commented Jul 19, 2021

Uh oh!

larsoner commented Jul 19, 2021

Uh oh!

larsoner commented Jul 19, 2021

Uh oh!

alexrockhill commented Jul 19, 2021

Uh oh!

larsoner commented Jul 19, 2021

Uh oh!

alexrockhill commented Jul 19, 2021

Uh oh!

larsoner commented Jul 19, 2021

Uh oh!

alexrockhill commented Jul 19, 2021

Uh oh!

larsoner commented Jul 19, 2021

Uh oh!

alexrockhill commented Jul 19, 2021

Uh oh!

larsoner commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexrockhill commented Jul 19, 2021

Uh oh!

larsoner commented Jul 19, 2021

Uh oh!

larsoner Jul 19, 2021

Choose a reason for hiding this comment

alexrockhill commented Jul 8, 2021 •

edited by larsoner

Loading

alexrockhill commented Jul 15, 2021 •

edited

Loading

larsoner commented Jul 19, 2021 •

edited

Loading