Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Allow multiple {non,}standard spaces (--output-spaces) #1596

Merged
merged 27 commits into from
May 3, 2019

Conversation

oesteban
Copy link
Member

This is a large refactor using two major inter-related features:

Correspondingly, the new interface for --output-spaces is also
incorporated (close #1588).

This is a large refactor using two major inter-related features:
  - Uses nipreps/smriprep#72 and its continuation within
    nipreps/smriprep#75 to permit the specification of several
    spatial normalization targets (close nipreps#1558).
  - Uses the refactor of Reports generation of
    nipreps/niworkflows#344, which enables rendering reports with
    several normalization spaces. Correspondingly, report generation
    code and config files have been removed.

Correspondingly, the new interface for ``--output-spaces`` is also
incorporated (close nipreps#1588).
@oesteban oesteban requested a review from effigies April 30, 2019 01:48
@oesteban
Copy link
Member Author

This is working seemingly fine with ds005 locally, including --use-aroma and --use-syn-sdc along with --output-spaces anat MNI152NLin2009cAsym:res-2 MNI152NLin6Asym:res-2 fsaverage:den-10k MNI152Lin:res-2.

I expect many issues building docs and running ds054 though.

@oesteban oesteban force-pushed the ref/multiple-output-spaces branch from c7b57de to 416610f Compare April 30, 2019 07:01
@effigies
Copy link
Member

If you want a review before the CI is fixed, let me know.

@oesteban
Copy link
Member Author

Let's wait for Circle to be all green 👍

@oesteban
Copy link
Member Author

oesteban commented Apr 30, 2019

Okay, this is looking much better. Last commit is a good candidate for getting all green lights.

Documentation is not failing anymore and it looks fine: https://3557-53608443-gh.circle-artifacts.com/0/home/circleci/out/docs/usage.html#defining-standard-and-nonstandard-spaces-where-data-will-be-resampled
Also interesting to check the description of the --output-spaces argument expanded by the sphinx argparse extension.

EDIT: ds054 is passing
EDIT: ds210 is passing although it says there are 3 runs in the report (only run-1 is shown). I've dug back for over a year and yet reports show only one run (while listing 3). @emdupre any thoughts of when this could've happened?

wrapper/fmriprep_docker.py Outdated Show resolved Hide resolved
@oesteban
Copy link
Member Author

oesteban commented Apr 30, 2019

Ref. ds210, it seems the reports generator has been confused with the echos since forever. In principle, there's only one run in our test version of ds210. (opened #1598)

@emdupre
Copy link
Collaborator

emdupre commented Apr 30, 2019

I can check and see if things have been updated, but it should be showing only one "echo" because that image is actually the optimal combination. We implemented that as the default in the last big ME PR !

@oesteban
Copy link
Member Author

I think that big PR is working alright, my suspicion is that this is a little bug of the reports generator.

Leave for a future PR the implementation of HCP-compatible cifti.

Also, run a ``KeySelect`` to extract the appropriate BOLD in standard
space resampling.
Otherwise, it is not guaranteed that the last commit will be picked.
@oesteban
Copy link
Member Author

oesteban commented May 3, 2019

@effigies this is finally ready for you

docs/usage.rst Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
docs/usage.rst Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
docs/usage.rst Show resolved Hide resolved
fmriprep/cli/run.py Outdated Show resolved Hide resolved
fmriprep/interfaces/reports.py Outdated Show resolved Hide resolved
fmriprep/workflows/base.py Show resolved Hide resolved
fmriprep/workflows/bold/base.py Outdated Show resolved Hide resolved
fmriprep/workflows/bold/resampling.py Show resolved Hide resolved
Co-Authored-By: oesteban <code@oscaresteban.es>
oesteban added a commit to oesteban/fmriprep that referenced this pull request May 3, 2019
@oesteban oesteban force-pushed the ref/multiple-output-spaces branch from 5ba13a6 to 695dd03 Compare May 3, 2019 18:12
@oesteban oesteban force-pushed the ref/multiple-output-spaces branch from b1c9f33 to 4afb89b Compare May 3, 2019 21:33
@oesteban oesteban merged commit a774bb5 into nipreps:master May 3, 2019
@oesteban oesteban deleted the ref/multiple-output-spaces branch May 3, 2019 21:43
effigies added a commit that referenced this pull request May 4, 2019
DOC: Companion of #1596 + punctual improvements of docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revise command line interface of spaces/references/outputs Support templates other than MNI152NLin2009cAsym
3 participants