-
Notifications
You must be signed in to change notification settings - Fork 10
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
JSO Compliance: R2 #139
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 |
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.
Just a few quick simple comments for now.
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.
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 master If > git pull origin master If origin points to your fork instead, first do > git remote add upstream https://github.com/JuliaSmoothOptimizers/RegularizedOptimization.jl.git
> git pull upstream master Now head back to your branch and rebase > git checkout RegularizedModels
> git rebase master Hopefully, there are no conflicts to fix. |
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
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 |
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.
Just a couple of minor comments.
Here are the |
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
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 <[email protected]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
Oui en effet, c'est la norme 2.
Rewrote
R2_alg.jl
to 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 ?