search

SuperElastix / elastix

Official elastix repository
http://elastix.dev
Apache License 2.0
449 stars 110 forks source link

BUG: Fix ConjugateGradientFRPR overrides PowellOptimizer, FRPROptimizer #1123

Closed N-Dekker closed 1 month ago

N-Dekker commented 1 month ago

There was a breaking change in the API of itk::PowellOptimizer and itk::FRPROptimizer, with ITK commit https://github.com/InsightSoftwareConsortium/ITK/commit/a7b0a734cf79ac82e70a61321b41ccfee4d5feec "ENH: Eliminates variable construct/destruct", Stephen R. Aylward, April 23, 2008: It added overloads of the virtual member functions, itk::PowellOptimizer::LineBracket, itk::PowellOptimizer::BracketedLineOptimize, and itk::FRPROptimizer::LineOptimize, which have an extra parameter, ParametersType & tempCoord. The old virtual member functions were then rewritten in terms of the corresponding new overloads.

As a consequence, it does not make sense anymore for elastix::ConjugateGradientFRPR to override the old virtual member functions of ITK. With this commit, it overrides the new overloads instead, with the extra ParametersType & tempCoord parameter.

This fix aims to restore the behavior of ConjugateGradientFRPR as it was intended. (ConjugateGradientFRPR was introduces with commit 1c64e2af588d0b996c4ddbe582d2a1c7a7c98dd0, Stefan (@stefanklein), 20 April 2006.)

The bug was found when trying to fix -Woverloaded-virtual warnings from GCC at https://my.cdash.org/index.php?project=elastix like:

itkFRPROptimizer.h:126:3: warning: 'virtual void itk::FRPROptimizer::LineOptimize(itk::FRPROptimizer::ParametersType, itk::FRPROptimizer::ParametersType&, double, itk::FRPROptimizer::ParametersType&)' was hidden

N-Dekker commented 1 month ago

@stefanklein Again (after pull request #1118) this looks like a major bug fix of ConjugateGradientFRPR (with the bug being introduced by an upgrade of ITK, back in 2008). It seems to me that this fix must be applied, in order to get the intended result from ConjugateGradientFRPR. But I guess this optimizer component is rarely used anyway, right?

You wrote about this component in the log message of commit 1c64e2af588d0b996c4ddbe582d2a1c7a7c98dd0 (20 April 2006):

Added the ConjugateGradientFRPR optimizer. It was already sitting in my elastix-tree for a while.This is a fully functional fletcher reeves conjugate gradient optimizer, using a dBrent exact line search, based on the description in Numerical Recipes in c++. It's not a really good optimizer, but it works, and is known. The exact line search might proof useful somewhere/somewhen.

Anyway, I guess it's OK to merge the fix.