MRG, FIX: avoid spurious "showing first five" message

[drammock]

the info message No picks and no groupby, showing the first five channels ... is incorrectly shown when the picks parameter to epochs.plot_info() is a (list of) channel names (the message should only happen when picks is a channel type or list of channel types).

on current master, this (wrongly) generates the info message; after this PR it does not:

from mne.viz.tests.test_epochs import _get_epochs
epochs = _get_epochs().load_data()
epochs.plot_image(picks='MEG 1332')

after this PR, the message is still (correctly) generated for:

epochs.plot_image(picks='mag')

[larsoner]

Breaks some ICA stuff

https://travis-ci.org/mne-tools/mne-python/jobs/530103782

You might be able to just do picks = _picks_to_idx(epochs.info, picks) directly, it should handle single strings just fine.

[drammock]

it's actually the following line that breaks it (the generator expression inside the all()). I'll try to find a better way.

[larsoner]

My point is really that instead of making this conditional any smarter (which your next commit did I think), you should just do picks = _picks_to_idx here, as it should already have all of the smartness built in. Then on the next line you know picks is a ndarray of int, and can just check the length.

[drammock]

that would change the behavior. Currently we only truncate to 5 picks if the user passes a channel type. Under your suggestion, if a user asked for 8 specific channels (by name or by index) we would only return them 5 channels.

[larsoner]

Currently we only truncate to 5 picks if the user passes a channel type

I would make it so that _picks_to_idx can optionally return how it made the picks (from channel names, channel types, from None, etc.). Then call it with return_how=True and check that here.

[drammock]

OK. I prefer picks='eeg' (or any channel type) returning the GFP since it's consistent with what @jona-sassenhagen implemented for evokeds in #6211 (see here). And since @larsoner says he's fine either way, I'll go ahead and implement that (shouldn't be too much work) and let @agramfort weigh in afterwards.

This will be an API change (same args, different behavior)... my instinct is that it's edge-case behavior anyway (not exactly a bug, but getting plots of 5 arbitrary channels was probably not desired behavior for users anyway) so maybe OK to skip the warning cycle? WDYT?

[jona-sassenhagen]

I think GFP is better for lines than for images because the GFP, not starting at 0, is squished to a small subset of the color range for images. You simply see less. This is not so much an issue for line plots.

[drammock]

ah, good point.

[larsoner]

I think GFP is better for lines than for images because the GFP, not starting at 0, is squished to a small subset of the color range for images.

When doing GFP (and really any one-sided data), we should probably switch the colormap to viridis or some other sequential rather than diverging colormap. It will help ameliorate this problem at least somewhat.

Knowing the complexity of our viz code it's entirely possible this will not be an easy change, though, so no need to do it here unless it turns out to be easy and does work well.

[jona-sassenhagen]

For all-positive images like GFP, we actually use Reds here. But maybe Viridis would be better still.

[codecov]

Codecov Report

Merging #6288 into master will increase coverage by 0.24%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6288      +/-   ##
==========================================
+ Coverage   88.98%   89.23%   +0.24%     
==========================================
  Files         416      417       +1     
  Lines       74973    75427     +454     
  Branches    12353    12419      +66     
==========================================
+ Hits        66718    67307     +589     
+ Misses       5367     5233     -134     
+ Partials     2888     2887       -1

[jona-sassenhagen]

I'm ok with the PR (review is stylistic), but switching to mean or gfp might be even better.

[jona-sassenhagen]

We could have a test where we count the length of what epochs.plot_image returns too.

[jona-sassenhagen]

I mean, 5 figs, <5 figs, >5 figs.

[drammock]

see my (monster) comment with the table. I'm proposing that we get rid of the magic number 5 and give people what they ask for, but make it harder for them to ask for lots of figs (i.e., they only can get lots if they list channel {names|indices} individually, or pass a group_by dict with lots and lots of keys). We can (should) probably add a note to the docstring warning people not to do too many at once, and if they want to view lots of epochs imagemaps, they should consider the topo version that allows them to click through each sensor one at a time.

[agramfort]

+1

[drammock]

