-
Notifications
You must be signed in to change notification settings - Fork 10
JSO Compliance: TR & TRDH #199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #199 +/- ##
===========================================
+ Coverage 61.53% 77.50% +15.97%
===========================================
Files 11 12 +1
Lines 1292 1525 +233
===========================================
+ Hits 795 1182 +387
+ Misses 497 343 -154 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TRDH Tests(NB: I tweaked the parameters, For TRDH (without reduce_TR)On this branch, On the master branch, For TRDH (with reduce_TR)On this branch, On the master branch, |
TR TestsFor TR with R2On this branch, On the master branch, For TR with TRDHOn this branch, On the master branch, |
| # model for prox-gradient step to update Δk if ||s|| is too small and ξ1 | ||
| φ1(d) = ∇fk' * d | ||
| mk1(d) = φ1(d) + ψ(d) | ||
| ν = (α * Δk)/(DNorm + one(T)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ν = (α * Δk)/(DNorm + one(T)) | |
| ν = 1/(DNorm + 1/(α * Δk)) |
Cf paper, this seems to be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be fixed in LMTR.
Co-authored-by: Dominique <dominique.orban@gmail.com>
src/TRDH_alg.jl
Outdated
| u_bound = kwargs[:u_bound] | ||
| has_bnds = has_bnds || any(u_bound .!= R(Inf)) | ||
| end | ||
| nlp = FirstOrderModel(f, ∇f!, x0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FirstOrderModel no longer exists: https://github.com/JuliaSmoothOptimizers/RegularizedProblems.jl/blob/main/src/types.jl#L4
We show now use ManualNLPModels.jl.
|
TR with LBFGS and R2 On the master branch, TR with LSR1 and TRDH On the master branch, |
|
Thank you @MaxenceGollier it seems that they are aligned, but in the second example with LSR1, why do we have 0 sub iterations whereas in the last one we have 1? |
|
I guess it is because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thank you!
@dpo did the two solvers in one PR because i couldn't modify
TRifTRDHwas not udpated (due to when I usesolve!in TR to call the subsolver)Also, a few TODOs in the doc because i am not sure what some parameters mean.
Finally, as in #172, the solvers behaviours remain the same and there are no allocs (there are still for TR since
opnormallocates).cc. @MohamedLaghdafHABIBOULLAH
I will showcase tomorrow that results here are identical to the master branch.