-
Notifications
You must be signed in to change notification settings - Fork 88
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
Comments
agreed! BrainVision electrode coordinates from the |
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 |
... and for M/EEG data we should write _electrodes.tsv, which we currently don't do. @sappelhoff can you confirm? |
@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. |
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: ***@***.***>
|
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 |
Keep with a warning maybe? |
I think this question is related to two overarching questions:
it should ... but only if we know that we don't write template data, but measured data. (digitized locations)
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: 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. |
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. |
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. |
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 thevhdr
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:
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._electrodes.tsv
, the montage can be plotted – because the positions were taken from theinfo
structure of the FIFF file.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.The text was updated successfully, but these errors were encountered: