Skip to content

Commit

Permalink
BUG: Fix ConjugateGradientFRPR overrides PowellOptimizer, FRPROptimizer
Browse files Browse the repository at this point in the history
There was a breaking change in the API of `itk::PowellOptimizer` and `itk::FRPROptimizer`, with ITK commit InsightSoftwareConsortium/ITK@a7b0a73 "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 1c64e2a, Stefan Klein, 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
  • Loading branch information
N-Dekker committed May 18, 2024
1 parent 93716a6 commit 33c3873
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,8 @@ class ITK_TEMPLATE_EXPORT ConjugateGradientFRPR
* superclass' implementation, stores bx as the current step length,
* invokes an iteration event, and sets the LineBracketing flag to 'false' */
void
LineBracket(double * ax, double * bx, double * cx, double * fa, double * fb, double * fc) override;
LineBracket(double * ax, double * bx, double * cx, double * fa, double * fb, double * fc, ParametersType & tempCoord)
override;

/** Given a bracketing triple of points and their function values, returns
* a bounded extreme. These values are in parameter space, along the
Expand All @@ -193,21 +194,22 @@ class ITK_TEMPLATE_EXPORT ConjugateGradientFRPR
* the superclass's implementation, stores extX as the current step length,
* and sets the LineOptimizing flag to 'false' again. */
void
BracketedLineOptimize(double ax,
double bx,
double cx,
double fa,
double fb,
double fc,
double * extX,
double * extVal) override;
BracketedLineOptimize(double ax,
double bx,
double cx,
double fa,
double fb,
double fc,
double * extX,
double * extVal,
ParametersType & tempCoord) override;

/**
* store the line search direction's (xi) magnitude and call the superclass'
* implementation.
*/
void
LineOptimize(ParametersType * p, ParametersType & xi, double * val) override;
LineOptimize(ParametersType * p, ParametersType & xi, double * val, ParametersType & tempCoord) override;

private:
elxOverrideGetSelfMacro;
Expand All @@ -217,6 +219,12 @@ class ITK_TEMPLATE_EXPORT ConjugateGradientFRPR

const char *
DeterminePhase() const;

// Private using-declarations, to avoid accidentally calling the wrong overload of those member functions, and to
// avoid `-Woverloaded-virtual` warnings from GCC (GCC 11.4) or clang (macos-12).
using itk::PowellOptimizer::LineBracket;
using itk::PowellOptimizer::BracketedLineOptimize;
using itk::FRPROptimizer::LineOptimize;
};

} // end namespace elastix
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,19 +307,20 @@ ConjugateGradientFRPR<TElastix>::GetValueAndDerivative(ParametersType & p, doubl

template <class TElastix>
void
ConjugateGradientFRPR<TElastix>::LineBracket(double * ax,
double * bx,
double * cx,
double * fa,
double * fb,
double * fc)
ConjugateGradientFRPR<TElastix>::LineBracket(double * ax,
double * bx,
double * cx,
double * fa,
double * fb,
double * fc,
ParametersType & tempCoord)
{
/** This implementation sets the LineBracketing flag to 'true', calls the
* superclass' implementation, remembers the current step length (bx), invokes
* an iteration event, and sets the LineBracketing flag to 'false' */

this->SetLineBracketing(true);
this->Superclass1::LineBracket(ax, bx, cx, fa, fb, fc);
this->Superclass1::LineBracket(ax, bx, cx, fa, fb, fc, tempCoord);
this->m_CurrentStepLength = *bx;
this->InvokeEvent(itk::IterationEvent());
this->SetLineBracketing(false);
Expand All @@ -333,21 +334,22 @@ ConjugateGradientFRPR<TElastix>::LineBracket(double * ax,

template <class TElastix>
void
ConjugateGradientFRPR<TElastix>::BracketedLineOptimize(double ax,
double bx,
double cx,
double fa,
double fb,
double fc,
double * extX,
double * extVal)
ConjugateGradientFRPR<TElastix>::BracketedLineOptimize(double ax,
double bx,
double cx,
double fa,
double fb,
double fc,
double * extX,
double * extVal,
ParametersType & tempCoord)
{
/** This implementation sets the LineOptimizing flag to 'true', calls the
* the superclass's implementation, remembers the resulting step length,
* and sets the LineOptimizing flag to 'false' again. */

this->SetLineOptimizing(true);
this->Superclass1::BracketedLineOptimize(ax, bx, cx, fa, fb, fc, extX, extVal);
this->Superclass1::BracketedLineOptimize(ax, bx, cx, fa, fb, fc, extX, extVal, tempCoord);
this->m_CurrentStepLength = *extX;
this->SetLineOptimizing(false);

Expand All @@ -361,10 +363,13 @@ ConjugateGradientFRPR<TElastix>::BracketedLineOptimize(double ax,
*/
template <class TElastix>
void
ConjugateGradientFRPR<TElastix>::LineOptimize(ParametersType * p, ParametersType & xi, double * val)
ConjugateGradientFRPR<TElastix>::LineOptimize(ParametersType * p,
ParametersType & xi,
double * val,
ParametersType & tempCoord)
{
this->m_CurrentSearchDirectionMagnitude = xi.magnitude();
this->Superclass1::LineOptimize(p, xi, val);
this->Superclass1::LineOptimize(p, xi, val, tempCoord);
} // end LineOptimize


Expand Down

0 comments on commit 33c3873

Please sign in to comment.