MRG: refactor plot_epochs_image and plot_compare_evokeds

[drammock]

Closes #6329

• epochs.plot_image() (works, one fig of GFP for each channel type)
• epochs.plot_image(picks=<ch_type>) (works, one fig of GFP)
• epochs.plot_image(picks=<ch_type>, combine='mean') (works)
• epochs.plot_image(picks=<ch_name>) (works)
• epochs.plot_image(picks=<ch_name>, combine='mean') (works, but warns)
• epochs.plot_image(picks=<list_of_ch_names>) (works, one fig per channel)
• epochs.plot_image(picks=<list_of_ch_names>, combine='mean') (works, one fig)

These are certainly not all the relevant test cases yet, feel free to add some. Some other TODOs:

• get it to show channel location in evoked inset (currently get RuntimeWarning "cannot find channel coordinates")

[codecov]

Codecov Report

Merging #6422 into master will increase coverage by 0.07%.
The diff coverage is 95.34%.

@@            Coverage Diff             @@
##           master    #6422      +/-   ##
==========================================
+ Coverage   89.33%   89.41%   +0.07%     
==========================================
  Files         416      416              
  Lines       74865    74839      -26     
  Branches    12341    12312      -29     
==========================================
+ Hits        66883    66914      +31     
+ Misses       5139     5112      -27     
+ Partials     2843     2813      -30

[lgtm-com]

This pull request introduces 3 alerts when merging af19ee6 into e1a160c - view on LGTM.com

new alerts:

• 2 for Unused import
• 1 for Unused local variable

[jona-sassenhagen]

It will take me a while to go through this, but thanks for taking my bad code and making it better!

[lgtm-com]

This pull request introduces 1 alert when merging e3c4316 into a2c7590 - view on LGTM.com

new alerts:

• 1 for Variable defined multiple times

[lgtm-com]

This pull request introduces 1 alert when merging 389c5da into b07a871 - view on LGTM.com

new alerts:

• 1 for Unused local variable

[drammock]

I'm pretty happy with where plot_epochs_image stands now. Here is a Jupyter gist showing several test cases. I haven't looked carefully yet at the test suite to see if all the important cases have coverage, but the gist will be a good starting point for filling any coverage gaps.

@agramfort I removed the parts of plot_compare_evokeds that do gradiometer pair combining because they were messing up plot_epochs_image. My preference is to do the rest of the refactor of plot_compare_evokeds API in a separate PR, unless you think it should get done here. WDYT?

[agramfort]

epochs.plot_image(picks='mag', combine='std') looks wrong. Colorbar should be Reds and I don't understand why the line plot below can be negative besides plots LGTM. I'll look at the code later. Great job !

[drammock]

I've now overhauled plot_compare_evokeds to work well with plot_epochs_image. Here's the updated Jupyter Gist. This involved API changes so we'll definitely need a deprecation cycle.

There are still some TODOS:

• revise tests for plot_compare_evokeds (haven't touched them yet, will likely fail)
• assess tests for plot_epochs_image (for coverage)
• go through docstrings to make sure everything is accurate w/r/t new behavior
• test topo mode of plot_compare_evokeds (I've been ignoring it so far)

Notable changes:

• plot_compare_evokeds now always returns a list, even if only one figure is generated (for consistency with plot_epochs_image; this makes it behave nicer in Jupyter notebooks)
• both functions now take a combine parameter specifying how to combine across sensors
• plot_compare_evokeds now can take colorbar instances, not just strings (allows for user-defined colormaps)
• probably others that I'm forgetting at the moment

[jona-sassenhagen]

Strongly appreciate what you're doing @drammock !

[jona-sassenhagen]

Can you cover split_legend in the gist?

[drammock]

@jona-sassenhagen the gist is updated but GitHub isn't rendering it so here's an nbviewer link

[drammock]

@jona-sassenhagen @agramfort tests are passing locally, so hopefully CIs do to; I think this is ready for code review. I've updated the gist with some additional tests of plot_compare_evokeds; let me know if there are additional tests you would like added there.

$ git diff master --stat
 9 files changed, 1210 insertions(+), 1350 deletions(-)

140 lines net negative! woohoo!
coverage of mne/viz/evoked.py is at 84%, with only 3 lines missed in plot_compare_evokeds and its helpers.

NOTE: there is one TODO in the code related to colorbar shrinking when there are discrete colors... not sure if we even need to do that shrinking? If it's highly desirable I can think of a couple ways to tackle it, but I'm neutral on whether it actually needs to be done.

[agramfort]

@drammock can you see why CIs complain? do you have a new rendered notebook? thanks heaps

[drammock]

@agramfort almost all tests pass; the remaining failure is one of the _fake_clicks in test_plot_epochs_basic, but I'm having trouble debugging because it passes for me locally. Maybe I can convince @larsoner to take a look at the travis traceback? Anyway, I've updated the gist with some additional tests of plot_compare_evokeds; let me know if there are additional tests you would like added there.

Remaining TODOs are at least:

• update what's new with the API changes (waiting on this in case of objections)
• colorbar shrinking when there are discrete colors? Not sure how important that is; the easy way of doing it doesn't work due to weird matplotlib behavior, but there are harder ways I could do if needed.

This should probably get at least two fairly thorough reviewers given how extensive the changes are.

[agramfort]

reviewed gist. It looks good ! tests pass for me too...

[jona-sassenhagen]

Looks great so far!!! Can you cover one more case? split_legend=True, cmap=something

[drammock]

@jona-sassenhagen the way the code is now, cells 30 and 31 of the gist will not change if you add split_legend=True to them. In other words, split_legend only has effect if both color and linestyle are in the main MPL legend; split_legend=True, cmap=something is going to look exactly the same as split_legend=False, cmap=something. I'm happy to discuss improvements to controlling legend/colorbar behavior if you have ideas.

[jona-sassenhagen]

Ah sure. Sorry. The case I meant was: let’s say you have 8 items and 4 colors and 2 linestyles. So you might have dark blue dotted, dark blue straight. Light blue dotted, light blue straight. Light red dotted, light red straight. Etc.

And the legend would only indicate linestyle, the colorbar the color. Does that make sense?

[drammock]

here's the updated Gist and an nbviewer link since GitHub is sometimes not rendering the gist. Other than a whats_new entry I'd say this is ready for merge if the CIs come back green. @jona-sassenhagen do you want one more chance to weigh in, or are you happy?

[drammock]

Only 1 remaining CI error is unrelated:

――――――――――――――― test_digitization_constructor[list of digpoints] ―――――――――――――――
data = [<DigPoint |        LPA : (0.0, 0.0, 0.0) mm        : MEG device frame>, <DigPoint |     HPI #2 : (0.0, 0.0, 0.0) mm        : isotrak frame>, <DigPoint | Unknown #42 : (0.0, nan, nan) mm        : unknown frame>]
    @pytest.mark.parametrize('data', [
        pytest.param(digpoints_list, id='list of digpoints'),
        pytest.param(dig_dict_list, id='list of digpoint dicts',
                     marks=pytest.mark.xfail(raises=ValueError)),
        pytest.param(['foo', 'bar'], id='list of strings',
                     marks=pytest.mark.xfail(raises=ValueError)),
    ])
    def test_digitization_constructor(data):
        """Test Digitization constructor."""
        dig = Digitization(data)
>       assert dig == data
E       assert [<DigPoint | ...nknown frame>] == [<DigPoint |  ...nknown frame>]
E         (pytest_assertion plugin: representation of details failed.  Probably an object has a faulty __repr__.)
E         <ExceptionInfo ValueError tblen=2>
data       = [<DigPoint |        LPA : (0.0, 0.0, 0.0) mm        : MEG device frame>, <DigPoint |     HPI #2 : (0.0, 0.0, 0.0) mm        : isotrak frame>, <DigPoint | Unknown #42 : (0.0, nan, nan) mm        : unknown frame>]
dig        = [<DigPoint |        LPA : (0.0, 0.0, 0.0) mm        : MEG device frame>, <DigPoint |     HPI #2 : (0.0, 0.0, 0.0) mm        : isotrak frame>, <DigPoint | Unknown #42 : (0.0, nan, nan) mm        : unknown frame>]
mne/tests/test_digitization.py:29: AssertionError

[larsoner]

Restarted the broken build and it passed, all green.

@jona-sassenhagen feel free to merge if you're happy (enough) and we can iterate more in subsequent PRs

[jona-sassenhagen]

Thanks a lot @drammock !!!