bids-standard / bids-2-devel

Discussions and suggestions of backwards incompatible changes to BIDS
https://bids.neuroimaging.io/
Creative Commons Attribution 4.0 International
10 stars 1 forks source link

Drop "ChannelCount" fields in ephys data #49

Open arnodelorme opened 3 years ago

arnodelorme commented 3 years ago

Some channel types have counts in the .eeg JSON files and others do not. Newly declared channel types GSR, RESP, and TEMP do not have counts

GSRChannelCount RESPChannelCount TEMPChannelCount

While EOG, EMG, ECG do. Worse, when the fields above are defined, the BIDS validator fails. This is made even worse by the fact that EOGChannelCount, EMGChannelCount, and ECGChannelCount are mandatory fields. The error from the validator is not informative when trying to add these fields (failed because of badly formated .eeg file). I have tried to look at the JS code but it is too convoluted. I beg whoever is able to change this code to return meaningful errors.

Overall, I would remove this redundant information of channel count (except for EEG and non-EEG channel counts). One can simply look at the channel file to see what type of channels are there. This is a source of potential error and inconsistency. Big choice obviously.

sappelhoff commented 3 years ago

This is made even worse by the fact that EOGChannelCount, EMGChannelCount, and ECGChannelCount are mandatory fields

what makes you think that these are mandatory? to my knowledge and this JSON schema, they aren't :thinking:

The error from the validator is not informative when trying to add these fields (failed because of badly formated .eeg file).

the error you get there is because you want to add fields to the *eeg.json file that are not defined in the specification. To me this makes sense, but others have said that it's problematic, see: https://github.com/bids-standard/bids-validator/issues/1044 and https://github.com/bids-standard/bids-specification/issues/637

I have tried to look at the JS code but it is too convoluted.

The JSON files are getting evaluated using https://json-schema.org/ --> and the schemas for the validator are all located here. I agree that it's very difficult to find and improvements of the documentation are really needed.

Overall, I would remove this redundant information of channel count (except for EEG and non-EEG channel counts). One can simply look at the channel file to see what type of channels are there.

I tend to agree with you. However, given that the channel counts are only "RECOMMENDED" it'd be fair enough for us (BIDS) to not add some new fields like GSRChannelCount, RESPChannelCount, etc. to the specification and just leave the state as it is. In BIDS 2.0 we could then (potentially, up to discussion) completely remove all ChannelCount fields, but not before that - because it'd be a backwards incompatible change.

cc @bids-standard/raw-electrophys-eeg

arnodelorme commented 3 years ago

what makes you think that these are mandatory? to my knowledge and this JSON schema, they aren't 🤔

You are right. https://github.com/bids-standard/bids-specification/blob/master/src/04-modality-specific-files/03-electroencephalography.md Sorry about that. They used to be mandatory I think.

The JSON files are getting evaluated using https://json-schema.org/ --> and the schemas for the validator are all located here. I agree that it's very difficult to find and improvements of the documentation are really needed.

It would be great for the validator to indicate which part of the file create problem.

Overall, I would remove this redundant information of channel count (except for EEG and non-EEG channel counts). One can simply look at the channel file to see what type of channels are there.

I tend to agree with you. However, given that the channel counts are only "RECOMMENDED" it'd be fair enough for us (BIDS) to not add some new fields like GSRChannelCount, RESPChannelCount, etc. to the specification and just leave the state as it is. In BIDS 2.0 we could then (potentially, up to discussion) completely remove all ChannelCount fields, but not before that - because it'd be a backward-incompatible change.

Yes, you would want to preserve backward compatibility of course.