ENH: Refactor of how spatial normalization targets and `--output-spaces` are maintained #1955

oesteban · 2020-01-31T08:00:06Z

Supersedes (and largely based on) #1915.

Distinguish user-defined `--output-spaces` from internal intermediary steps

This PR ensures that spaces not requested (i.e. fsaverage5 when running with freesurfer, fsLR surfaces when using --cifti-output, etc) are not included in the final output.

To do so, it utilizes the recently added infrastructure for Spaces and SpatialReferences.

To avoid name collisions of the outputs, it uses a hack on our current DerivativesDataSinks. Those will need to be revised with the BIDS-Derivs spec because cohort and res appear at the end before the suffix (that's a side effect of the hack). But I guess this is better than not allowing it or rewriting silently.

auto-comment · 2020-01-31T08:00:10Z

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

.circleci/ds210_outputs.txt

fmriprep/cli/run.py

fmriprep/workflows/bold/base.py

fmriprep/workflows/bold/resampling.py

effigies

Doc issues.

fmriprep/workflows/base.py

pep8speaks · 2020-01-31T22:07:11Z

Hello @oesteban! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-02-07 16:25:10 UTC

oesteban

Okay, getting there - this is coming along.

.circleci/ds210_outputs.txt

mgxd

Okay, this is coming together!

We'll have to add a brief line explaining only requested output spaces are produced here
https://github.com/poldracklab/fmriprep/blob/master/docs/outputs.rst#derivatives-of-fmriprep-preprocessed-data

I'll send a PR to your branch with some documentation updates

docs/spaces.rst

fmriprep/utils/spaces.py

fmriprep/workflows/base.py

fmriprep/workflows/bold/base.py

fmriprep/workflows/bold/outputs.py

fmriprep/workflows/bold/resampling.py

oesteban · 2020-02-05T18:31:48Z

This PR interacts with several issues:

Fixes carpetplot and aroma #1739
Fixes Error to generate the cifti files #1810
Implements and closes Allow spatial normalization to the same template, different cohort #1817

…r of anatomical iterables

… [skip ci] becuse they do exist now (addresses https://github.com/poldracklab/fmriprep/pull/1955/files#r375317164).

Co-Authored-By: Mathias Goncalves <[email protected]>

setup.cfg

oesteban changed the title ~~ENH: Refactor of how spatial normalization targets and --output-spaces are maintained~~ WIP/ENH: Refactor of how spatial normalization targets and --output-spaces are maintained Jan 31, 2020

oesteban mentioned this pull request Jan 31, 2020

ENH: Fix template keys output in normalization workflow, when cohort present nipreps/smriprep#163

Merged

effigies reviewed Jan 31, 2020

View reviewed changes

fmriprep/workflows/base.py Outdated Show resolved Hide resolved

fmriprep/workflows/base.py Outdated Show resolved Hide resolved

mgxd mentioned this pull request Feb 3, 2020

enh: standard space querying nipreps/niworkflows#455

Merged

oesteban force-pushed the patch/fix-outputs branch 3 times, most recently from 0875723 to 9d37a44 Compare February 4, 2020 01:31

oesteban changed the title ~~WIP/ENH: Refactor of how spatial normalization targets and --output-spaces are maintained~~ ENH: Refactor of how spatial normalization targets and --output-spaces are maintained Feb 4, 2020

oesteban commented Feb 4, 2020

View reviewed changes

.circleci/ds210_outputs.txt Show resolved Hide resolved

oesteban requested review from effigies and mgxd February 5, 2020 00:13

mgxd reviewed Feb 5, 2020

View reviewed changes

oesteban commented Feb 5, 2020

View reviewed changes

fmriprep/workflows/bold/resampling.py Outdated Show resolved Hide resolved

fmriprep/workflows/bold/resampling.py Outdated Show resolved Hide resolved

fmriprep/workflows/bold/resampling.py Outdated Show resolved Hide resolved

mgxd added 13 commits February 6, 2020 13:48

enh: rework output spaces to be explicit (nipreps#1892)

811fa2f

sty: pacify flake8

317ae8d

tst: remove extraneous output

21586b6

tst: fix outputs

ac1bd4f

doc: function signature

66f6fc8

fix: ensure consistent space usage

bb54012

rework: normalization spaces

bcba9c9

helper method for key selecting

0c6253d

fix: remove hardcoded

2abd275

fix+doc: reflect new spaces object

30b6bf7

doc: function signature changes

e2d998a

maint: transfer spaces handler to niworkflows

6112285

pin: all the moving pieces

839b41a

mgxd and others added 21 commits February 6, 2020 13:48

fix: high res cifti volume res

a24c017

fix: ensure internal volume standard spaces are also used

77128f7

fix: leverage dim option

06acdde

fix: exclude nonstandard spaces

6a8968f

fix: downsample high res cifti; dont hardcode # grayords

613f2bd

fix: isolate iterable source so joinnode is not considered a successo…

aff8f51

…r of anatomical iterables

fix: unify string conversion of space definitions

3f7b272

fix: bad space key in grayordinates and aroma workflows

11233b5

fix(circleci): revise outputs check of ds054 and ds005

8a7db97

docs: update documentation regarding spaces

4df41f7

fix: apply separate iterable node pattern on surface resampling workflow

68eb825

sty: fix all PEP8 errors

83188b3

fix(circle): finding and solving minimal problems in circle steps

ab76ad9

docs(spaces): remove reference to nonexistent cohorts in TemplateFlow…

7ac1ac7

… [skip ci] becuse they do exist now (addresses https://github.com/poldracklab/fmriprep/pull/1955/files#r375317164).

Apply suggestions from code review [skip ci]

feef364

Co-Authored-By: Mathias Goncalves <[email protected]>

enh: remove unused function definitions

5f92adb

rf: reflect Space changes

1455b74

RF: port over new methods, update smriprep pin

dfa4950

fix: fs outputs

0f4961e

doc: bad import

db6a336

apply suggestions from code review

0474d10

oesteban force-pushed the patch/fix-outputs branch from 401a6a8 to 0474d10 Compare February 6, 2020 21:48

mgxd approved these changes Feb 7, 2020

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

Update setup.cfg

249bbe3

mgxd merged commit c6cf932 into nipreps:master Feb 7, 2020

oesteban deleted the patch/fix-outputs branch February 7, 2020 17:58

This was referenced Feb 7, 2020

carpetplot and aroma #1739

Closed

Error to generate the cifti files #1810

Closed

Allow spatial normalization to the same template, different cohort #1817

Closed

FR: Arbitrary output resolution (for --output-spaces) #1968

Open

ENH: Refactor of how spatial normalization targets and --output-spaces are maintained #1955

ENH: Refactor of how spatial normalization targets and --output-spaces are maintained #1955

Uh oh!

Conversation

oesteban commented Jan 31, 2020

Distinguish user-defined --output-spaces from internal intermediary steps

Uh oh!

auto-comment bot commented Jan 31, 2020

PR Review

Code documentation

Documentation site

Tests

Data

Dependencies: smriprep

Dependencies: niworkflows

Dependencies: sdcflows

Dependencies: Nipype

Dependencies: other

Reports generated within CI tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pep8speaks commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-02-07 16:25:10 UTC

Uh oh!

oesteban left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mgxd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oesteban commented Feb 5, 2020

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ENH: Refactor of how spatial normalization targets and `--output-spaces` are maintained #1955

ENH: Refactor of how spatial normalization targets and `--output-spaces` are maintained #1955

Distinguish user-defined `--output-spaces` from internal intermediary steps

pep8speaks commented Jan 31, 2020 •

edited

Loading