MRG, VIZ, MAINT: Use new figure class for epochs.plot and ICA.plot_sources #8381

drammock · 2020-10-16T20:10:08Z

this is an element of #7751

Basically working already, though needs testing and maybe some cosmetics.

hoechenberger · 2020-10-21T18:02:08Z

Hello @drammock, maybe I'm doing something wrong – and I really haven't looked at the code changes here – but right-clicking on an IC "channel" name in a plot produced by ICA.plot_sources(epochs) should bring up a property plot, is that right? Right-clicking doesn't do anything for me on macOS, both with the default macOS and with the Qt5Agg backends. I surely must be missing something?

drammock · 2020-10-21T18:26:28Z

at the moment only ica.plot_sources(raw) has been updated. ica.plot_sources(epochs) and ica.plot_sources(evoked) haven't yet been changed.

hoechenberger · 2020-10-21T18:48:28Z

Thanks @drammock! It's beautiful!

What I've noticed:

clicking on the Help button doesn't open the help window – once I right-click on a component name and the property plot opens, the help window opens behind the property window
I get the macOS beach ball for a few seconds after doing the right click. I wonder if it would be possible to prevent this blocking, and if we could even print processing … or something similar to inform the user that we haven't crashed, but are merely busy?

drammock · 2020-11-03T18:32:07Z

@jasmainak is it easy for you to test the autoreject package with this branch, to make sure that epoch_colors still works as expected?

drammock · 2020-11-06T00:16:25Z

OK, now raw.plot(), epochs.plot(), ica.plot_sources(raw), and ica.plot_sources(epochs) all use the new figure classes. Assuming I didn't break anything with these last few commits, this is ready for review. (sorry for the monstrous diff; it turned out to be easier to do ICA and epochs at the same time). Notes:

here are the circle renderings of the tutorials I updated this time: ica tutorial and epochs tutorial. Note that in the ICA tutorial I used show_scrollbars=False in all the plot_sources() calls, which I think is generally better for tutorials except that you lose the x-axis label.
bad ICA components now get grey color instead of red color, similar to bad channels in the raw and epoch plots.
I changed the keyboard shortcuts for scrolling through epochs in the time domain, to make them more consistent with raw.plot(). Previously, in epochs.plot() → and ← would scroll a full window width at a time. Now, I've set it so that → and ← scroll a single epoch, and now shift→ and shift← scroll a full window width at a time. This is consistent with raw.plot() where → does 1/4 window width and shift→ does full window width.
The labeling of epochs at the bottom of the main window and along the bottom edge of the scrollbar are now numbers from epochs.selection instead of sequential indices. This overrides the work in MRG, FIX: make labels in epochs.plot start at 0, fixes issue #7260 #7299 so I'm keen to hear @SophieHerbst's opinion whether this is a welcome or unwelcome change.

I have deprecated the parameter event_colors in epochs.plot() in favor of the similarly-named event_color parameter. It turns out that raw.plot() has an event_color parameter that was generally more flexible than what used to be implemented for epochs. Now, they both have the same name, they work the same way, and they both do everything each of them used to be able to do. Here is the new docstring:

mne-python/mne/utils/docs.py

Lines 98 to 105 in cc4dff9

    
           docdict['event_color'] = """ 
        
           event_color : color object | dict | None 
        
               Color(s) to use for events. To show all events in the same color, pass any 
        
               matplotlib-compatible color. To color events differently, pass a `dict` 
        
               that maps event names or integer event numbers to colors (must include 
        
               entries for *all* events, or include a "fallback" entry with key ``-1``). 
        
               If ``None``, colors are chosen from the current Matplotlib color cycle. 
        
           """

The horizontal scrollbar in the epochs plot now has divisions showing epoch boundaries.
clicking in the data axes of epochs plots is now more similar to raw plots. In particular, you must click on data traces to mark bad epochs (previously clicking between the data traces was good enough). Now, clicking between data traces places the green vertical line (like it does on raw plots) and right-clicking removes the green vertical line (again, like raw plots).
all three plot types allow right-click on channel names to bring up additional context plots. raw plots → channel location, epoch plots → epochs image for that channel, ica plots → a plot_properties window for that component.

of these, I think items 3, 4, 5, and 7 are prominent (non-cosmetic) changes which I think should get buy-in from at least a couple other devs before merge, and some of them probably deserve changelog entries. Anything else you notice that seems weird --- anything at all --- please mention it now.

