Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Argument picks of method AutoReject.get_reject_log(self, epochs, picks=None) is not used. #226

Open
mscheltienne opened this issue Aug 10, 2021 · 5 comments

Comments

@mscheltienne
Copy link
Contributor

mscheltienne commented Aug 10, 2021

As spotted in PR #225, line 1042 to 1087 in autoreject.py:

def get_reject_log(self, epochs, picks=None):
      """Get rejection logs of epochs.

      .. note::
         If multiple channel types are present, reject_log['bad_epochs_idx']
         reflects the union of bad trials across channel types.

      Parameters
      ----------
      epochs : instance of mne.Epochs
          The epoched data for which the reject log is computed.
      picks : str | list | slice | None
          Channels to include. Slices and lists of integers will be
          interpreted as channel indices. In lists, channel *type* strings
          (e.g., ``['meg', 'eeg']``) will pick channels of those types,
          channel *name* strings (e.g., ``['MEG0111', 'MEG2623']`` will pick
          the given channels. Can also be the string values ``'all'`` to pick
          all channels, or ``'data'`` to pick data channels. None (default)
          will use the .picks attribute. Note that channels in
          ``info['bads']`` *will be included* if their names or indices are
          explicitly provided.

      Returns
      -------
      reject_log : instance of autoreject.RejectLog
          The reject log.
      """
      # XXX gut feeling that there is a bad condition that we miss
      ch_names = [cc for cc in epochs.ch_names]
      labels = np.ones((len(epochs), len(ch_names)))
      labels.fill(np.nan)
      reject_log = RejectLog(
          labels=labels,
          bad_epochs=np.zeros(len(epochs), dtype=np.bool),
          ch_names=ch_names)

      picks_by_type = _get_picks_by_type(info=epochs.info, picks=self.picks_)
      for ch_type, this_picks in picks_by_type:
          this_reject_log = self.local_reject_[ch_type].get_reject_log(
              epochs, threshes=self.threshes_, picks=this_picks)
          reject_log.labels[:, this_picks] = \
              this_reject_log.labels[:, this_picks]
          reject_log.bad_epochs = np.logical_or(
              reject_log.bad_epochs, this_reject_log.bad_epochs)
          reject_log.ch_names = this_reject_log.ch_names
      return reject_log

picks is not used; instead self.picks_ is used.

@jasmainak
Copy link
Member

What do you think should be the intended behavior?

Say you did

ar = Autoreject(picks=picks_train)
ar.fit(epochs_train)

ar.get_reject_log(epochs_test, picks_test)

Use picks_train i.e., ar.picks_ if picks_test is None ?

@mscheltienne
Copy link
Contributor Author

mscheltienne commented Aug 10, 2021

@jasmainak I have no idea. I never used it and I don't know what it's supposed to do. I just noticed that the argument of the function is not used at all; which is definitely not normal. I will probably give it a try in the coming weeks, but for now, as I never used it, I can't help you figure out what was the intended behavior.

I did notice that in the class _AutoReject (not sure what it is, I would have expected AutoReject to inherit from _AutoReject) the method _AutoReject.get_reject_log does properly use its picks argument.

@jasmainak
Copy link
Member

_Autoreject is for a single consensus or rho and the Autoreject class loops over them and computes the scores.

Maybe @dengemann can help us figure out what's going on with the picks but there might be something to fix

@dengemann
Copy link
Collaborator

I have to wrap my head around this ... I think we wrote this ~4 years ago. The general purpose of picking is that you can apply autoreject sequentially on MEG/EEG channels present in one recording but also exclude channels. The .picks_ attribute is good because it does some smart logic at fit time. But that picks does not override the attribute sounds like a bug. It sounds like a good test / example is needed here in which overriding the picks computed at fit time is performed.

@mscheltienne
Copy link
Contributor Author

mscheltienne commented Aug 10, 2021

Any way you put it, the fact you are asking an argument to the user; to then throw it into the trash looks very suspicious ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants