Fixing extended trace failure for Adam and AdaMax and generalising alpha parameter to accept callable object (scheduler)

[kishore-nori]

This is a small PR for Adam and AdaMax methods (thanks a lot for adding them to Optim.jl) to

• fix issue Extended trace for Adam fails #1096 (when using extended_trace=true) by adding alpha to the respective State structs, so that common_trace! works out of the box.
• generalise alpha (and effected update_state!) for both Adam and AdaMax such that they can accept a callable object - a function or in particular a scheduler. This helps in using the functionality from ParameterSchedulers.jl seamlessly, without adding it as a dependency.

[pkofod]

I am fine with these changes and thank you for fixing the original error. I do have to ask you to update the docstrings of the types to reflect the new feature and also add tests for the bug that was fixed as well as the new feature.

Thanks!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.75%. Comparing base (77501f4) to head (9d2a5f2).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1115      +/-   ##
==========================================
+ Coverage   85.26%   85.75%   +0.49%     
==========================================
  Files          45       45              
  Lines        3502     3518      +16     
==========================================
+ Hits         2986     3017      +31     
+ Misses        516      501      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kishore-nori]

Thank you for checking the PR and for the comments. I have added:

• details in the docstrings of Adam and AdaMax for scheduled alpha constructor
• added tests for Adam and AdaMax, covering the extended_trace=true option that was failing (Extended trace for Adam fails #1096)
• added tests for scheduled Adam and AdaMax convergence, and tests covering the extended_trace=true option to check if the alpha values are correct.

[pkofod]

Thank you

[kishore-nori]

I am glad, would be happy to know if you recommend any corrections or changes to make it merge ready.

Is it possible to run the CI workflow tests to check if the tests work fine? (locally they ran successfully)

[pkofod]

I am glad, would be happy to know if you recommend any corrections or changes to make it merge ready.

Is it possible to run the CI workflow tests to check if the tests work fine? (locally they ran successfully)

Running now :)

[pkofod]

Seem to fail, not sure why it failed in a newton test.

[kishore-nori]

Thanks a lot for running the CI. It unclear to me as well as to why Newton tests failed, and that too on MacOS. I ll investigate and get back.

[pkofod]

Not sure what happened to be honest.. This time it worked

[kishore-nori]

That's great! Thank you for re-running. Where can I check the earlier CI failure? (It somehow disappeared after new the CI runs)

[kishore-nori]

Ok I found the earlier CI runs to trace the failure: CI says Paraboloid Diagonal failed, which I think is the below test:

https://github.com/JuliaNLSolvers/OptimTestProblems.jl/blob/c1fba66c90b44934d13cd11b2502573f1c11fab8/src/optim_tests/multivariate/quad_transforms.jl#L107-L142

I have a feeling guardseed(0) failed to accomplish what it is supposed to and the random matrix used in the test could have been a "bad" one. But I am not sure if this problem could come from architecture or MacOS, or even "just" a failure of guardseed. However, shouldn't be related to the changes in this PR.

some related issues:

JuliaLang/julia#42752 and JuliaLang/julia#51225

[pkofod]

Thank you for looking into it. We'll have to see if it reappears but it is not relevant to the resolution of this pr.

[kishore-nori]

Is the PR good go?