Replace inst.pick_channels() with inst.pick()

[cbrnr]

This fixes deprecation warnings by replacing inst.pick_channels() with inst.pick() (where inst is one of Raw, Epochs, TFR classes, and possibly more). The same replacement is necessary for inst.pick_types().

[cbrnr]

There are also some docstrings that need to be adapted, but I'm not sure I even understood that change. Why were inst.pick_channels() and inst.pick_types() deprecated in favor of inst.pick()? This seems inconsistent with mne.pick_channels() and mne.pick_types() (which still exist) as well as inst.drop_channels() (which should be called inst.drop()).

Also, inst.pick_channels() has an ordered parameter, which is not present in inst.pick().

Finally, I'm not sure how inst.pick() is supposed to handle channel names that are equivalent to one of our types.

[larsoner]

There are also some docstrings that need to be adapted, but I'm not sure I even understood that change. Why were inst.pick_channels() and inst.pick_types() deprecated in favor of inst.pick()?

Have you done some investigation on your own looking at blame, GitHub PRs, linked issues, and maybe also dev discussion notes on Discord? I suspect this was discussed at some point previously

[larsoner]

Also, inst.pick_channels() has an ordered parameter, which is not present in inst.pick().

The behavior is equivalent to ordered=True which should make sense in most cases. Users who really want set-like behavior can get it with a bit more work at their own end.

Finally, I'm not sure how inst.pick() is supposed to handle channel names that are equivalent to one of our types.

Is this a problem for you in practice? This was also discussed at some point a long time ago (I think when we implemented generic string-based picking) and we decided this was going to be a very rare corner case. There might even be a test or two for it, not sure. Feel free to look into past comments and code blame etc. if you're still interested because I think there's stuff in our history about this somewhere as well

[cbrnr]

Have you done some investigation on your own looking at blame, GitHub PRs, linked issues, and maybe also dev discussion notes on Discord? I suspect this was discussed at some point previously

Yes. The PR with the changes is #11665, and the main discussion is in #11531. As far as I understood (I wasn't part of the original discussion), the main issue was inconsistent behavior between .pick_channels() and .pick().

Re the ordered parameter, so we deprecate/remove the ordered=False functionality? That's fine with me, but we should provide an upgrade path for people relying on that behavior.

Re channel names equalling channel types, I think this is definitely a rare case, but probably not rare enough to ignore. We even have a test, which uses channel types as names: https://github.com/mne-tools/mne-python/blob/main/mne/io/fiff/tests/test_raw_fiff.py#L1150

IMO, a nice API would have two independent parameters names and types, so that you could e.g. pick all MEG channels plus C3, Cz, and C4 like this:

inst.pick(types="meg", names=["C3", "Cz", "C4"])

I would also remove the exclude parameter, because a drop method (as the opposite of pick) would be more consistent. This method should also have the names and types parameters. For example, if you wanted to pick all EEG channels except for C3, Cz, and C4:

inst.pick(types="eeg").drop(names=["C3", "Cz", "C4"])

I think this would cover most (all?) use cases, but of course I might be missing something.

[larsoner]

I'm not sure I even understood that change. Why were inst.pick_channels() and inst.pick_types() deprecated in favor of inst.pick()?

