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

fepegar · 2018-02-28T18:35:59Z

Note: I ran make check-before-commit and got the following:

(nipype)  nipype   add-interp-to-niftyreg-regtools  make check-before-commit                                                                                           ◂◂◂◂◂◃◃◃◃◃
Checking specs and autogenerating spec tests
env PYTHONPATH=".:" python tools/checkspecs.py
Traceback (most recent call last):
  File "tools/checkspecs.py", line 15, in <module>
    from yapf.yapflib.yapf_api import FormatCode
ModuleNotFoundError: No module named 'yapf'
make: *** [specs] Error 1

I fixed it with pip install yapf.

mgxd

Thanks for the PR! Just a single comment

mgxd · 2018-02-28T19:09:00Z

nipype/interfaces/niftyreg/regutils.py

+        'CUB',
+        'SINC',
+        desc='Interpolation order to use to warp the floating image',
+        argstr='-interp %d')


you'll want to change this to %s

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?

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)

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}
?

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}

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.

codecov-io · 2018-02-28T19:42:34Z

Codecov Report

Merging #2471 into master will decrease coverage by <.01%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master    #2471      +/-   ##
==========================================
- Coverage   66.66%   66.66%   -0.01%     
==========================================
  Files         328      328              
  Lines       42551    42557       +6     
  Branches     5276     5277       +1     
==========================================
+ Hits        28366    28369       +3     
- Misses      13505    13507       +2     
- Partials      680      681       +1

Flag	Coverage Δ
#smoketests	`50.76% <28.57%> (-0.01%)`	⬇️
#unittests	`63.98% <57.14%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/niftyreg/regutils.py	`80.86% <57.14%> (-0.92%)`	⬇️

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 6ca791d...e34c662. Read the comment docs.

effigies · 2018-02-28T19:58:43Z

nipype/interfaces/niftyreg/regutils.py

+    # 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}


Can you go ahead and change 5 to 4 here and in RegResample?

fepegar · 2018-02-28T19:59:17Z

I've added what @effigies commented. Waiting for @mmodat to see if Enum values need to be modified.

According to NiftyReg source code: https://cmiclab.cs.ucl.ac.uk/mmodat/niftyreg/blob/master/reg-apps/reg_resample.cpp#L65

effigies · 2018-02-28T20:09:36Z

Can you make specs and add the auto test?

fepegar · 2018-02-28T20:25:27Z

@effigies I don't know what those are. Could you please point me to an example or reference?

effigies · 2018-02-28T20:32:17Z

In the source directory, run: make specs. This should update the following files:

Add and commit those.

fepegar · 2018-02-28T22:58:03Z

Done, but only test_auto_RegTools.py was updated.

Btw make specs worked only on Python 2.

effigies · 2018-02-28T23:02:03Z

Yup, you're right. We didn't change the spec on the other one.

