[ENH, MRG] Add mne.viz.Brain.plot_sensors and refactor mne.viz.plot_alignment

[alexrockhill]

This adds simple sphere plotting to mne.viz.Brain for sensors. The color is controllable by channel name and type as is scale and alpha/opacity.

Hopefully this is helpful to @GuillaumeFavelier to build off for a more advanced version in #8803, I didn't mean to supplant that PR only to provide a simple version to build from.

Here is a simple script to view the results on the sample data:

import os.path as op
import numpy as np
from mne.datasets import sample
import mne

data_path = sample.data_path()
sample_dir = op.join(data_path, 'MEG', 'sample')
subjects_dir = op.join(data_path, 'subjects')

evk = mne.read_evokeds(op.join(sample_dir, 'sample_audvis-ave.fif'))[0]
# should not be needed but see #9638
trans = mne.read_trans(op.join(sample_dir, 'sample_audvis_raw-trans.fif'))
brain = mne.viz.Brain('sample', subjects_dir=subjects_dir, units='mm')
brain.add_skull()
brain.add_head()
brain.add_sensors(evk.info,  trans=trans)

# to compare
mne.viz.plot_alignment(evk.info, surfaces=['brain', 'head'], subject='sample',
                                         subjects_dir=subjects_dir, trans=trans, meg=True, dig=True)

Closes #8803

[larsoner]

My major comment is that I think even without plotting any data values I think this is / should be a larger undertaking involving deduplication (and shared functionality) with plot_alignment. I'd rather not add new code that does 20-50% of what plot_alignment allows for plotting sensors using totally different code (and somewhat different API) to accomplish it...

[larsoner]

... one long-term view / option would be to make Brain the basis for plot_alignment, so that someday plot_alignment is just a think wrapper around Brain constructor + methods. But I think this is a bit of work, so for now I'd rather stick with just calling plot_alignment and Brain separately for the sensor + brain parts, respectively. Maybe once the GUI is done you could work on such a refactoring @alexrockhill :)

[alexrockhill]

My major comment is that I think even without plotting any data values I think this is / should be a larger undertaking involving deduplication (and shared functionality) with plot_alignment. I'd rather not add new code that does 20-50% of what plot_alignment allows for plotting sensors using totally different code (and somewhat different API) to accomplish it...

So I agree and ideally this would be refactored but, after looking at the plot_alignment code, it's kind of this beast of a monolithic block and not modularized at all which made it hard for me to understand what's going on/refactor. The easiest way to do it seems to me from the ground up with replicating results. I might suggest that a smaller PR like this can add the functionality on the way to the larger undertaking like you said about major refactoring to share much more code. I'll work on refactoring a bit more to share, but since plot_alignment can be in "head" or "meg" as well, it actually uses a much more complicated alignment protocol that I don't think ports over well at all.

[larsoner]

I might suggest that a smaller PR like this can add the functionality on the way to the larger undertaking like you said about major refactoring to share much more code

In general I don't like this approach. To me it's much cleaner to take the existing code and refactor it first. Then everything is modularized and understandable, and no functionality has changed. Then the next PR can be to reuse code from the private function(s) now called by plot_alignment in a new add_sensors method.

[alexrockhill]

In general I don't like this approach. To me it's much cleaner to take the existing code and refactor it first. Then everything is modularized and understandable, and no functionality has changed. Then the next PR can be to reuse code from the private function(s) now called by plot_alignment in a new add_sensors method.

Ok, I'll take a shot at refactoring in this PR.

[larsoner]

Sure but let's get the GUI and everything working first. It's a higher priority, and even though combining plot_alignment with Brain is clunky, it works. So this add_sensors to me is mostly just a nicer syntax, not new functionality that we need...

[alexrockhill]

Sure but let's get the GUI and everything working first. It's a higher priority, and even though combining plot_alignment with Brain is clunky, it works. So this add_sensors to me is mostly just a nicer syntax, not new functionality that we need...

For sure, I didn't mean right now :)

[alexrockhill]

Ok, I think this does some really nice refactorization and makes plot_sensors complete as far as the functionality of plot_alignment. The tutorial will fail with a warning about the coordinate frame being unknown for the fNIRS and SEEG channel location examples and who-knows-how-many if/when all the plot_alignments are switched over. I think best would be to fix the set_montage and I can prioritize that first as that will probably solve many of the examples.

I think the coordinate frame handling is a big improvement and clearer to me at least now.

[rob-luke]

@alexrockhill MY APPOLOGIES!! I meant to open a PR to your PR, but instead I have just pushed directly to your branch.

[rob-luke]

Mistake aside, I think the new API significantly simplified the example I just pushed. However, the colours of the optodes have changed (brain too, but I dont mind that). Is this intentional? If so, we should update the tutorial text. But I think the colours for the new version are too similar between sources and detectors (both look red to me).

Old colors

New colors

I also noticed that distance is now in mm? But was previously in m? Was this change also intentional?

[alexrockhill]

@alexrockhill MY APPOLOGIES!! I meant to open a PR to your PR, but instead I have just pushed directly to your branch.

That's okay, you're welcome to push commits.

[alexrockhill]

