-
Notifications
You must be signed in to change notification settings - Fork 10
Correct xi in SolverCore.solve! in LM and R2N solvers #239
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
Correct xi in SolverCore.solve! in LM and R2N solvers #239
Conversation
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.
Pull Request Overview
This PR corrects the model function mk in the SolverCore.solve! method for both LM and R2N solvers by adding a quadratic regularization term.
- Adds the regularization term
- 1/2 * σk * dot(d, d)to the model functionmk - Ensures consistent mathematical formulation across both solver implementations
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/R2N.jl | Updates model function to include quadratic regularization term |
| src/LM_alg.jl | Updates model function to include quadratic regularization term |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #239 +/- ##
===========================================
+ Coverage 61.53% 84.41% +22.88%
===========================================
Files 11 13 +2
Lines 1292 1630 +338
===========================================
+ Hits 795 1376 +581
+ Misses 497 254 -243 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Should we merge? @dpo @MaxenceGollier |
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.
Pull Request Overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
I think dpo had a comment (on zulip) |
….solve! for LM and R2N solvers
| while !done | ||
| sub_atol = stats.iter == 0 ? 1.0e-3 : min(sqrt_ξ1_νInv ^ (1.5), sqrt_ξ1_νInv * 1e-3) | ||
|
|
||
| solver.subpb.model.σ = σk |
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.
@dpo is this ok for you?
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.
Why should we remove this ?
solver.subpb.model.σwill not be updated anywhere ?
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.
I think that line needs to stay. It's the computation of stats and subtract 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.
We should close this as this issue has been treated here PR by Maxence.
No description provided.