-
-
Notifications
You must be signed in to change notification settings - Fork 380
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
WIP: Remove unused estimate learning rate at each iteration. #1411
Conversation
Thanks @ntustison ! I think this means we should also remove this option from antsMotionCorr.cxx, which also uses a conjugate gradient optimizer https://github.com/ANTsX/ANTs/blob/master/Examples/antsMotionCorr.cxx#L131 |
Examples/antsRegistration.cxx
Outdated
@@ -193,7 +193,6 @@ antsRegistrationInitializeCommandLineOptions(itk::ants::CommandLineParser * pars | |||
option->SetShortName('z'); | |||
option->SetUsageOption(0, "(1)/0"); | |||
option->SetDescription(description); | |||
option->AddFunction(std::string("1")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should have been removed, I get a seg fault now unless I specify --collapse-output-transforms 1
. The argument handling is inconsistent and probably needs to be refactored, but some args (like this) have the convention that a default is assumed. In others, there is a check for the number of functions and only if > 0 is the value queried.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I this going to impact ANTsR and ANTsPy? I believe these do set the option internally.
Thanks for trying this. Can you tell me exactly the command you're running because I can't reproduce? |
bash -c "../install/bin/antsRegistration --verbose 1 --dimensionality 3 --float 0 --output [ test,testWarped.nii.gz,testInverseWarped.nii.gz ] --interpolation Linear --use-histogram-matching 0 --winsorize-image-intensities [ 0.005,0.995 ] --initial-moving-transform [ tpl-MNI152NLin2009cAsym_res-01_T1w.nii.gz,T1w.nii.gz,1 ] --transform Rigid[ 0.1 ] --metric MI[ tpl-MNI152NLin2009cAsym_res-01_T1w.nii.gz,T1w.nii.gz,1,32,Regular,0.25 ] --convergence [ 100x50x25x10,1e-6,10 ] --shrink-factors 8x4x2x1 --smoothing-sigmas 3x2x1x0vox --transform Affine[ 0.1 ] --metric MI[ tpl-MNI152NLin2009cAsym_res-01_T1w.nii.gz,T1w.nii.gz,1,32,Regular,0.25 ] --convergence [ 100x50x25x10,1e-6,10 ] --shrink-factors 8x4x2x1 --smoothing-sigmas 3x2x1x0vox --transform SyN[ 0.1,3,0 ] --metric CC[ tpl-MNI152NLin2009cAsym_res-01_T1w.nii.gz,T1w.nii.gz,1,4 ] --convergence [ 10x7x5x0,1e-6,10 ] --shrink-factors 8x4x2x1 --smoothing-sigmas 3x2x1x0vox" This doesn't work, but it does work if I add |
Thanks @cookpa , I'll look at it in a few minutes. An alternative is to just add documentation to the option. I'm fine with whatever. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for me
Also, I'll take a look at the other programs (eg, antsMotionCorr). |
Thanks for sorting this @ntustison |
@ntustison Do you know when "we switched to the conjugate gradient descent optimizers for everything but the exponential transforms"? For nipype wrapping, it would be good to know at what point this had an effect. Edit: From a quick git-blame, it may have been around 5bedd97, which was released in ANTs 2.0. |
I think it was 2013, here |
Description
I'm guessing this is a legacy option before we switched to the conjugate gradient descent optimizers for everything but the exponential transforms. But even those don't use this flag as no scales estimator is applied in these two cases (B-spline and Gaussian).
Addresses #1405