mne-bids: read_raw_bids() respects info['bads'], but it really shouldn't

Describe the bug

While working in #491 I think I’ve discovered a bug in _handle_channels_reading(), which is used by read_raw_bids(). Specifically, when reading data and populating the list of “bads”, MNE-BIDS currently merges the information from _channels.tsv and info['bads'] in the data file. I think this is not correct, and only _channels.tsv should be honored.

https://github.com/mne-tools/mne-bids/blob/8eec738aa521344e5c772d8a744f4838059f28d8/mne_bids/read.py#L259-L263

Steps to reproduce

Checkout hoechenberger:mark-bads (the branch from #491). Run:

import os.path as op
import mne
from mne_bids import (make_bids_basename, write_raw_bids, read_raw_bids,
                      mark_bad_channels)

data_path = mne.datasets.sample.data_path()
raw_fname = op.join(data_path, 'MEG', 'sample', 'sample_audvis_raw.fif')
bids_root = op.join(data_path, '..', 'MNE-sample-data-bids')
bids_basename = make_bids_basename(subject='01', session='01',
                                   task='audiovisual', run='01')

raw = mne.io.read_raw_fif(raw_fname, verbose=False)
print(f'The following channels are marked as bad: {raw.info["bads"]}')

write_raw_bids(raw, bids_basename=bids_basename, bids_root=bids_root,
               overwrite=True, verbose=False)

# Mark two channels as bad, RETAINING existing bads.
bads = ['MEG 0112', 'MEG 0131']
mark_bad_channels(ch_names=bads, bids_basename=bids_basename,
                  bids_root=bids_root)

raw = read_raw_bids(bids_basename=bids_basename, bids_root=bids_root,
                    verbose=False)
print(f'After marking MEG 0112 and MEG 0131 as bad, the following channels '
      f'are now marked as bad: {raw.info["bads"]}')

# Mark the same channels as bad, NOT RETAINING any existing bads.
bads = ['MEG 0112', 'MEG 0131']
mark_bad_channels(ch_names=bads, bids_basename=bids_basename,
                  bids_root=bids_root, overwrite=True)

raw = read_raw_bids(bids_basename=bids_basename, bids_root=bids_root,
                    verbose=False)
print(f'After marking MEG 0112 and MEG 0131 as bad and passing '
      f'`overwrite=True`, the following channels '
      f'are now marked as bad: {raw.info["bads"]}')


Expected results

The following channels are marked as bad: ['MEG 2443', 'EEG 053']

After marking MEG 0112 and MEG 0131 as bad, the following channels are now marked as bad: ['MEG 0112', 'EEG 053', 'MEG 0131', 'MEG 2443']

After marking MEG 0112 and MEG 0131 as bad and passing `overwrite=True`, the following channels are now marked as bad: ['MEG 0131', 'MEG 0112']

Actual results

The following channels are marked as bad: ['MEG 2443', 'EEG 053']

After marking MEG 0112 and MEG 0131 as bad, the following channels are now marked as bad: ['MEG 0112', 'EEG 053', 'MEG 0131', 'MEG 2443']

After marking MEG 0112 and MEG 0131 as bad and passing `overwrite=True`, the following channels are now marked as bad: ['EEG 053', 'MEG 0131', 'MEG 0112', 'MEG 2443']

Additional information

Looking into sub-01_ses-01_task-audiovisual_run-01_channels.tsv, it’s clear that it’s being updated correctly:

$ grep bad sub-01_ses-01_task-audiovisual_run-01_channels.tsv
MEG 0112        MEGGRADPLANAR   T/m     0.10000000149011612     172.17630004882812      Planar Gradiometer      600.614990234375        bad     n/a
MEG 0131        MEGMAG  T       0.10000000149011612     172.17630004882812      Magnetometer    600.614990234375        bad     n/a
$

About this issue

  • Original URL
  • State: closed
  • Created 4 years ago
  • Comments: 26 (26 by maintainers)

Commits related to this issue

Most upvoted comments

ahhh, wonderful!

sounds good, but the writing API in #491 might introduce these kinds of inconsistencies.

No, it actually does away with it 😃 I’ve designed it such that (un)marking channels as “bad” will alter both, channels.tsv and Raw.info['bads'] 😃

based on thinking in #495 I believe mismatch should be a validator error. See for example what they do with niftis

Ok so… now, what should be the behavior if we encounter both, info['bads'] and status in channels.tsv if they don’t match?

Yes 😃 I think we now have a common understanding! Just need to figure out the way forward that everyone agrees with.

agree, the use of OPTIONAL vs RECOMMENDED vs REQUIRED is very much abused in BIDS