-
Notifications
You must be signed in to change notification settings - Fork 169
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
Formalize how channel types are shared between modalities that use channels.tsv
files.
#1436
Comments
Would it make sense to specify for each modality if the keywords in I also noted that the glossary for channel types does not specify which modality it refers to (https://bids-specification.readthedocs.io/en/stable/glossary.html#type-sense-1-columns). Currently rendered as |
I do like having channel definitions shared across modalities whenever possible. For instance, Then the website can only render the core channel types. And prescribe people with "recommended" modality per channel type? I acknowledge that this does not have to be a hard set thing because depending on the data set one modality is more important than another, but ideally people should be encouraged to separate between different data modalities and this can be made explicit by recommending a modality (or multiple of them) per type. |
In the biweekly maintainers meeting yesterday we discussed a plan that I think goes well together with your suggestions, Julius and Sein. Thanks! The current plan is to:
I'll take care of step 1 soon (next week, probably), and Chris will see whether he can carve out some time to do steps 2 and 3 (with help from the rest of us where needed) cc @rwblair @effigies @ericearl @bendhouseart -- let me know if I missed something. |
Sounds like a very good solution you came up with! I can draft a table for MOTION-BIDS which channels are "home" in motion and which are not. Would that help? |
@sappelhoff You got our discussion in there fully, thanks! |
yes @JuliusWelzel that'd be helpful, thanks -- you could post it into this thread. |
This is a first draft on how a table could look like in the spec for MOTION-BIDS. I used the columns.yaml file from the schema to check for duplicate channel types. I noticed the channel of type @sjeung Let me know if you spot any mistakes :) |
One uncertainty on my part when creating a BEP, which I am now realizing, is according to which criteria do I include which existing channel types in my schema objects? What is the rationale for including |
The
I find the definition of the
Although I would specify what is meant by "recording onset" --> perhaps the onset that is noted down in the Furthermore, the For reference, here are some similar metadata and their definitions:
In general I agree that we should not introduce a new channel type ( |
I agree it is kind of unfortunate to have this channel type in two versions -- if they do carry the same data. The definition for MEGMAG currently reads:
Whereas for MAGN, it reads:
Potential solutions:
I dislike the second suggestion because the type would have "MEG" in its name even when used outside of MEG, and because we already agreed (among maintainers) that it's probably not a smart idea to have channel types of the same name take on different meanings depending on the modality they are being used in. I dislike the first suggestion because we are essentially duplicating a channel type. But I believe that for backward compatibility reasons, this is the road I'd take. Any other suggestions? (btw @JuliusWelzel and @sjeung, shouldn't the |
Lastly @JuliusWelzel, I think you misunderstood the intention of which modalities channel types are shared with --> the idea is that all channel types can be used in any modality (what you have as "ALL" in your table) ... but that each channel type has a "home modality" (e.g., for |
I don't have any recollection of SYSCLOCK ... maybe @robertoostenveld could help :) Furthermore, it seems that none of the openneuro datasets is using it |
Thanks @JuliusWelzel for the table! Regarding the comment from @sappelhoff about ss[.000000] was not intended to imply integer digit limit. But I see how it is misleading. Regarding the And the (x,y,z) could simply be omitted or replaced with text description like "... to describe the mapping to the spatial axes" |
Good point, I will update the text accordingly.
I think it would be more feasible to
+1 for Stefan first proposed solution.
I like that! |
When it comes to latency channel values, I would stick to the numeric format for keeping the content of .tsv uniform because I remember running into some problems (both reading and writing) when I mix non-numeric datetime and numerics as entries for .tsv |
That is a good argument.
What would be best to address Stefan's comment? |
I understand from a historical perspective how the unidirectional references from EEG to MEG, and iEEG to EEG+MEG, etc., came about: that is the order in which the BEPs were implemented and merged with the specification. The MEG BEP did not yet have the EEG and iEEG to link to, and apparently was not updated when EEG and iEEG were later merged.
I also agree. Regarding "SYSCLOCK", I am quite confident that it will be a CTF-specific channel name... Searching for old CTF example datasets (which were from Montreal or Nijmegen), I found https://openfmri.org/dataset/ds000246/ with https://s3.amazonaws.com/openneuro/ds000246/ds000246_R1.0.0/uncompressed/sub-0001/meg/sub-0001_task-AEF_run-02_channels.tsv. That dataset is also here on openneuro, so apparently the search is not so powerful here. I also see "clock" in https://doi.org/10.34973/37n0-yc51, which is one that we prepared and released around the time that BEP008/MEG was merged. Apparently it passed the validator back then. Looking now at a CTF dataset, I see that the SYSCLOCK channel represents the time of acquisition, which is different from the time of recording. I.e. the acquisition to the computer is started first, data is checked, and then the record-to-disk button is pressed. Here is a plot of the values in the fieldtrip tutorial Edit: SYSCLOCK=0 might correspond to the time of the MEG electronics being started-up or rebooted (even without the acquisition running). That makes more sense, the MEG system would have been switched on in the morning and the measurement might have taken place in the afternoon. |
I noticed that we might have a redundancy in the proposed keywords for motion channel types currently. There is a channel name |
One other remark: we have |
Thanks for spotting this! We had the exact discussion a while back and agreed on GYRO over ANGVEL see here. I fixed it in 8cad179. |
We also discussed this on GH, but unfortunately I don't recall or find the reasoning. Maybe @sjeung can remember? |
@helenacockx is this just a suggestion for renaming |
Yes, that is what it is. I am also in favor. Note that it also applies to |
I have taken the liberty to push these changes to @JuliusWelzel's PR: @sjeung @JuliusWelzel please confirm that this is fine, then we can merge it and adjust the examples. I'll also have a look at the BIDS validator when the PR is merged. |
Looks good to me, thanks! |
Too me, too. I don't recall any particular reason to prefer ANGACC to ANGACCEL, except for the length. I am fine with renaming and will adjust example data sets accordingly. Thanks for the feedback Helena & for update Stefan! |
@sappelhoff I think this issue can be closed as it seems to be addressed here. For future BEPs, all available channels type can be found in this schema object, correct? If a new channel type is introduced, it will be added here as well. |
indeed @JuliusWelzel, thank you. I am closing this in favor of: however, #1529 will be taken off the 1.9 release as it's mostly visuals that can also be added after a release. |
came up during the BEP029 (motion) community review:
also related to:
Some BIDS modalities allow for
channels.tsv
metadata:channels.tsv
files MUST have atype
column that contains (upper case) strings from a defined vocabulary as specified in the modality specific sections. This column MUST be the second column inchannels.tsv
files.In the schema, the valid values for
type
are specified separately for EEG, MEG, iEEG, and NIRS, see:bids-specification/src/schema/objects/columns.yaml
Line 601 in 741d4ef
bids-specification/src/schema/objects/columns.yaml
Line 626 in 741d4ef
bids-specification/src/schema/objects/columns.yaml
Line 662 in 741d4ef
bids-specification/src/schema/objects/columns.yaml
Line 690 in 741d4ef
Even though in the spec, it says:
For EEG (types are shared with MEG and iEEG):
bids-specification/src/modality-specific-files/electroencephalography.md
Lines 217 to 219 in 741d4ef
For MEG (no mention of sharing):
bids-specification/src/modality-specific-files/magnetoencephalography.md
Lines 229 to 230 in 741d4ef
For iEEG (types are shared with MEG and EEG):
bids-specification/src/modality-specific-files/intracranial-electroencephalography.md
Lines 233 to 235 in 741d4ef
For NIRS (types are shared with EEG, MEG, and iEEG -- see last sentence):
bids-specification/src/modality-specific-files/near-infrared-spectroscopy.md
Lines 197 to 207 in 741d4ef
We should:
I propose that channel types should be shared between all BIDS modalities.
The text was updated successfully, but these errors were encountered: