mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

Add `picks` argument to `Raw.plot()`

Open ivopascal opened this issue 1 year ago • 6 comments

Describe the new feature or enhancement

Raw.plot() currently does not accept a picks= argument, even though Evoked.plot() and Epochs.plot() do. For a mostly-unified API it would be nice to add this parameter.

Describe your proposed implementation

This would be added as an argument to Raw.plot(). I suppose it can come after the n_channels argument. Ideally it would be the same for all classes, but that would break backward compatibility.

The behaviour and implementation should be similar to or identical to how it's done in Epochs.plot() and Evoked.plot().

Describe possible alternatives

Alternatively, the entire function call could be rewritten to match Evoked and Epochs. The would be nicer for the future, but it would break a lot of things in the short term, for a relatively minor improvement.

Additional context

Discussed on the MNE forum here: Unexpected behaviour Raw.pick

ivopascal avatar Jan 12 '24 11:01 ivopascal

Hello! 👋 Thanks for opening your first issue here! ❤️ We will try to get back to you soon. 🚴

welcome[bot] avatar Jan 12 '24 11:01 welcome[bot]

Technically order=pick_types(...) actually works like you suggest I think. But every time I want to do this I have to look up the argument name.

One backward-compatible way forward would be to do a legacy-style deprecation of order

  1. put a * in the arg list for raw.plot before order
  2. move order=None to the end
  3. put picks=None where order=None was
  4. allow order or picks to be specified, but not both

Eventually we could deprecate order but I suspect it's pretty widely used, and the backward compat code we'd need to keep around is only 5 lines I think:

if order is not None:
    if picks is not None:
        raise ValueError(...)
    picks = order
del order

then we can internally use picks instead of order, and process picks the normal way (_picks_to_idx internally will pass through the array of int that order currently supports!).

Let's see if others agree with this idea, but would you be interested in trying to make this change @ivopascal ?

larsoner avatar Jan 12 '24 17:01 larsoner

Yes, I'd be happy to implement it :)

ivopascal avatar Jan 12 '24 18:01 ivopascal

I'm fine with @larsoner's plan as long as picks=[4, 3, 2, 1, 0] works the same as order[4, 3, 2, 1, 0] (i.e., as long as it's still possible to put the channels in any arbitrary user-specified order on the plot)

drammock avatar Jan 15 '24 17:01 drammock

I said I'd implement it but I'm actually swamped with work right now, and it's taking me a bit more time than I expected to figure out how to work with the existing code.

I'd still be happy to implement it in March, but if someone else wants to put this together instead of waiting that's also good.

ivopascal avatar Jan 24 '24 17:01 ivopascal

Hmm, I don't think picks should replace order. Epochs.plot() and Evoked.plot() actually have both, which means you can have picks in an arbitrary order and preserve the "standard" ordering of channels. Only specifying order would then actually modify the order.

I think having as-much-as-possible uniformity between Epochs.plot(), Evoked.plot() and Raw.plot() would be nicest, so I propose having picks=[3,2,1] to be different than order=[3,2,1] and we should then keep both parameters.

ivopascal avatar Feb 29 '24 16:02 ivopascal

Closing because #12467 is merged and fixes the issue.

ivopascal avatar Apr 16 '24 07:04 ivopascal