larsoner · 2020-11-06T14:10:26Z

used show_scrollbars=False in all the plot_sources() calls, which I think is generally better for tutorials except that you lose the x-axis label.

We could allow show_scrollbars='xaxis' or something to specify just to keep the x axis. It would be better if the name were decorate=True then decorate='xaxis' would make more sense but I'm okay with a bit of a misnomer here (not worth deprecation cycle to make the name more general).

This overrides the work in #7299

This appeared to just be about 0- versus 1-based indexing so I think it's fine / complementary

Otherwise all these API changes sound like improvements to me, +1 for all of them. I'll wait to do a thorough review until we see if others agree and no other major changes are needed.

@hoechenberger I see a heart, does that mean you're happy with all points?

hoechenberger · 2020-11-06T14:14:05Z

@hoechenberger I see a heart, does that mean you're happy with all points?

Yes I’m quite happy! Still got to actually try it out, but I do love the list of changes

drammock · 2020-11-06T15:12:27Z

We could allow show_scrollbars='xaxis' or something to specify just to keep the x axis. It would be better if the name were decorate=True then decorate='xaxis' would make more sense but I'm okay with a bit of a misnomer here (not worth deprecation cycle to make the name more general).

WDYT about just always showing an x-axis label, regardless of whether scrollbars are visible or hidden?

larsoner · 2020-11-06T15:15:45Z

I'm not sure how that interacts with zen mode, but I never really use this so I'm happy to defer to your judgment on this

drammock · 2020-11-06T15:17:49Z

I'm not sure how that interacts with zen mode, but I never really use this so I'm happy to defer to your judgment on this

show_scrollbars=False is the same as starting in zen mode (z just toggles between the False and True states). So I'm proposing that in zen mode, you don't see scrollbars but you still see an x-axis label.

larsoner · 2020-11-06T16:43:41Z

I have no strong opinion, up to you. I would say if you want to go with as much backward compat as possible add show_scrollbars='xaxis' option, if it's not a big concern then just change the behavior of False

agramfort

it works like a charm !!!! and with 2000 lines removed 🍾

@drammock you are like 🎅 to me !

hoechenberger · 2020-11-07T11:34:39Z

LOL I can imagine that scene!!
"Daddy, what's for Christmas?" – "Best present EVER! 2,000 SLOC removed, BOOYAA!!! 🤡" – "😱"

larsoner · 2020-11-07T12:53:57Z

And longstanding functionality added (such as butterfly mode for epochs, right?) and waaaay more DRY code, etc. Bravo!

I'll take a look and try things Monday. @hoechenberger do you want to test it out? I don't use the epochs or ICA very much so I'm not the best person to do it.

larsoner · 2020-11-07T12:55:41Z

Does this close #4850, or is more needed for that?

hoechenberger · 2020-11-07T13:02:03Z

@hoechenberger do you want to test it out? I don't use the epochs or ICA very much so I'm not the best person to do it.

Yes but won't be able to do it before tonight or tomorrow UTC+1

drammock · 2020-11-07T16:33:18Z

Does this close #4850, or is more needed for that?

No but it should be quite easy to do. Will add that.

hoechenberger · 2020-11-09T21:48:53Z

@drammock I will try to give it another shot tomorrow, going to bed now :) But if you feel ready to get this merged, don't wait for me!

hoechenberger · 2020-11-10T08:27:36Z

Thanks for addressing #8304, @drammock! This seems to be fixed for me now. However, bad news is, instead of this traceback plus (sometimes) a segfault I now only receive the segfault 💩Could be an issue with PyQt5 on macOS, I don't know. The problem still seems to be related to resizing somehow. I'm wondering if it would be possible to disable redrawing while the window is being resized? You know, like in the Windows 3.11 days? Because I have the suspicion that this might be (part of) the issue here...

hoechenberger · 2020-11-10T08:31:48Z

But I don't intend to filibuster this PR just because my old computer is having some troubles. If I'm the only one experiencing this, maybe we can just go ahead and merge for now… I unfortunately won't be able to provide feedback / test again, though. But we can always iterate later. (Will hopefully have my 2019 MBP again in a week or two)

larsoner · 2020-11-10T12:30:12Z

Could be an issue with PyQt5 on macOS, I don't know.

