ENH: Route evoked.plot() through MNELineFigure - #13795
Conversation
When evoked.plot() creates its own figure (axes=None), now routes through _line_figure() to instantiate MNELineFigure instead of a plain matplotlib figure. This aligns with the 2D plotting figure-class refactor direction discussed in mne-tools#7751. Behavior unchanged for custom axes. Both plot_white() and plot_joint() remain out of scope here. Adds regression test to assert MNELineFigure instantiation for default path. Closes mne-tools#13747
|
Hi @larsoner, @drammock — just wanted to know the preferred direction for this refactor. |
|
Addressed review points @larsoner... could you please take another look when convenient? Thanks. |
|
Hi @larsoner, @drammock.......just a friendly ping. All checks are green and I’ve addressed the feedback from the previous review. |
|
Hi @PragnyaKhandelwal, as this changes plotting behavior, it needs visual inspection of the plots (not just the code) to make sure things still look correct. You can help speed up the review by:
(step 1 could be skipped in this case actually. Sometimes a screenshot is enough, but in this case we need to verify interactivity is preserved) |
|
Thanks for the guidance @drammock! I've completed the visual and architectural verification. Everything aligns with the current main branch while successfully migrating to the new figure class.
import mne
from mne.datasets import sample
from mne.viz._mpl_figure import MNELineFigure
data_path = sample.data_path()
evoked_fname = data_path / 'MEG' / 'sample' / 'sample_audvis-ave.fif'
evoked = mne.read_evokeds(evoked_fname, condition='Left Auditory')
fig = evoked.plot(spatial_colors=True)
print(f"Figure Class: {type(fig)}")
assert isinstance(fig, MNELineFigure)
print("Figure is an MNELineFigure `instance.")
|
|
Hi @drammock, I’ve provided the requested visual and interactive verification. Could you please review it when you have time? |
|
Hi @drammock! Can we get this merge...its ready for review.. |
larsoner
left a comment
There was a problem hiding this comment.
I think the MNELineFigure currently loses default matplotlib keybindings, can you check that part of the interactivity?
| try: | ||
| ch_types = np.array(inst.get_channel_types()) | ||
| except AttributeError: | ||
| ch_types = np.array(inst.info.get_channel_types()) |
There was a problem hiding this comment.
Okay it looks like some conditional was needed:
https://github.com/mne-tools/mne-python/actions/runs/32931350347/job/98064132069?pr=13795
But I think I'd rather explicitly isinstance(inst, DipoleFixed) and do something different there than use try/except
There was a problem hiding this comment.
Okay it looks like some conditional was needed:
https://github.com/mne-tools/mne-python/actions/runs/32931350347/job/98064132069?pr=13795
But I think I'd rather explicitly
isinstance(inst, DipoleFixed)and do something different there than usetry/except
Makes sense! I swapped out the try/except for an explicit isinstance(inst, DipoleFixed) check.
Since DipoleFixed doesn't inherit from ContainsMixin, it lacks get_channel_types() and only carries .info. Handling the info-based lookup directly is definitely cleaner than relying on an AttributeError.
Confirmed the fix against test_dipole_fitting_fixed, which reproduces the original CI failure without this change.
Drop the dead try/except in _line_figure (Evoked always exposes get_channel_types via ContainsMixin, so the AttributeError fallback was unreachable). Restore matplotlib's default keybindings in MNELineFigure by falling back to key_press_handler after MNE's own keypress handling, which was previously being discarded by _add_default_callbacks. Add a regression test that presses "q" and confirms the figure actually closes.
Addressed both points @larsoner — removed the unnecessary try/except and restored the default Matplotlib keybindings in |
Replace the try/except AttributeError in _line_figure with an explicit isinstance(inst, DipoleFixed) check, per review feedback. DipoleFixed.plot() routes through _plot_evoked/_line_figure but has no get_channel_types() of its own (no ContainsMixin), only .info, so it needs the info-based lookup; every other caller (Evoked) has get_channel_types() directly. The bare try/except masked this and broke CI when the fallback was removed.
|
Okay marking for merge-when-green, thanks in advance @PragnyaKhandelwal ! |


Reference issue (if any)
Closes #13747
Related to #7751
What does this implement/fix?
When evoked.plot() creates its own figure (axes=None), it now routes through _line_figure() to instantiate MNELineFigure instead of a plain Matplotlib figure.
This aligns the default 2D plotting path with the refactor direction discussed in #7751, while keeping behavior unchanged when users pass custom axes.
Scope is intentionally limited:
evoked.plot_white() is out of scope
evoked.plot_joint() is out of scope
A regression test was added to assert MNELineFigure instantiation for the default evoked.plot() path.
Additional information
This is a localized, backward-compatible enhancement intended as an incremental step toward #7751, not a full closure of that broader refactor.