ENH: refactor _handle_event_colors

[drammock]

closes #7101 , which turned out not to have been a bug after all. I was doing something like:

epochs['face'].plot(..., event_colors=dict(buttonpress='red'))

in order to control the color of buttonpress events in a plot of "face" epochs. The two problems with this:

1. epochs['face'] drops all the other entries in epochs.event_id except for {'face': 5}, so the message that "buttonpress" is in the color dict but not in events or event_id is technically correct. However:

2. the error message implies that passing event_colors=dict(face='blue') should work, which it does not (on current master) because the color handling only accepts event integers as valid keys for the color dict.

This PR refactors _handle_event_colors to accept either valid event integers, or valid event_id string keys (or a mix of both). It still won't fix my original problem of

epochs['face'].plot(..., event_colors=dict(buttonpress='red'))

because of epochs['face'] causing other event_id entries to be dropped (I'm open to suggestions about how to make that work). But it will allow

epochs['face'].plot(..., event_colors=dict(face='blue'))

as well as the more general case

epochs.plot(..., event_colors=dict(buttonpress='red', face='blue'))

[larsoner]

At least this failure appears legitimate:

https://travis-ci.org/mne-tools/mne-python/jobs/620412929#L3087

Otherwise LGTM +1 for merge

[codecov]

Codecov Report

Merging #7111 into master will decrease coverage by 0.07%.
The diff coverage is 91.89%.

@@            Coverage Diff            @@
##           master   #7111      +/-   ##
=========================================
- Coverage   89.78%   89.7%   -0.08%     
=========================================
  Files         444     444              
  Lines       78211   78997     +786     
  Branches    12672   12674       +2     
=========================================
+ Hits        70222   70865     +643     
- Misses       5174    5318     +144     
+ Partials     2815    2814       -1

[drammock]

@larsoner latest commits should fix the tests... any ideas about how to make this work?

ev = mne.pick_events(events, include=[5, 32])
epochs['face'].plot(events=ev, event_colors=dict(buttonpress='red'))

(i.e., using 'buttonpress' instead of 32 as the key)? Or should we not bother? AFAICT it would mean changing the epochs.plot API to accept an event_id dict, or making a public version of epochs._getitem() that exposes drop_event_id, neither of which seem desirable.

[larsoner]

We could make an epochs property epochs.drop_unused_event_id = True (by default) that, when set to False, will make it so that on __getitem__ the unused event_id entries stick around. I'm not sure how much downstream code this might break, though, and it seems difficult.

I actually don't mind adding an event_id param to epochs.plot. The docstring could be:

event_id : dict | None
    Event id dict to use. Useful when `events` contains events numbers not
    in ``epochs.event_id`` (e.g., because of event subselection). Values in
    ``event_id`` will take precedence over those in ``epochs.event_id``
    when there are overlapping keys.

[drammock]

I actually don't mind adding an event_id param to epochs.plot. The docstring could be:

separate PR then. I'll also throw in there a fix for the vertical lines at t=0; at present they are always drawn, and also drawn on top of the event markers, so if you pass events and event_colors to epochs.plot, whatever the event was that was used to generate the epochs in the first place will always be marked in green regardless of the color you ask for.

[larsoner]

I'll also throw in there a fix for the vertical lines at t=0; at present they are always drawn, and also drawn on top of the event markers, so if you pass events and event_colors to epochs.plot, whatever the event was that was used to generate the epochs in the first place will always be marked in green regardless of the color you ask for.

The simple and easy fix would be just to bump the zorder of the events markers to be higher than that of the always drawn t=0 ones. No new conditionals would be required.

[drammock]

The simple and easy fix would be just to bump the zorder of the events markers to be higher than that of the always drawn t=0 ones. No new conditionals would be required.

that was the plan, yes :)

[agramfort]

would it make sense to have short tests for _handle_event_colors function ? it's easier to test than relying on visual inspection of colors

[drammock]

would it make sense to have short tests for _handle_event_colors function ? it's easier to test than relying on visual inspection of colors

Probably a good idea. I'll add that after lunch today.

[larsoner]

Thanks @drammock