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

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 this issue Aug 6, 2022 · 12 comments
Milestone

Comments

@hoechenberger
Copy link
Member

hoechenberger commented Aug 6, 2022

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.

@sappelhoff
Copy link
Member

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 sappelhoff added this to the 0.11 milestone Aug 6, 2022
@agramfort
Copy link
Member

agramfort commented Aug 6, 2022 via email

@hoechenberger
Copy link
Member Author

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
Copy link
Member Author

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

@hoechenberger
Copy link
Member Author

@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
Copy link
Member

agramfort commented Aug 6, 2022 via email

@hoechenberger
Copy link
Member Author

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?

@alexrockhill
Copy link
Contributor

Keep with a warning maybe?

@sappelhoff
Copy link
Member

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: [ENH] Add a statement addressing metadata conflicts bids-standard/bids-specification#761
  2. whether template data should be written. I think the consensus is "no" ... some discussion here (and containing links): Template versus True digitization data #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
Copy link
Member Author

... 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
Copy link
Member

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
Copy link
Member

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.

@hoechenberger hoechenberger modified the milestones: 0.11, 0.12 Oct 6, 2022
@sappelhoff sappelhoff modified the milestones: 0.12, 0.13 Dec 18, 2022
@sappelhoff sappelhoff modified the milestones: 0.13, 0.14 Jul 27, 2023
@sappelhoff sappelhoff modified the milestones: 0.14, 0.15 Dec 9, 2023
@sappelhoff sappelhoff modified the milestones: 0.15, 0.16 May 19, 2024
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

4 participants