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

bug: write_raw_bids uses CapTrak coordsys for iEEG data (invalid) #723

Open
timonmerk opened this issue Mar 10, 2021 · 21 comments
Open

bug: write_raw_bids uses CapTrak coordsys for iEEG data (invalid) #723

timonmerk opened this issue Mar 10, 2021 · 21 comments
Labels

Comments

@timonmerk
Copy link

timonmerk commented Mar 10, 2021

EDIT SA: ieeg + captrak issue starts below in the thread here: #723 (comment)


Hello everyone,

first thanks for the great and handy tool working with BIDS data.
I am following this tutorial https://mne.tools/mne-bids/stable/auto_examples/convert_ieeg_to_bids.html, and I am wiriting the montage in order to write out BIDS comaptible data in such way:

montage = mne.channels.make_dig_montage(ch_pos=dict(zip(ch_names, elec)), \
                                            coord_frame='mni_tal')
raw_arr.set_montage(montage, on_missing='warn')
mne_bids.write_raw_bids(raw=raw_arr, bids_path=bids_path, overwrite=True)

This nicely creates the BIDS folder structure, but when one session has multiple runs, the electrode.tsv get's overwritten, so the electrode tsv created by the upper code is only written out in such way: sub-XYZ_ses-XYZ_acq-XYZ_space-XYZ_electrodes.tsv, but the "run" keyword is missing.
This creates reading errors when working with multiple runs in one session. When the "run" keyword get's included by hand, reading BIDS will read the electrodes.tsv and won't result in an error like this (when working with multple runs):

RuntimeError: Channels do not correspond between raw data and the channels.tsv file. For MNE-BIDS, the channel names in the tsv MUST be equal and in the same order as the channels in the raw data.

So maybe the solution would be rather simple just to include the run keyword?

@timonmerk timonmerk changed the title Why does write_raw_bids does not write the electrode.tsv run specific? Why does write_raw_bids not write the electrode.tsv run specific? Mar 10, 2021
@hoechenberger
Copy link
Member

hoechenberger commented Mar 10, 2021

Hello, thanks for your report!

but when one session has multiple runs, the electrode.tsv get's overwritten, so the electrode tsv created by the upper code is only written out in such way: sub-XYZ_ses-XYZ_acq-XYZ_space-XYZ_electrodes.tsv, but the "run" keyword is missing.

This is what is required by the BIDS specification: the electrodes are assumed to be identical across all runs within a session.

This creates reading errors when working with multiple runs in one session. When the "run" keyword get's included by hand, reading BIDS will read the electrodes.tsv and won't result in an error like this

Could you please elaborate – where would you include run by hand?

I'm also slightly confused because the error message seems to be about *_channels.tsv, not *_electrodes.tsv 🧐

@sappelhoff
Copy link
Member

For a recent discussion on what "entities" are BIDS compliant for electrodes.tsv, see: bids-standard/bids-examples#244

my hunch is that if you changed electrodes, then it should be a different session, not a different run.

That aside, I am also slightly confused about the error message :-)

Could you also tell us which version of mne-bids you are using? (master? 0.6?)

@timonmerk
Copy link
Author

Thanks for the quick reply!
The specification indeed tell's that the electrode.tsv are in this format:
_sub-[_ses-][_acq-][_space-]electrodes.json

So the run and task are not included. When I was running the write_raw_bids then for multiple runs in one session the electrode.tsv got simply overwritten.
And in this way I also could not read the files with read_raw_bids, since the electrode.tsv did not match across all channel.tsv files.

Maybe I am wrong here, but especially for the context of iEEG / DBS research, this becomes very difficult. One session would be here maybe a day that could consist of multiple measurements. So in this manner you would change the channel numbers or use in one task some analog channel, then have a rest recording where you don't have this channel. But in this way it would be required to have different sessions for this right?

The alternative that somehow worked is to specify an electrode tsv for every run, so I added the run keyword:
sub-XYZ_ses-XYZ_task-XYZ_acq-XYZ_run-XYZ_space-XYZ_electrodes.tsv. When I had then multiple different runs with different channels in one session, I could read the individual electrodes into the RawArray montage automatically.

Also I am using the 0.6 version.
And yes, the error message is misleading, saying that the channel.tsv is different to the raw data. But they are the same, it's that the electrode.tsv has different channels.
Here the full message:

grafik

@sappelhoff
Copy link
Member

One session would be here maybe a day that could consist of multiple measurements.

Okay, but even if you (as researchers) refer to the visit of a participant on a certain day as a "session", you could for the sake of BIDS organization say that this "visit-session" has several smaller measurement sessions.

For example do: sub-01_ses-Day1Meas1_*, sub-01_ses-Day1Meas2_*, sub-01_ses-Day2Meas1_*, ...

and (potentially) explain what you mean by that in the README.

Would that be an option?

@hoechenberger
Copy link
Member

Seeing this error message also got me thinking – we removed that in master, right? Would @timonmerk run into issues then? 😱

@sappelhoff
Copy link
Member

we removed that in master, right?

what do you mean exactly?

What I am now constantly worrying about is #709. In brief: So far we've been writing all kinds of labels for the _space-<label> entity, and have gotten away with it because the bids-validator did not check that the actual BIDS rule are being followed.

