mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.73k stars 1.32k forks source link

[MAINT] Refactor `mne.viz.Brain` to simplify #9597

Closed alexrockhill closed 3 years ago

alexrockhill commented 3 years ago

So right now mne.viz.Brain is more or less a wrapper around pyvista.mesh with some cool layers and curvatures as well that are really helpful for brains and electrophysiology 3D viewing. The part that I think is overly complicated in the internal representation is the views.

The question is could we just update mne.viz.Brain to be more like matplotlib.pyplot.axes with a mne.viz.BrainFigure? Then, the examples with multiple views can have the user handle two or more brain objects with the equivalent of matplotlib.pyplot.Figure.axes instead of handling them internally with all these complicated views and column and row subplots. That seems like it could be simpler and allow the user more control over which views they want.

larsoner commented 3 years ago

Part of the motivation behind the current show_view API is to mimic what's done with PySurfer. But the more important consideration I think is how Brain differs from the general matplotlib case -- Brain is designed so that each "subplot" shows the same information/data (except maybe separate hemis), just from different views. You .add_data and it affects all subplots the same way. matplotlib allows showing completely different data in each subplot/axis, so having an axis-level API for them makes more sense than it does for Brain.

For this reason re-engineering the class to have matplotlib-like individual-subplot access does not make sense to me, but instead we should perhaps just make the view setting simpler for the user. I think view setting is the only place that we really need individual control over subplots anyway?

drammock commented 3 years ago

FWIW, I recently had to write this helper function in order to set distances (adjacent views were overlapping / getting cut off by the edge of the figure).

def set_brain_view_distance(brain, views, hemi, distance):
    # zoom out so the brains aren't cut off or overlapping
    if hemi == 'split':
        views = np.tile(views, (2, 1)).T
    else:
        views = np.atleast_2d(views).T
    for rix, row in enumerate(views):
        for cix, view in enumerate(row):
            brain.show_view(view, row=rix, col=cix, distance=distance)

I would be quite happy if @alexrockhill's efforts here made it easy to, e.g., set distance on all views at once, without having to loop over rows and columns and re-specify the named view ("lat", "med", etc) and hemi that were already being shown in each cell. So to my mind the problem is not lack of individual "subplot" access --- I already have that with brain.show_view() --- but rather, a way to affect multiple "subplots" at once.

alexrockhill commented 3 years ago

Sounds reasonable, maybe it's already close to the easiest way to do views, I simplified a bit when I added the focal point that might be good

larsoner commented 3 years ago

For this use case (which sounds almost like the opposite of @alexrockhill's initial proposal?) I would add row='all' and col='all' and view=None (meaning: preserve existing unless changed by later kwargs) support. Then you would just have:

brain.show_view(view=None, row='all', col='all', distance=distance)

and ...

I simplified a bit when I added the focal point that might be good

... for this it would be the same just with focalpoint=... at the end EDIT: instead of distance.