Introduction of R2NLS Solver in JSOSolvers #303
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #303 +/- ##
==========================================
- Coverage 89.00% 85.96% -3.04%
==========================================
Files 7 9 +2
Lines 1200 1453 +253
==========================================
+ Hits 1068 1249 +181
- Misses 132 204 +72 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks @farhadrclass for the PR! I made some preliminary comments.
In general, this can be your working branch, but it would be preferable to have a PR with less features so we can review the core algorithm.
For instance, you can add the QRMumps solver later. Ideally, unless strongly motivated, I would not add any new dependency in this PR.
You are also missing the parameter set like the other solvers
Line 10 in ad9970a
The tests are not passing (why?), and the test in the docstring as well.
test/consistency.jl
Outdated
| @testset "NLS with $mtd" for (mtd, solver) in [ | ||
| ("trunk", trunk), | ||
| ("R2NLS", (unls; kwargs...) -> R2NLS(unls; kwargs...)), | ||
| ("R2NLS_CGLS", (unls; kwargs...) -> R2NLS(unls, subsolver = :cgls; kwargs...)), | ||
| ("R2NLS_LSQR", (unls; kwargs...) -> R2NLS(unls, subsolver = :lsqr; kwargs...)), | ||
| ("R2NLS_CRLS", (unls; kwargs...) -> R2NLS(unls, subsolver = :lsqr; kwargs...)), | ||
| ("R2NLS_LSMR", (unls; kwargs...) -> R2NLS(unls, subsolver = :lsmr; kwargs...)), | ||
| ("R2NLS_QRMumps", (unls; kwargs...) -> R2NLS(unls, subsolver= :qrmumps; kwargs...)), | ||
| ] |
There was a problem hiding this comment.
Why are you adding the strings here? I think the function was sufficient.
There was a problem hiding this comment.
I need to test different subsolver
test/runtests.jl
Outdated
| ("R2NLS_LSQR", (nlp; kwargs...) -> R2NLS(nlp, subsolver = :lsqr; kwargs...)), | ||
| ("R2NLS_CRLS", (nlp; kwargs...) -> R2NLS(nlp, subsolver = :lsqr; kwargs...)), | ||
| ("R2NLS_LSMR", (nlp; kwargs...) -> R2NLS(nlp, subsolver = :lsmr; kwargs...)), | ||
| ("R2NLS_QRMumps", (nlp; kwargs...) -> R2NLS(nlp, subsolver = :qrmumps; kwargs...)), |
| T, | ||
| V, | ||
| Op <: AbstractLinearOperator{T}, | ||
| Sub <: Union{KrylovWorkspace{T, T, V}, QRMumpsSolver{T}}, |
There was a problem hiding this comment.
| Sub <: Union{KrylovWorkspace{T, T, V}, QRMumpsSolver{T}}, | |
| Sub <: KrylovWorkspace{T, T, V}, |
|
@tmigot Thanks for the review, I see your point on QRMumps but, one of the main focus of this PR and my code is to use the QRMumps and hence I put them here (I was thinking to move them to a different location after but for now I think it is required for this PR), What do you think? On the tests reviews, I agree, I will clean it up and update it asap and also the same with documents Thanks for the review |
I don't really understand why. I get that the point of your code is to use the QRMumps, but we push all the code in several PR because we also need to make sure that it's correct and the best (and fastest) way to do it is by having focused PR. |
Introduces a `closed` flag and `close!` method to QRMumpsSolver for safe resource cleanup, with finalizer registration to avoid double-destroy. Integrates SparseMatrixCOO for Jacobian representation when using QRMumps, updates Jacobian value handling, and improves documentation with explicit cleanup examples.
Changed allowed npc_handler values to lowercase for consistency and updated documentation accordingly. Implemented the Armijo rule for non-positive curvature handling, including backtracking and forward tracking line search. Improved logic for step acceptance and regularization parameter updates, and added linesearch option to subsolve! call.
This PR introduces the new R2NLS solver, An inexact second-order quadratic regularization method designed specifically for nonlinear least-squares problems.
The objective is to solve
min ½‖F(x)‖²
where
F: ℝⁿ → ℝᵐis a vector-valued function defining the least-squares residuals.