I haven't seen any discussion on how to implement dark mode - what's the plan? Add a theme argument? Or just a boolean dark/light mode switch? I'm really looking forward to this feature, that way MNELAB can go all auto-dark (at least on macOS for now).

The plan is to not get in the way of existing matplotlib theme functionality. In other words, use rcparams entries for things like foreground and background, and references to the color cycle like 'C2' for accent color needs.

this is ready for preliminary testing / review from anyone interested (@cbrnr? @hoechenberger?). For ease of side-by-side comparison, the new plotting function is implemented as mne.viz.plot_raw_alt(raw, ...) so you can pop open one of those plus a regular raw.plot(...) window at the same time. Here is a snippet to get you started, that illustrates a lot of the functionality all at once:

import os
import numpy as np
import matplotlib.pyplot as plt
import mne
plt.ion()
sample_data_folder = mne.datasets.sample.data_path()
sample_data_raw_file = os.path.join(sample_data_folder, 'MEG', 'sample',
                                    'sample_audvis_raw.fif')
raw = mne.io.read_raw_fif(sample_data_raw_file, verbose=False)
raw.apply_proj()
ecg_proj_file = os.path.join(sample_data_folder, 'MEG', 'sample',
                             'sample_audvis_ecg-proj.fif')
ecg_projs = mne.read_proj(ecg_proj_file)
raw.add_proj(ecg_projs)
sample_data_noise_cov_file = os.path.join(sample_data_folder, 'MEG', 'sample',
                                          'ernoise-cov.fif')
noise_cov = mne.read_cov(sample_data_noise_cov_file)
events = mne.find_events(raw, stim_channel='STI 014')
event_dict = {'auditory/left': 1, 'auditory/right': 2}
event_color = {1: 'r', 2: 'g', -1: 'y'}
annotations = mne.Annotations(onset=(2, 3, 5), duration=(0.5, 2.5, 1),
                              description=('foo', 'bar', 'bad_baz'))
raw.set_annotations(annotations)
fig = mne.viz.raw.plot_raw_alt(
    raw, proj=True, events=events, event_id=event_dict,
    event_color=event_color, noise_cov=noise_cov,
    group_by='selection', butterfly=False, decim=2,
    show_options=True)

Known deficiencies:

1. data traces are now plotted with a matplotlib LineCollection which means the z-order of bad channels is not all the way at the back. Probably the right thing to do is exclude bads from the LineCollection and plot them separately; just haven't done that yet. Other suggestions welcome.
2. clipping is implemented as a masked array rather than as np.clip() (for the "clamp" case) or as rectangular clipboxes (for other cases). In principle it should be possible to still do it the old way even with a LineCollection, if people think the masked array looks bad.
3. not yet implemented: new feature of right-clicking to show channel location
4. not yet implemented: new feature to click-drag in non-annotation mode to get a pop-up spectrum plot

@drammock I'd love to test this next week (I'm currently on vacation).

@drammock Will taste later today!

@drammock So you switched clamping back to using np.clip instead of masked arrays? Why's that?

Because I realized that "clamp" (as in voltage clamp) had a particular meaning and users would expect to see flat lines (rather than empty space) where the signal exceeded its limits. I still use masked arrays (instead of a clipping mask) for the other case.

tested locally. Works great !

Looks really good! Here are a few differences I've noticed, not sure if they are intentional or not, but they're maybe worth discussing:

