MRG: When picking, remove inapplicable projectors

[hoechenberger]

What does this implement/fix?

Pick'ing may entirely eliminate a certain channel type from the resulting object. However, the list of projectors in info['projs'] will remain unchanged. This can lead to problems in later processing steps, where situations may arise where one is trying to apply a projector for a channel type that is not present in the data anymore.

MWE:

import mne
from mne.datasets import sample

data_path = sample.data_path()
raw_fname = data_path + '/MEG/sample/sample_audvis_filt-0-40_raw.fif'
raw = mne.io.read_raw_fif(raw_fname, preload=True)

raw.set_eeg_reference(projection=True)
raw.pick_types(meg=False, eeg=True)  # Drop MEG data
raw.plot_projs_topomap()

With master:
A problem arises because although we dropped all MEG data, the MEG projectors are still present.

Traceback (most recent call last):
  File "/private/tmp/mwe.py", line 10, in <module>
    raw.plot_projs_topomap()
  File "/Users/hoechenberger/Development/mne-python/mne/io/proj.py", line 280, in plot_projs_topomap
    fig = plot_projs_topomap(self.info['projs'], self.info, cmap=cmap,
  File "/Users/hoechenberger/Development/mne-python/mne/viz/topomap.py", line 324, in plot_projs_topomap
    use_info = pick_info(info, pick_channels(info_names, ch_names))
  File "<decorator-gen-8>", line 21, in pick_info
  File "/Users/hoechenberger/Development/mne-python/mne/io/pick.py", line 489, in pick_info
    raise ValueError('No channels match the selection.')
ValueError: No channels match the selection.

With this PR:
The MEG projectors get removed during the pick:

Removing projector <Projection | PCA-v1, active : False, n_channels : 102>
Removing projector <Projection | PCA-v2, active : False, n_channels : 102>
Removing projector <Projection | PCA-v3, active : False, n_channels : 102>

WIP because

• I'm looking for feedback, obviously
• tests are missing
• not sure if this would demand a deprecation cycle

Thanks!

[larsoner]

, where situations may arise where one is trying to apply a projector for a channel type that is not present in the data anymore.

This actually shouldn't be a problem, it should just be np.eye in that case. If it breaks something it's a bug downstream that we should fix first (regardless of whether or not we do adopt the behavior in this PR)

[hoechenberger]

If it breaks something it's a bug downstream that we should fix first

Ok, since there's a MWE in my first comment, we can use this as a bug report :)

[hoechenberger]

… although the problem here is not exactly about applying a projector, but about plotting it!

[larsoner]

Actually looking at the code and the problem, this probably is the right way to go. Let's get #8007 in, rebase this, and then see if we can fix any errors that crop op

[hoechenberger]

Thanks @larsoner, I will pick up working on this shortly.
(current code also contains a bug, which I realized while drafting #8003)

[hoechenberger]

Will rebase on master after #8005 and #8007 have been merged.

[hoechenberger]

Rebased on master.

In #8003, we changed the Note in del_proj():

        .. note:: The projection vector can only be removed if it is inactive
                  (has not been applied to the data), unless the channels it
                  was applied to no longer exist in the data.

Once this PR gets merged, however, the case "the channels [the projector] was applied to no longer exist in the data" shouldn't be possible anymore through use of the public API, right? Because the only way to remove channels is by picking, and this PR will ensure that picking also drops unusable projs. So I'm wondering if I should drop this part from the del_proj() docstring as part of this PR?

[hoechenberger]

I tried to capture the warnings in a test but it doesn't seem to work -- pytest claims no warning of any kind has been emitted, even though the warnings do appear on stdout!

        with pytest.warns(RuntimeWarning, match='Removing projector.*'):
>           raw.pick_types(meg=False, eeg=True)
E           Failed: DID NOT WARN. No warnings of type (<class 'RuntimeWarning'>,) was emitted. The list of emitted warnings is: [].

Any idea what might be the issue?

[larsoner]

don't use logger.warning but rather warn from ..utils

[hoechenberger]

Weird, ok… why is that?

[larsoner]

We always use warn. It does nice things like sets stacklevel automatically, and does or does not emit based on the logger.level, etc.

[hoechenberger]

Thanks, that's good to know!

[hoechenberger]

Ok let's see what CI says… I'm worried I will have to handle the new warning in 123456 different tests individually…

[hoechenberger]

… or I could just turn it into a logger.info. WDYT? Where do we draw the line between info and warning?

[larsoner]

This to me should just be logger.info. warnings should be if it's likely someone is doing something bad that could break something. Nobody's code should break (in principle at least) because we remove projs that don't apply to the data

[hoechenberger]

CI is failing because AverageTFR doesn't inherit from ProjMixin and therefore doesn't have a del_proj() method. What to do about that?

[larsoner]

I'm fine with keeping the drop projections as a separate function, but I don't see a good reason for _pick_drop_channels not to call it. I think it's less convoluted actually. pick_drop_channels should do everything required to pick and drop channels from an instance once you know the indices, one of which is (now) to drop irrelevant projectors.

Going in the direction you're advocating, we shouldn't even have a _pick_drop_channels but instead split it into one that picks just from info, one that just drops data if preloaded, etc., and replicate these calls four or five places now and more later if we add another mixin method that needs to _pick_drop...

[hoechenberger]

@larsoner Ok, implemented in bed0dcd

[hoechenberger]

CI is failing because AverageTFR doesn't inherit from ProjMixin and therefore doesn't have a del_proj() method. What to do about that?

@larsoner Any thoughts on this one? I've never worked with AverageTFR so I'm not sure what's the best approach here.

[larsoner]

You could check to see if it has that attribute and only call it if it does

[hoechenberger]

@larsoner Done

[hoechenberger]

@larsoner This is good to go from my end. Let me know if you would like me to rebase.

[agramfort]

@larsoner merge if happy

[larsoner]

Thanks @hoechenberger