JSO Compliance: R2 #139
Conversation
|
Thank you, @MaxenceGollier! We'll look in detail. Yes, callbacks are necessary. See #131. ps: you can mark functions that should eventually be removed with |
dpo
left a comment
There was a problem hiding this comment.
Just a few quick simple comments for now.
dpo
left a comment
There was a problem hiding this comment.
Thank you. Here are a few more comments.
I would like to see a comparison of the old and new R2 on the problems we have to make sure the results are the same (number of iterations and final values (f, h, ξ, …).
|
First, this version: Restarting Julia, the old version gives There is just a small difference in the fact that the iterates start at 0 and don't print the last one, I think the last one is useless because except for the stationarity measure nothing changes. |
|
Then, for benchmarks, For this version: and for the old version: |
|
Changing the line |
|
@MaxenceGollier I fixed the demos. Could you please rebase your branch? That way, you'll have the latest fixes (builds on FreeBSD + the demos working again). The procedure goes like this: > git checkout masterIf > git pull origin masterIf origin points to your fork instead, first do > git remote add upstream https://github.com/JuliaSmoothOptimizers/RegularizedOptimization.jl.git
> git pull upstream masterNow head back to your branch and rebase > git checkout RegularizedModels
> git rebase masterHopefully, there are no conflicts to fix. |
Co-authored-by: Dominique <dominique.orban@gmail.com>
Co-authored-by: Dominique <dominique.orban@gmail.com>
|
There are still rests of the older version which are just kept to not destroy other implementations in this package that rely on the older version. is useless. Also, I don't see why we should use in |
|
Your branch still has conflicts. Let’s keep “old” features for now. We’ll remove them later. I opened a PR a while ago to remove Fhist, etc., We can update it after this one’s been merged. |
|
Tests should pass now with 754feea |
754feea to
2c4b992
Compare
|
Here are the |
|
Here are the |
Co-authored-by: Dominique <dominique.orban@gmail.com>
Co-authored-by: Dominique <dominique.orban@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #139 +/- ##
==========================================
- Coverage 61.53% 60.21% -1.32%
==========================================
Files 11 11
Lines 1292 1302 +10
==========================================
- Hits 795 784 -11
- Misses 497 518 +21 ☔ View full report in Codecov by Sentry. |
|
Here are the |
Co-authored-by: Dominique <dominique.orban@gmail.com>
|
Here are the |
|
There are lots of large differences in the demos, in particular on the plots showing the number of inner iterations. I wonder if other solvers are able to call the new R2 and if the number of iterations is reported properly. Could you please check? Among others, these tests show differences:
Those problems are generated randomly but we initialize the seed, so they should be the same. |
|
Yes they do, when i test locally, all inner iterations are reported correctly. Can you show me with respect to what you are comparing ? I just see the demos-result for this branch (and not for the base one ?) For example for |
|
I was comparing to the demos in this PR: #138 |
|
It is weird because my local branch is up to date with this and when I run this locally on So I should have the same results as on #138, I don't really see where these plots are generated ? The |
|
They’re generated by the scripts in the examples folder. |
|
The issue should be solved with f0a273c now. |
| * `Fobj_hist`: an array with the history of values of the smooth objective | ||
| * `Hobj_hist`: an array with the history of values of the nonsmooth objective | ||
| * `Complex_hist`: an array with the history of number of inner iterations. | ||
| ψ(s; xₖ) is either h(xₖ + s) or an approximation of h(xₖ + s), ‖⋅‖ is a user-defined norm and σₖ > 0 is the regularization parameter. |
There was a problem hiding this comment.
Is
| ψ(s; xₖ) is either h(xₖ + s) or an approximation of h(xₖ + s), ‖⋅‖ is a user-defined norm and σₖ > 0 is the regularization parameter. | |
| ψ(s; xₖ) is either h(xₖ + s) or an approximation of h(xₖ + s), ‖⋅‖ is a user-defined norm and σₖ > 0 is the regularization parameter. |
Rewrote
R2_alg.jlto be more in phase with other JSO solvers.SolverCore.solve!( R2Solver, RegularizedNLPModel, stats )which implements the algorithm.Fhist,Hhist, etc. in my opinion). Also, the documentation i added does not show these functions.I initially just wanted to add callbacks so i don't know if this is relevant. What do you think @dpo, @geoffroyleconte, @MohamedLaghdafHABIBOULLAH ?