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

If no electrode positions are specified in the BIDS dataset metadata (electrodes.tsv), we should remove the montage upon loading the data

Open hoechenberger opened this issue 2 years ago • 12 comments

The dataset mentioned in https://mne.discourse.group/t/mne-bids-pipeline-critical-error-digitisation-points/5376 is a BrainVision dataset with a [Coordinates] section in the vhdr file, specifying the electrode positions. The dataset doesn't come with an _electrodes.tsv to specify the electrode positions in a BIDS-specific way, though.

When the dataset was loaded with MNE-BIDS and visualized, a topomap could correctly be created because channel positions were taken from [Coordinates].

However, I think this is a mistake, as electrode positions must go into _electrodes.tsv.

The following not-so-short MWE demonstrates this problem:

  • We load EEG data
  • we set a montage
  • we convert one EEG channel to mag to make MNE-BIDS believe it's an MEG dataset, so we an write this thing to a FIFF file. Here we abuse a bug (?) in MNE-BIDS: when writing M/EEG data, it doesn't create an _electrodes.tsv sidecar.
  • We load the data again.
  • Even though there was no _electrodes.tsv, the montage can be plotted – because the positions were taken from the info structure of the FIFF file.
# %%
from pathlib import Path
import mne
import mne_bids

# Load data & set a montage
ssvep_folder = mne.datasets.ssvep.data_path()
ssvep_data_raw_path = (ssvep_folder / 'sub-02' / 'ses-01' / 'eeg' /
                       'sub-02_ses-01_task-ssvep_eeg.vhdr')
ssvep_raw = mne.io.read_raw_brainvision(ssvep_data_raw_path, verbose=False)
ssvep_raw.set_montage('easycap-M1')

# Add a single MEG channel so we can save the data as a FIFF file in BIDS
ssvep_raw.set_channel_types({'PO10': 'mag'})

fname = Path('/tmp/sub-02_ses-01_task-ssvep_meg.fif')
ssvep_raw.save(fname, overwrite=True)
del ssvep_raw

# Create a BIDS dataset
raw = mne.io.read_raw(fname)
root = Path('/tmp/bids-test')
bp = mne_bids.BIDSPath(
    subject='02',
    session='01',
    task='ssvep',
    suffix='meg',
    extension='.fif',
    datatype='meg',
    root=root
)
mne_bids.write_raw_bids(
    raw=raw,
    bids_path=bp,
    overwrite=True,
)

# Load the data again
raw_loaded = mne_bids.read_raw_bids(bp)

# We do have a montage
raw_loaded.get_montage().plot()

My proposal is to call raw.set_montage(None) upon loading data. If we have electrode positions in BIDS metadata, we can subsequently add them; but if we don't have any, we should get rid of the positions stored in the original data without a representation in the metadata.

hoechenberger avatar Aug 06 '22 11:08 hoechenberger

agreed! BrainVision electrode coordinates from the [COORDINATES] section are very often template positions, and not measured (I haven't encountered another situation as of yet ... and I really did try a lot in 2020 when I wanted to merge Brain Products CapTrak digitization with BrainVision vhdr files via BrainVision Recorder et al.)

sappelhoff avatar Aug 06 '22 12:08 sappelhoff

I would just be careful that doing .set_montage(None) will remove head shape dig points for coregistration.

Message ID: @.***>

agramfort avatar Aug 06 '22 12:08 agramfort

I would just be careful that doing .set_montage(None) will remove head shape dig points for coregistration.

Message ID: @.***>

Very good point. Then we need to be a bit more picky and remove the DigPoints corresponding to EEG channel names only

hoechenberger avatar Aug 06 '22 12:08 hoechenberger

... and for M/EEG data we should write _electrodes.tsv, which we currently don't do. @sappelhoff can you confirm?

hoechenberger avatar Aug 06 '22 12:08 hoechenberger

@agramfort to clarify, all positions stored in _electrodes.tsv will be restored as DigPoints upon reading.

That's why I'm suggesting that we drop all EEG DigPoints from the info, and then read them from _electrodes.tsv -- even for combined M+EEG recordings. A bug (?) currently causes MNE-BIDS not to write the EEG channel positions to _electrodes.tsv when saving M+EEG data, so this needs to be fixed first.

hoechenberger avatar Aug 06 '22 13:08 hoechenberger

to me the electrodes file should overwrite what is present in the native format. I would be careful about removing stuff from native format if this information is not present in the sidecar files

Message ID: @.***>

agramfort avatar Aug 06 '22 13:08 agramfort

That is indeed a valid point...

@sappelhoff @adam2392 @alexrockhill @larsoner What is your opinion on this? The question is:

  • if data in a BIDS dataset stores information in info
  • but this information should, according to the standard, be supplied via a plain-text sidecar file
  • and that sidecar file doesn't exist

... should we drop or should we keep that information from the info?

hoechenberger avatar Aug 06 '22 14:08 hoechenberger

Keep with a warning maybe?

alexrockhill avatar Aug 06 '22 15:08 alexrockhill

I think this question is related to two overarching questions:

  1. which data should have precedence: the one in the neural data ... oder the one in the BIDS sidecars. I think the one in the sidecars, however it seems to be more complicated in some edge cases. See discussion an PR here: https://github.com/bids-standard/bids-specification/pull/761
  2. whether template data should be written. I think the consensus is "no" ... some discussion here (and containing links): https://github.com/mne-tools/mne-bids/issues/264

... and for M/EEG data we should write _electrodes.tsv,

it should ... but only if we know that we don't write template data, but measured data. (digitized locations)

. @sappelhoff can you confirm?

I don't know right now ... I think it's possible that we don't do it right now. I remember that in the past I had plans to write code that would detect whether electrode locations fit on a sphere, and if they do, then we'd not write electrodes.tsv, else we would.

sappelhoff avatar Aug 06 '22 18:08 sappelhoff

... and for M/EEG data we should write _electrodes.tsv,

it should ... but only if we know that we don't write template data, but measured data. (digitized locations)

. @sappelhoff can you confirm?

I don't know right now ... I think it's possible that we don't do it right now. I remember that in the past I had plans to write code that would detect whether electrode locations fit on a sphere, and if they do, then we'd not write electrodes.tsv, else we would.

I would not go through that burden. I think it is totally acceptable to off-load this to our users: Let them know clearly that if the data contains digpoints, they should be measured locations, not templates. We could write a respective log message when write_raw_bids() detects and processes digpoints.

I would really not try to add a heuristic here – this is yet again something that gives me the "we're trying to be too clever" vibes, which in the past has proven time and again to cause problems.

hoechenberger avatar Aug 06 '22 18:08 hoechenberger

You have a point and I would be fine with sending an appropriate log message and/or re-iterating this point in the docstr and/or a tutorial.

sappelhoff avatar Aug 06 '22 18:08 sappelhoff

to me fixing native files can be a pain due to format specificities etc. Editing one metadata may imply you break something else as your writer is not bullet proof. For me sidecar files are a beautiful way to adjust the problematic files acquired and therefore should simple overwrite what is present in the native file.

agramfort avatar Aug 07 '22 10:08 agramfort