make specs should work on both (#1813). What issue did you have with Python 3?

fepegar · 2018-02-28T23:04:45Z

(nipype)  nipype   add-interp-to-niftyreg-regtools  make specs                                                                                                         ◂◂◂◂◃◃◃◃◃◃
Checking specs and autogenerating spec tests
env PYTHONPATH=".:" python tools/checkspecs.py
Traceback (most recent call last):
  File "tools/checkspecs.py", line 14, in <module>
    from nipype.interfaces.base import BaseInterface
  File "/Users/fernando/git/nipype/nipype/__init__.py", line 12, in <module>
    from .utils.config import NipypeConfig
  File "/Users/fernando/git/nipype/nipype/utils/__init__.py", line 4, in <module>
    from .config import NUMPY_MMAP
  File "/Users/fernando/git/nipype/nipype/utils/config.py", line 25, in <module>
    from future import standard_library
ModuleNotFoundError: No module named 'future'
make: *** [specs] Error 1

effigies · 2018-02-28T23:52:39Z

Ah. That can be resolved with pip3 install future.

fepegar · 2018-03-01T09:05:35Z

In that case, shouldn't yapf and future be included the requirements?

satra · 2018-03-01T12:31:40Z

@fepegar - this https://github.com/nipy/nipype/blob/master/nipype/info.py#L134 clarifies how bits and pieces are installed. the basic install is intended for running, not developing nipype.

pip install nipype should always install future since that is a requirement (if you do python setup.py install, then it depends on your version of setuptools, how the dependencies are installed).

pip install nipype[tests] should install some additional bits for tests
pip install nipype[specs] should install all the pieces required for make specs.
pip install nipype[all] will install everything

we hope to clarify user/developer documentation on testing in the next update (end of march).

fepegar · 2018-03-01T14:03:55Z

I'd expect anything needed by make specs to be installed when I run python setup.py develop.

effigies · 2018-03-01T14:35:50Z

We should probably update that recommendation. Everything I've read for a few years has recommended using pip over running setup.py directly.

satra · 2018-03-01T14:52:05Z

@fepegar - unfortunately that's not how python setup.py develop works. there is no standard way to install the dev modules.

@effigies - we can create a dev option for extras-requires and then update docs to do: pip install -e .[dev] . perhaps dev should include tests, specs, and doc building, but no external dependencies - nipy/dipy.

fepegar added 2 commits February 28, 2018 18:16

Add interpolation order parameter

ffccbc6

Change my affiliation

c2e9295

mgxd reviewed Feb 28, 2018

View reviewed changes

Add _format_arg overload to RegTools NiftyRegCommand

d55716a

effigies reviewed Feb 28, 2018

View reviewed changes

Fix interp enum value for RegResample and RegTools

1468583

According to NiftyReg source code: https://cmiclab.cs.ucl.ac.uk/mmodat/niftyreg/blob/master/reg-apps/reg_resample.cpp#L65

Run make specs for RegTools

e34c662

effigies approved these changes Mar 1, 2018

View reviewed changes

effigies added this to the 1.0.2 milestone Mar 1, 2018

effigies merged commit e679215 into nipy:master Mar 1, 2018

effigies mentioned this pull request Mar 1, 2018

update testing docs #2445

Closed

effigies mentioned this pull request Mar 1, 2018

MAINT: Add dev install option, update CONTRIBUTING #2477

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fepegar commented Feb 28, 2018

mgxd left a comment

mgxd Feb 28, 2018

fepegar Feb 28, 2018

effigies Feb 28, 2018

fepegar Feb 28, 2018

mgxd Feb 28, 2018

effigies Feb 28, 2018

codecov-io commented Feb 28, 2018 •

edited

Loading

effigies Feb 28, 2018

fepegar Feb 28, 2018

fepegar commented Feb 28, 2018

effigies commented Feb 28, 2018

fepegar commented Feb 28, 2018

effigies commented Feb 28, 2018

fepegar commented Feb 28, 2018

effigies commented Feb 28, 2018

fepegar commented Feb 28, 2018

effigies commented Feb 28, 2018

fepegar commented Mar 1, 2018 •

edited

Loading

satra commented Mar 1, 2018

fepegar commented Mar 1, 2018

effigies commented Mar 1, 2018

satra commented Mar 1, 2018

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

	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)

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

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

Conversation

fepegar commented Feb 28, 2018

mgxd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 28, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fepegar commented Feb 28, 2018

effigies commented Feb 28, 2018

fepegar commented Feb 28, 2018

effigies commented Feb 28, 2018

fepegar commented Feb 28, 2018

effigies commented Feb 28, 2018

fepegar commented Feb 28, 2018

effigies commented Feb 28, 2018

fepegar commented Mar 1, 2018 • edited Loading

satra commented Mar 1, 2018

fepegar commented Mar 1, 2018

effigies commented Mar 1, 2018

satra commented Mar 1, 2018

codecov-io commented Feb 28, 2018 •

edited

Loading

fepegar commented Mar 1, 2018 •

edited

Loading