-
Notifications
You must be signed in to change notification settings - Fork 163
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
[ENH] Clarify the relation of motion.tsv columns to channels.tsv rows #1699
[ENH] Clarify the relation of motion.tsv columns to channels.tsv rows #1699
Conversation
This relates to https://github.com/bids-standard/bids-validator/issues/1889. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1699 +/- ##
=======================================
Coverage 87.97% 87.97%
=======================================
Files 16 16
Lines 1356 1356
=======================================
Hits 1193 1193
Misses 163 163 ☔ View full report in Codecov by Sentry. |
Hum... I am sorry I missed that but I had assumed that those TSV had column headers given our common principles about tabular data: https://bids-specification.readthedocs.io/en/stable/common-principles.html#tabular-files
|
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.
This correctly clarifies that the motion data file containing channel time series must be headerless.
Yeah, in retrospect I wish we'd pushed a bit to make them either standard TSV files or stim/physio-like |
Fair. Should we update common principles though? |
Possibly. I don't know what I'd want them to say at this point. |
I seem to recall that this was being discussed at some point. I just fail to find the thread (e.g., in bids-standard/bids-examples#425). @sjeung @JuliusWelzel do you remember discussions about this and could potentially link to them?
Not pretty, but could do this: bids-specification/src/common-principles.md Lines 431 to 433 in 365f321
Each TSV file MUST start with a header line listing the names of all columns
(with the exception of physiological and other continuous recordings
+, and motion.tsv files). |
IMO we should either migrate to headers right now or not at all in BIDS 1.x. There hasn't been much data uploaded at this point and the text of the spec is ambiguous, so we can clarify in the opposite direction as well. |
Slight preference for having headers for internal consistency but I may be missing the kind of headaches it may involve for motion data |
I think the protocol from 2021 was before we agreed on separating channel files for multiple tracking systems. Unfortunately I could not find any log of .gz discussion which I recall was around community review phase, and the main reason against .gz was time pressure before release of the upcoming BIDS version back then. So I would be happy to make motion spec coherent with other continuous time series data .tsv files in BIDS like stim or physio in the future. (no headers, and tsv.gz) |
Ah, yes. I read that as "March 21, 2023", not "March 2021". If that is that out-of-date, then I do not think that qualifies as a decision that was missed during merge, but an outdated decision. I am +1 for allowing |
289b68e
to
83c52d6
Compare
The motion spec does not say whether there is a header column or not, only that the columns must match the rows of channels.tsv. From unofficial communications, I believe the intent is that motion.tsv files do not have column headers, and channels.tsv defines the column names.
This changes it to state explicitly that there are no column headers, and that the ordering is defined by the rows in channels.tsv.
cc @sjeung @JuliusWelzel