1. The path in the window title bar now gets shortened rather abruptly (in my case ...ts/mne_data/MNE-sample-data/MEG/sample/sample_audvis_raw.fif). I don't know if I like that more than showing the full path. A third option would be to shorten in a more sensible manner, e.g. either make sure that we don't shorten folder names (.../mne_data/MNE-sample-data/MEG/sample/sample_audvis_raw.fif or maybe /Users/clemens/.../sample/sample_audvis_raw.fif). Finally, we could also just show the file name with not path at all.
2. Clicking between traces should bring up a vertical line with the associated time shown in the lower left corner. This line changed from green to yellow. Also, it is very difficult to click between traces, especially if there are many channels - this leads to channels being marked/unmarked as bad. Note that this is also true for the old plotting function, but I still think this is not ideal. We should either make sure that clicking outside traces sets the green line, or limit marking as bad to clicking on channel labels only. Or maybe setting the green line should require a keypress before clicking.
3. Event labels are now shown in the corresponding event colors instead of black. This looks nice in the example, but it might lead to labels that are difficult to read if the event color is e.g. yellow. Personally, I like the new (colored) version better.
4. The font size of event and annotation labels is now larger. I feel that with the old smaller font size annotations were sometimes easier to read because they didn't overlap that much.
5. The description in the annotation window should mention that left-clicking and dragging existing annotations works only if snap mode (p) is on. Maybe the current snap mode status should also be shown somewhere in the annotation window.
6. I'm sure there is a reason why the scale bar (s) doesn't work in your example, but this is not obvious and maybe should be explained somewhere.
7. The new plotting function prints STI 014 in italic - does this bear any meaning?
8. I noticed that STI 014 is always shown at the bottom (also in the old function) - is this intentional? In another example file where the STIM channel is called "Status" this behavior doesn't work (i.e. the STIM channel is not always shown).
9. The checkboxes in the projector window should always be squares and not rectangles depending on the window size.

The path in the window title bar now gets shortened rather abruptly (in my case ...ts/mne_data/MNE-sample-data/MEG/sample/sample_audvis_raw.fif).

This truncation to 60 characters was previously only done when len(raw._filenames) > 1. I couldn't figure out why you would want to truncate a path that said /really/long/path/to/some/file.fif (+ 3 more) but not truncate one that said /really/really/long/path/to/a/single/file.fif so I altered the logic a bit. Based on your feedback I've now implemented a function to do "clean" truncation that omits full path elements at a time, starting from the middle and working outwards, and never omitting the rightmost element (the filename). It doesn't guarantee you'll end up with something under 60 characters anymore, but if the filename alone is that long maybe it's OK to handle it ungracefully.

Clicking between traces should bring up a vertical line with the associated time shown in the lower left corner. This line changed from green to yellow.

