-
Notifications
You must be signed in to change notification settings - Fork 159
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
[MISC] Clarify participant_id in participants.tsv file if it exists #738
Conversation
Thanks for taking a stab at this @adam2392 --> I think it's good to clarify it. With regards to your question --> I have no clue. Perhaps the other @bids-standard/maintainers know. I had a look at the Longitudinal and multi-site studies section to see whether it clarifies things, but I think it should rather be clarified itself (using SHOULD / MUST terminology). Let me give a few examples of what I think should change:
And then under Sessions file I find it confusing that the text refers to a "participant key file" ... probably,
|
|
The extra session layer (at least one `/ses-<label>` subfolder) should | ||
The extra session layer (at least one `/ses-<label>` subfolder) SHOULD | ||
be added for all subjects if at least one subject in the dataset has more than | ||
one session. Skipping the session layer for only some subjects in the dataset is | ||
not allowed. If a `/ses-<label>` subfolder is included as part of the | ||
directory hierarchy, then the same `ses-<label>` tag must also be | ||
one session. If a `/ses-<label>` subfolder is included as part of the | ||
directory hierarchy, then the same `ses-<label>` tag MUST also be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping the session layer for only some subjects in the dataset is
not allowed
I actually think that the original text here meant to say that:
The extra session layer (at least one
/ses-<label>
subfolder) MUST
be added for all subjects if at least one subject in the dataset has more than
one session.
instead of the SHOULD you used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the other hand, that doesn't really make sense, because there are experiments where some subjects perform an extra session 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think skipping the ses-<X>
in some subjects is allowed if say not all subjects have the same sessions. Rn this results in a warning in validator, which basically all my data has this warning :p.
Re MUST vs SHOULD:
The extra session layer (at least one /ses-<label> subfolder) MUST
be added for all subjects if at least one subject in the dataset has more than
one session.
seems reasonable to me... The problem is how does one validate this? If one has multiple "sessions" per subject, then that implies there are potentially different electrode/channels across those sessions, so the user would want to store it as such. Does this basically mean if one has:
sub-01/
sub-01_ses-01_task-blah_ieeg.vhdr
...
it's allowed, but if you have:
sub-01/
sub-01_ses-01_task-blah_ieeg.vhdr
sub-01_ses-02_task-blah_ieeg.vhdr
...
it's not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think skipping the ses- in some subjects is allowed if say not all subjects have the same sessions. Rn this results in a warning in validator, which basically all my data has this warning
yes, I experienced that warning multiple times myself 🙄 I think this case is more common than thought when this warning was originally implemented. That is, more often it's a choice, rather than an overlooked error.
re: your example --> yes 🤔 but I think your current changes (apart from my comments below) are actually fine and clarify the situation without changing the state of anything (which is what we should aim for), so I'd be fine with leaving the SHOULD, not interpreting it as a MUST post-hoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So basically turning it into a MUST would make the following non-BIDS, correct?
├── sub-1
│ └── eeg
└── sub-2
├── ses-1
│ └── eeg
└── ses-2
└── fmri
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's how I interpret it as well. What's annoying is that first the text says "should" (not MUST), and then the follow up sentence is:
Skipping the session layer for only some subjects in the dataset is not allowed.
so basically the "should" is actually a MUST.
However:
- I am not sure why this rule makes sense
- I am pretty sure that I have seen datasets not conforming to this rule
- There is no validator support for this rule as far as I know
🙄 perhaps we can discuss this tonight in the maintainers meeting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the inconsistency should be fixed or clarified.
I mean I find the folder structure I described above "ugly", but thank goodness we don't just rely on my sense of aesthetics.
😉
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
The extra session layer (at least one `/ses-<label>` subfolder) should | ||
The extra session layer (at least one `/ses-<label>` subfolder) SHOULD | ||
be added for all subjects if at least one subject in the dataset has more than | ||
one session. Skipping the session layer for only some subjects in the dataset is | ||
not allowed. If a `/ses-<label>` subfolder is included as part of the | ||
directory hierarchy, then the same `ses-<label>` tag must also be | ||
one session. If a `/ses-<label>` subfolder is included as part of the | ||
directory hierarchy, then the same `ses-<label>` tag MUST also be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think skipping the ses- in some subjects is allowed if say not all subjects have the same sessions. Rn this results in a warning in validator, which basically all my data has this warning
yes, I experienced that warning multiple times myself 🙄 I think this case is more common than thought when this warning was originally implemented. That is, more often it's a choice, rather than an overlooked error.
re: your example --> yes 🤔 but I think your current changes (apart from my comments below) are actually fine and clarify the situation without changing the state of anything (which is what we should aim for), so I'd be fine with leaving the SHOULD, not interpreting it as a MUST post-hoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor wording/formatting requests.
Co-authored-by: Taylor Salo <tsalo006@fiu.edu>
Co-authored-by: Taylor Salo <tsalo006@fiu.edu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Only remaining suggestion is to split sentences with newlines.
Thanks @adam2392! |
Closes: #737
Question: "In case of single-session studies, this file has one compulsory column participant_id that consists of sub-, followed by a list of optional columns describing participants. " is in the current spec.
What does that mean? In case of single-session studies? Does that mean in single-session studies, this file is compulsory with participant_id a certain way?