[BUG] `mne.viz.Brain` volume rendering doesn't show up

[alexrockhill]

If you don't set alpha=0, you get an opaque inflated brain along with your volume source estimate. I think the default arguments could handle this better.

To replicate:

import mne
from mne.datasets import sample
from mne import read_evokeds
from mne.minimum_norm import apply_inverse, read_inverse_operator

data_path = sample.data_path()
meg_path = data_path / 'MEG' / 'sample'
fname_inv = meg_path / 'sample_audvis-meg-vol-7-meg-inv.fif'
fname_evoked = meg_path / 'sample_audvis-ave.fif'

snr = 3.0
lambda2 = 1.0 / snr ** 2
method = "dSPM"  # use dSPM method (could also be MNE or sLORETA)

# Load data
evoked = read_evokeds(fname_evoked, condition=0, baseline=(None, 0))
inverse_operator = read_inverse_operator(fname_inv)
src = inverse_operator['src']

# Compute inverse solution
stc = apply_inverse(evoked, inverse_operator, lambda2, method)
stc.crop(0.0, 0.2)

brain = mne.viz.plot_source_estimates(stc, subject='sample', subjects_dir=data_path / 'subjects', src=src)

[alexrockhill]

It looks like @larsoner and @GuillaumeFavelier have modified this recently from the git blame. I haven't worked with this a ton so I'm happy to write a PR but it would be nice to get advice on what should be done.

[larsoner]

This is expected. The inflated brain distorts geometry in a way that is incompatible with volumetric structures. Maybe we should have a warning if you add_data with volumes while you have surf='inflated'.

[alexrockhill]

I don't think that when you call mne.viz.plot_source_estimates there should be an inflated hemisphere out of place as well as the expected volume source estimate. Are you sure about that @larsoner?

[larsoner]

The inflated brains don't have correct geometry like real left/right hemis, they are synthetic. So even if you shift it to the side (which we could do) it will overlap with the volumes in an unrealistic way. So I guess we could implement the same sideways shift logic we have for when hemi='both' and either use it when volumes are present (or more simply but not backward compatible, always use it) but either way I think we need to warn the user that they are doing something that will probably not work properly.

[alexrockhill]

Maybe the default should just be to set the alpha on the inflated brain to 0 if we have to have it for the Brain object to work, but probably better would be to use a pial surface or something as default that aligns with the source estimate.

[alexrockhill]

Did we ever find a solution on this? The default for mne.viz.plot_source_estimates doesn't have to be to use the inflated brain at all. We could just use the pial surface with some reasonable alpha by default and maybe also fix if inflated gets used that it gets shifted appropriately. I'll go ahead and make a PR for that unless there are better ideas.

[alexrockhill]

Ok, I tried to change the defaults but man, this interaction between functions like plot_source_estimates and mne.viz.Brain is really complex and seems impossible to maintain. When I change it to pial and alpha, I get no volume rendering at all and a pial surface with a blue sphere at the peak activation location.

@larsoner, what would you think about breaking up the add_data function into more manageable units and steering users toward that rather than these monolith plot_alignment and plot_source_estimates functions? I personally really prefer methods like brain.add_head which breaks things up into manageable units. There's just so many arguments to add_data it seems impenetrable. Especially when I change the surface arguments and instead of getting a transparent colored volume, I get a single blue sphere, to me that doesn't make any sense. Things like brain.add_colorbar could be split out of brain.add_data also.

[alexrockhill]

import mne
from mne.datasets import sample
from mne import read_evokeds
from mne.minimum_norm import apply_inverse, read_inverse_operator

data_path = sample.data_path()
meg_path = data_path / 'MEG' / 'sample'
fname_inv = meg_path / 'sample_audvis-meg-vol-7-meg-inv.fif'
fname_evoked = meg_path / 'sample_audvis-ave.fif'

snr = 3.0
lambda2 = 1.0 / snr ** 2
method = "dSPM"  # use dSPM method (could also be MNE or sLORETA)

# Load data
evoked = read_evokeds(fname_evoked, condition=0, baseline=(None, 0))
inverse_operator = read_inverse_operator(fname_inv)
src = inverse_operator['src']

# Compute inverse solution
stc = apply_inverse(evoked, inverse_operator, lambda2, method)
stc.crop(0.0, 0.2)

brain = mne.viz.plot_source_estimates(stc, subject='sample', subjects_dir=data_path / 'subjects',
                                      src=src, surface='pial', alpha=0.1)

[alexrockhill]

Ah okay one thing that was complicating matters is that on the current main, the volume source estimate coloring that's in the first screenshot above no longer shows up with the first minimally reproducible script above so somehow that got broken lately.

[larsoner]

Argh yes we should fix that bug first. Maybe doing a plot that currently on broken main produces an all black screenshot would be a good way to test, because it's easy to check in CIs (e.g., that the .mean() of the screenshot is greater than 0.005 for example)

[larsoner]

Things like brain.add_colorbar could be split out of brain.add_data also.

I suggest we start this way by refactoring parts of the function into new more manageable pieces. Eventually if brain.add_data is just a bunch of calls to simpler functions that's great. I suspect the problem will be that some of these steps/parts will interact, but maybe we can account for that by making the new methods pay attention to the current Brain state to "do the right thing". We'll see in the PRs I guess!

[alexrockhill]

What do you think of deprecating (eventually) brain.add_data? It just looks like it takes in a bunch of arguments that together make up an stc and it might make a lot more sense just to pass an stc. Then, if you wanted to add an array, you'd have to make an stc and src if volume-based. I think that's really what works with MNE most elegantly. We could leave brain.add_data in there for a long time but slowly port over dependencies from plot_source_estimates etc.

[alexrockhill]

I think it would dramatically simplify the API to just have two lines of code making a brain object and calling brain.add_stc rather than going though this external API in _3d.py. This would be a pretty big refactor though, it would be nice to break it up into more manageable parts. I'll try and make some progress and at least fix the bug soonish.

In the nearer term, it might make sense to make the functions in mne.viz._3d internal functions since they don't yield nice plots with default arguments so that people know to use stc.plot.

[alexrockhill]

Ok, I have no idea why the volume isn't rendering now. I think it's an issue with the volume_pos mapper but I tried setting the resolution to None and that didn't fix it. I know the VTK debugging is pretty tricky so I might leave this bug fix for whomever wrote that part.

[alexrockhill]

I thought about it a bit more and maybe a major refactoring isn't the best use of time either, since stc.plot and stc.plot_3d work just fine. My suggestion would be to make plot_source_estimates and internal function since it's not maintained to give the right output for every class given the input. I checked and it's not used in any tutorials or examples. It would need a deprecation cycle but it's quite confusing to try and use those when they are really just helper functions for the stc methods. Does that sound reasonable? Otherwise this could probably just be closed once the bug is fixed.

[larsoner]

I thought about it a bit more and maybe a major refactoring isn't the best use of time either,

Agreed, seems like something we can do later

My suggestion would be to make plot_source_estimates and internal function since it's not maintained to give the right output for every class given the input

plot_source_estimates has been around since at least MNE-Python 0.5, i.e., 10 years:

https://github.com/mne-tools/mne-python/blob/2de217d600e5b198820adb992d5f1b8bda2b006e/mne/viz.py#L875

And while a lot of these will be false positives, the "2k" count here gives me pause as well:

https://github.com/search?q=plot_source_estimates&type=code

So I'd rather fix than deprecate. Or the easiest route is to just .. note:: after the one-line description of the function that it isn't guaranteed yet to give the correct output for each input, and that stc.plot for surface and stc.plot_3d for vol source estimates should be preferred for optimal behavior