tron: support MINRES via krylov callback #318
Conversation
…; add bench script and save results - Add run_krylov_subsolver! wrapper to enforce trust-region via Krylov callback if workspace doesn't accept `radius`. - Use wrapper inside projected_newton! so MINRES can run inside TRON trust-region loop. - Save benchmark results to bench/results_tron_minres_vs_cg.csv.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for MINRES as a subsolver in TRON by implementing a compatibility wrapper that handles workspaces that don't accept the radius keyword argument. When radius is not supported, it falls back to using a Krylov callback to enforce the trust-region constraint.
- Simplified the subsolver parameter handling in the main
tronfunction - Added
run_krylov_subsolver!wrapper to handle different Krylov workspace signatures - Created benchmark script and saved results comparing CG vs MINRES performance
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/tron.jl | Refactored subsolver handling and added compatibility wrapper for Krylov workspaces |
| bench/bench_tron_minres_vs_cg.jl | New benchmark script comparing CG and MINRES subsolvers |
| bench/results_tron_minres_vs_cg.csv | Saved benchmark results |
| bench/README.md | Documentation for the benchmark |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| workspaces that don't accept the `radius` keyword (e.g. Minres). We first | ||
| attempt to call with `radius`, and on a MethodError we retry without it. | ||
| """ | ||
| function run_krylov_subsolver!(workspace, A, b; radius=nothing, rtol, atol, timemax, verbose) |
There was a problem hiding this comment.
The function signature should specify default values for all keyword arguments to maintain consistency. Currently rtol, atol, timemax, and verbose lack defaults while radius has one.
| function run_krylov_subsolver!(workspace, A, b; radius=nothing, rtol, atol, timemax, verbose) | |
| function run_krylov_subsolver!(workspace, A, b; radius=nothing, rtol=1e-6, atol=0.0, timemax=Inf, verbose=0) |
Copilot uses AI. Check for mistakes.
| # If workspace has no `x` field, don't stop via callback. | ||
| return false |
There was a problem hiding this comment.
The fallback behavior when workspace lacks an x field could lead to infinite loops or unexpected behavior since the trust-region constraint won't be enforced. Consider logging a warning or throwing an informative error instead.
| # If workspace has no `x` field, don't stop via callback. | |
| return false | |
| error("Krylov workspace of type $(typeof(cb_workspace)) does not have an `x` field. Trust-region constraint cannot be enforced. Please use a compatible workspace type.") |
Copilot uses AI. Check for mistakes.
| # Stop when the norm of the current solution reaches the radius. | ||
| return norm(xcur) >= radius |
There was a problem hiding this comment.
Computing the full norm at every iteration could be expensive for large problems. Consider using norm(xcur)^2 >= radius^2 to avoid the square root computation.
| # Stop when the norm of the current solution reaches the radius. | |
| return norm(xcur) >= radius | |
| # Stop when the squared norm of the current solution reaches the squared radius (avoids sqrt). | |
| return norm(xcur)^2 >= radius^2 |
Copilot uses AI. Check for mistakes.
|
Thanks @arnavk23 ! I will have a look in detail. Regarding benchmarks, I think it would be more useful to add a |
|
Sorry. It turns out #306 is premature. MINRES doesn't yet have the @farhadrclass You added detection on nonpositive curvature to MINRES and MINRES-QLP. Would you mind adding the |
|
@dpo So should I close this pr? |
2 participants
tron: support MINRES via krylov callback when radius kw not supported; add bench script and save results
radius.Closes #306