Skip to content

FIX: Un-set incorrect default options in TOPUP #2637

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
effigies merged 3 commits into from
Jul 27, 2018
+16 −42
Merged

FIX: Un-set incorrect default options in TOPUP #2637

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e6467f
fixing argstr for reg_lambda input in TOPUP
djarecka Jul 13, 2018
ec31d97
removing default values from topup (providing FSL default values in t…
djarecka Jul 13, 2018
a0a602c
removing misleading info about the default values
djarecka Jul 27, 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
31 changes: 12 additions & 19 deletions nipype/interfaces/fsl/epi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,32 +199,28 @@ class TOPUPInputSpec(FSLCommandInputSpec):
# TODO: the following traits admit values separated by commas, one value
# per registration level inside topup.
warp_res = traits.Float(
10.0, usedefault=True,
argstr='--warpres=%f',
desc=('(approximate) resolution (in mm) of warp '
'basis for the different sub-sampling levels'
'.'))
subsamp = traits.Int(1, usedefault=True,
argstr='--subsamp=%d', desc='sub-sampling scheme')
'(default in FSL 10).'))
Copy link
Member

@satra satra Jul 19, 2018

Choose a reason for hiding this comment

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

what are FSL 10, FSL 1, and FSL 8?

Copy link
Collaborator Author

@djarecka djarecka Jul 19, 2018

Choose a reason for hiding this comment

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

when I'm reading this today it's not clear indeed... I meant that in (a version of) FSL the default value for this arg is 10.
The story is that some months ago I added usedefault=True to places where default value of traits was chosen, but this changed the output of some workflows and we got complains about it, so I thought that we can just keep the FSL defaults and only inform about these values (but in the description, not in the trait's default). I'm open to other suggestions, I'm really not sure what is the best strategy.

Copy link
Member

@effigies effigies Jul 26, 2018

Choose a reason for hiding this comment

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

Isn't the problem that the documented defaults and actual defaults differ, and thus passing these "default" values changes the behavior of the interface? If so, I think adding this description will confuse users.

I think I would, for now, make a comment in the interface docstring such as:

Some default values provided in the TOPUP documentation appear to differ from
the defaults used when no argument is provided.
For reference we provide the following list.
If an empirical default value is known, it is accurate as of version <VERSION>.

======  ===========  =========
Option   Documented  Empirical
======  ===========  =========
``warp_res``  10     <UNKNOWN>
``othervar``   9      8
...
======  ===========  =========

Copy link
Collaborator Author

@djarecka djarecka Jul 26, 2018

Choose a reason for hiding this comment

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

@effigies - yes this is probably the main problem, some flags shouldn't be used at all as default. I will review this later and add the table you suggested. It is extra work for us to keep this updated, but it might be the only solution that make sense...

This is not the only interface that might have similar problems (at least not the only one that possibly was changed after my PR with usedefault=True)

Copy link
Member

@effigies effigies Jul 26, 2018

Choose a reason for hiding this comment

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

Maybe put a big warning that the values may be out-of-date? We're not going to be able to regularly update it, but letting people know there's an issue worth looking into and what we knew at this point seems like the best we can do.

subsamp = traits.Int(argstr='--subsamp=%d',
desc='sub-sampling scheme (default in FSL 1)')
fwhm = traits.Float(
8.0,
usedefault=True,
argstr='--fwhm=%f',
desc='FWHM (in mm) of gaussian smoothing kernel')
desc='FWHM (in mm) of gaussian smoothing kernel (default in FSL 8)')
config = traits.String(
'b02b0.cnf',
argstr='--config=%s',
usedefault=True,
desc=('Name of config file specifying command line '
'arguments'))
max_iter = traits.Int(
5, usedefault=True,
argstr='--miter=%d', desc='max # of non-linear iterations')
argstr='--miter=%d',
desc='max # of non-linear iterations (default in FSL 5)')
reg_lambda = traits.Float(
1.0, usedefault=True,
argstr='--miter=%0.f',
desc=('lambda weighting value of the '
'regularisation term'))
argstr='--lambda=%0.f',
desc=('Weight of regularisation, default '
'depending on --ssqlambda and --regmod switches.'))
ssqlambda = traits.Enum(
1,
0,
Expand Down Expand Up @@ -259,10 +255,9 @@ class TOPUPInputSpec(FSLCommandInputSpec):
desc=('Minimisation method 0=Levenberg-Marquardt, '
'1=Scaled Conjugate Gradient'))
splineorder = traits.Int(
3, usedefault=True,
argstr='--splineorder=%d',
desc=('order of spline, 2->Qadratic spline, '
'3->Cubic spline'))
'3->Cubic spline (default in FSL 3)'))
numprec = traits.Enum(
'double',
'float',
Expand Down Expand Up @@ -321,11 +316,9 @@ class TOPUP(FSLCommand):
>>> topup.inputs.output_type = "NIFTI_GZ"
>>> topup.cmdline # doctest: +ELLIPSIS
'topup --config=b02b0.cnf --datain=topup_encoding.txt \
--fwhm=8.000000 --imain=b0_b0rev.nii --miter=5 \
--out=b0_b0rev_base --iout=b0_b0rev_corrected.nii.gz \
--imain=b0_b0rev.nii --out=b0_b0rev_base --iout=b0_b0rev_corrected.nii.gz \
--fout=b0_b0rev_field.nii.gz --jacout=jac --logout=b0_b0rev_topup.log \
--rbmout=xfm --dfout=warpfield --miter=1 --splineorder=3 --subsamp=1 \
--warpres=10.000000'
--rbmout=xfm --dfout=warpfield'
>>> res = topup.run() # doctest: +SKIP

"""
Expand Down
30 changes: 6 additions & 24 deletions nipype/interfaces/fsl/tests/test_auto_TOPUP.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,13 @@ def test_TOPUP_inputs():
usedefault=True,
),
estmov=dict(argstr='--estmov=%d', ),
fwhm=dict(
argstr='--fwhm=%f',
usedefault=True,
),
fwhm=dict(argstr='--fwhm=%f', ),
in_file=dict(
argstr='--imain=%s',
mandatory=True,
),
interp=dict(argstr='--interp=%s', ),
max_iter=dict(
argstr='--miter=%d',
usedefault=True,
),
max_iter=dict(argstr='--miter=%d', ),
minmet=dict(argstr='--minmet=%d', ),
numprec=dict(argstr='--numprec=%s', ),
out_base=dict(
Expand Down Expand Up @@ -87,26 +81,14 @@ def test_TOPUP_inputs():
requires=['encoding_direction'],
xor=['encoding_file'],
),
reg_lambda=dict(
argstr='--miter=%0.f',
usedefault=True,
),
reg_lambda=dict(argstr='--lambda=%0.f', ),
regmod=dict(argstr='--regmod=%s', ),
regrid=dict(argstr='--regrid=%d', ),
scale=dict(argstr='--scale=%d', ),
splineorder=dict(
argstr='--splineorder=%d',
usedefault=True,
),
splineorder=dict(argstr='--splineorder=%d', ),
ssqlambda=dict(argstr='--ssqlambda=%d', ),
subsamp=dict(
argstr='--subsamp=%d',
usedefault=True,
),
warp_res=dict(
argstr='--warpres=%f',
usedefault=True,
),
subsamp=dict(argstr='--subsamp=%d', ),
warp_res=dict(argstr='--warpres=%f', ),
)
inputs = TOPUP.input_spec()

Expand Down