ENH: Use new `DerivativesDataSink` from NiWorkflows 1.2.0 #2114

oesteban · 2020-04-30T15:30:53Z

This PR integrates the new API that will be released with 1.2.0.

Changes proposed in this pull request

Documentation that should be reviewed

auto-comment · 2020-04-30T15:30:58Z

Thank your for raising your pull request.

Some of the fMRIPRep maintainers will review your changes as soon as time permits.
I'm attaching below a Review Checklist for the reviewers, to check off during the
review.

PR Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

As the reviewer I confirm that there are no conflicts of interest for me to review this work.

Please check what applies in the following aspects of the PR:

Code documentation

New functions and modules are documented following the guidelines.
Existing code required amendments in documentation and has been updated.
Sufficient inline comments ensure readability by other contributors in the long term.

Documentation site

The modifications to fMRIPrep in this PR do not require extra documentation. This is a common situation when a bug fix that does not change the code base substantially is submitted.
This PR requires documentation. The corresponding documentation will be submitted as an additional PR in the future.
This PR requires extra documentation, and will be included within this PR. Please check the boxes that apply best:
- An existing feature is substantially modified, changing the interface and/or logic of a module.
- A new feature is being added, therefore full documentation of it will be included
- This PR addresses an error in existing documentation.
Yes, all expected sections of documentation were generated by the CI build, and look as expected

Tests

The new functionalities in this PR are covered by the existing tests
New test batteries have been included with this PR

Data

This PR does not require any additional data to be uploaded to OSF.
This PR requires additional data for tests.

Dependencies: smriprep

This PR does not require any update on smriprep; or
smriprep has correctly been pinned.

Dependencies: niworkflows

This PR does not require any update on niworkflows; or
niworkflows has correctly been pinned.

Dependencies: sdcflows

This PR does not require any update on sdcflows; or
sdcflows has correctly been pinned.

Dependencies: Nipype

This PR does not require any update on nipype; or
nipype has correctly been pinned.

Dependencies: other

This PR does not require any update on other dependencies; or
other dependencies have correctly been pinned.

Reports generated within CI tests

Yes, I have checked the reports of ds005 and they are not modified or the changes are as expected
Yes, I have checked the reports of ds054 and they are not modified or the changes are as expected
Yes, I have checked the reports of ds010 and they are not modified or the changes are as expected

oesteban

Okay, the PR is working locally and I have high expectations that it'll work on Circle. I've left a few comments to make your reviews a bit easier, hope that helps.

oesteban · 2020-05-02T16:42:42Z

.circleci/ds005_fasttrack_outputs.txt

@@ -9,69 +9,69 @@ fmriprep/logs/CITATION.md
 fmriprep/logs/CITATION.tex
 fmriprep/sub-01
 fmriprep/sub-01/func
-fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-01_AROMAnoiseICs.csv
-fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-01_desc-confounds_regressors.json


pybids sanitizes the zero padding, and I'm fine with it - others?

oesteban · 2020-05-02T16:44:00Z

.circleci/ds005_fasttrack_outputs.txt

-fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-02_space-T1w_desc-brain_mask.nii.gz
-fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-02_space-T1w_desc-preproc_bold.json
-fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-02_space-T1w_desc-preproc_bold.nii.gz
+fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-1_AROMAnoiseICs.csv


I believe this is pretty nonstandard (even w.r.t. any current draft). Should we fix this in a separate PR, now that the implementation will be flexible enough to just update the inputs to the datasink?

Let's do a separate PR, because we need to talk through the consequences with interested users.

oesteban · 2020-05-02T16:49:45Z

.circleci/ds005_fasttrack_outputs.txt

+fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-1_desc-confounds_regressors.json
+fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-1_desc-confounds_regressors.tsv
+fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-1_desc-MELODIC_mixing.tsv
+fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-1_space-fsaverage5_hemi-L_bold.func.gii


I've also added this as a default pattern:

hemi-X.func.gii -> hemi-X_bold.func.gii

