-
Notifications
You must be signed in to change notification settings - Fork 10
Enable subsolver tolerances tuning, fix negative tolerance #93
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
Enable subsolver tolerances tuning, fix negative tolerance #93
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #93 +/- ##
==========================================
+ Coverage 57.47% 58.10% +0.62%
==========================================
Files 11 11
Lines 1204 1222 +18
==========================================
+ Hits 692 710 +18
Misses 512 512
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
Here are the |
dpo
left a comment
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! Can we use TRDH inside LM and LMTR now?
|
|
||
| f = LSR1Model(model) | ||
| λ = norm(grad(model, zeros(model.meta.nvar)), Inf) / 10 | ||
| λ = 1.0e-1 # norm(grad(model, zeros(model.meta.nvar)), Inf) / 10 |
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.
Are the results very different if you leave the default lambda ?
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.
Not that much, I updated the tables and the benchmarks in the article.
|
|
||
| verbose = 0 # 10 | ||
| ϵ = 1.0e-6 | ||
| ϵ = 1.0e-5 |
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 this one used?
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.
Yes, this is the tolerance for
Not at the moment be this would be a very minor change to make them work. I can do it in my next PR once this one is merged. |
|
Here are the |
| options3 = ROSolverOptions(spectral = false, psb = false, ϵa = ϵi, ϵr = ϵri, maxIter = maxIter_inner) | ||
| options3bis = ROSolverOptions(spectral = false, psb = false, ϵa = ϵi, ϵr = ϵri, maxIter = maxIter_inner, reduce_TR = false) | ||
| options4 = ROSolverOptions(spectral = true, ϵa = ϵi, ϵr = ϵri, maxIter = maxIter_inner) | ||
| options4bis = |
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.
Let's give all these options descriptive names and avoid "bis" (it's French and will confuse non-French speakers).
|
Here are the |
|
Here are the |
* tune inner solver parameters, fix error negative tol * improve tables, change test parameters, modif demos * keep default value bpdn * update tables parameters * update parameters name noredTR
I changed the algorithms so that it is possible to set some specific tolerances in the
subsolver_options. I did a slight modification in the defaultsubsolver_optionsso that it keeps the existing behaviour.It allowed me to find better parameter for the benchmarks.
I also updated the stopping criterion in TRDH so that it handles negative (but small)
ξ1orξvalues, and fixed what I think is a bug in R2 with theneg_tolparameter (which is always positive).