ENH: Integrate new infrastructure in NiWorkflows to handle spatial references #159

mgxd · 2020-01-21T23:02:11Z

upstream PR - nipreps/fmriprep#1915

Changes

change output_spaces OrderedDict in function signatures to spaces, a Spaces object.

Maintenance

Remove --template, --fs-output-spaces, and --no-freesurfer command line arguments

pep8speaks · 2020-01-22T18:00:31Z

Hello @mgxd! 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-01-29 15:31:06 UTC

mgxd · 2020-01-22T19:24:56Z

@oesteban this roughly models the behavior discussed in nipreps/fmriprep#1604, though since we're essentially treating this as a new language (spac-ese?) it would be good to cement the conventions, especially when multiple entities are given.

An example of a potentially ambiguous input:
--output-spaces MNI152:label1-a:label2-b

Should this produce 1 MNI152 output with label1 / label2 specifications, or 2 MNI152 outputs each pertaining to a label spec?

To make this clearer, I propose introducing the , character to denote multiple specifications for a single space.

i.e.

--output-spaces MNI152NLin2009cAsym:res-1:res-2 should produce 2 resolutions of the MNI152NLin2009cAsym template
--output-spaces MNIPediatricAsym:cohort-1,res-2 should produce 1 resolution of cohort 1
--output-spaces MNIPediatricAsym:cohort-1:res-2 should produce the default (unspecified) resolution of cohort-1, and a lower resolution default MNIPediatricAsym (if there is one available)

WDYT?

smriprep/cli/run.py

smriprep/workflows/norm.py

… conversions

Will work when the PR to niworkflows is merged.

I/O refactor suggestions

…version

…ements.txt

oesteban · 2020-01-29T15:23:03Z

Please check out mgxd#2

enh: update after merge of the niworkflows PR

effigies · 2020-01-29T15:48:55Z

smriprep/workflows/anatomical.py

+        spaces,
+        debug=False,
+        name='anat_preproc_wf',
+        skull_strip_fixed_seed=False


Just a note that we've apparently abandoned our usual rule of no default arguments (except name) in workflow building functions.

We might want to think about either re-enforcing it or making an explicit statement in a style guide of when default arguments should be used.

Sounds reasonable

effigies

This LGTM.

oesteban · 2020-01-29T16:11:16Z

@mgxd why blocked?

codecov-io · 2020-01-29T16:15:37Z

Codecov Report

Merging #159 into master will decrease coverage by 0.07%.
The diff coverage is 85.18%.

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
- Coverage   77.64%   77.56%   -0.08%     
==========================================
  Files          15       14       -1     
  Lines         841      798      -43     
  Branches      104       90      -14     
==========================================
- Hits          653      619      -34     
+ Misses        145      139       -6     
+ Partials       43       40       -3

Flag	Coverage Δ
#ds005	`72.77% <25.92%> (-0.65%)`	⬇️
#ds054	`58.84% <55.55%> (-0.99%)`	⬇️

Impacted Files	Coverage Δ
smriprep/workflows/anatomical.py	`77.14% <ø> (-0.33%)`	⬇️
smriprep/interfaces/reports.py	`77.21% <0%> (-4.12%)`	⬇️
smriprep/workflows/outputs.py	`70.87% <100%> (ø)`	⬆️
smriprep/workflows/norm.py	`100% <100%> (ø)`	⬆️
smriprep/workflows/base.py	`85.45% <100%> (ø)`	⬆️
smriprep/cli/run.py	`77.07% <100%> (+2.73%)`	⬆️
smriprep/interfaces/templateflow.py	`92.5% <100%> (+1.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb708ac...082bfc5. Read the comment docs.

oesteban · 2020-01-29T16:16:43Z

DS005 needs a cache wipeout bc it's picking up an old normalization reportlet (it should come out without normalization)

mgxd · 2020-01-29T16:17:04Z

need to do some testing on fmriprep side

oesteban · 2020-01-29T16:21:04Z

You are referring to nipreps/fmriprep#1915, right?

oesteban · 2020-01-29T16:29:54Z

smriprep/workflows/base.py

-        Name of ANTs skull-stripping template (e.g., 'OASIS30ANTs') and
-        dictionary of template specifications.
-    subject_id : str
+    skull_strip_template : :py:class:`~niworkflows.utils.spaces.Space`


btw, intersphinx is working with niworkflows :)

- Don't deploy to Pypi if ``build_docs`` fails (close nipreps#150) - Do not cache reportlets folder (see nipreps#159 (comment))

mgxd · 2020-01-30T19:55:15Z

i think this is fine to merge in - let's try to hold off release until nipreps/fmriprep#1915 is resolved

mgxd changed the title ~~Rework: in~~ Rework: internal / output spaces Jan 21, 2020

mgxd requested review from oesteban and effigies January 23, 2020 16:14

oesteban reviewed Jan 24, 2020

View reviewed changes

smriprep/cli/run.py Show resolved Hide resolved

smriprep/cli/run.py Outdated Show resolved Hide resolved

smriprep/cli/run.py Outdated Show resolved Hide resolved

smriprep/cli/run.py Outdated Show resolved Hide resolved

smriprep/workflows/norm.py Outdated Show resolved Hide resolved

oesteban mentioned this pull request Jan 24, 2020

ENH: Separate Space and SpatialReferences nipreps/niworkflows#451

Merged

mgxd force-pushed the fix/outputs branch from 879ef5d to 2946c02 Compare January 27, 2020 20:58

mgxd added 12 commits January 27, 2020 15:59

rework: normalization spaces

92d57f2

doc+rf: bring over spaces object, update docstring

1616ca0

maint: remove deprecated arguments, reflect Spaces changes

b97ab72

maint: newline, new spaces keyword

974a490

fix: filter standard volumes within smriprep

2d111f6

fix: drop output_spaces keyword, correct Spaces import

fe9315a

sty: remove unused import

23c6b79

pin: bump niworkflows to test spaces object

ad30a81

fix: skull strip template needs to be indexed

278f838

tst: fix most output space parsing tests

997c1e5

add: comma separator for multiple labels

58f7315

fix: tests

5a65c7f

mgxd force-pushed the fix/outputs branch from 2946c02 to 5a65c7f Compare January 27, 2020 21:00

oesteban and others added 9 commits January 27, 2020 13:57

enh: refactor with SpatialReferences object from niworkflows

7dcdf94

fix(cli): Use Space.from_string to read --skull-strip-template

cc9b9cc

fix(interface): templateflow desc interface

62f0003

fix: handle no --output-spaces present in the CLI.

15c7a4e

fix(ds005): since --output-spaces is not defined, do not generate…

9d848b8

… conversions

fix(docs): make sure all workflows build, remove smriprep.cli.utils

f28bb3e

docs: ensure intersphinx links niworkflows' new classes [skip ci]

0391b4f

Will work when the PR to niworkflows is merged.

Merge pull request #1 from oesteban/patch/fix/outputs

9db1c06

I/O refactor suggestions

enh: ensure output-spaces are parsed properly

40d3458

oesteban and others added 2 commits January 28, 2020 19:11

enh: update after merge of the niworkflows PR

0d9eafb

enh: use spatialreferences as spaces handler, update niworkflows min …

2c8fe8e

…version

mgxd force-pushed the fix/outputs branch from 40d3458 to 2c8fe8e Compare January 29, 2020 03:44

oesteban and others added 4 commits January 28, 2020 21:43

Merge branch 'fix/outputs' into patch/fix/outputs

648747a

fix: add a default value to --output-spaces

ed081f9

fix(docs): update templateflow pin and niworkflows pin in docs/requir…

e7541b4

…ements.txt

sty: flake8 fixes

c8dbd06

mgxd force-pushed the fix/outputs branch from de923d2 to c8dbd06 Compare January 29, 2020 14:03

mgxd added 2 commits January 29, 2020 10:30

Merge branch 'fix/outputs' into patch/fix/outputs

6fec7d7

Merge pull request #2 from oesteban/patch/fix/outputs

082bfc5

enh: update after merge of the niworkflows PR

effigies reviewed Jan 29, 2020

View reviewed changes

effigies approved these changes Jan 29, 2020

View reviewed changes

effigies added this to the 0.5.0 milestone Jan 29, 2020

mgxd added the blocked label Jan 29, 2020

oesteban approved these changes Jan 29, 2020

View reviewed changes

oesteban changed the title ~~Rework: internal / output spaces~~ ENH: Integrate new infrastructure in NiWorkflows to handle spatial references Jan 29, 2020

oesteban added a commit to oesteban/smriprep that referenced this pull request Jan 29, 2020

FIX: Improvements to the CircleCI workflow

943b8bb

- Don't deploy to Pypi if ``build_docs`` fails (close nipreps#150) - Do not cache reportlets folder (see nipreps#159 (comment))

oesteban mentioned this pull request Jan 29, 2020

FIX: Improvements to the CircleCI workflow #162

Merged

mgxd removed the blocked label Jan 30, 2020

mgxd merged commit 557b689 into nipreps:master Jan 30, 2020

mgxd deleted the fix/outputs branch January 30, 2020 19:55

This was referenced Feb 5, 2020

RFC: Revising Contributor Guidelines on adding new features to fMRIPrep nipreps/fmriprep#1963

Closed

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Integrate new infrastructure in NiWorkflows to handle spatial references #159

ENH: Integrate new infrastructure in NiWorkflows to handle spatial references #159

mgxd commented Jan 21, 2020

pep8speaks commented Jan 22, 2020 •

edited

Loading

mgxd commented Jan 22, 2020

oesteban commented Jan 29, 2020

effigies Jan 29, 2020

oesteban Jan 29, 2020

effigies left a comment

oesteban commented Jan 29, 2020

codecov-io commented Jan 29, 2020 •

edited

Loading

oesteban commented Jan 29, 2020

mgxd commented Jan 29, 2020

oesteban commented Jan 29, 2020

oesteban Jan 29, 2020

mgxd commented Jan 30, 2020

ENH: Integrate new infrastructure in NiWorkflows to handle spatial references #159

ENH: Integrate new infrastructure in NiWorkflows to handle spatial references #159

Conversation

mgxd commented Jan 21, 2020

Changes

Maintenance

pep8speaks commented Jan 22, 2020 • edited Loading

Comment last updated at 2020-01-29 15:31:06 UTC

mgxd commented Jan 22, 2020

oesteban commented Jan 29, 2020

effigies Jan 29, 2020

Choose a reason for hiding this comment

oesteban Jan 29, 2020

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

oesteban commented Jan 29, 2020

codecov-io commented Jan 29, 2020 • edited Loading

Codecov Report

oesteban commented Jan 29, 2020

mgxd commented Jan 29, 2020

oesteban commented Jan 29, 2020

oesteban Jan 29, 2020

Choose a reason for hiding this comment

mgxd commented Jan 30, 2020

pep8speaks commented Jan 22, 2020 •

edited

Loading

codecov-io commented Jan 29, 2020 •

edited

Loading