Solving a linear system with a symmetric positive definite matrix#1093
Solving a linear system with a symmetric positive definite matrix#1093perazz merged 8 commits intofortran-lang:masterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1093 +/- ##
==========================================
- Coverage 68.55% 68.31% -0.25%
==========================================
Files 396 399 +3
Lines 12746 12788 +42
Branches 1376 1383 +7
==========================================
- Hits 8738 8736 -2
- Misses 4008 4052 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for your contribution. It looks pretty good. A couple of general things:
Moreover, I see that you have created a new submodule. It's fine, but since we already have an |
|
Thanks @loiseaujc I have addressed all the points:- Let me know if anything else needs adjustment. |
a600086 to
bfd9b12
Compare
|
Solved all merge conficts! |
There was a problem hiding this comment.
Pull request overview
This PR adds two interfaces to stdlib_linalg for solving linear systems with symmetric positive definite (SPD) matrices, addressing issue #1069. The implementation wraps LAPACK's POTRS and POSV routines to provide both a two-step workflow (cholesky + solve_chol) for repeated solves and a one-shot convenience interface (cholesky_solve) for single solves.
Changes:
- Added
solve_cholinterface for solving systems using pre-computed Cholesky factors (POTRS wrapper) - Added
cholesky_solveinterface for one-shot factorization and solve (POSV wrapper) - Implemented error handlers
handle_potrs_infoandhandle_posv_infofor LAPACK return codes
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/linalg/stdlib_linalg_solve.fypp | Implementation of solve_chol and cholesky_solve routines following existing solve_lu patterns |
| src/linalg/stdlib_linalg.fypp | Public interface declarations with comprehensive documentation comments |
| src/lapack/stdlib_linalg_lapack_aux.fypp | LAPACK error handlers for POTRS and POSV routines |
| test/linalg/test_linalg_solve_chol.fypp | Test suite covering basic solves, multiple RHS, overwrite behavior, and all supported types |
| test/linalg/CMakeLists.txt | Integration of new test suite into build system |
| example/linalg/example_solve_chol.f90 | Example demonstrating two-step workflow with pre-computed factors |
| example/linalg/example_cholesky_solve.f90 | Example demonstrating one-shot solve interface |
| example/linalg/CMakeLists.txt | Registration of example programs |
| doc/specs/stdlib_linalg.md | Comprehensive documentation for both interfaces with syntax, arguments, and examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@jvdp1 Addressed all Copilot suggestions |
|
It looks good to me and seems pretty much ready for merging. Could you add two additional tests though:
just to make sure the |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
def2fff to
921ba4b
Compare
|
@loiseaujc |
loiseaujc
left a comment
There was a problem hiding this comment.
Code-wise, it looks really good. And very thorough unit-testing. I'll go through the specs tomorrow but I think you're very close.
perazz
left a comment
There was a problem hiding this comment.
I stand by @loiseaujc's comments as far as the quality of the PR.
I would like to have a discussion on the choice of the API names: I think solve_chol and cholesky_solve are interchangeable, so not very clear from the user's standpoint. Given we have solve, solve_lu, solve_lstsq, solve_constrained_lstsq, ... a solve_cholesky or solve_chol interface looks more in line with the current API to me. in other words, I think cholesky_solve would be confusing because it does not "imply" any difference from solve_chol. Here is a proposal to unify the API:
solve_cholorsolve_cholesky-> replacecholesky_solvei.e. start from matrix "A"; retain thelowerargumentsolve_upper_choleskyorsolve_upper_chol-> solve with(u, b, x)pre-formed (droplowerargument)solve_lower_choleskyorsolve_lower_chol-> solve with(l, b, x)pre-formed (droplowerargument)
|
Thanks @perazz, |
loiseaujc
left a comment
There was a problem hiding this comment.
The subroutines solve_lower_chol and solve_upper_chol are essentially duplicated code with a single change. Maintenance wise, a better approach (I guess) would be to have a low-level driver solve_factorized_cholesky(a, b, x, uplo, err) with all arguments required, and then have solve_lower_chol and solve_upper_chol simply call this low-level driver with the appropriate uplo input. Note that the low-level driver need not be exported at the module level.
perazz
left a comment
There was a problem hiding this comment.
Thank you @aamrindersingh, it's in a pretty good state. I added a couple clarifying edits (please double check no leftovers)
|
As far as I can see, I think it looks pretty much ready. |
|
Great job @aamrindersingh ! As far as I'm concerned, this PR is ready for merging. I'll give @jvdp1 and @jalvesz a few days if they want to check out the code and have any comments. If they green light it, I guess we'll merge by the end of next week. |
|
With no more outstanding comments, I will go ahead and merge this one. |
Resolves #1069
This PR adds two interfaces to
stdlib_linalgfor solving linear systems with symmetric positive definite (SPD) matrices. Thesolve_cholinterface is for two step workflows where you factorize once and solve multiple times , common in iterative algorithms like quadratic programming. Usage:call cholesky(A, L, lower=.true.)thencall solve_chol(L, b, x, lower=.true.). It wraps LAPACK'sPOTRS. Thecholesky_solveinterface is a one shot convenience that factorizes and solves in a single call using LAPACK'sPOSV:call cholesky_solve(A, b, x). This is simpler for one time solves where convenience matters more than reusing factorizations.The key design decision is making
lowerrequired insolve_cholto prevent silent failures when it doesn't match the factorization step. If someone callscholesky(A, L, lower=.false.)but forgets to specifylower=.false.insolve_chol, they would get wrong results silently. Making it required forces explicit matching. In contrast,cholesky_solvekeepsloweroptional (defaults to.true.) since factorization happens internally. Both interfaces supportreal(sp/dp/qp/xdp)andcomplex(sp/dp/qp/xdp)when available, following the existingsolve_lunaming pattern for consistency.Testing includes basic SPD solves, multiple right-hand sides, upper/lower triangular forms, real and complex types, error handling, residual verification (
||A*x - b|| / ||b|| < sqrt(epsilon)), and comparison withsolve_lu. All tests pass with GFortran 13.2 and Intel ifx 2024.0.