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

FIX: Make positional arguments to LaplacianThickness require previous argument #2848

Merged
merged 7 commits into from
Feb 15, 2019
14 changes: 9 additions & 5 deletions nipype/interfaces/ants/segmentation.py
Original file line number Diff line number Diff line change
Expand Up @@ -208,25 +208,29 @@ class LaplacianThicknessInputSpec(ANTSCommandInputSpec):
keep_extension=True,
hash_files=False)
smooth_param = traits.Float(
argstr='%f',
argstr='%s',
desc='Sigma of the Laplacian Recursive Image Filter (defaults to 1)',
position=4)
prior_thickness = traits.Float(
argstr='%f',
argstr='%s',
desc='Prior thickness (defaults to 500)',
requires=['smooth_param'],
position=5)
dT = traits.Float(
argstr='%f',
argstr='%s',
desc='Time delta used during integration (defaults to 0.01)',
requires=['prior_thickness'],
position=6)
sulcus_prior = traits.Float(
argstr='%f',
argstr='%s',
desc='Positive floating point number for sulcus prior. '
'Authors said that 0.15 might be a reasonable value',
requires=['dT'],
position=7)
tolerance = traits.Float(
argstr='%f',
argstr='%s',
desc='Tolerance to reach during optimization (defaults to 0.001)',
requires=['sulcus_prior'],
position=8)


Expand Down
59 changes: 59 additions & 0 deletions nipype/interfaces/ants/tests/test_segmentation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-
# vi: set ft=python sts=4 ts=4 sw=4 et:

from ..segmentation import LaplacianThickness
from .test_resampling import change_dir

import os
import pytest


@pytest.fixture()
def change_dir(request):
orig_dir = os.getcwd()
filepath = os.path.dirname(os.path.realpath(__file__))
datadir = os.path.realpath(os.path.join(filepath, '../../../testing/data'))
os.chdir(datadir)

def move2orig():
os.chdir(orig_dir)

request.addfinalizer(move2orig)


@pytest.fixture()
def create_lt():
lt = LaplacianThickness()
# we do not run, so I stick some not really proper files as input
lt.inputs.input_gm = 'diffusion_weighted.nii'
lt.inputs.input_wm = 'functional.nii'
return lt


def test_LaplacianThickness_defaults(change_dir, create_lt):
lt = create_lt
base_cmd = 'LaplacianThickness functional.nii diffusion_weighted.nii functional_thickness.nii'
assert lt.cmdline == base_cmd
lt.inputs.smooth_param = 4.5
assert lt.cmdline == base_cmd + " 4.5"
lt.inputs.prior_thickness = 5.9
assert lt.cmdline == base_cmd + " 4.5 5.9"


def test_LaplacianThickness_wrongargs(change_dir, create_lt):
lt = create_lt
lt.inputs.tolerance = 0.001
with pytest.raises(ValueError, match=r".* requires a value for input 'sulcus_prior' .*"):
lt.cmdline
lt.inputs.sulcus_prior = 0.15
with pytest.raises(ValueError, match=r".* requires a value for input 'dT' .*"):
lt.cmdline
lt.inputs.dT = 0.01
with pytest.raises(ValueError, match=r".* requires a value for input 'prior_thickness' .*"):
lt.cmdline
lt.inputs.prior_thickness = 5.9
with pytest.raises(ValueError, match=r".* requires a value for input 'smooth_param' .*"):
lt.cmdline
lt.inputs.smooth_param = 4.5
assert lt.cmdline == 'LaplacianThickness functional.nii diffusion_weighted.nii ' \
'functional_thickness.nii 4.5 5.9 0.01 0.15 0.001'
14 changes: 10 additions & 4 deletions nipype/interfaces/base/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,10 +202,16 @@ def _check_requires(self, spec, name, value):
for field in spec.requires
]
if any(values) and isdefined(value):
msg = ("%s requires a value for input '%s' because one of %s "
"is set. For a list of required inputs, see %s.help()" %
(self.__class__.__name__, name,
', '.join(spec.requires), self.__class__.__name__))
if len(values) > 1:
fmt = ("%s requires values for inputs %s because '%s' is set. "
"For a list of required inputs, see %s.help()")
else:
fmt = ("%s requires a value for input %s because '%s' is set. "
"For a list of required inputs, see %s.help()")
msg = fmt % (self.__class__.__name__,
', '.join("'%s'" % req for req in spec.requires),
name,
self.__class__.__name__)
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 pretty sure this logic was backwards. Does anybody want to double-check me?

Copy link
Member Author

Choose a reason for hiding this comment

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

eyeballed it -- seems to produce logically correct statement at this moment, so all is good ;)

raise ValueError(msg)

def _check_xor(self, spec, name, value):
Expand Down