MRG, FIX, VIZ: refactor plot_evoked_topomap

[drammock]

closes #7595

Setup for testing:

import numpy as np
import matplotlib.pyplot as plt
from mne.datasets import sample
from mne import read_evokeds

path = sample.data_path()
fname = path + '/MEG/sample/sample_audvis-ave.fif'
condition = 'Left Auditory'
evoked = read_evokeds(fname, condition=condition, baseline=(None, 0))
times = np.linspace(0, 0.5, 7)

interactive usage

evoked.plot_topomap(times='interactive')

current master

this PR

interactive without colorbar

evoked.plot_topomap(times='interactive', colorbar=False)

current master

IndexError: index 2 is out of bounds for GridSpec with size 2

this PR

standard non-interactive usage

evoked.plot_topomap(times=times)

current master

this PR

standard usage no colorbar

evoked.plot_topomap(times=times, colorbar=False)

current master

this PR

specifying rows/columns

evoked.plot_topomap(times=times, ncols=3, nrows='auto')

current master

this PR

rows/columns, no colorbar

evoked.plot_topomap(times=times, ncols=3, nrows='auto', colorbar=False)

current master

this PR

user passes axes

fig, axs = plt.subplots(2, 2)
evoked.plot_topomap(times=times[:2], axes=axs[0], colorbar=False)
evoked.plot_topomap(times=times[2], axes=axs[1], colorbar=True)

current master

Also returns RuntimeError: Axes and times must be equal in sizes. after closing the incomplete figure.

this PR

[drammock]

cc @fraimondo you may be interested to test these changes, to make sure your nrows / ncols usage still works as expected.

[agramfort]

@hoechenberger you will be interested in this too

[drammock]

Ready for review. Note that @hoechenberger's use case that motivated this fix is still somewhat problematic, but I don't see a clear solution to either of these problems:

1. If a user plots to axes of an existing figure in multiple steps (as shown above), the colorbar will only be accurate for the subplots that were drawn at the same time as the colorbar. So, in the example above, the colorbar scale only is accurate for the bottom-left plot, not for the two top-row plots. I think this is unavoidable. May be worth a Notes section warning in the docstring.

2. In the same example, the colorbar fills the whole subplot space. Although this looks bad in the example above, I think it's actually what we should do when the user passes in their own axes; we should assume that they can use gridspec kwargs if they want to.

[codecov]

Codecov Report

Merging #7706 into master will increase coverage by 4.31%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #7706      +/-   ##
==========================================
+ Coverage   85.74%   90.05%   +4.31%     
==========================================
  Files         455      455              
  Lines       84571    84621      +50     
  Branches    13400    13399       -1     
==========================================
+ Hits        72512    76203    +3691     
+ Misses       9323     5549    -3774     
- Partials     2736     2869     +133     

[hoechenberger]

• If a user plots to axes of an existing figure in multiple steps (as shown above), the colorbar will only be accurate for the subplots that were drawn at the same time as the colorbar. So, in the example above, the colorbar scale only is accurate for the bottom-left plot, not for the two top-row plots. I think this is unavoidable. May be worth a Notes section warning in the docstring.

I would almost think that common sense would tell you that, but yes, maybe a few words in the Notes section are justified anyway. :)

2. In the same example, the colorbar fills the whole subplot space. Although this looks bad in the example above, I think it's actually what we should do when the user passes in their own axes

Yes to me this is expected behavior and should be exactly this way.

Just wondering, we had also discussed whether having a separate cbar_axes kwarg could make sense. But I guess this solution here (last axes used for colorbar) is sufficiently flexible already. Considering a case where maybe I'd like to print 5 topographies with their own colorbar each into the same figure, I could simply create a 5-by-2 grid and plot the topomaps one after another, right?

Aside from my few comments, this looks very good to me and is definitely a great improvement to what we had before.

Please also add a changelog entry.

[hoechenberger]

❤️ The future is now ❤️

[hoechenberger]

I think this is the right thing to do. 👍

[drammock]

@larsoner the Azure failure here was a one-hour timeout, but it looks like it managed to run all the tests successfully, and just barely ran out of time during the "finalize job" step. So I'm marking this as MRG, but let's talk (offline?) about pruning execution time some more (your favorite subject, I know).

[larsoner]

We already have a PR for the PyVista tests being slow, which accounts for a lot of the slowest ones:

https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=6946&view=logs&jobId=a846d25a-e32c-5640-1b53-e815fab94407&j=a846d25a-e32c-5640-1b53-e815fab94407&t=f8799db5-b972-5f58-516d-bc88b8357f99

But feel free to open an issue about the other ones. I'm happy to take a look soon

[hoechenberger]

Aside from the few tiny nitpicks, LGTM!

[larsoner]

Code LGTM, thanks @drammock !