The reason is that the suffix was being missed. In case you're wondering why I didn't add a _T1w suffix to the .surf.gii of anatomical derivatives, the reason is that .surf.gii only contains geometrical information, so there's no concept of modality expressed by the suffix. One less theoretical reason is that finding _T1w.surf.gii implies the surface was extracted from a T1w and, for instance FLAIR or T2w were not used. Agreed this is not a super-solid argument, but I believe should be enough to feel comfortable with the new naming as regards GIFTIs.

@mgxd @effigies, just double-checking you did not oversee this one, as it could be controversial.

Hmm. Yeah, maybe let's not do this until we finalize derivatives, to minimize the number of times we change derivatives.

The default pattern is already written in niworkflows (which means, rolling back requires one more PR cycle). In addition, the new pattern would be more regular so hopefully more likely to be in line with the finalized derivatives.

If the reason to roll back is just practical (i.e., the reason to introduce this change is not counterargued), I'd rather keep it.

I'm okay with this. It looks like it's not covered explicitly anywhere in the derivatives draft, so we should make a point to argue for it.

oesteban · 2020-05-02T16:50:38Z

.circleci/ds005_fasttrack_outputs.txt

+fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-2_desc-confounds_regressors.json
+fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-2_desc-confounds_regressors.tsv
+fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-2_desc-MELODIC_mixing.tsv
+fmriprep/sub-01/func/sub-01_task-mixedgamblestask_run-2_space-fsaverage5_hemi-L_bold.func.gii


Similarly to the AROMA noise components, we might want to update this and generate space-fsaverage_den-10k here (for a future PR).

oesteban · 2020-05-02T16:51:45Z

docs/workflows.rst

@@ -50,7 +50,6 @@ single reference template (see `Longitudinal processing`_).
            ('T1w', {}),
            ('fsnative', {})
        ]),
-        reportlets_dir='.',


Reportlets can now be written out directly to <output>/fmriprep/sub-xx/figures/

oesteban · 2020-05-02T17:02:13Z

fmriprep/workflows/bold/outputs.py

@@ -256,60 +256,133 @@ def init_func_derivatives_wf(

    fs_outputs = spaces.cached.get_fs_spaces()
    if freesurfer and fs_outputs:
-        from niworkflows.interfaces.surf import GiftiNameSource


I haven't removed this for backwards compatibility, but the new Path2BIDS should simplify its logic (no need anymore for building up the new pattern).

oesteban · 2020-05-02T17:04:24Z

fmriprep/workflows/bold/outputs.py

        ])

    return workflow
+
+
+def init_bold_preproc_report_wf(mem_gb, reportlets_dir, name='bold_preproc_report_wf'):


