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

Optional column names in channels.tsv #1410

Closed
JojoVh opened this issue Jan 31, 2022 · 7 comments
Closed

Optional column names in channels.tsv #1410

JojoVh opened this issue Jan 31, 2022 · 7 comments
Labels

Comments

@JojoVh
Copy link

JojoVh commented Jan 31, 2022

Dear Bids Validator developers,

According to the BIDSv1.6.0 the only column names that are compulsory in the channels.tsv are "name", "type" and "units" (in this order).
However, it seems like that the current BIDS Validator requires the 4th and the 5th column to be "low_cutoff" and "high_cutoff". I think this is a mistake, isn't it?

image

https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/03-electroencephalography.html#example-channelstsv

image

Thank you for your help,

Best wishes
on behalf of the ICN lab from Charité Berlin

@sappelhoff
Copy link
Member

Hi Jonathan,

the link from the spec that you posted is about EEG data. However, from the screenshots you posted it looks like you have iEEG data. For this kind of data, low and high cutoff are indeed required.

See here:

https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/04-intracranial-electroencephalography.html#channels-description-_channelstsv

@JojoVh
Copy link
Author

JojoVh commented Jan 31, 2022

Dear Stefan

Thank you very much for your reply. You are completely right.
It seems that the error is generated because the column "sampling frequency" came before the high and low cutoff.
Now I understand that the BIDS validator is correct, but the order of the columns printed by fieldtrip data2bids function is not. I will post this issue hence at their platform.

Thank you very much again,
Best wishes
Jonathan

@JojoVh JojoVh closed this as completed Jan 31, 2022
@sappelhoff
Copy link
Member

I see, thanks for raising the issue in fieldtrip then! Good luck with your data curation.

@richardkoehler
Copy link

@JonathanVHoecke I actually think you closed your issue prematurely, you have a valid point.

As we can see from your screenshot, the data includes all of the required columns. However, bids-validator only checks the first columns in your data and throws an error if the first columns are not all required columns, i.e. it is order-sensitive.

However, the BIDS iEEG specification (to my knowledge) actually does not make any statements about the actual order of the columns. This is in contrast to the EEG specification which actually makes this statement:

The required columns are channel name, type and units in this specific order.

I think at this moment this behavior of bids-validator is not absolutely correct. Also, ideally the error message should be more precise, e.g. WRONG_ORDER_TSV_COLUMN_IEEG_CHANNELS.
Additionally, the iEEG specification for channels.tsv is ambiguous. While the table clearly states that required columns are name, type, units, high_cutoff and low_cutoff, the text states:

The two required columns are channel name and type.

@sappelhoff What do you think is the way forward here, and am I missing anything?

I think ideally, one would:

  • fix the amibiguity in the BIDS specification and change the text to:

The required columns are channel name, type, units, low_cutoff and high_cutoff.

  • optionally, add the following statement to homogenize requirements with the EEG specification. However, this would break forward-compatibility for some iEEG datasets, right?

... in this specific order.

  • split the cases "MISSING" and "WRONG_ORDER" in bids-validator, to give a more informative error or warning message.

@sappelhoff
Copy link
Member

However, the BIDS iEEG specification (to my knowledge) actually does not make any statements about the actual order of the columns. This is in contrast to the EEG specification which actually makes this statement:

True, that was a shortcoming that has been fixed in the current development version that will soon be released, see:

https://bids-specification.readthedocs.io/en/latest/04-modality-specific-files/04-intracranial-electroencephalography.html#channels-description-_channelstsv

Also, ideally the error message should be more precise, e.g. WRONG_ORDER_TSV_COLUMN_IEEG_CHANNELS.

Agreed.

Additionally, the iEEG specification for channels.tsv is ambiguous. While the table clearly states that required columns are name, type, units, high_cutoff and low_cutoff, the text states:

Also true, this has been fixed in the current development version as well (see link above)

What do you think is the way forward here, and am I missing anything?

I think all issues you pointed out except the "misleading" validator error message have been fixed.

Regarding the validator message, I guess a PR would be welcome, but we'd need a volunteer who first thinks more deeply about this issue, weighs pros and cons, and then suggests a concrete proposal how to fix it. I don't have time for that (currently) unfortunately. If neither of you two have time either, then perhaps raising an issue would be fine enough for now, so that in the future somebody can solve this.

@richardkoehler
Copy link

Thanks for clarifying, I am glad to hear that this has been solved already!
I have looked on I would definitely volunteer to work on this, but I have never written any code in JS so I am not sure I am the right person for this as I would first have to get familiar with the semantics.
I did briefly look into the source code and I see why you say that it needs some thought; currently the columns are being checked in their order one-by-one. I will open a new issue and see if I'm maybe able to work on this with some additionall help.

@JojoVh
Copy link
Author

JojoVh commented Feb 1, 2022

Thank you very much @sappelhoff for your clarification.
I am glad to learn that the column names are indeed specified in a fixed order.
Thank you @richardkoehler for your support in this issue!

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

3 participants