fix computation of ν in TRDH and LMTR#295
fix computation of ν in TRDH and LMTR#295MaxenceGollier wants to merge 4 commits intoJuliaSmoothOptimizers:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug in the computation of the step length parameter ν in the TRDH (Trust Region with Diagonal Hessian) and LMTR (Levenberg-Marquardt Trust Region) algorithms, as referenced in issue #231. The formulas were previously incorrect and have been updated to use a new pattern that properly incorporates the trust region radius and regularization terms.
Changes:
- Updated ν computation in TRDH to use
1 / (DNorm + 1 / (α * Δk))instead of(α * Δk)/(DNorm + 1) - Updated ν computation in LMTR to use
1 / (σmax^2 + 1 / (α * Δk))instead ofα * Δk / (1 + σmax^2 * (α * Δk + 1))
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/TRDH_alg.jl | Updated both occurrences of ν computation (lines 329 and 479) to use the corrected formula pattern with proper scaling by trust region radius and diagonal norm |
| src/LMTR_alg.jl | Updated both occurrences of ν computation (lines 278 and 450) to use the corrected formula pattern with proper scaling by trust region radius and Jacobian operator norm squared |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/test_bounds.jl
Outdated
| for (mod, mod_name) ∈ ((SpectralGradientModel, "spg"),) | ||
| # ((DiagonalPSBModel, "psb"),(DiagonalAndreiModel, "andrei")) work but do not always terminate | ||
| for (h, h_name) ∈ ((NormL0(λ), "l0"), (NormL1(λ), "l1")) | ||
| continue # FIXME |
There was a problem hiding this comment.
I added this because the TRDH tests with bounds now failed, see this run: https://github.com/JuliaSmoothOptimizers/RegularizedOptimization.jl/actions/runs/22235640816.
@dpo, I am not sure what to do here. should we just accept that TRDH fails ? I think #281 might fix this actually.
There was a problem hiding this comment.
Maybe the value α = 1 / eps(T) is too extreme?! Do the tests pass with, e.g., α = 1 / √eps(T)?
There was a problem hiding this comment.
I am trying to investigate, i found another bug:
The documentation says:
RegularizedOptimization.jl/src/TRDH_alg.jl
Line 150 in 8b7a6b7
where the phi is defined as
RegularizedOptimization.jl/src/TRDH_alg.jl
Line 115 in 8b7a6b7
but then we compute xi as
RegularizedOptimization.jl/src/TRDH_alg.jl
Lines 369 to 373 in 8b7a6b7
i.e with a first order model and not the quadratic model.
I am confused. This whole reduce_TR thing is a bit odd in my opinion too, shouldn't we decide which is best and just keep one like in R2DH ?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #295 +/- ##
===========================================
+ Coverage 61.53% 86.62% +25.08%
===========================================
Files 11 13 +2
Lines 1292 1652 +360
===========================================
+ Hits 795 1431 +636
+ Misses 497 221 -276 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| DNorm = norm(D.d, Inf) | ||
|
|
||
| ν = (α * Δk)/(DNorm + one(T)) | ||
| ν = 1 / (DNorm + 1 / (α * Δk)) |
There was a problem hiding this comment.
I agree, thank you. In addition, DNorm can be computed as norm(max.(D.d, 0), Inf) (although that allocates). That was a late update to the convergence theory; it looks like it didn't make it into the paper.
There was a problem hiding this comment.
Can be or should be ?
It looks like it was a bug.
See the last point of #231.