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

[ENH] Clarify the relation of motion.tsv columns to channels.tsv rows #1699

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

effigies
Copy link
Collaborator

@effigies effigies commented Feb 8, 2024

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

@effigies
Copy link
Collaborator Author

effigies commented Feb 8, 2024

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fd6612e) 87.97% compared to head (83c52d6) 87.97%.

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.
📢 Have feedback on the report? Share it here.

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Feb 9, 2024

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

Each TSV file MUST start with a header line listing the names of all columns (with the exception of physiological and other continuous recordings).

Copy link
Collaborator

@sjeung sjeung left a 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.

@effigies
Copy link
Collaborator Author

effigies commented Feb 9, 2024

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

Each TSV file MUST start with a header line listing the names of all columns (with the exception of physiological and other continuous recordings).

Yeah, in retrospect I wish we'd pushed a bit to make them either standard TSV files or stim/physio-like .tsv.gz files. As it is, they're not either, which means we now have a third way of specifying column headers, but so be it. I do not want to relitigate a merged BEP.

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Feb 9, 2024

Fair. Should we update common principles though?

@effigies
Copy link
Collaborator Author

Possibly. I don't know what I'd want them to say at this point.

@sappelhoff
Copy link
Member

in retrospect I wish we'd pushed a bit to make them either standard TSV files or stim/physio-like .tsv.gz files. As it is, they're not either, which means we now have a third way of specifying column headers, but so be it. I do not want to relitigate a merged BEP.

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?

I don't know what I'd want them to say at this point.

Not pretty, but could do this:

MUST NOT be a series of space characters. Each TSV file MUST start with a header
line listing the names of all columns (with the exception of
[physiological and other continuous recordings](modality-specific-files/physiological-and-other-continuous-recordings.md)).

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). 

@JuliusWelzel
Copy link
Collaborator

Hi, thanks for picking this up @effigies. I found a protocol from March 21, which said we would use headers in the motion.tsv file. For me, having headers in the motion.tsv file makes sense for a future release.

@effigies
Copy link
Collaborator Author

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.

@Remi-Gau
Copy link
Collaborator

Slight preference for having headers for internal consistency but I may be missing the kind of headaches it may involve for motion data

@sjeung
Copy link
Collaborator

sjeung commented Feb 13, 2024

I think the protocol from 2021 was before we agreed on separating channel files for multiple tracking systems.
We dropped headers after it became obsolete with the introduction of tracksys entity in names of channels.tsv files.
Having no headers is consistent with BIDS validator and example data set as well.

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)

@effigies
Copy link
Collaborator Author

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 motion to be .tsv or .tsv.gz, but there was just a suggestion in #197 to permit Parquet files, which I think would be a better fit than either. If that goes places, I would say we don't add .tsv.gz, but simply add .parquet.

@sappelhoff sappelhoff merged commit f320aba into bids-standard:master Feb 22, 2024
26 checks passed
@effigies effigies deleted the enh/clarify-motion-columns branch February 22, 2024 19:28
@sappelhoff sappelhoff changed the title ENH: Clarify the relation of motion.tsv columns to channels.tsv rows [ENH] Clarify the relation of motion.tsv columns to channels.tsv rows Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants