nipreps / fmriprep

fMRIPrep is a robust and easy-to-use pipeline for preprocessing of diverse fMRI data. The transparent workflow dispenses of manual intervention, thereby ensuring the reproducibility of the results.
https://fmriprep.org
Apache License 2.0
638 stars 294 forks source link

Multi-part extensions should not infect JSON #2294

Closed effigies closed 4 years ago

effigies commented 4 years ago

fMRIPrep produces outputs named sub-19_task-MerlinMovie_space-fsLR_den-91k_bold.dtseries.json to match sub-19_task-MerlinMovie_space-fsLR_den-91k_bold.dtseries.nii.

In the spec, the example given is:

pipeline1/
    sub-001/
        func/
            sub-001_task-rest_run-1_space-fsLR_res-1_den-10k_bold.dtseries.nii
            sub-001_task-rest_run-1_space-fsLR_res-1_den-41k_bold.dtseries.nii
            sub-001_task-rest_run-1_space-fsLR_res-2_den-10k_bold.dtseries.nii
            sub-001_task-rest_run-1_space-fsLR_res-2_den-41k_bold.dtseries.nii
            sub-001_task-rest_run-1_space-fsLR_bold.json

PyBIDS also looks for sidecars with the extension .json, not .dtseries.json.

A first pass at this was taken in https://github.com/nipreps/niworkflows/pull/558 but dropped.

There was concern about not having sufficient disambiguation, so if we still can see that, then we need to make an issue on the spec. For my part, I would prefer not to change the spec, but to ensure that we always provide enough entities to ensure that no ambiguity is possible.

What version of fMRIPrep are you using?

20.2.0

effigies commented 4 years ago

@bpinsard There's a question of whether this should be handled in 20.2.1 or 20.3.0. It will change the derivatives, but I think the current derivatives are in error. Therefore I think it would be acceptable to fix in the LTS series.

An uglier but more conservative option would be to could keep the .dtseries.json files with their current data, and add .json files with valid BIDS keys.

mgxd commented 4 years ago

I agree - I think this is a 20.2.1 issue. I'm inclined to go the conservative route, since any breaking derivatives change should be avoided in the LTS. @bpinsard WDYT?

effigies commented 4 years ago

The question is whether the current outputs are in error. We permit changes to derivatives that remove or modify outputs that are in error.

If we do want to use the conservative route, I think the strategy needs to be to declare a frozen set of keys that will continue to go into .dtseries.json, and then all others go into .json.

effigies commented 4 years ago

@mgxd Let's go with the conservative route for now. If it's acceptable to remove the "invalid" JSON files now, it's acceptable to remove them later.