+--------------------+----------+-------------+----------------------------+
| picks              | group_by | combine     | result                     |
+====================+==========+=============+============================+
|                    |          | callable    |                            |
|                    |          +-------------+                            |
|                    |          | 'mean'      |                            |
|                    |          +-------------+                            |
|                    | None     | 'median'    | 1 fig per ch_type          |
|                    |          +-------------+                            |
|                    |          | 'std'       |                            |
|                    |          +-------------+                            |
| None,              |          | 'gfp', None |                            |
| ch_type,           +----------+-------------+----------------------------+
| list of ch_types   |          | callable    |                            |
|                    |          +-------------+                            |
|                    |          | 'mean'      | 1 fig per dict key         |
|                    |          +-------------+                            |
|                    | dict     | 'median'    | (dict values index into    |
|                    |          +-------------+  whatever `picks` yields,  |
|                    |          | 'std'       |  not orig. sensor indices) |
|                    |          +-------------+                            |
|                    |          | 'gfp', None |                            |
+--------------------+----------+-------------+----------------------------+
|                    |          | callable    |                            |
|                    |          +-------------+                            |
|                    |          | 'mean'      | 1 fig                      |
|                    |          +-------------+                            |
|                    | None     | 'median'    | (allows combining across   |
|                    |          +-------------+  different sensor types,   |
|                    |          | 'std'       |  but emits RuntimeWarning) |
|                    |          +-------------+                            |
|                    |          | 'gfp'       |                            |
| int,               |          +-------------+----------------------------+
| list of int,       |          | None        | 1 fig per pick             |
| ch_name,           +----------+-------------+----------------------------+
| list of ch_names   |          | callable    | 1 fig per dict key         |
|                    |          +-------------+                            |
|                    |          | 'mean'      | (dict values index into    |
|                    |          +-------------+  whatever `picks` yields,  |
|                    | dict     | 'median'    |  not orig. sensor indices) |
|                    |          +-------------+                            |
|                    |          | 'std'       | (only works if each dict   |
|                    |          +-------------+  value selects sensors all |
|                    |          | 'gfp', None |  of same type: ValueError) |
+--------------------+----------+-------------+----------------------------+

I think this table summarizes what the behavior should be. It's different from
current behavior only in one case: when picks is a (list of) channel types, and
group_by and combine are both None. Currently in that case you get 5
arbitrary channels; as proposed in table that returns 1 fig per ch_type (GFP).
Do we all agree on that?

If so, I think some changes that make sense are:

• we don't need to accept group_by='type' anymore because it never gives a
  different result from group_by=None

• when picks=specific_indices_or_names, we should not act differently when
  group_by=dict(...) selects heterogeneous sensor types (ValueError) versus
  when group_by=None, combine='anything_but_None' selects heterogeneous
  sensor types (RuntimeWarning). Either both should be Error or both Warning.

• Three related changes:

  1. Don't allow non-None for picks if group_by is a dict. Just infer
     picks from the dict values.
  2. when group_by dict values are integers, treat them as indexing into
     the full list of ch_names (necessary given prev. point).
  3. allow group_by dict values to be channel names, not just integers
     (optional, but would be nice)

• whenever group_by is None, construct a dict anyway depending on values of
  picks and combine. That should simplify the code path, because we'd
  always be plotting from a dict (one figure per dict key).

[larsoner]

Fine by me, okay for you @jona-sassenhagen ?

[jona-sassenhagen]

One further complication: we should think to what extent this applies to more functions, e.g. plot_compare_evokeds and evoked.plot_image.

[jona-sassenhagen]

Let me be more clear, I like @drammock 's long suggestion, but I think we should probably homogenise this across as many plotters as possible.

[larsoner]

Okay to just fix these for 0.18, and do the others in 0.19? Or should we just move all to 0.19? I don't think it's worth delaying release to try to get them all, and I'm not sure it will be so easy.

[jona-sassenhagen]

My intuition would be to delay to .19. But if @drammock disagrees, I wouldn't object.

[larsoner]

Okay with me, we can backport to 0.18.1 if it ends up being simple enough anyway

[drammock]

I worked on this a bunch yesterday. Give me 1 more day to see if I can wrap it up. If not then I'll ask to just merge the fix to the message. I agree that it's a good idea to do a similar refactor for all plotters but I don't necessarily agree that it must all be in one PR

…

On May 16, 2019 3:51:14 AM AKDT, Eric Larson ***@***.***> wrote: Okay with me, we can backport to 0.18.1 if it ends up being simple enough anyway

[drammock]

@larsoner @jona-sassenhagen I got really close to finishing the big refactor, but realized there won't be time for a proper review of it in time to release. So here's the minimal change that just fixes the info message.

[larsoner]

+1 for merging this for 0.18, and doing the big refactoring in 0.19.

Ultimately it's probably a good idea for the big refactoring to happen after release because often corner case viz bugs take a while to manifest and are difficult to catch in testing/review anyway.