Exceptions thrown during a callback will cause PyQt to abort. This should in theory be handled by matplotlib -- IIRC they have some mechanisms in place to make it so that any PyQt event handler of theirs does not pass on the exceptions (this is why they get printed instead). So this is probably a bug with their implementation somewhere. It would be great if we could isolate this and open an issue, or fix the bug in their code.

@hoechenberger are you on the latest matplotlib version? And what version of PyQt5?

mne/viz/tests/test_raw.py

larsoner · 2020-11-10T15:00:12Z

Pushed a commit to showcase butterfly=True, group_by='selection'. Feel free to remove this commit and put it elsewhere if the spot I chose isn't good enough. Other than that and the Travis failure, LGTM!

hoechenberger · 2020-11-10T16:15:11Z

@larsoner
I'm using Matplotlib 3.3.2, Qt 5.12.9, and PyQt 5.12.3

show ICA properties on right-click of component name renable right-click on ch name fix EOG scaling better pageup behavior when at bottom of channel list disable butterfly for ICA better keypress testing travis old deps debugging refactor plot_properties integration make bads color consistent with plot_raw fix _fake_click location to work when MNE_BROWSE_RAW_SIZE not set touch tutorial update what's new remove unreachable condition persist changes to ICA exclude WIP: handle epochs too take advantage of numpy version bump fix ticks fix scrolling click to mark bad; refactor allow bad epoch marking fix instructions get vlines working add image plot on right-click add epoch histogram bad ch color trumps bad epoch color use plt_show for epochs support custom colors for (autoreject compatibility) snap to epoch boundaries when clicking scrollbar verbose=False on channel/component context click figures make it work for ica.plot_sources(epochs) touch epochs tutorial fix CIs (disable keypress j if no projs present) clean up help window code make "show histogram" key a toggle retain xlabel in zen mode

fix: make epoch_colors work in butterfly mode [skip travis]

update what's new

drammock · 2020-11-13T13:19:53Z

@larsoner all green here, can we get this in so more folks can start testing it?

larsoner · 2020-11-13T13:22:10Z

Yes, thanks @drammock !

agramfort · 2020-11-13T13:58:29Z

awesome @drammock <https://github.com/drammock> !

…

hoechenberger · 2020-11-13T15:03:18Z

🎈

larsoner · 2020-11-13T16:07:14Z

@drammock looks like these are yours:

https://app.circleci.com/pipelines/github/mne-tools/mne-python/4732/workflows/ffa530e7-f767-4af5-b1b3-ebe0b679c0f9/jobs/23476

Can you check and open a quick PR to fix?

drammock marked this pull request as ready for review October 20, 2020 19:35

drammock force-pushed the use-fig-class-for-ica branch from d1d68c1 to 7743e89 Compare October 28, 2020 22:14

drammock changed the title ~~VIZ, MAINT: Use new figure class for ICA.plot_sources~~ VIZ, MAINT: Use new figure class for epochs.plot and ICA.plot_sources Oct 28, 2020

drammock mentioned this pull request Oct 28, 2020

FIX, VIZ: ensure figs appear #8450

Merged

drammock force-pushed the use-fig-class-for-ica branch from 7743e89 to e4cc537 Compare October 29, 2020 14:46

drammock mentioned this pull request Nov 2, 2020

Lost interactive features during MNE Matplotlib integration in to Kivy application #8142

Closed

drammock force-pushed the use-fig-class-for-ica branch from 13e4d7c to 8142e8d Compare November 3, 2020 17:21

drammock force-pushed the use-fig-class-for-ica branch 5 times, most recently from 48752f6 to cc4dff9 Compare November 5, 2020 22:04

drammock changed the title ~~VIZ, MAINT: Use new figure class for epochs.plot and ICA.plot_sources~~ MRG, VIZ, MAINT: Use new figure class for epochs.plot and ICA.plot_sources Nov 7, 2020

agramfort approved these changes Nov 7, 2020

View reviewed changes

larsoner reviewed Nov 10, 2020

View reviewed changes

mne/viz/tests/test_raw.py Show resolved Hide resolved

larsoner reviewed Nov 10, 2020

View reviewed changes

mne/viz/tests/test_raw.py Outdated Show resolved Hide resolved

drammock force-pushed the use-fig-class-for-ica branch 3 times, most recently from 0798bcf to 68bca25 Compare November 12, 2020 17:16