The yellowish was an early decision to go with the matplotlib cycler colors for everything (in hopes of making dark mode work via plt.style.use). But using cycler colors seems unlikely to happen (see #8217) so I've changed the color back.

it is very difficult to click between traces, especially if there are many channels - this leads to channels being marked/unmarked as bad. Note that this is also true for the old plotting function, but I still think this is not ideal. We should either make sure that clicking outside traces sets the green line, or limit marking as bad to clicking on channel labels only. Or maybe setting the green line should require a keypress before clicking.

Personally I'm in favor of marking bads only when clicking channel names (not data traces). But this is a UI change that might upset many users. Note that getting the "hit" logic correct for the traces is surprisingly tricky; MPL provides a pickradius argument in points, but in my testing it's pretty hard to get the "right" value (what is "right" seems dependent on window size and how noisy the data is). For now I'll probably not fix this, since it's not really a regression.

Event labels are now shown in the corresponding event colors instead of black. This looks nice in the example, but it might lead to labels that are difficult to read if the event color is e.g. yellow. Personally, I like the new (colored) version better.

I like the colored version too, but I've reverted to black for now (it was not an intentional change).

The font size of event and annotation labels is now larger.

This was an unintentional change and has been fixed.

The description in the annotation window should mention that left-clicking and dragging existing annotations works only if snap mode (p) is on. Maybe the current snap mode status should also be shown somewhere in the annotation window.

Good idea, will do.

I'm sure there is a reason why the scale bar (s) doesn't work in your example, but this is not obvious and maybe should be explained somewhere.

That was a result of testing cruft (I had briefly commented out the lines that handle it). Fixed now.

The new plotting function prints STI 014 in italic - does this bear any meaning?

Italicized channel names is done when channels are whitened (toggle with w or disable by not passing in noise_cov). But the STI channels should not be italicized because they never get whitened. Fixed now.

I noticed that STI 014 is always shown at the bottom (also in the old function)

This is a "feature" of group_by='selection'. If stim channels called "status" don't work here, the fix would be to utils/config.py:_get_stim_channel()

The checkboxes in the projector window should always be squares and not rectangles depending on the window size.

Surprisingly tricky. Working on it.

Regarding the green line, what about continuously reporting the time under the mouse cursor somewhere in the plot or in a separate (toggle-able statusbar)? This would make the green line unnecessary (not that anyone would notice because it's so hard to get it to appear in the first place).

@larsoner this may be close to ready for your eyes. Summary of where things stand:

• group_by='selection' looks nicer and works a little smoother IMO
• new functionality: right-click on channel name -> show channel location popup
• traces are drawn with LineCollections, which seems to be a little faster but not much.
  • this required doing clipping as a masked array, which looks worse than proper clipping. I tried to clip the LineCollection with a PolyCollection as a clipping path (didn't work)... maybe could convert a PolyCollection to a compound path? Compound paths are possible, but the only example shows them being made with drawing primitives.
  • I tried to speed up butterfly mode by doing it with imshow. It was fairly fast (I think faster than the LineCollection) as long as data points weren't connected, but this looked bad (a sort of "cloud of data points" rather than collection of traces, and bad channels basically disappeared into the background); as soon as I connected the dots (with a scikit-image drawing function) it looked good but took forever. It was also hard to get clipping to work right with that approach. Code for this is local at the moment (not in this PR).
• the green vertical line is done with blitting, to prevent having to redraw all data traces each time (was especially laggy in butterfly mode). Annotation is still very laggy with butterfly mode; haven't tried to do it with blitting yet.

Do you have any suggestions / ideas / advice about which way might be most promising to go on these issues?

I was able to create a compound path to mask the LineCollection, but it looks like I'll need a separate LineCollection for each channel type (in butterfly mode) and a separate one for each channel in regular mode (in which case, we're back to using Line2D 🤦)

That is implemented on drammock/mne-python:clipboxes

FWIW here's what the imshow version of butterfly mode looks like (implemented on drammock/mne-python:butterfly-image)

I'm beginning to think that the only way to significantly speed this up is to use blitting for everything. In which case I suggest moving forward with reviewing/merging this one after I fix the clipping and clean up the cruft, and doing the blitting in a separate PR.

I'm beginning to think that the only way to significantly speed this up is to use blitting for everything.

Could be. So maybe go back to Line2D and we'll try to speed up by blitting only the things we need to blit?

I'm beginning to think that the only way to significantly speed this up is to use blitting for everything.

Could be. So maybe go back to Line2D and we'll try to speed up by blitting only the things we need to blit?

I've reverted the LineCollection stuff now, so I think this is ready for a review. Here is what remains undone (but I think each of these deserves its own PR):

• use blitting to speed up plotting (current PR only uses blitting on the green vertical line)
• new feature: click-drag (when not annotating) to pop open a spectral plot for that span of data
• UX change: there's been talk of removing the "click data to mark bad" because it can be hard to not click data if you want a vertical marker line, and because we already have a way to do (un)mark bads by clicking channel names. I expect this to be controversial and I think so far only @cbrnr and I have weighed in (we're both in favor of the change)

so I think this is ready for a review.

Want to get CIs happy first, then I'll look?

@larsoner last CI run everything passed except coverage and one Travis run. I've just pushed some commits that should help coverage. The travis failures were all AttributeError: 'NoneType' object has no attribute 'mpl_connect' and seem to suggest that figures are not getting created properly. Those tests are passing locally for me, so I'm not sure what I'm doing wrong. Any insight?

These errors look legitimate (tick numbers and BAD_ vs BAD):

https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=9316&view=logs&jobId=1d294aa9-8c9e-52da-e2cc-28ca059e1c9b&j=1d294aa9-8c9e-52da-e2cc-28ca059e1c9b&t=520de7d9-efc8-52cf-0de1-994cbfe115fe

For the mpl_connect thing, maybe this:

https://stackoverflow.com/questions/31933393/event-handling-matplotlibs-figure-and-pyplots-figure

But if it's happening on our minimal/old deps build, I would try replicating that environment locally. Then you should see the same error. It could be it's a matplotlib bug that has been fixed and maybe we need to either skip on old mpl or bump our matplotlib requirement.

These errors look legitimate (tick numbers and BAD_ vs BAD):

https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=9316&view=logs&jobId=1d294aa9-8c9e-52da-e2cc-28ca059e1c9b&j=1d294aa9-8c9e-52da-e2cc-28ca059e1c9b&t=520de7d9-efc8-52cf-0de1-994cbfe115fe

Argh, that's a new one I just introduced.

But if it's happening on our minimal/old deps build, I would try replicating that environment locally. Then you should see the same error.

Yep, now I get that locally.

It could be it's a matplotlib bug that has been fixed and maybe we need to either skip on old mpl or bump our matplotlib requirement.

Will see if I can find out when this behavior changed in MPL and/or how to work around it with old MPL.

Thanks @drammock for doing this amazing work! And sorry for being so late providing feedback.

I just gave the Raw browser a shot. Here's what I've found on macOS with Qt5Agg backend.

• looks so much better now than before 😍😍😍

• toggling draggable Annotations mode via p doesn't seem to work for me

• when in Annotations mode (a) and enabling draggable edges, hovering the mouse curser over an edge and then pressing a to exit Annotations mode leaves a highlighted edge behind:

  

• I was irritated that closing the channel selection window would close the entire Raw browser, this seems unexpected for a user

• I find it difficult to easily see at a quick glance which channels have been selected in the channel selection window. Fill colors, edges, or size of the circles should be changed to improve accessibility:

  

• Same goes for right-clicking a channel to view its location: it takes me a fraction of a second too long to find the channel on the plot:

  

• I tried for an entire minute but I couldn't get "left-click-and-drag to view the PSD plot" to work. All that happened was either nothing or I would toggle channels between bad and not bad. What am I doing wrong?

• With all these new windows, we're becoming a little bit like that image manipulation software with the weird name in its earlier days. I wonder: would it be feasible to assign a fixed area on the left side to always keep the relevant tools visible? Or to at least open the windows such that they are arranged (and sized) like here (but that might not be possible via Matplotlib?):

  

• The show_options kwarg should probably be renamed to better match what it actually does?

• There should be a keyboard shortcut (that also gets listed in the Help window) to toggle the SSP Projection Vectors window

• The Help and Prj buttons do not change color when hovering over them; the Toggle all button in the SSP Projection Vectors window does (which is better IMHO)

• I would prefer a clear(er) indication about which interaction mode I'm currently in and which magic has been applied to the figure I'm viewing. Here, too, a panel docked on the left side could help.

• Qt5Agg has a status bar on the bottom by default (but it seems we're disabling it). Couldn't we make use of it?

ok @larsoner I've restored channel type plotting order, and CIs are still happy (except codecov, which should improve once ICA uses the figure class, because old code that has unhit lines will get removed)

🥳

@agramfort did you forget to sqash-merge?

If so we should undo and force push a squashed version because of the number of commits

Just force-pushed to master. 😨 please check to make sure I didn't do something wonky.

Just force-pushed to master. 😨 please check to make sure I didn't do something wonky.

Can we make master a protected branch to prevent such accidents in the future?

The accident was actually pushing the green button the wrong way, and the fix was a force push that would not have been allowed under the protection rules. FWIW so far I've seen more harm / wasted time from protected branches than good on other repos.

What we could maybe do is only allow the squash and merge option for the green button on this repo. That would have prevented the issue and shouldn't add much if any maintenance overhead. Rebase or standard merge is used very rarely by us nowadays.

Okay just changed it so that squash merge is the only choice. About once a year (at most) I've wanted to do a different type (the highly collaborative beamforming PR is the only one I remember) and I'm okay with doing that manually from the command line or temporarily enabling another mode when needed.