This change is a bit out of scope. This workflow is not used anywhere (I wrote it when there were plans to show a before/after reportlet without/with preprocessing. This will potentially be used at some point, and I thought it was more findable here in the outputs module.

setup.cfg

setup.py

effigies

Ran out of time to finish this before the meeting. Let's also revert the unnecessary setup.* changes here, and that will make sure we test the latest commit over at niworkflows.

Will finish review this afternoon.

effigies · 2020-05-06T16:55:39Z

fmriprep/workflows/bold/base.py

@@ -826,7 +826,7 @@ def init_func_preproc_wf(bold_file):
    # Fill-in datasinks of reportlets seen so far
    for node in workflow.list_node_names():
        if node.split('.')[-1].startswith('ds_report'):
-            workflow.get_node(node).inputs.base_directory = reportlets_dir
+            workflow.get_node(node).inputs.base_directory = output_dir


This will also put HTML snippets in figures/, right?

effigies

One additional comment.

fmriprep/workflows/bold/outputs.py

This PR integrates the new API that will be released with 1.2.0. References: nipreps/niworkflows#507

... in Circle's docker containers.

- [x] ``res-0X`` -> ``res-X`` - [x] ``hemi-X.func.gii`` -> ``hemi-X_bold.func.gii``

oesteban · 2020-05-07T21:04:35Z

This is looking good. Can we just merge now? Last call for last minute reviews?

effigies · 2020-05-07T21:18:57Z

Let's do it.

oesteban · 2020-05-07T21:42:40Z

Okay -

(sorry, couldn't hold it)

oesteban force-pushed the enh/new-datasinks branch 3 times, most recently from 45ccd1c to 83d4754 Compare May 2, 2020 16:46

oesteban commented May 2, 2020

View reviewed changes

oesteban marked this pull request as ready for review May 3, 2020 14:28

effigies reviewed May 6, 2020

View reviewed changes

fmriprep/workflows/bold/outputs.py Show resolved Hide resolved

oesteban mentioned this pull request May 7, 2020

PIN: nipreps/niworkflows#485 #2108

Closed

oesteban added 7 commits May 7, 2020 07:53

ENH: Use new DerivativesDataSink from NiWorkflows 1.2.0

6b72929

This PR integrates the new API that will be released with 1.2.0. References: nipreps/niworkflows#507

enh: move init_bold_preproc_report_wf into outputs module.

3025c9b

enh: configuring new datasinks

d51106e

enh: revise CIFTI2 datasinks

e922a9f

chore: minor updates to setup, to force fresh install of niworkflows...

f64b671

... in Circle's docker containers.

fix: generation of surface timeseries

25b7605

fix: update output naming tests

adc1563

- [x] ``res-0X`` -> ``res-X`` - [x] ``hemi-X.func.gii`` -> ``hemi-X_bold.func.gii``

oesteban force-pushed the enh/new-datasinks branch 2 times, most recently from 48bd94e to a86d1ef Compare May 7, 2020 17:08

fix: remove the res-native in outputs with space defined

7aa3453

oesteban force-pushed the enh/new-datasinks branch from a86d1ef to 7aa3453 Compare May 7, 2020 17:10

fix: add compress=True to those DerivativesDataSinks missing it

82e6394

oesteban merged commit 3712c39 into nipreps:master May 7, 2020

oesteban deleted the enh/new-datasinks branch May 7, 2020 21:45

oesteban mentioned this pull request May 20, 2020

Figure naming incorrect when t2s-coreg option used in v20.0.7 #2127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Use new `DerivativesDataSink` from NiWorkflows 1.2.0 #2114

ENH: Use new `DerivativesDataSink` from NiWorkflows 1.2.0 #2114

oesteban commented Apr 30, 2020

auto-comment bot commented Apr 30, 2020

oesteban left a comment

oesteban May 2, 2020

oesteban May 2, 2020

effigies May 7, 2020

oesteban May 2, 2020

oesteban May 7, 2020

effigies May 7, 2020

oesteban May 7, 2020

effigies May 7, 2020

oesteban May 2, 2020

oesteban May 2, 2020

oesteban May 2, 2020

oesteban May 2, 2020

effigies left a comment

effigies May 6, 2020

effigies left a comment

oesteban commented May 7, 2020

effigies commented May 7, 2020

oesteban commented May 7, 2020

ENH: Use new DerivativesDataSink from NiWorkflows 1.2.0 #2114

ENH: Use new DerivativesDataSink from NiWorkflows 1.2.0 #2114

Conversation

oesteban commented Apr 30, 2020

Changes proposed in this pull request

Documentation that should be reviewed

auto-comment bot commented Apr 30, 2020

PR Review

Code documentation

Documentation site

Tests

Data

Dependencies: smriprep

Dependencies: niworkflows

Dependencies: sdcflows

Dependencies: Nipype

Dependencies: other

Reports generated within CI tests

oesteban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

oesteban commented May 7, 2020

effigies commented May 7, 2020

oesteban commented May 7, 2020

ENH: Use new `DerivativesDataSink` from NiWorkflows 1.2.0 #2114

ENH: Use new `DerivativesDataSink` from NiWorkflows 1.2.0 #2114