Mistake aside, I think the new API significantly simplified the example I just pushed. However, the colours of the optodes have changed (brain too, but I dont mind that). Is this intentional? If so, we should update the tutorial text. But I think the colours for the new version are too similar between sources and detectors (both look red to me).

Old colors

New colors

I also noticed that distance is now in mm? But was previously in m? Was this change also intentional?

Brain has a keyword argument units that can be set to either mm or m so that can go either way.

The colors were just my rough matplotlib "gray"/"red"/"brown" but should really be copied directly, I can update that.

[drammock]

@alexrockhill your comment on this PR says it "does some nice refactorization" but as far as I can tell this is still a completely separate implementation from what is done in plot_alignment (with no refactored-out shared helper functions). Am I missing something?

[alexrockhill]

Ok I refactored quite a bit to clean everything up and make nice _freesurfer.py functions.

@alexrockhill your comment on this PR says it "does some nice refactorization" but as far as I can tell this is still a completely separate implementation from what is done in plot_alignment (with no refactored-out shared helper functions). Am I missing something?

Hmmm, yes I haven't gotten to the refactoring plot_alignment, that is quite an undertaking. I was just talking about refactoring from the previous commit so that the functions could be used more easily in plot_alignment in the future.

[drammock]

OK... can you remove MRG from the PR title then? Seems like I needn't have reviewed just yet.

[alexrockhill]

OK... can you remove MRG from the PR title then? Seems like I needn't have reviewed just yet.

I'm giving it a shot modifying plot_alignment now, maybe it won't be so bad. Also this will involve fixing the set_montage not setting the coordinate frame to "head" (#9627) so that might cause widespread changes in examples + tutorials and maybe even sample data (mne sample is in the proper coordinate frame but the misc seeg data is "unknown").

[alexrockhill]

Fixes #9635 now and will fix #9627 in a minute.

I gave it a real try to do larger refactoring but the lack of a requirement of the subjects_dir and subject and the support for "head" and "meg" coordinate frames makes there really not all that much that can be done with the same helper function, unfortunately. I think they are just going to have to rely on their own internal functions for coordinate frames for the most part. Happy to chat about this.

[alexrockhill]

Ok @drammock, it works now and all the tests should pass. Everything is refactored and actually the differences were reconcilable with a bit more effort. You're welcome to review or I'm happy to wait until @larsoner gets back. I just wanted to finish this so that I knew what the _freesurfer.py functions would end up looking like for the GUI so that it wasn't a difficult merge.

[alexrockhill]

Ok so I tried both a _plot_sensors and even a _plot_sensor approach to refactoring the shared code that you both @agramfort and @larsoner pointed out is redundant. The issue is that the refactored code is very inelegant and loses support for the enhanced coloring scheme that I had added to add_sensors. I agree that the function calls are almost identical but the logic is done a different way that makes it very inelegant when combined. In mne.viz.Brain, which is the way I much prefer, things are plotted with boolean flags in smaller steps that put all the pieces together elegantly. In plot_alignment, everything is passed in complicated lists of options. When trying to combine these two styles in one API, the complexity multiplies and it's just a mess.

[larsoner]

In mne.viz.Brain, which is the way I much prefer, things are plotted with boolean flags in smaller steps that put all the pieces together elegantly. In plot_alignment, everything is passed in complicated lists of options. When trying to combine these two styles in one API, the complexity multiplies and it's just a mess

I am still -1 on a divergent API. I'd rather stick to and expand the plot_alignment API somehow rather than introduce a new interface, as above: #9585 (comment)

I'd rather have slightly less clean code in a shared private function than these divergent APIs with non-DRY code

[alexrockhill]

Okay, now the API is the same... there go my nice color plotting options though, guess it's we're just stuck with the defaults

[larsoner]

there go my nice color plotting options though, guess it's we're just stuck with the defaults

For this PR, sure. I think if we want to add color options (which I agree is a good idea and worth doing!) we should figure out how to do it in plot_alignment and Brain.plot_sensors jointly in a follow-up PR

[agramfort]

cc @GuillaumeFavelier this is doing what we discussed yesterday

[larsoner]

No need to take verbose here

[agramfort]

+1

[alexrockhill]

my comment here was concerning the diff of the entire file. What was the big picture logic? add such a function like _plot_sensors that takes a renderer param so Brain and plot_alignment can use the same code to display sensors?

I just wanted to add add_sensors to Brain but @larsoner said I had to refactor plot_alignment to do that ;)

[larsoner]

Looks very close!

[larsoner]

no need to pass verbose here

[larsoner]

No need to take verbose here

[larsoner]

Hopefully the last round...

[larsoner]

Thanks @alexrockhill !

[rob-luke]

Awesome

[larsoner]

@alexrockhill this is wrong:

https://mne.tools/dev/auto_tutorials/forward/35_eeg_no_mri.html#sphx-glr-auto-tutorials-forward-35-eeg-no-mri-py

See how it looks on stable:

https://mne.tools/stable/auto_tutorials/forward/35_eeg_no_mri.html#sphx-glr-auto-tutorials-forward-35-eeg-no-mri-py

I did not check but I strongly suspect this PR broke the projected alignment / coordinate frame. Can you look into it?