feat(linalg): add generalized least squares solver#1105
feat(linalg): add generalized least squares solver#1105aamrindersingh wants to merge 12 commits intofortran-lang:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a generalized least-squares (GLS) solver for the stdlib_linalg module, addressing issue #1047 (2 of 2). The GLS solver handles least-squares problems with correlated errors described by a covariance matrix, using LAPACK's GGGLM routine to minimize the Mahalanobis distance (Ax-b)^T W^{-1} (Ax-b).
Changes:
- Adds
generalized_lstsqfunction interface supporting real and complex types with correlated error handling via SPD/Hermitian covariance matrices - Implements memory-safe design that always copies the covariance matrix W internally and follows the
overwrite_apattern from existing solvers - Provides comprehensive error handling via
handle_ggglm_infoconsistent with existing LAPACK wrapper patterns
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/linalg/stdlib_linalg_least_squares.fypp | Core implementation of generalized_lstsq with Cholesky factorization and GGGLM solver integration |
| src/linalg/stdlib_linalg.fypp | Public interface declaration with documentation for the new generalized_lstsq function |
| src/lapack/stdlib_linalg_lapack_aux.fypp | New handle_ggglm_info error handler following established patterns for LAPACK error processing |
| test/linalg/test_linalg_lstsq.fypp | Test suite with basic GLS solver tests and identity covariance validation against OLS |
| example/linalg/example_generalized_lstsq.f90 | Example program demonstrating GLS usage with correlated errors |
| example/linalg/CMakeLists.txt | Build configuration update to include the new example |
| doc/specs/stdlib_linalg.md | Comprehensive API documentation for generalized_lstsq with syntax, arguments, and usage example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1105 +/- ##
==========================================
- Coverage 68.55% 67.98% -0.58%
==========================================
Files 396 403 +7
Lines 12746 12850 +104
Branches 1376 1383 +7
==========================================
- Hits 8738 8736 -2
- Misses 4008 4114 +106 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 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 7 out of 7 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 |
loiseaujc
left a comment
There was a problem hiding this comment.
Again, I took a quick glance at your code and will try to go deeper into it by the end of the week. Looks pretty good so far.
|
@loiseaujc Addressed all comments |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
a73a5df to
296727a
Compare
|
@loiseaujc Rebased onto master, CI should pass now. |
perazz
left a comment
There was a problem hiding this comment.
Thanks for this PR @aamrindersingh, I have added some comments.
| `b`: Shall be a rank-1 array of the same kind as `a`, containing the right-hand-side vector. It is an `intent(in)` argument. | ||
|
|
||
| `prefactored_w` (optional): Shall be an input `logical` flag. If `.true.`, `w` is assumed to contain a matrix square root \( B \) such that \( W = B \cdot B^T \). This can be a Cholesky factor or any other valid square root (e.g., SVD-based). Default: `.false.`. This is an `intent(in)` argument. | ||
|
|
There was a problem hiding this comment.
You could mention here that, if the Cholesky factor is used, user need to have zeroed-out the other triangular part.
There was a problem hiding this comment.
why asking that to the user, and not to do it internally?
If asked to the user, should there be an internal check to ensure that it is really the case?
There was a problem hiding this comment.
The Cholesky factors are not the only matrix square-root one can use. If you use svd and take the square-root of the singular values, it'll be a valid square-root although it won't have the upper or lower triangular structure of the Cholesky factor. In order to check internally whether the upper (lower) triangular part has been zeroed-out by the user, we would need to know for sure that the pre-factored W matrix has been obtained with Cholesky. And even there, we would need to know whether U or L is being used.
There was a problem hiding this comment.
OK. thank you for the explanation. Could we then add something like that to warn the user that is up to its responsability:
"It is the user's responsibility to ensure that the prefactored matrix B is valid."
loiseaujc
left a comment
There was a problem hiding this comment.
Here are some more minor comments. Really close.
|
Done @loiseaujc |
jvdp1
left a comment
There was a problem hiding this comment.
Overall it looks good to me. Here are a few suggestions.
| `b`: Shall be a rank-1 array of the same kind as `a`, containing the right-hand-side vector. It is an `intent(in)` argument. | ||
|
|
||
| `prefactored_w` (optional): Shall be an input `logical` flag. If `.true.`, `w` is assumed to contain a matrix square root \( B \) such that \( W = B \cdot B^T \). This can be a Cholesky factor or any other valid square root (e.g., SVD-based). Default: `.false.`. This is an `intent(in)` argument. | ||
|
|
There was a problem hiding this comment.
why asking that to the user, and not to do it internally?
If asked to the user, should there be an internal check to ensure that it is really the case?
jvdp1
left a comment
There was a problem hiding this comment.
Thank you @aamrindersingh . Here are some additional comments. It is taking a good shape!
| W(3,:) = [0.25_dp, 0.5_dp, 1.0_dp] | ||
|
|
||
| ! Solve generalized least-squares | ||
| call solve_generalized_lstsq(W, A, b, x) |
There was a problem hiding this comment.
could this be combined in a single example file?
| !> [optional] state return flag. On error if not requested, the code will stop | ||
| type(linalg_state_type), optional, intent(out) :: err | ||
| !> Result array x[n] | ||
| ${rt}$, allocatable, target :: x(:) |
There was a problem hiding this comment.
| ${rt}$, allocatable, target :: x(:) | |
| ${rt}$, allocatable :: x(:) |
|
@jvdp1 Done with the fixes |
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
perazz
left a comment
There was a problem hiding this comment.
Couple comments to remove an unnecessary allocation. lgtm otherwise
| logical(lk) :: copy_a, copy_w, is_prefactored | ||
| ${rt}$, pointer :: amat(:,:), lmat(:,:) | ||
| ${rt}$, allocatable, target :: amat_alloc(:,:), lmat_alloc(:,:) | ||
| ${rt}$, allocatable :: d(:), y(:), work(:) |
There was a problem hiding this comment.
Two work allocations will be too expensive. let's use a static variable for work size query
| ${rt}$, allocatable :: d(:), y(:), work(:) | |
| ${rt}$, allocatable :: d(:), y(:), work(:) | |
| ${rt}$ :: work_size(1) |
| allocate(work(1)) | ||
| call ggglm(m, n, p, amat, lda, lmat, ldb, d, x, y, work, -1_ilp, info) | ||
| lwork = ceiling(real(work(1), kind=${rk}$), kind=ilp) | ||
| deallocate(work) | ||
| allocate(work(lwork)) |
There was a problem hiding this comment.
Avoid double allocation here
| allocate(work(1)) | |
| call ggglm(m, n, p, amat, lda, lmat, ldb, d, x, y, work, -1_ilp, info) | |
| lwork = ceiling(real(work(1), kind=${rk}$), kind=ilp) | |
| deallocate(work) | |
| allocate(work(lwork)) | |
| call ggglm(m, n, p, amat, lda, lmat, ldb, d, x, y, work_size, -1_ilp, info) | |
| lwork = ceiling(real(work_size(1), kind=${rk}$), kind=ilp) | |
| allocate(work(lwork)) |
|
@aamrindersingh : Would you have time to address the last comments and resolve the conflicts ? |
Resolves #1047 (2 of 2)
This PR adds the
generalized_lstsqinterface tostdlib_linalgfor least-squares problems with correlated errors described by a symmetric positive definite covariance matrix. Usage:x = generalized_lstsq(W, A, b). This minimizes the Mahalanobis distance(Ax-b)^T W^{-1} (Ax-b)using LAPACK'sGGGLM.The key design decisions are:
generalized_lstsqalways copiesWinternally to protect user data fromGGGLM's destructive operations. This applies regardless of theprefactored_wflag, the inputWmatrix is never modified.overwrite_apattern fromsolve_luwherecopy_adefaults to.true.to preserveAunless the user explicitly opts into destruction for performance.handle_ggglm_infoerror handler was added to parse LAPACK return codes, consistent with existinghandle_gelsd_infoandhandle_gglse_infopatterns.Testing includes:
lstsqpatterns