As far as I understood (I wasn't part of the original discussion), the main issue was inconsistent behavior between .pick_channels() and .pick().

And possibly silent buggy behavior, and the usual considerations of having one-right-way to do things, and trying not to break people's code, etc.

Re the ordered parameter, so we deprecate/remove the ordered=False functionality? That's fine with me, but we should provide an upgrade path for people relying on that behavior.

No, I consider this issue closed/resolved by @legacy as it's the consensus we landed on. So no further action needed.

Is this a problem for you in practice? This was also discussed at some point a long time ago (I think when we implemented generic string-based picking) and we decided this was going to be a very rare corner case. There might even be a test or two for it, not sure. Feel free to look into past comments and code blame etc.

Re channel names equalling channel types, I think this is definitely a rare case, but probably not rare enough to ignore. We even have a test, which uses channel types as names: https://github.com/mne-tools/mne-python/blob/main/mne/io/fiff/tests/test_raw_fiff.py#L1150

Yes you found the test I was talking about! Did you look into the history and discussion, though? It looks like this was a concern that was brought up and addressed explicitly in #4511 (comment) (with other API discussion in #4502). Given this extensive prior discussion, I'd rather not continue to discuss this unless we see in practice that it is making things difficult for people. We have had this code in main for 5 years and nobody has complained, which is at least some non-zero evidence that our current approach is working.

IMO, a nice API would have two independent parameters names and types, so that you could e.g. pick all MEG channels plus C3, Cz, and C4 like this: ... I would also remove the exclude parameter

We spent a decent amount of time years ago deciding an API and implementing it. I don't think we've met the burden for adding new params and breaking our existing API. I don't think we should strive to make all our APIs ideal when it breaks backward compatibility, or strive to cover every corner case. I think we need to be practical and focus on stuff people need, and avoid breaking stuff that already works, even if the API is a bit ugly.

[larsoner]

This seems inconsistent with mne.pick_channels() and mne.pick_types() (which still exist) as well as inst.drop_channels() (which should be called inst.drop()).

... these functions we could think about at some point, though, because we decided to focus on instance methods to start and did not make a decision about the:

#11531 (comment)

But we should have a separate issue for that since it's going to be another non-trivial discussion separate from replacing instance method usage (this PR).

[cbrnr]

We spent a decent amount of time years ago deciding an API and implementing it. I don't think we've met the burden for adding new params and breaking our existing API. I don't think we should strive to make all our APIs ideal when it breaks backward compatibility, or strive to cover every corner case. I think we need to be practical and focus on stuff people need, and avoid breaking stuff that already works, even if the API is a bit ugly.

We just introduced a new API, namely the .pick() method. It would have been nice if this had been discussed more widely.

Yes you found the test I was talking about! Did you look into the history and discussion, though? It looks like this was a concern that was brought up and addressed explicitly in #4511 (comment) (with other API discussion in #4502). Given this extensive prior discussion, I'd rather not continue to discuss this unless we see in practice that it is making things difficult for people. We have had this code in main for 5 years and nobody has complained, which is at least some non-zero evidence that our current approach is working.

But the workaround will stop working, because there won't be a separate pick_types() method around, or no?

We spent a decent amount of time years ago deciding an API and implementing it. I don't think we've met the burden for adding new params and breaking our existing API. I don't think we should strive to make all our APIs ideal when it breaks backward compatibility, or strive to cover every corner case. I think we need to be practical and focus on stuff people need, and avoid breaking stuff that already works, even if the API is a bit ugly.

I think the addition of a new method, which is going to replace two legacy methods, would have been an opportunity to make the API less ugly. It is arguably worse than before IMO. But I won't argue for it anymore if no one else thinks it worth doing.

Re mne.pick* functions, I agree this is a separate issue, and my intention here was to focus exclusively on the instance methods. Actually, the real reason was that I got a ton of deprecation warnings when using a TFR function, because the old methods are still all over the place.

[larsoner]

We just introduced a new API, namely the .pick() method. It would have been nice if this had been discussed more widely.

I think you might be misreading the history -- assuming you're talking about inst.pick, it was added to Raw, Epochs, and Evoked in 0.17 in the PR linked above:

https://github.com/mne-tools/mne-python/pull/4511/files#diff-5fc421f470c2e71b5fea8f8f50f24e5d7f400eb1be4daca053d0b4d0fb884877R22

Docs seem to confirm this back to at least 0.21 (older ones are archived and I don't want to unzip them). Discussion of its functionality appears to have been pretty extensive based on the linked discussions in #4511 and #4502 (and possibly elsewhere).

The (only?) new-ish picking API change is to change how pick_channels works based on discussions in #11531 (comment) and @legacy some stuff in favor of .pick, but that's meant to be more of a cosmetic change (see below).

because there won't be a separate pick_types() method around, or no?

We have no plans to remove it. @legacy we plan to keep indefinitely.

the real reason was that I got a ton of deprecation warnings when using a TFR function

This part is weird -- I don't think @legacy should warn. Is it possible you were getting warnings when using pick_channels? You should only get those if passing ordered=True would change the result, which is the behavior we settled on in #11531 (comment) to tell people "you are maybe not getting the result you want right now, and the result will change in the next release where we change to ordered=True by default". Maybe open a separate issue about this to debug?

[cbrnr]

I think you might be misreading the history [...]

You're right, I thought this was added recently. However, it's a pity that what @agramfort suggested way back then (which is exactly what I suggested in this PR again) didn't get enough traction (although many people in the original thread liked it a lot).

We have no plans to remove it. @legacy we plan to keep indefinitely.

OK, this clears up the confusion. I thought they were going to be removed.

Still, it is not ideal that disambiguating channel names and types can only be done with these legacy functions. I don't think adding names and types parameters would be a huge problem, because the existing picks parameter could stay. We'd just need to make sure that these are not used in combination.

This part is weird -- I don't think @legacy should warn. Is it possible you were getting warnings when using pick_channels?

It's not a warning, but this message after calling AverageTFR.plot():

NOTE: pick_channels() is a legacy function. New code should use inst.pick(...).

This is because a private function still uses pick_channels(): https://github.com/mne-tools/mne-python/blob/main/mne/time_frequency/tfr.py#L3289

[drammock]

This is because a private function still uses pick_channels()

well, that is definitely something we can/should change now, without further discussion.

[drammock]

Thanks for tackling this... I had meant to work on this immediately after the addition of @legacy to these methods, but clearly it slipped off my plate. Lots of CI failures, probably all down to repeat examples of the one comment I've left below

[cbrnr]

@drammock would you like to take over? I thought this would be much quicker, and now I need to focus on other things. If there's no hurry, I can of course continue to work on it later (probably in a couple of weeks). I think I've fixed all (simple) issues except for some real bugs that need a little more work.

[drammock]

@drammock would you like to take over? I thought this would be much quicker, and now I need to focus on other things. If there's no hurry, I can of course continue to work on it later (probably in a couple of weeks). I think I've fixed all (simple) issues except for some real bugs that need a little more work.

I think I can tackle this tomorrow or Monday (but if I fail to, feel free to pick it back up when you have time)

[marsipu]

Thank you, I am glad you are looking into this. I have two more that I just noticed in mne/channels/channels.py:
l. 222 sel = pick_channels(
l. 229 inst.pick_channels(common_channels, ordered=True)

[cbrnr]

This is a bit of a never-ending story. Is it really too late to discuss/revert/modify this change? I still think replacing .pick_channels() and .pick_types() was not a great idea for the following reasons:

1. The replacement .pick() method cannot select channel names that are equal to a type (I know, corner case, but still).
2. The replacement .pick() does not exist for all objects that have .pick_channels() and .pick_types() (e.g. Covariance).
3. The global mne.pick_channels() and mne.pick_types() functions remain available.
4. The picking API should really have been .pick(names=..., types=...).
5. The legacy methods still have to be used in certain cases (including our own tests), which is not ideal especially for end-users (understandably, users will avoid legacy functions).

If the idea of replacing .pick_channels() and .pick_types() with .pick() was to make picking easier, the current implementation fails to deliver that IMO. I actually think that keeping .pick_types() and .pick_channels() would have been the better option (easier, more consistent, no corner cases).

[drammock]

The replacement .pick() method cannot select channel names that are equal to a type (I know, corner case, but still).

This is (1) assumed to be rare, and (2) can be easily addressed by renaming the problematic channels

The replacement .pick() does not exist for all objects that have .pick_channels() and .pick_types() (e.g. Covariance).

I proposed to remedy this in my summary of the dev mtg discussion. Specifically these:

pick_channels_cov -> new method cov.pick() that mirrors the method signature and functionality of the .pick() method in the Mixin (i.e., self, picks, exclude=(), *, verbose=None).

pick_channels_forward. This one is different because Forward doesn't keep track of bads, but I think otherwise the method signature and functionality of .pick() should work? Therefore propose a new method forward.pick(self, picks, *, verbose=None) (note lack of exclude, since there are no bads).

It seems that wasn't part of #11665, but it can/should still happen, either here or in a separate PR.

The global mne.pick_channels() and mne.pick_types() functions remain available.

why is this a problem? We have lots of redundant function/method pairs; personally I don't think we should but that is a larger discussion / not specific to the picking functions/methods, and should get its own issue.

The picking API should really have been .pick(names=..., types=...).

this is an assertion, not an argument. I'll just note that several folks participated in the discussions about the API, both at the live dev meeting and in #11531, and that #11531 was open for nearly two months before it was closed by #11665. There was ample time for API discussion.

The legacy methods still have to be used in certain cases (including our own tests), which is not ideal especially for end-users (understandably, users will avoid legacy functions).

There is nothing wrong with testing legacy functionality to make sure we don't break it. It's also OK because end users shouldn't ever need to interact with our tests in their roles as users. If they're contributing new features or bugfixes, they have their contributor hat on and should expect to see some cobwebs when they pull back the curtain. Now, if we're using legacy methods in tests other than tests specifically of those methods then we should change/update the tests that use legacy methods incidentally. I've just done that (hopefully I've caught all of them now).

[cbrnr]

Thanks @drammock for your thoughts. I know the decision has been made, I just wanted to understand it better. I still don't know why this change was made and why it is supposed to be better than the previous separate methods. It's OK though, no need to discuss this further.

I proposed to remedy this in #11531 (comment). Specifically these:

Yes, this would certainly improve consistency! I would add it in a separate PR.

The global mne.pick_channels() and mne.pick_types() functions remain available.

why is this a problem?

It's not a big deal, but again, consistency. The question could also be: why didn't we keep inst.pick_channels() and inst.pick_types() then.

There was ample time for API discussion.

Yes, unfortunately I missed it. And several people, including @agramfort were in favor of .pick(names=..., types=...) (at least in the issue discussion, I don't know what was discussed in the dev meetings).

I guess it's not too late to add that as an enhancement to the API, or is it? It can be completely optional, and maybe at some later time we can make it the primary way to pick channels and types.

[larsoner]

Thanks @drammock for your thoughts. I know the decision has been made, I just wanted to understand it better. I still don't know why this change was made and why it is supposed to be better than the previous separate methods. It's OK though, no need to discuss this further... I guess it's not too late to add that as an enhancement to the API, or is it?

I understand it's probably annoying and disappointing to have missed the discussion, but if we come to a decision as a group everyone in the end has to be willing to live with it to some extent. If there is some bug or something objectively wrong that we need to fix that's one thing. But continuing to push discussion of redesigning the API and discussing its pain points -- most or all of which rehash issues that were already discussed and weighed and deliberated originally -- seems a bit unfair to the people who participated in terms of taking up their (our) time discussing them again. So yes I think in instances like this it's too late unfortunately, and ideally we wouldn't have spent as long as we have already discussing it over the last two weeks.

To the point about splitting into names and types specifically, I think what you propose is less of an enhancement but a backward incompatible change. But either way, it was brought up in the original discussion and -- despite it having support from multiple people as you point out -- it was not chosen in the final implementation that was merged. As such I don't think we should reconsider it -- at least not without new substantial evidence and/or user complaints about why it's bad or limited or whatever that we didn't have when we originally designed the API. I don't think we're very close to that threshold here.

pick_channels_cov ... pick_channels_forward

Specifically regarding these I consider them a (very) low priority as I think they get used mostly by experts. Most of the time as a user the correct channels for cov and forward are picked for you when computing inverses, etc. Or to put it in terms of how often we describe these to people by use in examples and tutorials:

$ git grep "pick_types(" tutorials/ examples/ | wc -l
114
$ git grep "pick_channels(" tutorials/ examples/ | wc -l
28
$ git grep "pick(" tutorials/ examples/ | wc -l
25
$ git grep "pick_channels_cov(" tutorials/ examples/ | wc -l
0
$ git grep "cov.*\.pick_channels(" tutorials/ examples/ | wc -l
2
$ git grep "pick_channels_forward(" tutorials/ examples/ | wc -l
0
$ git grep "f\(wd\|or\).*\.pick_channels(" tutorials/ examples/ | wc -l
0

I think we have very few uses of these things in our docs. This is consistent with my experience helping people use MNE as well -- end-users don't use these. And experts can live with doing a little bit more work. As a side point, cov can only be picked by channel names (not types) because it doesn't store info, so changing that one to use .pick is a bit awkward since it's a bit incomplete in terms of what it can do relative to standard raw.pick-like behavior.

So for this PR I think we should change as many user-facing examples as is reasonable, merge, and move on to something else more pressing!

[cbrnr]

I'm sorry, I just wanted to understand the decision/motivation for the change, which is not clear to me from the discussion on GitHub.

[drammock]

I'm sorry, I just wanted to understand the decision/motivation for the change, which is not clear to me from the discussion on GitHub.

@dengemann's initial comment in #11531 said:

it turns out that passing a list of channels to .pick_channels() versus .pick() will lead to different results: both functions will pick but only .pick() will act like similar to a fancy index in numpy, that is, reorder the channels of the instance.

He goes on to describe how this can lead to silent bugs when data from MNE is processed with NumPy / sklearn / etc. That was the main motivation for changing the pick_channels() default from ordered=False to ordered=True. There was an additional consideration of "there should be one way to do things" which pushed toward standardizing on .pick(), but a pragmatic "don't break old code" consideration to keep .pick_channels() around as a legacy method.

Now consider @larsoner's comment later in that thread:

overall I'm +0.5 for @legacy and +1 for deprecating pick_channels completely. And if we try to make this change in our own code and tests we might see if/where using .pick falls short.

We've now (nearly?) done that in this PR, and it's worth assessing whether the gymnastics necessary to switch over to .pick() are too convoluted/unintuitive/error-prone to be worth adopting. Once all the tests are passing, I invite you to self-review this PR and call out any specific cases where the "new way" is objectionable. Feel free to also examine git grep "\.pick_channels\(" and git grep "\.pick_types\(" to see what wasn't changed and see if you think it could be, or if it constutites evidence of ".pick() should behave differently to cover this case". It may well be that .pick() cannot fully replace .pick_channels() --- this PR should tell us that.

[larsoner]

I'm sorry, I just wanted to understand the decision/motivation for the change, which is not clear to me from the discussion on GitHub.

My comment above has to do with what I (errantly?) perceived as a motivation to redesign the API and revise previous decisions built by consensus after deliberation. As a few examples, here, here, or here:

IMO, a nice [.pick] API would have two independent parameters names and types, ... I would also remove the exclude parameter, because a drop method (as the opposite of pick) would be more consistent. This method should also have the names and types parameters.

Is it really too late to discuss/revert/modify this change?

I guess it's not too late to add that as an enhancement to the API, or is it?

I read these comments as suggestions to "reopen" a decision-making process that we already "closed" by consensus, as opposed to merely an effort to understand the "why" behind those decisions. Apologies if I misread your intent from these statements! But hopefully you can also perhaps see how someone could misread your intentions from them?

In any case, hopefully @drammock's explanations above help clarify things further. Looks like CircleCI just needs to be fixed so we can merge.

[cbrnr]

Thanks for clarifying @drammock. I don't have any energy left to continue this discussion, so although I very much appreciate your suggestion to review this PR again, I'm just going to accept whatever you decide. There are more important things than discussing our pick API.

[larsoner]

Fixed a conflict and marking for merge-when-green, thanks in advance @cbrnr @drammock !

[drammock]

argh I thought this was still in draft mode. Wasn't finished. I'll do a follow-up PR