BUG: Let elx::ConjugateGradientFRPR override ITK's FRPROptimizer
#1118
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`elx::ConjugateGradientFRPR` member functions GetValueAndDerivative and LineOptimize did originally override the corresponding virtual member functions of ITK's FRPROptimizer. This property was broken by a change in the signature of those ITK member functions: ITK commit InsightSoftwareConsortium/ITK@a088a95, "PERF: Changed to pass by reference for faster performance", 2008-04-23 This commit restores those "overrides", by adjusting the signature of the `elx::ConjugateGradientFRPR` member functions, in accordance with those ITK member functions. Bug found by macos-12/clang compiler warnings, like: > warning: 'elastix::ConjugateGradientFRPR::GetValueAndDerivative' hides overloaded virtual function [-Woverloaded-virtual] Note: These two `elx::ConjugateGradientFRPR` member functions don't seem to be tested at the CI, currently.
N-Dekker
commented
May 14, 2024
Comment on lines
-170
to
+171
| virtual void | ||
| GetValueAndDerivative(ParametersType p, double * val, ParametersType * xi); | ||
| void | ||
| GetValueAndDerivative(ParametersType & p, double * val, ParametersType * xi) override; |
There was a problem hiding this comment.
@mstaring @stefanklein This might be a serious bug fix, at least if ConjugateGradientFRPR is still a relevant optimizer. Because of an ITK update back in 2008, ConjugateGradientFRPR did not properly override ITK's FRPROptimizer member functions anymore. (Note that this functionality is untested anyway.)
|
While it may or may not be an important bug fix, I think it must be merged anyway! |
|
Agree!
From: Niels Dekker ***@***.***>
Sent: Tuesday, May 14, 2024 5:12 PM
To: SuperElastix/elastix ***@***.***>
Cc: Stefan Klein ***@***.***>; Mention ***@***.***>
Subject: Re: [SuperElastix/elastix] BUG: Let `elx::ConjugateGradientFRPR` override ITK's FRPROptimizer (PR #1118)
Waarschuwing: Deze e-mail is afkomstig van buiten de organisatie. Klik niet op links en open geen bijlagen, tenzij u de afzender herkent en weet dat de inhoud veilig is.
Caution: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
While it may or may not be an important bug fix, I think it must be merged anyway!
-
Reply to this email directly, view it on GitHub<#1118 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAF2LNLK4CD3OGB6KDFVMN3ZCISVPAVCNFSM6AAAAABHWJYO5GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJQGQ4TQMJYHE>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
N-Dekker
added a commit
that referenced
this pull request
May 16, 2024
Following ITK commit InsightSoftwareConsortium/ITK@c701410, June 14, 2011, which modified the signature of `ImageSource::SplitRequestedRegion`. Similar to pull request #1118 commit 34bdc3b "BUG: Let `elx::ConjugateGradientFRPR` override ITK's FRPROptimizer" Bug found by macos-12/clang `-Woverloaded-virtual` warnings.
N-Dekker
added a commit
that referenced
this pull request
May 16, 2024
Adjusted `ParabolicErodeDilateImageFilter::SplitRequestedRegion` to ensure that its signature matches `ImageSource::SplitRequestedRegion` (again). Following ITK commit InsightSoftwareConsortium/ITK@c701410, June 14, 2011, which modified the signature of `ImageSource::SplitRequestedRegion`, by replacing signed with unsigned integers. Similar to pull request #1118 commit 34bdc3b "BUG: Let `elx::ConjugateGradientFRPR` override ITK's FRPROptimizer" Note: This commit changes the behavior (restoring the originally intended behavior), because `ParabolicErodeDilateImageFilter::SplitRequestedRegion` does avoid the current dimension (`m_CurrentDimension`), when setting the `splitAxis`. Bug found by macos-12/clang `-Woverloaded-virtual` warnings.
N-Dekker
added a commit
that referenced
this pull request
May 17, 2024
Adjusted `ParabolicErodeDilateImageFilter::SplitRequestedRegion` to ensure that its signature matches `ImageSource::SplitRequestedRegion` (again). Following ITK commit InsightSoftwareConsortium/ITK@c701410, June 14, 2011, which modified the signature of `ImageSource::SplitRequestedRegion`, by replacing signed with unsigned integers. Similar to pull request #1118 commit 34bdc3b "BUG: Let `elx::ConjugateGradientFRPR` override ITK's FRPROptimizer" Note: This commit changes the behavior (restoring the originally intended behavior), because `ParabolicErodeDilateImageFilter::SplitRequestedRegion` does avoid the current dimension (`m_CurrentDimension`), when setting the `splitAxis`. Bug found by macos-12/clang `-Woverloaded-virtual` warnings.
This was referenced May 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
elx::ConjugateGradientFRPRmember functions GetValueAndDerivative and LineOptimize did originally override the corresponding virtual member functions of ITK's FRPROptimizer. This property was broken by a change in the signature of those ITK member functions: ITK commit InsightSoftwareConsortium/ITK@a088a95, "PERF: Changed to pass by reference for faster performance", 2008-04-23This commit restores those "overrides", by adjusting the signature of the
elx::ConjugateGradientFRPRmember functions, in accordance with those ITK member functions.Bug found by macos-12/clang compiler warnings, like:
Note: These two
elx::ConjugateGradientFRPRmember functions don't seem to be tested at the CI, currently.