Add monotonicity parameter to LMSolver and TRSolver #226
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds monotonicity parameter support to both LMSolver and TRSolver with history tracking functionality. By default, both solvers remain monotone (parameter value 1), but non-monotone variants can be enabled by setting m_monotone > 1.
- Add
m_monotoneparameter to both solver constructors with default value of 1 - Implement history tracking via
m_fh_histarrays to store objective function values - Update objective function decrease calculations to use maximum historical values for non-monotone variants
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/TR_alg.jl | Add monotonicity parameter and history tracking to TRSolver with updated objective decrease calculation |
| src/LM_alg.jl | Add monotonicity parameter and history tracking to LMSolver with updated objective decrease calculation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| set_solver_specific!(stats, :smooth_obj, fk) | ||
| set_solver_specific!(stats, :nonsmooth_obj, hk) | ||
| set_solver_specific!(stats, :prox_evals, prox_evals + 1) | ||
| m_monotone > 1 && (m_fh_hist[stats.iter % (m_monotone - 1) + 1] = fk + hk) |
There was a problem hiding this comment.
[nitpick] The history update logic is duplicated in two places. Consider extracting this into a helper function or variable to reduce code duplication and improve maintainability.
There was a problem hiding this comment.
Maybe in a future PR
Co-authored-by: Copilot <[email protected]>
MaxenceGollier
left a comment
MaxenceGollier
left a comment
There was a problem hiding this comment.
All tests are failing please fix @MohamedLaghdafHABIBOULLAH
|
Working on it, first the tests are not reproducible and second the opnorm is having an unstable issue... |
MaxenceGollier
left a comment
MaxenceGollier
left a comment
There was a problem hiding this comment.
weird, all test produced
bpdn-with-bounds-ls-LM-l0: Error During Test at /home/runner/work/RegularizedOptimization.jl/RegularizedOptimization.jl/test/test_bounds.jl:45
Got exception outside of a @test
DimensionMismatch: array could not be broadcast to match destination
From the stacktrace, it seems that the problematic line is
RegularizedOptimization ~/work/RegularizedOptimization.jl/RegularizedOptimization.jl/src/LM_alg.jl:387
which is
@. u_bound_m_x = u_bound - xk
I guess this is because u_bound_m_x is wrongly initialized which is the case! (see my fix below)
Co-authored-by: Maxence Gollier <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #226 +/- ##
===========================================
+ Coverage 61.53% 86.28% +24.74%
===========================================
Files 11 13 +2
Lines 1292 1589 +297
===========================================
+ Hits 795 1371 +576
+ Misses 497 218 -279 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MaxenceGollier
left a comment
MaxenceGollier
left a comment
There was a problem hiding this comment.
Ok, does the feature work with m_monotone > 1 at least locally on your side ? If so, then you can rebase in my opinion
3 participants
Add monotonicity parameter to LMSolver and TRSolver with history tracking, maybe we should do the same in LMTR