The actual BIDS rules are, that labels for ``_space-` must come from a list of restricted keywords.

Now the bids-validator has been extended to actually check that ... but we haven't caught up with mne BIDS yet, I think. 😬

is that what you meant?

@hoechenberger
Copy link
Member

@sappelhoff what I meant is that we now allow a deviation between sidecar and raw channels. I'm wondering if @timonmerk would've even figured something was wrong so easily with master

@timonmerk
Copy link
Author

I have one additional comment, in the bids-example repo I found also a session where the number of channels in the electrode.tsv does not match with the ones in the channel.tsv
https://github.com/bids-standard/bids-examples/tree/master/ieeg_visual_multimodal/sub-som682/ses-somecog01/ieeg
(see run 01)
I could not reproduce the reading error, since the brainvision files are empty.

But would it be a huge difficulty to adapt mne-bids, that it still reads the electrodes.tsv using read_raw_bids even if the number of channels isn't equal? If the channels aren't found in the electrode.tsv those fields could in the montage maybe be empty.

I would also volunteer to work on that, since in our lab we wrote multiple datasets already, where this problem occurs. In this case the rewrite for session splits like sub-01_ses-Day1Meas1_, sub-01_ses-Day1Meas2_, sub-01_ses-Day2Meas1_ will make the session number explode and the dataset structure would be quite messy.

@richardkoehler
Copy link
Contributor

If I may briefly offer my opinion on this (please correct me if I am wrong with any assumptions):

MNE-BIDS is currently comparing channels.tsv (or the raw data) and electrodes.tsv and throws an error when the recorded channels and electrodes don't match up in name, number and order.
However, recorded channels and electrodes aren't the same thing (BIDS specifications). While an electrode is "a single point of contact", a channel could be for example a bipolar measurement obtained from two electrodes.
So, while electrodes stay the same during a session, channels don't necessarily have to.
For this reason, I believe that BIDS doesn't state anywhere that the electrodes.tsv and channels.tsv files have to match up.

Maybe it would be an option for MNE-BIDS to simply compare the indices in electrodes.tsv and in the data channels and if they match up, create the montage. For any channels that don't have a corresponding entry in electrodes.tsv, maybe just throw a warning.
Not sure if any of this makes sense :)

@hoechenberger
Copy link
Member

hoechenberger commented Mar 12, 2021

@richardkoehler Thanks for your message!

MNE-BIDS is currently comparing channels.tsv (or the raw data) and electrodes.tsv and throws an error when the recorded channels and electrodes don't match up in name, number and order.

This is not the case anymore in our development version, which I suppose we should release sooner than later …

See:
#698
#704

@richardkoehler
Copy link
Contributor

That's great news!! I will check out the dev version then :)

@timonmerk
Copy link
Author

@hoechenberger and @sappelhoff Thanks a lot for the comments!

I am trying now to install the mne-bids development vesion. I should clone the repo and then just call make right?
Unfortunately I receive this error:

mne-bids 0.7.dev0 is already the active version in easy-install.pth
Installing mne_bids-script.py script to C:\Users\ICN_admin\Anaconda3\Scripts
Installing mne_bids.exe script to C:\Users\ICN_admin\Anaconda3\Scripts
error: [WinError 5] Access is denied: 'C:\Users\ICN_admin\Anaconda3\Scripts\mne_bids.exe'
make: *** [Makefile:30: inplace] Error 1

I tried to run git bash as Admin, and even set the Path variable in the Windows system settings to 'C:\Users\ICN_admin\Anaconda3\Scripts'

Probably I behave really stupid here, but conda install mne-bids --dev also did not work and still showed 0.6 as version and not 0.7.dev0

@hoechenberger
Copy link
Member

hoechenberger commented Mar 13, 2021

Hello, if you have already cloned the repo, call

pip install -e .

from the repo directory and it should work 💪

@richardkoehler
Copy link
Contributor

richardkoehler commented Mar 15, 2021

The development version actually fixes the initial problems @timonmerk and I were having when reading in the data!
However, we noticed that when reading in the dataset (iEEG), including the electrodes.tsv, the coord_frame defaults to "head" and not "mri", even when we are in MNI space (e.g., "space-MNI152NLin2009bAsym" and "iEEGCoordinateSystem": "Other"). I'm not too familiar with the MNE coordinate frames, but after reading a couple of threads I am assuming this behavior is intentional?
In any case, if we then want to rewrite the iEEG data to a new BIDS-Dataset using write_raw_bids(), because of the coord_frame "head" the space defaults to "CapTrak" (even though we are dealing with iEEG data) and we get the following error message:
grafik
So write_raw_bids() creates an Error, which it then catches. Maybe the default space for writing out the electrodes.tsv should instead be "Other" for iEEG data?
We have also run across the following message when using write_raw_bids(), even though coord_frame is "head". When this happens, the data files are written, but the electrodes.tsv is simply ignored and not written out:
raw_dig
write_electrodes_tsv

Alternatively, if we set the "iEEGCoordinateSystem" in the sidecar file to "ACPC" and then read in the data, we get a montage with "RAS non-zero origin" coord_frame looking like this: <DigPoint | EEG #1 : (26.1, 37.0, 24.3) mm : RAS (non-zero origin) frame>. We are then able to write out the data using write_raw_bids(). However, in the newly written sidecar file the "iEEGCoordinateSystem" is "Other" and not "ACPC" anymore.
This current inconsistency is most likely due to incompatibility of coordinate frames/systems between BIDS and MNE right?
If I can contribute to fixing these issues (if this is desired), let me know! Maybe it would be an option to give the user the possibility to define the "space-" value when writing out the dataset, possibly by using the "space-" entity from the BIDSPath object? Is there a current open thread (I haven't found any) where a discussion on how to deal with electrodes across BIDS and MNE is going on?

@sappelhoff
Copy link
Member

even when we are in MNI space (e.g., "space-MNI152NLin2009bAsym" and "iEEGCoordinateSystem": "Other")

if you are in MNI152NLin2009bAsym space, why don't you set "iEEGCoordinateSystem": "MNI152NLin2009bAsym"? the space entity and value vor iEEGCoordinateSystem in a sidecar file should correspond in most (all?) cases 🤔

In any case, if we then want to rewrite the iEEG data to a new BIDS-Dataset using write_raw_bids(), because of the coord_frame "head" the space defaults to "CapTrak" (even though we are dealing with iEEG data) and we get the following error message:

interesting ... perhaps @adam2392 can help, and maybe it's related to #676 -> "Other" would certainly be more sensible for iEEG than CapTrak

We have also run across the following message when using write_raw_bids(), even though coord_frame is "head". When this happens, the data files are written, but the electrodes.tsv is simply ignored and not written out:

that check should only be triggered when dealing with EEG data 🤔 so either you have a wrong call somewhere (not specifying iEEG) ... or MNE-BIDS does a wrong interpretation (bug)

This current inconsistency is most likely due to incompatibility of coordinate frames/systems between BIDS and MNE right?

yes, this is a big box of issues that has been opened very recently - so there are likely quite a few cases (including your current one) that need to be fixed/covered 🤔

The main reason for that is that the bids-validator now covers testing the space entity and is enforcing a style that so far we ignored in mne-bids :-/

It'd be great if you could help us to get this story straight. The main problem is that we currently have only @adam2392 well experienced with iEEG. I personally have no idea about MRI spaces and their interaction with iEEG.

the solution would probably involve a deep dive into the code base, trying to find bugs (and fixing them), and suggesting workarounds where mne and bids coordsystems just don't line up

@adam2392
Copy link
Member

Hi @richardkoehler we've added some more details to the convert ieeg to bids example: #717

The main issue with MNE and BIDS is that there is only one iEEG coordinate frame that is really acceptable: mni_tal, which corresponds to MNI305, or fsaverage. Everything else currently needs to be added outside of MNE-BIDS/MNE-Python.

Here is a thread on how to specify electrodes localized on a FreeSurfer T1w image (not fsaverage): bids-standard/bids-specification#747.

Moving Forward

Any thoughts on how to best synchronize MNE and MNE-BIDS coordinate frames?

@richardkoehler
Copy link
Contributor

if you are in MNI152NLin2009bAsym space, why don't you set "iEEGCoordinateSystem": "MNI152NLin2009bAsym"? the space entity and value vor iEEGCoordinateSystem in a sidecar file should correspond in most (all?) cases 🤔

The reason is that if I do this is that the Standard template identifiers from BIDS haven't been implemented in MNE-BIDS yet. I get the following message:
grafik

that check should only be triggered when dealing with EEG data 🤔 so either you have a wrong call somewhere (not specifying iEEG) ... or MNE-BIDS does a wrong interpretation (bug)

I think that you are right. This is only triggered if there are still EEG channels in the recording, even if our BIDSPath object is set to "ieeg".

It'd be great if you could help us to get this story straight. The main problem is that we currently have only @adam2392 well experienced with iEEG. I personally have no idea about MRI spaces and their interaction with iEEG.

I am happy to contribute as much as I can, however I'm just starting to get the hang of how the coordinate frames in MNE work.

Any thoughts on how to best synchronize MNE and MNE-BIDS coordinate frames?

@adam2392 I agree that the only MNE coordinate frame really useful is mni_tal (or if no registration to an MNI space is available, maybe "mri" for the "raw" images). As I wrote above my personal experience using MNE is really limited. Should we maybe move this discussion to a dedicated thread where we can specifically discuss the integration of BIDS and MNE coordinate systems via MNE-BIDS because this issue was originally opened for a different problem?

@adam2392
Copy link
Member

^ @sappelhoff @hoechenberger @agramfort is it best to start a discussion in MNE-python?

@agramfort
Copy link
Member

can I beg for an executive summary of the issue? maybe on discord during the sprint this week?

@hoechenberger
Copy link
Member

hoechenberger commented Mar 15, 2021

I lost track too, summary on Discord would be great :)

@sappelhoff sappelhoff changed the title Why does write_raw_bids not write the electrode.tsv run specific? bug: write_raw_bids uses CapTrak coordsys for iEEG data (invalid) Mar 25, 2021
@alexrockhill
Copy link
Contributor

I think the issue at the end here is fully solved by #983, not sure as much about the beginning of the conversation about CapTrak. If you could look @richardkoehler, and see if the recent changes solve your issues and allow you to use the BIDS standard template coordinate frames easily, that would be great!

@sappelhoff sappelhoff added the bug label Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants