bids-specification icon indicating copy to clipboard operation
bids-specification copied to clipboard

BEP 012: Functional MRI derivatives

Open effigies opened this issue 5 years ago • 6 comments

This PR has been extracted from nipreps/fmriprep#207, following the incorporation of BEP 003 - Common Derivatives (#265).

I've gone through a first pass of bringing it into conformation with BEP 003. I think there are no direct conflicts, so I would like to open this up to community comment.

I think the @bids-standard/derivatives-electrophys team should have a look at the _timeseries.<ext> components. I suspect that at least some of this could be written more generally to ensure that we make BEP021 a natural extension rather than an awkward re-architecting.

Link HTML render of the BEP: https://bids-specification--519.org.readthedocs.build/en/519/derivatives/functional-derivatives.html

Moderators: @effigies @cmaumet

effigies avatar Jun 30 '20 22:06 effigies

To continue the conversation from https://github.com/poldracklab/fmriprep/issues/2232#issuecomment-671653585:

@effigies said:

Regarding the decomposition.json, it looks like a table serialized to JSON, so I wonder why we wouldn't target TSVs. The derivatives spec, while a useful tool for standardizing common things, is inherently incomplete, and tools are allowed to produce unspecified derivatives. It makes more sense to me to spend a year or two with things in an unstable but useful state than wedging data into a specified but awkward structure.

@emdupre said:

I think we've generally aimed to build off of the fMRI derivatives BEP, which currently recommends a decomposition.json file specifically. Though I may have misunderstood how that file was originally envisioned !

Personally, I like having the metadata file in a JSON rather than a TSV, but that's only because it feels more "BIDS-y" to me. Though I'm certainly not the expert in this discussion on that 😸 All that to say, and seconding @\oesteban: Maybe we should have this conversation on that specific part of the derivatives BEP ?

Personally, I'd like to see these decomposition metrics and classifications supported in as BIDS Derivatives-compatible a manner as possible, since adding metadata to decompositions is very common, especially if you want to retain as much information as possible (e.g., variance explained for PCA components in CompCor, AROMA classifications, etc.). If a TSV targeting the decomposition JSON, with its own JSON describing the TSV's columns, would be the best way to do it, then could that be supported here?

tsalo avatar Aug 11 '20 16:08 tsalo

Per maintainers call today, two possible routes forward for decomposition statistics are (1) house everything in jsons (as tedana currently does) or (2) add a new suffix for TSVs containing these stats (e.g., _decompStats.tsv). The former is completely valid under the current rules, and the latter can always be adopted later, so the consensus was to stick with a json file for the immediate future, for both fMRIPrep and the Functional Derivatives BEP.

tsalo avatar Aug 18 '20 17:08 tsalo

maintenance note: added a link to the rendered BEP in the top message of this PR.

Remi-Gau avatar Feb 13 '23 10:02 Remi-Gau

Codecov Report

Patch and project coverage have no change.

Comparison is base (6d7eb0f) 87.83% compared to head (1c79cbc) 87.83%. Report is 37 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     nipreps/fmriprep#519   +/-   ##
=======================================
  Coverage   87.83%   87.83%           
=======================================
  Files          16       16           
  Lines        1356     1356           
=======================================
  Hits         1191     1191           
  Misses        165      165           

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

codecov[bot] avatar Jun 28 '23 16:06 codecov[bot]

In light of https://github.com/bids-standard/bids-specification/pull/981, specifically the _motion suffix, https://github.com/bids-standard/bids-specification/blob/843f4508018a2a8a3aecef3b26cfee399c3e39c8/src/schema/objects/suffixes.yaml#L672-L676 should https://github.com/bids-standard/bids-specification/blob/1c79cbc19f7ab9b99f38376978b37d78c8ce5759/src/derivatives/functional-derivatives.md?plain=1#L258-L269 revert to _timeseries, change to something unambiguous, or will the sidecar and context be sufficient to differentiate the different _motion files?

shnizzedy avatar Oct 13 '23 23:10 shnizzedy

I guess we need to see if registration-derived motion parameters are too different to use the same suffix. I assumed we had a case not too dissimilar and we could treat these as the same type of data, kind of like how you could create synthetic bold series from a forward model, but I have not considered this in detail.

effigies avatar Oct 14 '23 01:10 effigies