mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

RFC, MAINT: changes to 2D plotting

Open drammock opened this issue 4 years ago β€’ 18 comments

This issue is for planning changes to our 2D plotting capabilities (roadmap link). As a starting point, here are (I think?) all the functions/methods that make 2D plots, grouped roughly by the kind of plot they generate. Also included is a section for 3D plots that always use matplotlib rather than mayavi/pyvista. Each section is followed by some comments / proposals about what might be done. Some of the proposed changes are big, others are quite minor. When folks have weighed in, I'll convert it to a consolidated checklist for keeping track of which ones are done.

Browse-type windows (scrollable data)

  • raw.plot
  • epochs.plot
  • ica.plot_sources(raw)

possible changes

  1. Anything we can do to speed up the responsiveness of scrolling / scaling of browse-type windows would be desirable.
  2. ~~One possible approach to unifying the underlying code would be a new class MNEBrowseFigure that inherits from matplotlib.figure.Figure and automatically does a bunch of stuff in its __init__ like generating the scrollbars and buttons, setting margins, etc. This would also allow the (somewhat unwieldy) params dictionaries that are passed back and forth to be instead stored as attributes of the Figure object (nested under an mne attribute so they can be accessed as fig.mne.property_name, thereby segregating the MNE-specific attributes from the matplotlib ones).~~ done in #7955
  3. ~~Streamline all browse-type windows (e.g. same color for bad channels/components, same behavior for right/left-clicking on traces/labels, etc.)~~ see #6531. done in #8381
  4. unify API:
    • ~~raw.plot() has event_color, epochs.plot() has event_colors, and they behave differently. Standardize on the name and behavior of raw, because (1) only changes API in one place, and (2) behavior of raw.plot(..., event_color) is more flexible / better than the equivalent arg in epochs.plot()~~ done in #8381
  5. ~~make annotation text at top toggleable (#8578)~~ addressed in #8624

Interactive lineplots (click-drag to pop up scalp topographies, click to show channel names)

  • raw.plot_psd
  • epochs.plot_psd
  • evoked.plot
  • evoked.plot_white
  • evoked.plot_joint

possible changes

  1. evoked.plot_joint currently generates separate figures for each channel type; all the others generate a single figure with subplots for each channel type. It's worth considering whether there's a way for plot_joint to be single-figure like the others.
  2. ~~all of these have options for spatial_colors; the head circle in the inset axes gets stretched to an ellipse when the window is resized. Should fix the aspect ratio of the inset axes if possible.~~ done in #8545

scalp field maps (possibly w/ trellis)

  • {raw/epochs/evoked/mne.viz}.plot_projs_topomap
  • epochs.plot_psd_topomap
  • evoked.plot_topomap
  • ica.plot_components
  • mne.preprocessing.corrmap
  • mne.viz.plot_arrowmap
  • mne.viz.plot_tfr_topomap

possible changes

  1. make sure all of these have vlim='joint' option
  2. when vlim='joint', there should only be 1 colorbar on far right edge (currently still get 1 per axes)
  3. all of these should have the same trellis-style capability (nrows, ncols)
  4. consistent colorbar labelling (some have units, some don't)
  5. ~~fix #8563~~ done in #8589
  6. fix #4122

image plots

  • epochs.plot_image (separate fig per ch_type, has ERP/ERF w/ confint in subplot)
  • evoked.plot_image (one fig for all ch_types)
  • mne.viz.plot_source_spectrogram
  • mne.viz.plot_cov (two figs: cov matrices; SVD spectra)
  • mne.viz.plot_csd

possible changes

  1. does plot_cov need to return 2 figures? Why not one 2x3 fig instead of two 1x3 figs?
  2. is there value in plot_cov having a vlim='joint' option?
  3. can epochs.plot_image be made to work as a single fig?
  4. for some reason plot_csd defaults to viridis colormap. Consistency would dictate that it use Reds like we do for one-sided data everywhere else.

many axes in a topo-style layout

  • raw.plot_psd_topo
  • epochs.plot_topo_image (w/ colorbar)
  • evoked.plot_topo
  • mne.viz.plot_compare_evokeds(axes='topo')

possible changes

  1. raw.plot_psd_topo defaults to black background and "click to magnify" pops up a white-on-black PSD trace for that channel. In contrast, epochs.plot_topo_image does a white-on-black overview but pops up normally-colored magnifications, while evoked.plot_topo does a white-background overview and normally-colored magnifications. These should be consistent (default to white BG always).
  2. plot_compare_evokeds(axes='topo') will generate separate figures for each channel type. Since this approach to topo plotting can be extremely slow, it's worth considering to throw an error if this is requested with multiple ch_types.

layouts / montages

  • {raw/epochs/evoked/mne.viz}.plot_sensors(kind='topomap')
  • mne.viz.plot_montage(kind='topomap')
  • mne.viz.plot_layout

possible changes

  1. similar to the discussion about being too MEG-centric in #7710, why should raw.plot_sensors() default to 'mag'?
  2. raw.plot_sensors(ch_groups='position') doesn't actually color-code the 8 different regions?

barplots

  • epochs.plot_drop_log
  • ica.plot_scores

possible changes

  1. plot_drop_log blocks execution, but is not interactive in any way. It should not block.
  2. epochs.plot_drop_log default title is "unknown", should be "unknown subject" to be consistent with the corresponding mne.viz function

static time series (possibly multi-panel)

  • ica.plot_overlay
  • ica.plot_sources(evoked)
  • mne.viz.plot_compare_evokeds
  • mne.viz.plot_dipole_amplitudes
  • mne.viz.plot_filter
  • mne.viz.plot_ideal_filter
  • mne.viz.plot_head_positions
  • mne.viz.plot_snr_estimate

possible changes

  1. plot_compare_evokeds is only interactive if axes='topo', and should not otherwise block execution
  2. TODO: many in this category are ones I rarely use, should revisit after more extensive testing
  3. plot_compare_evokeds does not have a scalings parameter (#8556)

misc

  • ica.plot_properties (4 heterogeneous subplots)
  • mne.viz.plot_bem (images + overlaid curves, in a trellis)
  • mne.viz.plot_connectivity_circle (highly custom)
  • mne.viz.plot_events
  • various help windows for the interactive plots
  • settings windows for turning proj on and off

possible changes

  1. Similar to the browse-style proposal, some lightweight custom figure classes might help here to streamline the code (i.e., for the help windows and proj settings windows)
  2. I'm kind of surprised that matplotlib doesn't have built-in chord diagram support. We should consider spinning off our plot_connectivity_circle into a more general function and get it into matplotlib, so others can use it (and to lower our maintenance burden). See also this implementation. (Surprisingly, both Bokeh and Plotly used to have chord diagrams and apparently no longer do? Seems like circos and D3 (and an R wrapper of D3) are the only maintained implementations nowadays)
  3. examine the usage of _prepare_trellis for plot_bem; it's rather unlike all the other uses of trellis in the codebase. Probably nothing wrong with it, but worth looking at / keeping in mind if changes to _prepare_trellis happen for other reasons.

matplotlib 3D

  • mne.viz.plot_dipole_locations(mode='orthoview')
  • mne.viz.plot_montage(kind='3d')
  • mne.viz.plot_sensors(kind='3d')

possible changes

  1. defaults seem to be a little different in plot_montage vs plot_sensors. Make them consistent / deduplicate where possible.

drammock avatar May 07 '20 22:05 drammock

thanks for looking into this

agramfort avatar May 08 '20 07:05 agramfort

Nice summary! I suggest that when you go to actually work on one of these (whichever you want first), start a new issue with any questions. If we try to discuss everything here it will probably become unwieldy.

That being said, just a few comments to get things going:

Anything we can do to speed up the responsiveness of

I've looked at this a number of times and I'm not optimistic, MPL seems to be the bottleneck and I don't see us moving away from it

MNEBrowseFigure

This would be a lot of work but I think it would be worth it. I've wanted features of raw plotting that aren't available in the epochs plotting, and this would fix it.

does plot_cov need to return 2 figures? Why not one 2x3 fig instead of two 1x3 figs?

No good reason that I can see assuming you make some nice gridspec layout. As long as people can pass axes it's easy to get the old behavior back

larsoner avatar May 08 '20 20:05 larsoner

I just tried to work on #6531, but getting both raw and ica plots behave consistently is pretty difficult at the moment. Using gray for bad channels and bad components is not a problem, but assigning left and right clicks on labels to different functions depending on whether we plot raw or ica data would definitely benefit from refactoring the plotting functions.

Would it be OK to include #6531 here?

cbrnr avatar May 12 '20 14:05 cbrnr

Would it be OK to include #6531 here?

definitely. Cross-linking it should be sufficient, but free to edit the initial comment above to provide a summary of that issue / more details if you like.

drammock avatar May 12 '20 15:05 drammock

crossref #7850 - problems with plot_raw_sensors:

  • axes title vs suptitle
  • seems to do tight_layout or similar adjustment when it shouldn't
  • callbacks appear to be linked between subplots?

cc @hoechenberger

drammock avatar May 29 '20 14:05 drammock

Hey @drammock, thanks for working on this!

One thing I've been wondering for a while now is why for

Browse-type windows (scrollable data)

I cannot click-and-drag to select & view e.g. a topoplot & the PSD of a Raw segment? Wouldn't this be a useful functionality when one is outside of annotation mode?

hoechenberger avatar Jul 03 '20 12:07 hoechenberger

I just skimmed over the Matplotlib 3.3 changelog and it seems some of the new features could really help simplify some of our code. I particularly like the semantic axes layout based on ASCII art! And tight_layout no longer forgetting about suptitle :)

hoechenberger avatar Jul 18 '20 06:07 hoechenberger

I just skimmed over the Matplotlib 3.3 changelog and it seems some of the new features could really help simplify some of our code.

Unfortunately we can't rely on people having these features / 3.3 until it is our minimum supported version. We typically support releases up to ~2 years back. So set up a reminder notification for 2022 :)

We maybe could backport some of the fixes/changes, but given the complexity of matplotlib I'm not sure how feasible it would be.

And tight_layout no longer forgetting about suptitle :)

I have played a tiny bit with constrained_layout and it seemed to work quite well, too.

larsoner avatar Jul 18 '20 12:07 larsoner

We typically support releases up to ~2 years back.

Do we have a strict policy on this? I've recently read about NEP 29 and was wondering if we should adopt something similar.

drammock avatar Jul 18 '20 21:07 drammock

I'm also wondering, since we're dropping Python 3.5 support with the next release and all… A little bit of additional change probably won't hurt? Also, was there actually ever anybody out there seriously using Python 3.5 after 3.6 had been released? Sounds like a masochistic endeavour... This just to say, I'm not sure if it's really necessary and benefitting our users if we're super strict about this "2-year rule".

hoechenberger avatar Jul 19 '20 07:07 hoechenberger

the argument for not going to fast is that some users want to use mne on a linux server where python versions are the stable ones from apt or yum etc. It's never as up to date as conda. Not everybody is admin on his computing infracstructure

agramfort avatar Jul 19 '20 07:07 agramfort

I understand the reasoning, but both pip and conda can be used without admin permissions.

cbrnr avatar Jul 19 '20 07:07 cbrnr

can you change the python version with pip?

agramfort avatar Jul 19 '20 07:07 agramfort

can you change the python version with pip?

That's a feature of pipenv at least, not sure if that works with plain pip though.

hoechenberger avatar Jul 19 '20 08:07 hoechenberger

I don't know how feasible this is but...

It would really useful to be able to link time domain 2d plots with time domain 3d plots. The most simple use case is visualizing sensor topology on evoked sensor data along side sourcespace stc plots. Similar utility for PSD in both sensor and source domain.

bloyl avatar Jul 20 '20 23:07 bloyl

I noticed two more problems in epochs.plot_psd().

  1. The ax keyword is different that what seems to be the standard (axes instead of ax).
  2. The function has not exclude keyword.

A similar plot for ERPs has that keyword, which is quite useful for quality control of preprocessing steps. If I am not missing anything, these lines from plot_evoked could almost be copied directly to plot_psd.

    if len(exclude) > 0:
        if isinstance(exclude, str) and exclude == 'bads':
            exclude = bad_ch_idx
        elif (isinstance(exclude, list) and
              all(isinstance(ch, str) for ch in exclude)):
            exclude = [info['ch_names'].index(ch) for ch in exclude]
        else:
            raise ValueError(
                'exclude has to be a list of channel names or "bads"')


        picks = np.array([pick for pick in picks if pick not in exclude])

I could try it out and commit something if it makes sense given the bigger scope of this refactoring

eort avatar May 05 '21 14:05 eort

I could try it out and commit something if it makes sense given the bigger scope of this refactoring

go for it!

drammock avatar May 05 '21 15:05 drammock

can you change the python version with pip?

That's a feature of pipenv at least, not sure if that works with plain pip though.

@hoechenberger or with venv for a more barebones approach. But I likes me some πŸ‘πŸ½ pipenv πŸ‘πŸ½ and pyenv ❀️

ktavabi avatar Aug 03 '22 00:08 ktavabi