API: picks as list of string

[kingjr]

Picking a particular channel is currently relatively complex for new users.

It either involves pythonic tricks:

picks = [ii for ii, ch in enumerate(epochs.ch_names) if ch == 'mychan']
epochs.plot(picks=picks)

Or master of MNE's functions:

picks = mne.pick_channels(evoked.info. [`my_chan`])
epochs.plot(picks=picks)

Proposal 1

:+1: 0 Since picks is always referring to a list of int and ch_names is always a list of str, I propose that we accept picks ot be a list of str:

evoked.plot(picks=list(range(10)))
epochs.plot(picks=['my_chan'])

Proposal 2

:+1: 0

Accepting the parameter picks to be a list of int (for indexing), a list of str (to find channel names) or a dict (to quickly select channel types).

evoked.plot(picks=list(range(10)))
evoked.plot(picks=['MEG 1132', 'MEG 1022'])
evoked.plot(picks=dict(grad=True, eeg=False))

• advantage: no deprecation, simplification of channel picking
• disadvantage: keep clunky pick_type API

Proposal 3

:+1: 0

Same as proposal 2, but change pick_types to accept a types key that would accept list of str. This would allow

evoked.plot(picks=list(range(10)))
evoked.plot(picks=['MEG 1132', 'MEG 1022'])
evoked.plot(picks=dict(grad=True, eeg=False))
evoked.plot(picks=dict(types=('eeg', 'mag')))
mne.pick_types(info, types=('eeg', 'mag'))

• advantage: easier to loop across channel types, no deprecation
• disadvantage: more than one way to do pick_types

Proposal 4:

:+1: 4 (@larsoner, @kingjr, @nbara, @choldgraf)

Same as proposal 1, but automatically detect if str refer to ch_names of ch_types.

evoked.plot(picks=list(range(10)))
evoked.plot(picks=['MEG 1132', 'MEG 1022'])
evoked.plot(picks=('eeg', 'meg'))

• inconvenient: risk of conflict between channel names and channel types
• advantage: easier to loop across channel types

[dengemann]

@kingjr I had thought about this for long time and am glad you bring it up. I think it can be a good idea. It will be unproblematic whenever the function picks are passed to has access to .info. I sometimes thought we should have written mne.pick_types that way to accept a list of channel keys. But I would not suggest to depracate it. Maybe a keys argument could be added there. On Fri, 18 Aug 2017 at 09:25, Jean-Rémi KING notifications@github.com wrote:

Picking a particular channel is currently relatively complex for new users.

It either involves pythonic tricks:

picks = [ii for ii, ch in enumerate(epochs.ch_names) if ch == 'mychan'] epochs.plot(picks=picks)

Or master of MNE's functions:

picks = mne.pick_channels(evoked.info. [my_chan]) epochs.plot(picks=picks)

Since picks is always referring to a list of int and ch_names is always a list of str, I propose that we accept picks ot be a list of str:

epochs.plot(picks=['my_chan'])

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0fivoEudkx2qgdaG_9TUCtDEreRSCfks5sZTyAgaJpZM4O7MZP .

[kingjr]

@dengemann For pick_types we could accept picks to be a dict

[dengemann]

I don't yet see what you have in mind. The function takes keyword arguments for channel types. It is annoying that you cannot just loop over types but have to. Create bool flags and then deal with meg that can be 'mag', 'grad' or True/False. If you could do:

pick_types(info, keys=('mag', 'eeg', 'eog'))

That would simplify lots of code.

On Fri, 18 Aug 2017 at 09:34, Jean-Rémi KING notifications@github.com wrote:

@dengemann https://github.com/dengemann For pick_types we could accept picks to be dict

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502#issuecomment-323282298, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0fipb5WZB4Oiuk_6H0CbGWQDHtM8dDks5sZT6MgaJpZM4O7MZP .

[kingjr]

Proposal 2

Sorry, I meant accepting the parameter picks to be a list of int (for indexing), a list of str (to find channel names) or a dict (to quickly select channel types).

evoked.plot(picks=list(range(10)))
evoked.plot(picks=['MEG 1132', 'MEG 1022'])
evoked.plot(picks=dict(grad=True, eeg=False))

EDIT: Copied to top

[dengemann]

