MRG: Allow str list in picks

[larsoner]

WIP as proof of concept including changes to mne/viz functions (and a few other misc ones) as requested by @agramfort.

Assuming people are convinced, need to do:

• complete tests for _picks_to_idx
• update the introduction (or other?) tutorial
• properly document options
• go through codebase and unify all picks logic
• make CIs happy
• finalize docstring via feedback
• replace all XXX with finalized docstring, or use doccer depending on MAINT: DRY our docstrings #5753

Closes #4502.

[agramfort]

quite a few red lines ... thx @larsoner for taking a stab at this so quickly

[codecov-io]

Codecov Report

Merging #4511 into master will increase coverage by 0.01%.
The diff coverage is 99.04%.

@@            Coverage Diff             @@
##           master    #4511      +/-   ##
==========================================
+ Coverage   88.73%   88.75%   +0.01%     
==========================================
  Files         401      401              
  Lines       72354    72422      +68     
  Branches    12154    12122      -32     
==========================================
+ Hits        64205    64278      +73     
- Misses       5214     5218       +4     
+ Partials     2935     2926       -9

[jona-sassenhagen]

Would presumably simplify stuff like this https://github.com/mne-tools/mne-python/blob/master/mne/viz/evoked.py#L1272 and this https://github.com/mne-tools/mne-python/blob/master/mne/viz/evoked.py#L1613

[cbrnr]

This is a great idea! Let me know if you need help.

[larsoner]

Do you want to take over? It would probably get done faster that way

[larsoner]

FYI @cbrnr I'm going to try to come back to this this week

[larsoner]

I'm going to move this to 0.17. It's not really a blocker and we might want to get some mileage out of testing it before springing it on folks.

[larsoner]

Okay @agramfort I think the API proposal is complete now. Hopefully it's clear from the updated docstring and examples. Can you take a look?

I have only updated docstrings in one file (epochs.py) so we can converge on it. Once that's done I'll do the terrible task of trying to change them everywhere in the codebase. Eventually everything should use _picks_to_idx and I've changed a number of them, but not all.

[larsoner]

@cbrnr do you have time to take a look?

[larsoner]

@jona-sassenhagen do you have time to look at the docstring for picks with these changes?

[cbrnr]

I don't have time to go through the code at the moment, but I was wondering how you deal with ambiguity. For example, what if I have a channel labeled eeg and I want to pick it (instead of picking all channels with type eeg)?

Also, in case this PR requires the channel type mapping, it would be great if it didn't make de-duplicating this mapping any more difficult (#4851).

[larsoner]

It will throw an error and the user can use a more specific function

[larsoner]

This one is a bit too invasive to squeeze in just before release, better to make it an early 0.18 enhancement

[larsoner]

@jona-sassenhagen @cbrnr @mmagnuski are you still interested in this option? If so, let me know if you're happy with the docstring above. If not, I can close.

[mmagnuski]

@larsoner I'm very interested, sorry I didn't respond earlier, I'll take a look tomorrow!

[mmagnuski]

BTW, this will definitely improve my teaching experience. For some reason many of my students had problems remembering about epochs.ch_names.index(ch_name) and even more when should be a list comprehension. :)

[agramfort]

no more objection on this in 2019 for me :)

but you need to rebase @larsoner

[larsoner]

This is ready for review/merge from my end.

[mmagnuski]

Travis complains:

python setup.py check --restructuredtext --strict
running check
make[1]: Leaving directory '/home/travis/build/mne-tools/mne-python'
Makefile:126: recipe for target 'pep' failed
make: *** [pep] Error 2

but that's likely unrelated?

Besides, 👍 fill_doc to the rescue! 🚀 :)

[larsoner]

@massich can you take a look and merge if you're happy? I think everyone was happy with the API, and the last commits were just about rebasing and filling out the doc using doccer.

[massich]

just opinions here and there. Thx @larsoner the code is much more readable.

Maybe instead of 'code titles' I would prefer them to be private functions somewhere, or something. But they are fine.