Skip to content

Positional arguments which mandate all previous ones to be present #2847

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

Closed
yarikoptic opened this issue Jan 11, 2019 · 0 comments · Fixed by #2848
Closed

Positional arguments which mandate all previous ones to be present #2847

yarikoptic opened this issue Jan 11, 2019 · 0 comments · Fixed by #2848
Milestone

Comments

@yarikoptic
Copy link
Member

Summary

This is a supplement to #2846 fixing up ANTs LaplacianSmoothness interface. The main issue is that the cmdline call description

$> LaplacianThickness 
Usage:   LaplacianThickness WM.nii GM.nii   Out.nii  {smoothparam=3} {priorthickval=5}  {dT=0.01}  use-sulcus-prior optional-laplacian-tolerance=0.001

is not saying that smoothparam is a keyword to use on the command line -- it is just a "description" for that positional field. So if I wanted to just change the optional-laplacian-tolerance to 0.999 (and assuming that all those =value correspond to defaults I would need to call

LaplacianThickness WM.nii GM.nii   Out.nii 3 5 0.01 0 0.999

Actual behavior

ATM, Nipype interface, if I just specify \.opt_tolerance=0.999 (and none of the other prior optional positional arguments) then it would produce (do not have a nipype test, only an integration test result observation):

LaplacianThickness WM.nii GM.nii   Out.nii 0.999

which would place that 0.999 into where ANTs expects (and consumes) the smoothparam.

Expected behavior

If nipype to not carry the defaults for all those fields (which might change etc within ANTs, so not a good idea I guess), then at least I expect it to blow stating that specification of some later positional argument requires specification (or a default value) for all preceding it arguments.

Here is the unittest on top of #2846

def test_LaplacianThickness_wrongargs(change_dir, create_lt):
    lt = create_lt
    lt.inputs.prior_thickness = 5.9
    # 500 must not be placed as smooth_param
    assert lt.cmdline != 'LaplacianThickness functional.nii diffusion_weighted.nii functional_thickness.nii 5.9'
    # probably should have just raised an exception that "smooth_param"
    # should also be defined

which currently would fail!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants