-
Notifications
You must be signed in to change notification settings - Fork 111
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
_electrodes.tsv required for iEEG #1771
Comments
here, @effigies states that electrodes.tsv is actually optional for iEEG: though I don't know where that information is coming from. Where do you take your information from @dorahermes? Is it the fact that the file names in the "file name templates" are not in square brackets? For EEG it definitely makes sense to have this file as OPTIONAL, because sometimes there is just no information on electrode positions. Should this file really be REQUIRED for iEEG? And do we perhaps need to change something in the schema, if electrodes.tsv shows up as required even for EEG? |
Yes, I was assuming it was REQUIRED given the lack of square brackets in the specification. Since the specification more clearly states that even the _channels.tsv file is RECOMMENDED it may make more sense that the _electrodes.tsv file is RECOMMENDED as well for iEEG data. This would also be more backwards compatible with these OpenNeuro data, but would at least create a warning. The problem is that, in contrast with EEG data where channel positions have standardized names, iEEG data often do not make any sense if electrode positions are not shared (it is like sharing voxel timeseries from different brain regions that are randomly shuffled). |
It had already been a valid regex in the validator before the MEG discussion, and nothing in the spec says it is required. Likewise,
seems to clearly indicate that it is optional for EEG. I'm not sure why I thought it was required for EEG at the time of the linked comment. |
From bids-standard/bids-specification#1550 (comment), because some systems record electrode location directly into the data files, some researchers would prefer to avoid duplicating the electrode information and potentially having a metadata synchronization problem. |
That is never the case for iEEG, only MEG. |
Please also note that _electrodes.tsv was required in the original iEEG-BIDS document |
I can't access that. Sent a request, if you have admin. In any event, if it should have been required, we should open a PR to clearly state that it is a required file in the spec and implement a validator check. |
FWIW, here are the datasets with iEEG directories with no I agree that people using non-BIDS files to provide electrode information is bad. Are there any that are doing things properly but using data files that include electrode position information? If not, that would be a strong argument for moving to REQUIRED. In any event, I'm +1 for making this at least RECOMMENDED. |
+1 for making it at least RECOMMENDED as well (for all, EEG, iEEG, MEG). I would be okay with making it REQUIRED for iEEG (depends on what @dorahermes thinks is a good way forward).
One of these dataset is by @adam2392 --> https://openneuro.org/datasets/ds003029/versions/1.0.6 Why don't you share electrodes.tsv files, Adam? |
No coordinates :/. So it would just be an empty file? I guess that is fine too? It would just be n/a tho. |
I see, is that an argument to not make electrodes.tsv required? How do you respond to this comment by @dorahermes in https://github.com/bids-standard/bids-validator/issues/1771#issuecomment-1714213732?
How are people using your data when coordinates are not shared @adam2392 ? |
The general brain regions are known and stated in the metadata I think. The interpretation of the time series is more clinical such that certain electrodes correspond to an suspected epileptic region. None of this required the exact positions so we never got them processed. However I did push for it :(. The only con is that these data are not spatially comparable to other datasets and someone cannot validate that said electrode is in a brain position the metadata says it is in. However, beyond that I think it's still usable albeit not for brain plotting. |
I would disagree for our specific example. It is like sharing voxel time series where the brain lobe is known or some macro structure but you don't know the exact xyz coordinate (in our case). In general yes this is true unless further described in the metadata. I am okay saying the file is required tbh. And then users can write n/a. The more REQUIRED the better. |
+1 There are several iEEG cases that were discussed during the iEEG extension where it may be extremely challenging to obtain exact electrode positions, but data are still of value (such as intraoperative mapping or this large dataset by @adam2392 !) The community feel for these cases, as also noted by @adam2392 , was that the point of BIDS is not to be restrictive, but to provide a guideline of where to put electrode contact information such that others can find it. If the validator generates an error or warning about a missing _electrodes.tsv file, this would help curators see where such information should be stored. (With respect this dataset I could not find the electrode_layout that is referred to above, as the readme states that it was not allowed to share these sourcedata files. Within iEEG _electrodes.stv files, x,y,z positions are not just used for brain plotting, but for interpretation, linking to MRI data, atlases etc. If there is a need for a more general 'anatomical label' that should be discussed in a separate issue and probably related to the Atlas BEP) |
To provide additional context. Back then, it was more difficult to pipeline the electrode locations (at least for me). That and getting a hold of the MRI/CT data in usable format was a great pain that eventually was not worth it for our purposes. Then BIDs came on the scene and I was relatively new user there, but greatly vibed with the mission. Data sharing in general was just making its initial waves in neurology (it's still in its infancy rn), so I got a lot of pushback and friction to even share the data from collaborators that were not funded by the NIH/NSF. At that point, I did the "best I could" w/ what was allowed. However, it is my impression now that there are more and more pushes to share data publicly when publishing. Moreover, the NIH is releasing a mandate(?) that requires this. Thus, I think making the electrodes.tsv REQUIRED forces the user to think about how to get this data. Moreover, having the necessary tools now to more easily get the electrodes.tsv locations (e.g. MNE-Python now supports this for ieeg) makes it so there is less of an excuse for not getting the coordinates. tldr: it is unfortunate that there are data w/o the electrodes.tsv, but it should be REQUIRED as discussed above imo. A backwards-compatible fix is to allow the user to upload a file w/ all coordinates to |
IMHO the examples described above rather call for a RECOMMENDED status, where the validator would send a warning if no I think adding an What's your opinion @effigies? |
I will defer to everyone else, since I don't work on this as often anymore. Thoughts on the warning: Perhaps the validator could state in the warning something about "MRI scan of the subject brain and CT scans with the electrodes implanted data" are basically what's needed with some "reasonable resolution" (e.g. fast scans of <30 slices are useless, which we got a lot of). |
I think making the file required is fine. The values in tabular files have always been allowed to be set to |
I tested, by the way, and there is no rule in the current validator that prevents As a side thought, it probably would be useful in a case like this to put structure names as a column in electrodes.tsv, so you still get some context without referring back to the README. |
okay, fine by me then 👍 |
My current approach is to tag issues with legacy or schema, and once the legacy validator is killed off for good, all legacy issues can be closed. I don't want to stop anybody motivated from fixing a bug in the legacy validator, though, so I haven't been closing them. But I'm okay either way. Feel free to close or leave open. |
Thanks, that's a good approach. |
I implemented this in bids-standard/bids-specification#1896, but this would invalidate all of the ieeg examples with The inheritance principle does not associate, e.g., from ieeg_epilepsy:
There are two problems with this dataset:
To resolve this, the two most straightforward options are:
Less straightforward options:
Just as a quick survey, looking at the examples and at OpenNeuro, all current In terms of pain to users, I would probably rank these 3 (least), 4, 2, 1 (most). In terms of pain to a developer, I would rank it 2 (least), 1, 3, 4 (most). (I'm ranking 3 less painful than 4 for users because it should be more efficient to do an adapted inheritance-principle-style search than a repeated check for non-existent files. For development, I think (4) would be deceptively nasty or we'd end up just hacking it.) |
I would like to hear which option @ftadel prefers as I am not sure what the consequences of 1. would be. I think the ieeg_epilepsy example is actually a really nicely curated one that applies to many sites. |
Thanks for the summary, Chris. Do I understand 1. correctly, that in order to validate that an iEEG data file has a corresponding electrodes file , we need to apply fixes in the form of ... ... for this example 👇
turn into "valid" through these changes 👇
OR through these changes 👇
? That is: Require at least one At least that is what I understand from your:
So for our present case, the BIDS inheritance principle is ill defined, because, as Dora points out, it is a normal (and easily understandable) situation to have an iEEG data file, and supplied electrodes information in 2 (or n) different spaces. These spaces can then be useful for different analyses or users ... but in essence, it is an association of iEEG data with electrode location data (and different "views" on the "electrode locations" are what is supplied)
same as above: in the present (IMHO reasonable) example, the inheritance principle doesn't fit what we need ... so I wonder whether it could be adapted. Overall, I appreciate that this situation is difficult, and I am tending towards option 2., with the exceptions that I would not stop requiring it, and instead rather relax our credo of "when we require something we must be able to validate it" ... a few exceptions may be ok. @dorahermes I think @ftadel has mentioned before that he is no longer working in the field ... but I fail to find his corresponding comment on GH. |
Thanks @sappelhoff! I think this situation is difficult, because the _electrodes.tsv file is by definition session specific rather than run specific.
|
Indeed, I left the neuroimaging field last year. |
It seems from the ieeg-BIDS documentation that the _electrodes.tsv file is required for ieeg data.
However, there are now several pretty impressive iEEG datasets on OpenNeuro dataset without electrode positions but no errors, and another dataset with electrode positions in a .mat rather than a .tsv file without errors. Is this perhaps a validator issue?
The text was updated successfully, but these errors were encountered: