Implement SQMR wrapper - #1088
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds an SQMR solver by wrapping the existing QMR implementation to support symmetric (including indefinite) preconditioning, and wires it into the generic krylov_solve interface with accompanying tests.
Changes:
- Added
sqmr/sqmr!API implemented as a symmetric wrapper aroundqmr/qmr!. - Registered SQMR in
Krylov.jlviainclude("sqmr.jl"). - Extended the QMR test suite to cover SQMR and
krylov_solve(Val(:sqmr), ...).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/test_qmr.jl | Adds coverage for SQMR with symmetric indefinite preconditioning and via krylov_solve. |
| src/sqmr.jl | Introduces sqmr/sqmr! implementation and hooks into krylov_solve. |
| src/Krylov.jl | Includes sqmr.jl so the new solver is available in the module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
I am sorry @arnavk23 but you didn't implemented SQMR. The code generated seems to be something given by an AI and it doesn't really make sense. If you want to implement SQMR, don't use any AI tool and open the book of Youssef Saad on iterative methods to understand what is SQMR. |
Thanks for pointing me in the right direction. I was still going about this and the responses I was getting felt a little bit on the nose. Was about to flag this as Do Not Merge or Draft until I found what was wrong. But you saved me the hassle. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@amontoison can you check again. Any suggestions are welcome. |
|
It seems that reimplemented QMR, not SQMR. If you add new methods: #635 |
Removed unnecessary blank lines in the test file.
…his allows for more flexibility in the workspace structure and makes it easier to modify the workspace in the future without changing the algorithm code. The changes also include updates to the test files to reflect the new workspace accessors.
…torage and half the per-iteration cost
There was a problem hiding this comment.
🟡 Changes recommended
A critical SQMR test failure and a moderate adjoint-product count issue remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
An implementation of SQMR for the solution of symmetric square linear systems Ax = b.
SQMR is implemented here as a symmetric wrapper around QMR with centered preconditioning. This lets the method accept symmetric indefinite preconditioners while reusing the existing QMR machinery.
Fixes #1027