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.

Sign up for GitHub

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: Make default native resolution explicit #494

Merged
merged 6 commits into from
Apr 7, 2020
Merged

ENH: Make default native resolution explicit #494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e0f04ec
ENH: Make default native resolution explicit
mgxd Apr 3, 2020
a3edc3d
enh: strip native resolution identifier from derivatives
mgxd Apr 6, 2020
73f4dc1
ENH: avoid setting nonstandard references resolutions
mgxd Apr 6, 2020
be05b4e
Merge branch 'master' of github.com:nipreps/niworkflows into fix/defa…
mgxd Apr 6, 2020
d830e55
DOC: add space usage to docstring
mgxd Apr 6, 2020
f59e306
doc: insert urls to discussions [skip ci]
mgxd Apr 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions niworkflows/interfaces/bids.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,11 +342,12 @@ class DerivativesDataSink(SimpleInterface):
>>> dsink.inputs.source_file = str(tricky_source)
>>> dsink.inputs.keep_dtype = True
>>> dsink.inputs.desc = 'preproc'
>>> dsink.inputs.space = 'MNI152NLin6Asym_res-01'
>>> dsink.inputs.RepetitionTime = 0.75
>>> res = dsink.run()
>>> res.outputs.out_meta # doctest: +ELLIPSIS
'.../niworkflows/sub-02/ses-noanat/func/sub-02_ses-noanat_task-rest_run-01_\
desc-preproc_bold.json'
space-MNI152NLin6Asym_res-01_desc-preproc_bold.json'

>>> lines = Path(res.outputs.out_meta).read_text().splitlines()
>>> lines[1]
Expand All @@ -365,12 +366,13 @@ class DerivativesDataSink(SimpleInterface):
>>> dsink.inputs.source_file = str(tricky_source)
>>> dsink.inputs.keep_dtype = True
>>> dsink.inputs.desc = 'preproc'
>>> dsink.inputs.space = 'MNI152NLin6Asym_res-native'
>>> dsink.inputs.RepetitionTime = 0.75
>>> dsink.inputs.meta_dict = {'RepetitionTime': 1.75, 'SkullStripped': False, 'Z': 'val'}
>>> res = dsink.run()
>>> res.outputs.out_meta # doctest: +ELLIPSIS
'.../niworkflows/sub-02/ses-noanat/func/sub-02_ses-noanat_task-rest_run-01_\
desc-preproc_bold.json'
space-MNI152NLin6Asym_desc-preproc_bold.json'

>>> lines = Path(res.outputs.out_meta).read_text().splitlines()
>>> lines[1]
Expand Down Expand Up @@ -454,6 +456,10 @@ def _run_interface(self, runtime):
if len(self.inputs.in_file) > 1 and not isdefined(self.inputs.extra_values):
formatstr = formatbase + '{suffix}{i:04d}{dtype}{ext}'

if self.inputs.space and '_res-native' in self.inputs.space:
# strip native tag
self.inputs.space = self.inputs.space.split('_')[0]

Comment on lines +459 to +462
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just drop the resolution entity in the connection? Or the resolution entity is currently crammed into the space input in fMRIPrep and you are working around that hacky solution (honestly, I can't remember)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the resolution is appended to the space input, a hack we implemented to allow multiple resolutions of the same template

space = '_space-{}'.format(self.inputs.space) if self.inputs.space else ''
desc = '_desc-{}'.format(self.inputs.desc) if self.inputs.desc else ''
suffix = '_{}'.format(self.inputs.suffix) if self.inputs.suffix else ''
Expand Down
15 changes: 11 additions & 4 deletions niworkflows/utils/spaces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,10 +660,17 @@ def __call__(self, parser, namespace, values, option_string=None):
spaces.checkpoint()
for val in values:
val = val.rstrip(":")
# Should we support some sort of explicit "default" resolution?
# https://github.com/nipreps/niworkflows/pull/457#discussion_r375510227
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep this reference and add a reference to this PR as comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

# if ":res-" not in val or ":resolution-" not in val:
# val = ":".join((val, "res-default"))
if (
val not in NONSTANDARD_REFERENCES
and not val.split(':')[0].startswith('fs')
and ":res-" not in val
and ":resolution-" not in val
):
# by default, explicitly set volumetric resolution to native
# relevant discussions:
# https://github.com/nipreps/niworkflows/pull/457#discussion_r375510227
# https://github.com/nipreps/niworkflows/pull/494
val = ":".join((val, "res-native"))
for sp in Reference.from_string(val):
spaces.add(sp)
setattr(namespace, self.dest, spaces)
Expand Down
29 changes: 21 additions & 8 deletions niworkflows/utils/tests/test_spaces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,25 @@ def parser():
return pars


@pytest.mark.parametrize("spaces,expected", [
(("MNI152NLin6Asym",), 1),
(("fsaverage:den-10k", "MNI152NLin6Asym"), 2),
(("fsaverage:den-10k:den-30k", "MNI152NLin6Asym:res-1:res-2"), 4),
@pytest.mark.parametrize("spaces, expected", [
(("MNI152NLin6Asym",), ("MNI152NLin6Asym:res-native",)),
(("fsaverage:den-10k", "MNI152NLin6Asym"), (
"fsaverage:den-10k", "MNI152NLin6Asym:res-native"
)),
(("fsaverage:den-10k:den-30k", "MNI152NLin6Asym:res-1:res-2"), (
"fsaverage:den-10k", "fsaverage:den-30k",
"MNI152NLin6Asym:res-1", "MNI152NLin6Asym:res-2"
)),
(("fsaverage:den-10k:den-30k", "MNI152NLin6Asym:res-1:res-2", "fsaverage5"), (
"fsaverage:den-10k", "fsaverage:den-30k",
"MNI152NLin6Asym:res-1", "MNI152NLin6Asym:res-2"
)),
(("fsaverage:den-10k:den-30k", "MNI152NLin6Asym:res-1:res-2",
"fsaverage5"), 4),
(("fsaverage:den-10k:den-30k", "MNI152NLin6Asym:res-1:res-2",
"fsaverage:den-10k:den-30k", "MNI152NLin6Asym:res-1:res-2"), 4),
"fsaverage:den-10k:den-30k", "MNI152NLin6Asym:res-1:res-2"), (
"fsaverage:den-10k", "fsaverage:den-30k",
"MNI152NLin6Asym:res-1", "MNI152NLin6Asym:res-2"
)),
(("MNI152NLin6Asym", "func"), ("MNI152NLin6Asym:res-native", "func"))
])
def test_space_action(parser, spaces, expected):
"""Test action."""
Expand All @@ -33,7 +44,9 @@ def test_space_action(parser, spaces, expected):
assert isinstance(parsed_spaces, SpatialReferences)
assert all(isinstance(sp, Reference) for sp in parsed_spaces.references), \
"Every element must be a `Reference`"
assert len(parsed_spaces.references) == expected
assert len(parsed_spaces.references) == len(expected)
for ref, expected_ref in zip(parsed_spaces.references, expected):
assert str(ref) == expected_ref


@pytest.mark.parametrize("flag,expected", [(('--spaces',), True), (None, False)])
Expand Down