WIP: Fix for itkGradientDescentOptimizerv4#2572
WIP: Fix for itkGradientDescentOptimizerv4#2572gregsharp wants to merge 1 commit intoInsightSoftwareConsortium:mainfrom
Conversation
Issue 2570. AdvanceOneStep() is now called prior to GetValueAndDerivative() rather than afterward. Tests in ITKOptimizersv4TestDriver have been updated to match the algorithmic change.
github-actions
Bot
added
area:Numerics
| // Verify that the optimizer stays at the initial position if | ||
| // number of iterations is set to one. | ||
| std::cout << "\nCheck the optimizer position (parameters) are equal to the initial " | ||
| "value when number of iterations is set to one:" |
There was a problem hiding this comment.
Shouldn't that be the case for 0 iterations? One iteration should update the parameters once?
| initialPosition[1] = -100; | ||
| metric->SetParameters(initialPosition); | ||
|
|
||
| itkOptimizer->SetNumberOfIterations(1); |
There was a problem hiding this comment.
The existing behavior is:
Zero iterations: No function evaluations performed
One iteration: One function evaluation, update position (Parameters)
The new behavior is:
Zero iterations: No function evaluations performed
One iteration: One function evaluation, do not update position (Parameters)
So it depends upon what is the definition of an iteration. It would be helpful to have documentation for what is supposed to happen.
There was a problem hiding this comment.
Thanks for doing thins @gregsharp. I have not experienced (consciously) the issue and have not tried the fix, but I trust it is much needed 💯. Thus my review is just about some superficial/stylistic aspects:
- Commit messages should adhere to the required style. Among others:
- A short subject/heading descriptive of the change (what does this fix?). Reading
Fix for itkGradientDescentOptimizerv4does not allow us to say what this fixes. Think that your commit message and PR will be read many, many times, and having a clear, self-contained body of explanation about the change is invaluable. - The linked Chris Beam's post on how to write good commit messages is an invaluable guide.
- Use the body of the message to briefly re-state the purpose of the PR and what it fixes: your description and expected and actual behavior passages in the issue are enlightening about it I'd say, and invaluable for anyone visiting the commit history (and this last one happens frequently). After that you can add what you have:
- A short subject/heading descriptive of the change (what does this fix?). Reading
`AdvanceOneStep()` is now called prior to `GetValueAndDerivative()` rather than afterward (add an explanation if the above is not enough).
Update related tests in the v4 Optimizers framework to match the algorithmic change.
Note that the Drivers (or TestDrivers) are just a mechanism of the testing framework, not the relevant name of the module.
- If this PR closes the cross-referenced issue, it can automatically be closed when this PR gets merged using any of the appropriate GitHub keys.
- Please check the appropriate checkboxes in the PR checklist. It allows maintainers and those requiring to see the history to know a little bit more about the changes. I'd say that the first four items apply to your PR.
- Please remove the PR's HTML markup instructions (if you feel like it should be written, a PR would be welcome 💯).
Another inline comment.
Additionally, and more related to the discourse post,
- As for the the fix due to the need to fork the repository and style formatting ([comment in discourse](
Anorther inline comment.)) due to omissions or assumptions inCONTRIBUTING.md, improvements under the form of PRs to it are more than welcome !
I ignore if that may have been related to not having executedSetupForDevelopment.shor this one not having worked. If that is the case, a fix would be welcome !
If the style is not auto-formatted locally, theclang-format linteraction will flag the style inconsistencies; and in therory adding theaction:ApplyClangFormatin a PR comment should auto-format the code, but there seems to be some issue with theGitHubpermissions (?) so that the auto-formatting can take place automatically by the action. - Brad's comments in the discourse thread (here and here)in the cross-referenced issue about the lack of better testing might be worthwhile considering, or be opened as issues so that they can be tracked, and hopefully addressed at some point.
Thanks for all the hard work in fixing this 💯.
| catch (const itk::ExceptionObject & e) | ||
| { | ||
| return EXIT_FAILURE; | ||
| } |
There was a problem hiding this comment.
The above try/catch block can be shortened using ITK_TRY_EXPECT_NO_EXCEPTION(itkOptimizer->StartOptimization());. It is preferred for the sake of compactness and to avoid boilerplate code.
I think this only works for PRs made from branches within ITK repo, which is not the way PRs are normally made in ITK. |
Sure. If the PR contains multiple commits, should I squash the work into a single commit and update the message at that time?
Sorry I don't understand.
Sorry, also I don't understand, Where is the HTML markup?
Even describing the current state of Optimizerv4 tests and making improvement suggestions in a ticket is a lot of work. But if you think it is useful I can try. It seems sufficiently independent of this ticket to be separate. Regarding the code formatting, none of the code I touched matches the style guide found in the ITK Software Guide (with regards to indenting, curly brace placement). So I just kind of manually adjusted things so they look the same as the existing code. Thank you for review! |
|
Thanks for considering the comments @gregsharp 💯.
Depends on the commits; if they serve to different purposes (e.g.
When you open the PR, there is a body of text inside HTML comment markup
Sounds reasonable. I did not mean that it should be done within this PR. If you feel like you do have a good understanding of what should be done to improve the current state following Brad's comments, opening an issue would be best. But I understand that that requires in itself a non-negligible amount of work.
The automatic style enforcing tool (i.e.
👍 |
|
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
stale
Bot
added
the
status:Use_Milestone_Backlog
|
@gregsharp — superseded by PR #6196. After deep-diving this draft I confirmed the bug you diagnosed in #2570 is real (a TDD regression GTest fails by ~4 score-units against a quadratic fixture on current Your authorship is preserved via cc @dzenanz @jhlegarreta — the new PR is ready for fresh review at #6196. Closing this draft as superseded; reopening would duplicate review work. |
Issue InsightSoftwareConsortium#2570: an observer attached to IterationEvent on GradientDescentOptimizerv4 (or its subclasses) was given an internally inconsistent (value, position) pair. The optimizer was firing the event at the end of AdvanceOneStep(), after UpdateTransformParameters() had already moved the position from X to X+step, while m_CurrentMetricValue still held v(X). Observers reading GetCurrentMetricValue() and GetCurrentPosition() saw value-at-old-position with the new position. Move InvokeEvent(IterationEvent()) into the loop in GradientDescentOptimizerv4Template::ResumeOptimization(), fired BEFORE AdvanceOneStep() so the reported (value, position) pair always agrees. Remove the now-redundant InvokeEvent at the end of each subclass's AdvanceOneStep override (RegularStep, QuasiNewton, GradientDescentLineSearch, ConjugateGradientLineSearch). Adds a GTest regression guard (itkGradientDescentOptimizerv4ObserverGTest.cxx) that fails on the unfixed code with a ~4-unit value/position mismatch on a quadratic fixture, and passes on the fixed code. Closes InsightSoftwareConsortium#2570. Supersedes the WIP draft InsightSoftwareConsortium#2572 by @gregsharp; the test demonstrates Greg's diagnosis is correct, the fix here is a simpler one-event-move that does not change iteration semantics. Co-Authored-By: Gregory C. Sharp <gregsharp.geo@yahoo.com>
Issue InsightSoftwareConsortium#2570: an observer attached to IterationEvent on GradientDescentOptimizerv4 (or its subclasses) was given an internally inconsistent (value, position) pair. The optimizer was firing the event at the end of AdvanceOneStep(), after UpdateTransformParameters() had already moved the position from X to X+step, while m_CurrentMetricValue still held v(X). Observers reading GetCurrentMetricValue() and GetCurrentPosition() saw value-at-old-position with the new position. Move InvokeEvent(IterationEvent()) into the loop in GradientDescentOptimizerv4Template::ResumeOptimization(), fired BEFORE AdvanceOneStep() so the reported (value, position) pair always agrees. Remove the now-redundant InvokeEvent at the end of each subclass's AdvanceOneStep override (RegularStep, QuasiNewton, GradientDescentLineSearch, ConjugateGradientLineSearch). Adds a GTest regression guard (itkGradientDescentOptimizerv4ObserverGTest.cxx) that fails on the unfixed code with a ~4-unit value/position mismatch on a quadratic fixture, and passes on the fixed code. Closes InsightSoftwareConsortium#2570. Supersedes the WIP draft InsightSoftwareConsortium#2572 by @gregsharp; the test demonstrates Greg's diagnosis is correct, the fix here is a simpler one-event-move that does not change iteration semantics. Co-Authored-By: Gregory C. Sharp <gregsharp.geo@yahoo.com>
Issue InsightSoftwareConsortium#2570: an observer attached to IterationEvent on GradientDescentOptimizerv4 (or its subclasses) was given an internally inconsistent (value, position) pair. The optimizer was firing the event at the end of AdvanceOneStep(), after UpdateTransformParameters() had already moved the position from X to X+step, while m_CurrentMetricValue still held v(X). Observers reading GetCurrentMetricValue() and GetCurrentPosition() saw value-at-old-position with the new position. Move InvokeEvent(IterationEvent()) into the loop in GradientDescentOptimizerv4Template::ResumeOptimization(), fired BEFORE AdvanceOneStep() so the reported (value, position) pair always agrees. Remove the now-redundant InvokeEvent at the end of each subclass's AdvanceOneStep override (RegularStep, QuasiNewton, GradientDescentLineSearch, ConjugateGradientLineSearch). Adds a GTest regression guard (itkGradientDescentOptimizerv4ObserverGTest.cxx) that fails on the unfixed code with a ~4-unit value/position mismatch on a quadratic fixture, and passes on the fixed code. Closes InsightSoftwareConsortium#2570. Supersedes the WIP draft InsightSoftwareConsortium#2572 by @gregsharp; the test demonstrates Greg's diagnosis is correct, the fix here is a simpler one-event-move that does not change iteration semantics. Co-Authored-By: Gregory C. Sharp <gregsharp.geo@yahoo.com>
Issue InsightSoftwareConsortium#2570: an observer attached to IterationEvent on GradientDescentOptimizerv4 (or its subclasses) was given an internally inconsistent (value, position) pair. The optimizer was firing the event at the end of AdvanceOneStep(), after UpdateTransformParameters() had already moved the position from X to X+step, while m_CurrentMetricValue still held v(X). Observers reading GetCurrentMetricValue() and GetCurrentPosition() saw value-at-old-position with the new position. Move InvokeEvent(IterationEvent()) into the loop in GradientDescentOptimizerv4Template::ResumeOptimization(), fired BEFORE AdvanceOneStep() so the reported (value, position) pair always agrees. Remove the now-redundant InvokeEvent at the end of each subclass's AdvanceOneStep override (RegularStep, QuasiNewton, GradientDescentLineSearch, ConjugateGradientLineSearch). Adds a GTest regression guard (itkGradientDescentOptimizerv4ObserverGTest.cxx) that fails on the unfixed code with a ~4-unit value/position mismatch on a quadratic fixture, and passes on the fixed code. Closes InsightSoftwareConsortium#2570. Supersedes the WIP draft InsightSoftwareConsortium#2572 by @gregsharp; the test demonstrates Greg's diagnosis is correct, the fix here is a simpler one-event-move that does not change iteration semantics. Co-Authored-By: Gregory C. Sharp <gregsharp.geo@yahoo.com>
4 participants
Issue 2570.
AdvanceOneStep() is now called prior to GetValueAndDerivative() rather than afterward.
Tests in ITKOptimizersv4TestDriver have been updated to match the algorithmic change.
PR Checklist
Refer to the ITK Software Guide for
further development details if necessary.