WIP: Fix for itkGradientDescentOptimizerv4

[gregsharp]

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

• [x] No API changes were made (or the changes have been approved)
• [x] No major design changes were made (or the changes have been approved)
• [x] Added test (or behavior not changed)
• [x] Updated API documentation (or API not changed)
• [ ] Added license to new files (if any)
• [ ] Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
• [ ] Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for further development details if necessary.

[dzenanz]

adding the action:ApplyClangFormat in a PR comment should auto-format the 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.

[gregsharp]

Commit messages should adhere to the required style. Among others:

Sure. If the PR contains multiple commits, should I squash the work into a single commit and update the message at that time?

Note that the Drivers (or TestDrivers) are just a mechanism of the testing framework, not the relevant name of the module.

Sorry I don't understand.

Please remove the PR's HTML markup instructions (if you feel like it should be written, a PR would be welcome 100).

Sorry, also I don't understand, Where is the HTML markup?

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.

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!

[jhlegarreta]

Thanks for considering the comments @gregsharp :100:.

Sure. If the PR contains multiple commits, should I squash the work into a single commit and update the message at that time?

Depends on the commits; if they serve to different purposes (e.g. DOC, STYLE, PERF, BUG, etc.), best is to leave them as separate commits. Otherwise, we prefer squashing before the branch gets merged, or alternatively amending the commit and push-forcing as changes are done to answer the comments or fix tests that do not pass.

Sorry I don't understand.

ITKOptimizersv4TestDriver is just a command used in the CMakeLists.txt file to perform some actions on tests; it does not exist in the source code tree, so it is not meaningful from the point of the view of the ITK modules or source tree.

Sorry, also I don't understand, Where is the HTML markup?

When you open the PR, there is a body of text inside HTML comment markup <!-- --> in the PR message area. Not sure if people remove it, but I think it is desirable (just leaving the PR checkboxes that apply).

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.

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.

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.

The automatic style enforcing tool (i.e. clang-format, which should be installed when executing the SetupForDevelopment.sh script) should do the job for you. Although I admit that in the past I had issues with it. Also, unfortunately some of the decisions made when the tool was adopted have not made yet to the ITK SW Guide. I'm sorry for that.

Thank you for review!

:+1:

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.