Yes. I think we are discussing complementary ideas. I like the list of types notation more as it is far more concise. It could not only simplify passing picks but creating them. In some situations you would still need the full integer arrays. On Fri, 18 Aug 2017 at 09:53, Jean-Rémi KING notifications@github.com wrote:

Sorry, I meant accepting the parameter picks to be a list of int (for indexing), a list of str (to find channel names) or a dict (to quickly select channel types).

evoked.plot(picks=dict(grad=True, eeg=False)) evoked.plot(picks=range(10)) evoked.plot(picks=['MEG 1132', 'MEG 1022'])

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502#issuecomment-323285866, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0fiveZMMqfJzTETEgCaaZeyMOOh9Qlks5sZULmgaJpZM4O7MZP .

[kingjr]

I see. However, the issue is that a list of types would conflict with a list of names.

evoked.plot(picks=['MEG 1132', 'MEG 1022'])
evoked.plot(picks=['mag', 'grad'])

I'm thus in favor of Proposal 2, i.e. accepting list of string and dict.

I'll update the above description.

[larsoner]

@kingjr I updated your proposal to to be Py3k-friendly, and to reorder with list(range(...)) first (since it's what we have now)

[larsoner]

It does seem like proposal 2 would just require a ~6-line private function anytime we use picks, so it's not so bad from an implementation standpoint. And from a user standpoint it's pretty simple.

It does have overlap, e.g. inst.pick_types(['EEG 001', 'EEG 002']) will be the same as inst.pick_channels(['EEG 001', 'EEG 002']), but I can live with that.

@kingjr if your sense is that this will reduce pain substantially for new users, +1 from me

[dengemann]

I would still prefer proposal 1 (list of channel types). It would allow me to kill lots of code in virtually any project. I don't think we would ever hit a case where channel names and type names are identical. For updating mne.pick_types along these lines we would anyways have to add a new keyword. On Fri, 18 Aug 2017 at 13:56, Eric Larson notifications@github.com wrote:

It does seem like proposal 2 would just require a ~6-line private function anytime we use picks, so it's not so bad from an implementation standpoint. And from a user standpoint it's pretty simple.

It does have overlap, e.g. inst.pick_types(['EEG 001', 'EEG 002']) will be the same as inst.pick_channels(['EEG 001', 'EEG 002']), but I can live with that.

@kingjr https://github.com/kingjr if your sense is that this will reduce pain substantially for new users, +1 from me

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502#issuecomment-323334482, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0fioDVNKQm7ofrms7znsfzr1d0jV-Gks5sZXv3gaJpZM4O7MZP .

[kingjr]

I would still prefer proposal 1 (list of channel types). It would allow me to kill lots of code in virtually any project.

Can you describe in what way Proposal 2 wouldn't?

I don't think we would ever hit a case where channel names and type names are identical.

This would still require a "smart" guess; not safe IMO.

For updating mne.pick_types along these lines we would anyways have to add a new keyword.

Why? We wouldn't need to update pick_types in any of these proposal

[larsoner]

Why? We wouldn't need to update pick_types in any of these proposal

Agreed, we would just update any function that has picks argument. Instead of taking only list of int, take the other things, too. pick_types and pick_channels would remain the same.

[dengemann]

I would independently from these proposals like to get rid of the dict / pick_types api because it is too irregular and makes looping over channel types a pain. In terms of elegance ('mag', 'eeg') is much more convenient then dict(meg='mag', eeg=True).

On Fri, 18 Aug 2017 at 14:12, Eric Larson notifications@github.com wrote:

Why? We wouldn't need to update pick_types in any of these proposal

Agreed, we would just update any function that has picks argument. Instead of taking only list of int, take the other things, too. pick_types and pick_channels would remain the same.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502#issuecomment-323337394, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0fio2M4YHkN9w32M6DBcPQV0NxSvkBks5sZX-9gaJpZM4O7MZP .

[kingjr]

I agree; although for this we can propose an orthogonal PR where mne.pick_types accepts a list of str

mne.pick_types(info, types=('mag', 'grad', 'eog'))

[dengemann]

Sounds like a good plan. Once you have picks you can use it downstream the old way. On Fri, 18 Aug 2017 at 14:43, Jean-Rémi KING notifications@github.com wrote:

I agree; although for this we can propose an orthogonal PR where mne.pick_types accepts a list of str

mne.pick_types(info, types=('mag', 'grad', 'eog'))

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502#issuecomment-323343606, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0fipWp45r_uQqwp1zLWXQ56NJi6-dHks5sZYcJgaJpZM4O7MZP .

[choldgraf]

I'm +1 with

mne.pick_types(info, types=('mag', 'grad', 'eog'))

I think this is much more descriptive than having a gigantic list of true/false.

I'm also +1 for allowing lists of strings. I'm a little hesitant to allow for dict(eeg=True) since I agree that is a clunky API already.

What if instead of dict(grad=True.., we allowed dict(types=['eeg', 'meg', ...])?

[larsoner]

I think that picks=dict(types=['eeg']) is about the same clunkiness as picks=dict(eeg=True).

Thinking about it more I actually don't mind picks=['eeg', 'meg']. I think that collisions will be extremely rare. In those cases where they do collide, we can raise an error about ambiguity and tell people to use picks=pick_channels or picks=pick_types explicitly.

[choldgraf]

I'm +1 with that. Just do something like

PICK_TYPES = ['eeg', 'meg', 'ecog', 'blahblah']
if any(any(ii == np.array(data.ch_names)) for ii in PICK_TYPES):
    warn('wtf dude use better names')

[larsoner]

It will be slightly more complicated because we need to check if all are channels, or all are types, and raise an error if it doesn't fit exactly one of those criteria, but yes something like that.

[kingjr]

ok, I'm updating the proposal sheet, wait for what @agramfort think to make any attempt PR, as API change isn't the least controversial to do.

[larsoner]

We just need to ensure he first sees that #3842 was merged so that he's in a good mood when reading this proposal

[choldgraf]

I have another proposal: what if we allow users to supply a dataframe to the picks argument. This dataframe would contain a single column, 'data', and each row of this column would be another dataframe that would contain a list of Series objects, one for each data type to be plotted.

[choldgraf]

also we can deprecate the picks argument and make it pick_pandas

[kingjr]

@choldgraf I actually thought of this, it'd be make so much more sense.... accessing the channel position, type etc would be so simple...

[larsoner]

Only if we can do a dataframe of dataframes, each with a single column for each channel type

Just so we're clear, my vote is for Proposal 4. I'm going to update the proposal to put my +1 there, everyone else feel free to do the same (people without permission to change, leave a comment and we'll update)

[larsoner]

@nbara you had an overall :+1: feel free to have another look at the top comment if you have time

[choldgraf]

so simple!

(in seriousness, I'm +1 with proposal 4 too)

[kingjr]

Only if we can do a dataframe of dataframes, each with a single column for each channel type

?

epochs.info['chs'] = pd.DataFrame(dict(name='MEG 001', type='mag', pos=xyz, ..., ...))
# find chan type
epochs.ch_names.query('type=="mag"')
# find specific combinations
epochs.ch_names['name'].apply(lambda x: 'meg 10' in x)

[choldgraf]

@kingjr you aren't supposed to expose our secret plot to slowly introduce pandas into everything in MNE!

[larsoner]

@kingjr that was a joke, using Pandas for this sort of thing seems way overkill

[dengemann]

Guys let's do one revolution at a time. Overcoming the picks dict would already be a great step for mnekind. On Fri, 18 Aug 2017 at 19:27, Eric Larson notifications@github.com wrote:

@kingjr https://github.com/kingjr that was a joke, using Pandas for this sort of thing seems way overkill

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502#issuecomment-323413336, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0fih68WleBLfhj2JjPV5ZjlJ2824rsks5sZcmQgaJpZM4O7MZP .

[dengemann]

Thinking of mne.pick_types I think we never had a single example supporting meg/eeg/eog/ etc. as positional arguments.

We could indeed dare to make types the 2nd argument:

picks = mne.pick_types(raw.info, ('mag', 'eeg'))

That would be simply beautiful. On Fri, 18 Aug 2017 at 19:29, Denis-Alexander Engemann < denis.engemann@gmail.com> wrote:

Guys let's do one revolution at a time. Overcoming the picks dict would already be a great step for mnekind. On Fri, 18 Aug 2017 at 19:27, Eric Larson notifications@github.com wrote:

@kingjr https://github.com/kingjr that was a joke, using Pandas for this sort of thing seems way overkill

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502#issuecomment-323413336, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0fih68WleBLfhj2JjPV5ZjlJ2824rsks5sZcmQgaJpZM4O7MZP .

[choldgraf]

IMO that is the most conceptually simple way to pick types, +1

[dengemann]

That wouldn't even necessitate deprecation of anything I would say. @choldgraf @kingjr @larsonser On Fri, 18 Aug 2017 at 19:38, Chris Holdgraf notifications@github.com wrote:

IMO that is the most conceptually simple way to pick types, +1

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502#issuecomment-323415822, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0fijkyT3LOn0KyOZ35BV3dW1KzP2Ycks5sZcv5gaJpZM4O7MZP .

[nbara]

I'm +1 for Proposal 4 as well!

[agramfort]

ok I coming late here but my first reaction is that "There should be one-- and preferably only one --obvious way to do it."

regarding your proposal 4 that seems to satisfy most of you:

evoked.plot(picks=list(range(10)))  # already works

evoked.plot(picks=['MEG 1132', 'MEG 1022'])  # can be done with:
evoked.copy().pick_channels(['MEG 1132', 'MEG 1022']).plot()

evoked.plot(picks=dict(grad=True, eeg=False))  # can be done with:
evoked.copy().pick_types(meg='grad', eeg=False).plot()

by putting the picking in the data container we don't need to have a complex pick logic in every viz or computation function.

thoughts?

[larsoner]

ok I coming late here but my first reaction is that "There should be one-- and preferably only one --obvious way to do it."

I agree as a general principle, and this does clearly violate it. The question in my mind is: does the added simplicity and meeting of users expectations justify it?

by putting the picking in the data container we don't need to have a complex pick logic in every viz or computation function.

It's still a bit harder. Most users probably want picks to do what 4 proposes, and have some disappointment when it doesn't.

Well the complex logic wouldn't exactly be in every function. It would be in a single private function (that gets called by each function). But conceptually this replaces one DRY function that should already exist but probably doesn't (to validate picks are list-like of int and are all valid values).

[dengemann]

@agramfort think of how our proposal would help looping over channel types to get picks. At least for pick_types. You just loop over ('meg', 'eeg') etc.

The dict of bool / string demanded by its current API is mildly spoken a PITA. You need to make exceptions for "meg" which can be True/False/"grad"/"mag". Then it is not parsimonious. Why eeg=True if it can just be "eeg". If we could overcome for once our purist API change culture we would make certainly 99% of all users significantly more happy. Or differently put the one way to do it that we currently suggest is not the right / best one.

Please think of it twice. On Sat, 19 Aug 2017 at 14:25, Eric Larson notifications@github.com wrote:

ok I coming late here but my first reaction is that "There should be one-- and preferably only one --obvious way to do it."

I agree as a general principle, and this does clearly violate it. The question in my mind is: does the added simplicity and meeting of users expectations justify it?

by putting the picking in the data container we don't need to have a complex pick logic in every viz or computation function.

It's still a bit harder. Most users probably want picks to do what 4 proposes, and have some disappointment when it doesn't.

Well the complex logic wouldn't exactly be in every function. It would be in a single private function (that gets called by each function). But conceptually this replaces one DRY function that should already exist but probably doesn't (to validate picks are list-like of int and are all valid values).

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502#issuecomment-323520147, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0fivNHYzXcU_GvGy3w4qIyXLSd6tyRks5sZtRWgaJpZM4O7MZP .

[agramfort]

how would you write this:

picks = pick_types(raw.info, meg=False, eeg=True, stim=False, eog=False,
                   exclude='bads')

epochs = Epochs(raw, events, event_id, tmin, tmax, proj=False,
                picks=picks, baseline=None, preload=True,
                verbose=False)

would we deprecate some pick_ methods or functions to avoid duplications?

[choldgraf]

wouldn't you just do:

Epochs(raw, events, event_id, tmin, tmax, ..., picks=['eeg'], ...

or if you wanted a subset of channels:

Epochs(raw, events, event_id, tmin, tmax, ..., picks=['ch_01', 'ch_03']) ?

[agramfort]

this would exclude bads automatically? don't you make a difference between ('eeg') and ['eeg'] ie tuple for type and list of ch names?

[choldgraf]

ah I see what you mean - in that case I think it'd be fine to include an exclude kwarg people can use in Epochs...it seems like a natural thing to do.

re: type vs. list of names, I think @larsoner's thought was that it would be rare enough not to be a problem and that we could handle it with good warnings or errors. That said, I agree it feels clunky

[larsoner]

In practice I don't expect collisions, and when they do happen it's easy to redirect to functions.

I'd say behave as exclude='bads' and if you want something more advanced (e.g., bads or ignore_ref non-defaults) use the functions. -1 on adding extra kwargs to deal with all of these cases.

In my mind this is about improving convenience and removing barriers to entry for minimal maintenance and implementation cost, not replacing the existing, more complete API.

[nbara]

@agramfort If I may, as a relatively new user, I find channel picking in MNE to be extremely unintuitive. Syntaxes like these in particular seem very convoluted to me :

evoked.copy().pick_channels(['MEG 1132', 'MEG 1022']).plot()
evoked.copy().pick_types(meg='grad', eeg=False).plot()

I appreciate that an API change is not insignificant, but in this case I would find it most welcome.

(just my 2¢)

[agramfort]

@nbara would you suggest to also simplify these lines:

evoked.copy().pick_channels(['MEG 1132', 'MEG 1022']) evoked.copy().pick_types(meg='grad', eeg=False)

ie without the plot? or it's the fact that you need to pick data before plotting?

[dengemann]

@agramfort

picks = pick_types(raw.info, types=('eeg',), exclude='bads')

epochs = Epochs(raw, events, event_id, tmin, tmax, proj=False,
                picks=picks, baseline=None, preload=True,
                verbose=False)

We would not need to deprecate anything. Once types is not None, all other flags will be ignored. Already updating this function would clean up the code as it allows you to get picks very easily:

for ch_type in ('mag', 'grad', 'meg', 'eeg'):
    picks = mne.pick_types(raw.info, types=(ch_type,))
    # or
    picks = mne.pick_types(raw.info, (ch_type,))
    pass

[agramfort]

What I am thinking is that we could have a single pick method

evoked.pick(picks, channels, types)

so what @nbara suggests would be written:

evoked.copy().pick(channels=['MEG 1132', 'MEG 1022']).plot() evoked.copy().pick(types=['grad']).plot()

so the semantic of picks stays picks=list of int

[dengemann]

Which would be compatible with the upgrade of mne.pick_types which always returns array of int. Keep in mind that we need picks in data that don't have info, pure numpy. On Mon, 21 Aug 2017 at 11:45, Alexandre Gramfort notifications@github.com wrote:

What I am thinking is that we could have a single pick method

evoked.pick(picks, channels, types)

so what @nbara suggests would be written:

evoked.copy().pick(channels=['MEG 1132', 'MEG 1022']).plot() evoked.copy().pick(types=['grad']).plot()

so the semantic of picks stays picks=list of int

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502#issuecomment-323698389, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0figcYHbqJ-eMnE7vkf5ZDK2QbNQgYks5saVG7gaJpZM4O7MZP .

[agramfort]

would we still need to refactor pick_types?

you envision a pb with Covariance class?

[dengemann]

I need mne.pick_types all the time. But taking what you and @nbara suggest to the next level we could have:

mne.pick(info, types, channels=None, exclude='bads', include=None)

On Mon, 21 Aug 2017 at 11:59, Denis-Alexander Engemann < denis.engemann@gmail.com> wrote:

Which would be compatible with the upgrade of mne.pick_types which always returns array of int. Keep in mind that we need picks in data that don't have info, pure numpy. On Mon, 21 Aug 2017 at 11:45, Alexandre Gramfort notifications@github.com wrote:

What I am thinking is that we could have a single pick method

evoked.pick(picks, channels, types)

so what @nbara suggests would be written:

evoked.copy().pick(channels=['MEG 1132', 'MEG 1022']).plot() evoked.copy().pick(types=['grad']).plot()

so the semantic of picks stays picks=list of int

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/issues/4502#issuecomment-323698389, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0figcYHbqJ-eMnE7vkf5ZDK2QbNQgYks5saVG7gaJpZM4O7MZP .

[agramfort]

@nbara please let me know what you think