drammock and others added 6 commits November 12, 2020 16:57

closes mne-tools#4850

08bc9f8

docstring, codespell, whitespace, coverage

2533bef

fix: make epoch_colors work in butterfly mode [skip travis]

closes mne-tools#8304

95fdab6

ENH: Add butterfly

f6e1f95

fix tests

73060df

update what's new

drammock force-pushed the use-fig-class-for-ica branch from 68bca25 to 73060df Compare November 12, 2020 22:58

larsoner merged commit 8cf161e into mne-tools:master Nov 13, 2020
5 checks passed

drammock deleted the use-fig-class-for-ica branch November 13, 2020 15:26

This was referenced Nov 13, 2020

FIX: Circle failures #8516

Merged

RFC, MAINT: changes to 2D plotting #7751

Open

Marking bad channels and bad ICs is inconsistent #6531

Closed

jasmainak mentioned this pull request Dec 15, 2020

Fixing pickling of AutoReject objects autoreject/autoreject#193

Merged

drammock mentioned this pull request Apr 26, 2021

raw.plot raises KeyError with event_colors #9339

Closed

MRG, VIZ, MAINT: Use new figure class for epochs.plot and ICA.plot_sources #8381

MRG, VIZ, MAINT: Use new figure class for epochs.plot and ICA.plot_sources #8381

drammock commented Oct 16, 2020

hoechenberger commented Oct 21, 2020 •

edited

drammock commented Oct 21, 2020 •

edited

hoechenberger commented Oct 21, 2020

drammock commented Nov 3, 2020

drammock commented Nov 6, 2020 •

edited

larsoner commented Nov 6, 2020

hoechenberger commented Nov 6, 2020

drammock commented Nov 6, 2020

larsoner commented Nov 6, 2020

drammock commented Nov 6, 2020

larsoner commented Nov 6, 2020

agramfort left a comment

hoechenberger commented Nov 7, 2020

larsoner commented Nov 7, 2020

larsoner commented Nov 7, 2020

hoechenberger commented Nov 7, 2020

drammock commented Nov 7, 2020

hoechenberger commented Nov 9, 2020

hoechenberger commented Nov 10, 2020 •

edited

hoechenberger commented Nov 10, 2020 •

edited

larsoner commented Nov 10, 2020

larsoner commented Nov 10, 2020

hoechenberger commented Nov 10, 2020

drammock commented Nov 13, 2020

larsoner commented Nov 13, 2020

agramfort commented Nov 13, 2020 via email

hoechenberger commented Nov 13, 2020

larsoner commented Nov 13, 2020

MRG, VIZ, MAINT: Use new figure class for epochs.plot and ICA.plot_sources #8381

MRG, VIZ, MAINT: Use new figure class for epochs.plot and ICA.plot_sources #8381

Conversation

drammock commented Oct 16, 2020

hoechenberger commented Oct 21, 2020 • edited

drammock commented Oct 21, 2020 • edited

hoechenberger commented Oct 21, 2020

drammock commented Nov 3, 2020

drammock commented Nov 6, 2020 • edited

larsoner commented Nov 6, 2020

hoechenberger commented Nov 6, 2020

drammock commented Nov 6, 2020

larsoner commented Nov 6, 2020

drammock commented Nov 6, 2020

larsoner commented Nov 6, 2020

agramfort left a comment

Choose a reason for hiding this comment

hoechenberger commented Nov 7, 2020

larsoner commented Nov 7, 2020

larsoner commented Nov 7, 2020

hoechenberger commented Nov 7, 2020

drammock commented Nov 7, 2020

hoechenberger commented Nov 9, 2020

hoechenberger commented Nov 10, 2020 • edited

hoechenberger commented Nov 10, 2020 • edited

larsoner commented Nov 10, 2020

larsoner commented Nov 10, 2020

hoechenberger commented Nov 10, 2020

drammock commented Nov 13, 2020

larsoner commented Nov 13, 2020

agramfort commented Nov 13, 2020 via email

hoechenberger commented Nov 13, 2020

larsoner commented Nov 13, 2020

hoechenberger commented Oct 21, 2020 •

edited

drammock commented Oct 21, 2020 •

edited

drammock commented Nov 6, 2020 •

edited

hoechenberger commented Nov 10, 2020 •

edited

hoechenberger commented Nov 10, 2020 •

edited