bids-standard / bids-specification

Brain Imaging Data Structure (BIDS) Specification
https://bids-specification.readthedocs.io/
Creative Commons Attribution 4.0 International
274 stars 157 forks source link

Add `descriptions.tsv` to the schema #1707

Closed tsalo closed 5 months ago

tsalo commented 7 months ago

Closes none, but continues from #1613.

I didn't figure out how to render the filenames in a macro. I think allowing descriptions.tsv files at the subject or session level introduced more complexity than it was worth, unfortunately.

I also haven't figured out how to add meaningful rules about the file to the schema. Specifically, we probably want to raise a warning if any desc values described in the file aren't in the dataset, or vice versa.

Changes proposed:

tsalo commented 7 months ago

Switching to a draft until I figure out how to render the filename template.

tsalo commented 7 months ago

I've given up on rendering the template with a macro, but I'll bring up adding checks for the file in the schema meeting today.

Presumably we would want rules to check if all desc-<label> entities are reflected in the file and that no definitions are included that aren't present in the dataset. Both should probably be warnings, rather than errors.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.93%. Comparing base (09ba933) to head (f756906).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1707 +/- ## ======================================= Coverage 87.93% 87.93% ======================================= Files 16 16 Lines 1351 1351 ======================================= Hits 1188 1188 Misses 163 163 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tsalo commented 7 months ago

I will open an issue about my two pain points:

EDIT: I've opened #1710.

Remi-Gau commented 5 months ago

@tsalo Can't remember if there is a reason this was not merged?

tsalo commented 5 months ago

No reason- I didn't realize I had two approvals. I'll update from main and then merge once CI passes.