mne-tools / mne-python

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

RFC: consistent API for topomap plotting #11032

Closed drammock closed 2 years ago

drammock commented 2 years ago

In working on the API / implementation of .plot_topomap() for the new Spectrum class, I went to our existing API for inspiration. Turns out it's rather fragmented. Here's a table of the signatures for:

...along with notes about what each parameter does.

Epochs Evoked AverageTFR Covariance Projection Spectrum affects
bands fmin,fmax bands data: freqs to plot
ch_type ch_type ch_type ch_type ch_type data: channel type to plot
normalize normalize data: divide each band by total power
agg_fun agg_fun data: how to aggregate over freq bins within band
dB dB data: log-scale
scalings scalings data: V → μV etc
proj proj proj data: projectors applied or not
"tmin,tmax" times tmin,tmax data: which times get subplots
average data: window duration around times
image_interp image_interp image_interp dataviz: spatial interpolation
extrapolate extrapolate dataviz: extrapolation style
border border border dataviz: extrapolation boundary value
contours contours contours contours dataviz: contour lines
outlines outlines outlines outlines outlines outlines axes: head/skirt/etc
sphere sphere sphere sphere sphere sphere axes: head param
sensors sensors sensors sensors axes: bool or MPL string like r+
show_names show_names show_names axes: sensor names
mask mask axes: which sensors
mask_params mask_params axes: style sensors
time_unit subplots: titles
time_format subplots: titles
res res res res subplots: side length in pixels
size size size size subplots: side length in inches
cmap cmap cmap cmap cmap cmap colormap
vlim vmin,vmax vmin,vmax vmin,vmax vlim vlim colormap: limits
colorbar colorbar colorbar colorbar colorbar: on/off
cbar_fmt cbar_fmt cbar_fmt cbar_fmt cbar_fmt colorbar: label
units unit units colorbar: label
axes axes axes axes axes axes figure: use preexisting axes
title title title figure: title
nrows figure: subplot layout
ncols figure: subplot layout
show show show show show show figure: show or not
n_jobs n_jobs
verbose verbose verbose
info info source of sensor info
bandwidth EPOCHS ONLY (multitaper params)
adaptive EPOCHS ONLY (multitaper params)
low_bias EPOCHS ONLY (multitaper params)
normalization EPOCHS ONLY (multitaper params)
baseline TFR ONLY (baselining)
mode TFR ONLY (baselining)
noise_cov COVARIANCE ONLY (whitener)

As you can see, there are many inconsistencies, some of them rather egregious (e.g., 'unit' vs 'units' for the same functionality). Some inconsistency is to be expected since some of these methods are doing different things (e.g., Evoked.plot_topomap() just plots the average signal value and does not offer any frequency-specific / power plotting).

I would like to take this opportunity to standardize the plot_topomap APIs to the extent possible. I know we're often a bit lax about backward compatibility when it comes to plotting functions, but bringing these all into some semblance of alignment will be a pretty big API change I think. One possible approach to consider is coming up with a new name and letting the old method names hang around for a couple of releases (with deprecation warnings). Possibilities:

Would love to hear others' thoughts on this.

hoechenberger commented 2 years ago

plot_field is already used for visualizing the field projected onto a 3D surface (scalp/helmet)

... and is something I only discovered a few months ago.

If we'd had something like plot_topomap(..., mode: Literal["2d", "3d"] = "2d"), it would have been much easier to discover. That said, I'd actually appreciate merging the functionality of both functions into plot_field()

If this doesn't work because the list of parameters would explode, I'd like to see plot_field_2d() / plot_field_3d() instead (or plot_topomap_2d() / plot_topomap_3d())

cbrnr commented 2 years ago

I think standardizing the API for the different topoplots is a very good idea! I like plot_topography(), a slightly shorter variant would be plot_topo(). Deprecating the existing methods is a good and tried approach, I like it. I would include a "2d"/"3d" parameter rather than have two separate functions.

agramfort commented 2 years ago

Most people use plain defaults for plotting. I would not come up with a new method name. It’s been there for too long and there are too many code relying on this. I would rather deprecate the parameters (eg if we go for units then add units and raise deprecation warning with unit parameter).