mne-tools / mne-bids

MNE-BIDS is a Python package that allows you to read and write BIDS-compatible datasets with the help of MNE-Python.
https://mne.tools/mne-bids/
BSD 3-Clause "New" or "Revised" License
131 stars 85 forks source link

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

Open hoechenberger opened 2 years ago

hoechenberger commented 2 years ago

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:

# %%
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.

sappelhoff commented 2 years ago

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.)

agramfort commented 2 years ago

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

Message ID: @.***>

hoechenberger commented 2 years ago

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 commented 2 years ago

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

hoechenberger commented 2 years ago

@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.

agramfort commented 2 years ago

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: @.***>

hoechenberger commented 2 years ago

That is indeed a valid point...

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

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

alexrockhill commented 2 years ago

Keep with a warning maybe?

sappelhoff commented 2 years ago

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.

hoechenberger commented 2 years ago

... 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.

sappelhoff commented 2 years ago

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.

agramfort commented 2 years ago

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.