search

bids-standard / bids-specification

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

[FIX] Move ``_stim.<ext>`` specification within the Task Events module #1750

Closed oesteban closed 2 months ago

oesteban commented 3 months ago

This fix builds on #1749 to separate _stim and _physio in the specs.

Currently _stim files are specified sideways as an addon or extra branch of _physio, which can be confusing and misrepresent their intent.

Moving them with the other stimuli definitions increases the consistency of the spec and the findability of _stim specifications.

This fix requires #1749 for a more consistent prescription of tsv.gz files.

sappelhoff commented 3 months ago

This fix builds on https://github.com/bids-standard/bids-specification/issues/1748 to separate _stim and _physio in the specs.

did you mean to reference:

oesteban commented 3 months ago

Since #1749 is looking very close to the finish line, the following link https://github.com/oesteban/bids-specification/compare/fix/large-tabular-files...oesteban:bids-specification:fix/stim-files-with-stimuli can be used to look at the changes specific of this PR without distractions.

codecov[bot] commented 3 months ago

Codecov Report

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

Project coverage is 87.93%. Comparing base (dbcb237) to head (cd9c926).

:exclamation: Current head cd9c926 differs from pull request most recent head 00114cc. Consider uploading reports for the commit 00114cc to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1750 +/- ## ======================================= 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.

Remi-Gau commented 3 months ago

I introduced a merged conflict. Will fix.

oesteban commented 3 months ago

I introduced a merged conflict. Will fix.

I can take care, will be faster

EDIT - fixed!

oesteban commented 3 months ago

Good with me

What should we do about the admonition linting? I haven't managed to ignore them or making the plugin work.

oesteban commented 3 months ago

Perhaps moving them inside the admonition?

oesteban commented 3 months ago

Perhaps moving them inside the admonition?

That actually did the trick!

Remi-Gau commented 3 months ago

I think this should be ready to merge after the 5 days period.

Remi-Gau commented 3 months ago

I agree with the changes here in principle, pending this review item:

* https://github.com/bids-standard/bids-specification/pull/1750/files#r1548105139

it'd be important that the reference to an example points at a valid example

BTW: did a quick check on openneuro via the datalad superdataset and I could not find a dataset with stim files except the one that @oesteban had mentioned.

oesteban commented 2 months ago

@Remi-Gau @sappelhoff @effigies - may I merge this? I'd like to rebase BEP020, which will benefit substantially from getting stim out of physio.