Skip to content

ENH: Add interpolation order parameter to NiftyReg's RegTools #2471

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

Merged
merged 5 commits into from
Mar 1, 2018
Merged

ENH: Add interpolation order parameter to NiftyReg's RegTools #2471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ffccbc6
Add interpolation order parameter
fepegar Feb 28, 2018
c2e9295
Change my affiliation
fepegar Feb 28, 2018
d55716a
Add _format_arg overload to RegTools NiftyRegCommand
fepegar Feb 28, 2018
1468583
Fix interp enum value for RegResample and RegTools
fepegar Feb 28, 2018
e34c662
Run make specs for RegTools
fepegar Feb 28, 2018
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
2 changes: 1 addition & 1 deletion .zenodo.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@
"name": "Flandin, Guillaume"
},
{
"affiliation": "Stereotaxy Core, Brain & Spine Institute",
"affiliation": "University College London",
"name": "P\u00e9rez-Garc\u00eda, Fernando",
"orcid": "0000-0001-9090-3024"
},
Expand Down
19 changes: 18 additions & 1 deletion nipype/interfaces/niftyreg/regutils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ class RegResample(NiftyRegCommand):
# Need this overload to properly constraint the interpolation type input
def _format_arg(self, name, spec, value):
if name == 'inter_val':
inter_val = {'NN': 0, 'LIN': 1, 'CUB': 3, 'SINC': 5}
inter_val = {'NN': 0, 'LIN': 1, 'CUB': 3, 'SINC': 4}
return spec.argstr % inter_val[value]
else:
return super(RegResample, self)._format_arg(name, spec, value)
Expand Down Expand Up @@ -295,6 +295,15 @@ class RegToolsInputSpec(NiftyRegCommandInputSpec):
desc=desc,
argstr='-smoG %f %f %f')

# Interpolation type
inter_val = traits.Enum(
'NN',
'LIN',
'CUB',
'SINC',
desc='Interpolation order to use to warp the floating image',
argstr='-interp %d')
Copy link
Member

Choose a reason for hiding this comment

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

you'll want to change this to %s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I've checked the source code and this parameter (of the command line tool) expects 0, 1, 3 or 4, as in RegResample:

nipype/nipype/interfaces/niftyreg/regutils.py

Lines 61 to 68 in 045b28e

# Interpolation type
inter_val = traits.Enum(
'NN',
'LIN',
'CUB',
'SINC',
desc='Interpolation type',
argstr='-inter %d')

So I basically copied that section from RegResample. Should anything be modified for both interfaces? I don't know much about traits, but it feels like the current code can only produce 0, 1, 2 or 3.

Also, I'm a bit new to PRs. To change something, should I just commit in my local fork and push to the same branch normally?

Copy link
Member

Choose a reason for hiding this comment

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

You'll need to copy this part as well:

nipype/nipype/interfaces/niftyreg/regutils.py

Lines 123 to 128 in 045b28e

def _format_arg(self, name, spec, value):
if name == 'inter_val':
inter_val = {'NN': 0, 'LIN': 1, 'CUB': 3, 'SINC': 5}
return spec.argstr % inter_val[value]
else:
return super(RegResample, self)._format_arg(name, spec, value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Although shouldn't line 124 be
inter_val = {'NN': 0, 'LIN': 1, 'CUB': 3, 'SINC': 4}
instead of
inter_val = {'NN': 0, 'LIN': 1, 'CUB': 3, 'SINC': 5}
?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why 2 and 4 are skipped, perhaps @mmodat has some input.

# If change is needed, it should look something like this
inter_val = {'NN': 0, 'LIN': 1, 'CUB': 2, 'SINC': 3}

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it. https://cmiclab.cs.ucl.ac.uk/mmodat/niftyreg/blob/master/reg-apps/reg_resample.cpp#L65

Their documentation could certainly be clearer on the point.



class RegToolsOutputSpec(TraitedSpec):
""" Output Spec for RegTools. """
Expand Down Expand Up @@ -326,6 +335,14 @@ class RegTools(NiftyRegCommand):
output_spec = RegToolsOutputSpec
_suffix = '_tools'

# Need this overload to properly constraint the interpolation type input
def _format_arg(self, name, spec, value):
if name == 'inter_val':
inter_val = {'NN': 0, 'LIN': 1, 'CUB': 3, 'SINC': 4}
return spec.argstr % inter_val[value]
else:
return super(RegTools, self)._format_arg(name, spec, value)


class RegAverageInputSpec(NiftyRegCommandInputSpec):
""" Input Spec for RegAverage. """
Expand Down
1 change: 1 addition & 0 deletions nipype/interfaces/niftyreg/tests/test_auto_RegTools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ def test_RegTools_inputs():
argstr='-in %s',
mandatory=True,
),
inter_val=dict(argstr='-interp %d', ),
iso_flag=dict(argstr='-iso', ),
mask_file=dict(argstr='-nan %s', ),
mul_val=dict(argstr='-mul %s', ),
Expand Down