-
Notifications
You must be signed in to change notification settings - Fork 15
Add R2N and R2NLS solvers with HSL and QRMumps support #347
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
base: main
Are you sure you want to change the base?
Add R2N and R2NLS solvers with HSL and QRMumps support #347
Conversation
Introduces new second-order quadratic regularization solvers R2N and R2NLS for unconstrained and nonlinear least-squares optimization. Adds support for HSL (MA97, MA57) and QRMumps direct solvers, updates documentation and README, and extends test coverage for the new solvers. Updates dependencies and compat entries in Project.toml.
|
|
||
| if !subsolver_solved && npcCount == 0 | ||
| @warn("Subsolver failed to solve the system") | ||
| # TODO exit cleaning, update stats |
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.
there should be a (existing?) flag to set stats.status with for this early stop.
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 but I think we need a new one and we can decide on it Tuesday?
src/R2N.jl
Outdated
|
|
||
| # Check Indefinite (Negative Eigenvalues) | ||
| if !force_sigma_increase && num_neg > 0 | ||
| # Curvature = s' (H+σI) s |
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.
inconsistent comment
src/R2N.jl
Outdated
| # A: Armijo (Upper Bound) - Is step too big? | ||
| armijo_pass = fck <= f0_val + c_goldstein * α * grad_dir | ||
|
|
||
| # B: Curvature (Lower Bound) - Is step too small? |
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.
this is not a condition on the curvature, this is the Goldstein condition. Update comments and variable names accordingly.
src/R2N.jl
Outdated
| α_min = zero(T) # Lower bound of valid interval | ||
| α_max = T(Inf) # Upper bound of valid interval | ||
| iter_ls = 0 | ||
| max_iter_ls = 100 # Safety break #TODO Prof Orban? |
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.
This should be a kwarg
src/R2N.jl
Outdated
| ρk = (fck_max - fck) / (fck_max - stats.objective + ΔTk) #TODO Prof. Orban check if this is correct the denominator | ||
| else | ||
| # Avoid division by zero/negative. If ΔTk <= 0, the model is bad. | ||
| ρk = (ΔTk > 10 * eps(T)) ? (stats.objective - fck) / ΔTk : -one(T) |
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.
Delta Tk theoretically cannot be negative. An error should be thrown in that case, see how this case is handled in the other solver.
Keep rho_k as usual.
src/R2N.jl
Outdated
| k = mod(stats.iter, non_mono_size) + 1 | ||
| solver.obj_vec[k] = stats.objective | ||
| fck_max = maximum(solver.obj_vec) | ||
| ρk = (fck_max - fck) / (fck_max - stats.objective + ΔTk) #TODO Prof. Orban check if this is correct the denominator |
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.
denominator should be Delta Tk.
src/R2N.jl
Outdated
| if npc_handler == :gs # Goldstein Line Search | ||
| npcCount = 0 | ||
|
|
||
| # --- Goldstein Line Search --- |
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.
put the line search algo in a separate function.
src/R2N.jl
Outdated
| k = mod(stats.iter, non_mono_size) + 1 | ||
| solver.obj_vec[k] = stats.objective | ||
| fck_max = maximum(solver.obj_vec) | ||
| ρk = (fck_max - fck) / (fck_max - stats.objective + ΔTk) #TODO Prof. Orban check if this is correct the denominator |
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.
if very first iteration and subsolver very first iteration return npc, and we selected npc_handler = prev, then we need a better way, maybe use a flag to tell sigma to increase and try agian
src/R2N.jl
Outdated
| ρk = (fck_max - fck) / (fck_max - stats.objective + ΔTk) #TODO Prof. Orban check if this is correct the denominator | ||
| else | ||
| # Avoid division by zero/negative. If ΔTk <= 0, the model is bad. | ||
| ρk = (ΔTk > 10 * eps(T)) ? (stats.objective - fck) / ΔTk : -one(T) |
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.
this is the issue , iteration should be failed then move to next
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
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.
Lots more comments on R2NLS.
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Rename model argument from `nlp` to `nls` throughout and update DefaultParameter lambdas to accept the new name. Rename solver residual field `Fx` to `r`. Improve QRMumps solver initialization and memory handling: use nls_meta, use views for COO arrays/values, zero the augmented RHS with an in-place loop, add a user-facing destroy! wrapper and keep a finalizer. Use views when constructing SparseMatrixCOO from QRMumps buffers and update jacobian/value update calls to the new `nls` API. Introduce `scp_est` option and refactor Cauchy/step safeguard logic (compute Cauchy step via curvature or operator norm), change initial σk heuristic to use Jacobian scale, and correct predicted/actual reduction and non-monotone bookkeeping to use residual/objective consistently. Tidy up logging columns and remove stale cp_step logging. Fix several small bugs in σ update rules and function argument names; general cleanup and minor performance improvements (views, fewer allocations).
Bump LinearOperators compat to 2.12.0 and refactor R2N internals. - Remove local ShiftedOperator implementation (use library/operator-level solutions). - Add LineModel field (h) and model type parameter to R2NSolver, initialize it in the constructor and recreate it when resetting with a new NLP. - Replace large custom Goldstein line-search block with SolverTools.redirect! + armijo_goldstein; compute step (s) and objective (ft) via that helper. - Replace several uses of fck with ft (clarify naming), adjust ρk and σ update logic, and ensure H updates and objective/stats use the computed ft. - Minor logging/formatting, whitespace, and small correctness tweaks (vectorized assignments, return values in reset!, etc.). These changes centralize line-search behavior, simplify solver code, and prepare for compatibility with newer LinearOperators behavior.
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.
I propose a cleaner subsolver API.
src/R2NLS.jl
Outdated
|
|
||
| function destroy!(s::QRMumpsSolver) #for user use, in case they want to free memory before the finalizer runs | ||
| free_qrm(s) | ||
| end |
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 don't think it is possible to call this function since it is declared inside the constructor. At any rate, I don't see what its purpose would be.
Co-authored-by: Dominique <[email protected]>
Add an AbstractR2NLSSubsolver interface and unify subproblem solvers behind initialize!, update_subsolver!, solve_subproblem! and get_jacobian. Implement QRMumpsSubsolver (replacing the old QRMumpsSolver) and a GenericKrylovSubsolver with convenience constructors LSMRSubsolver, LSQRSubsolver and CGLSSubsolver. Extend R2NLSParameterSet with compute_cauchy_point and inexact_cauchy_point flags (and validation) and expose new subsolver exports. Refactor R2NLSSolver to accept either a subsolver Type or a preallocated AbstractR2NLSSubsolver instance, update internal fields, instantiate/update subsolvers, and drive the main solve loop through the new subsolver API. Update the trust-region / Levenberg–Marquardt loop to compute the Cauchy point conditionally, update σ initialization heuristic, streamline acceptance logic, and adjust logging field names. Remove legacy QRMumpsSolver and old subsolve dispatches; add resource cleanup (finalizer) for QRMumpsSubsolver.
|
@dpo I have refactor the R2NLS, |
Introduces new second-order quadratic regularization solvers R2N and R2NLS for unconstrained and nonlinear least-squares optimization. Adds support for HSL (MA97, MA57) and QRMumps direct solvers, updates documentation and README, and extends test coverage for the new solvers. Updates dependencies and compat entries in Project.toml.