-
Notifications
You must be signed in to change notification settings - Fork 216
feat(linalg): add generalized least squares solver #1105
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: master
Are you sure you want to change the base?
Changes from 6 commits
9e16d37
82ab371
fbb6c69
703ae17
4764aa9
296727a
5a70184
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| ! Generalized least-squares solver with correlated errors | ||
| program example_generalized_lstsq | ||
| use stdlib_linalg_constants, only: dp | ||
| use stdlib_linalg, only: generalized_lstsq | ||
| implicit none | ||
|
|
||
| real(dp) :: A(3,2), b(3), W(3,3) | ||
| real(dp), allocatable :: x(:) | ||
|
|
||
| ! Design matrix: intercept + slope | ||
| A(:,1) = 1.0_dp | ||
| A(:,2) = [1.0_dp, 2.0_dp, 3.0_dp] | ||
|
|
||
| ! Observations | ||
| b = [1.0_dp, 2.1_dp, 2.9_dp] | ||
|
|
||
| ! Covariance matrix (correlated errors) | ||
| W(1,:) = [1.0_dp, 0.5_dp, 0.25_dp] | ||
| W(2,:) = [0.5_dp, 1.0_dp, 0.5_dp] | ||
| W(3,:) = [0.25_dp, 0.5_dp, 1.0_dp] | ||
|
|
||
| ! Solve generalized least-squares | ||
| x = generalized_lstsq(W, A, b) | ||
|
|
||
| print '("GLS fit: intercept = ",f8.4,", slope = ",f8.4)', x(1), x(2) | ||
| ! GLS fit: intercept = 0.0500, slope = 0.9500 | ||
|
|
||
| end program example_generalized_lstsq |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,10 +7,10 @@ | |||||
| submodule (stdlib_linalg) stdlib_linalg_least_squares | ||||||
| !! Least-squares solution to Ax=b | ||||||
| use stdlib_linalg_constants | ||||||
| use stdlib_linalg_lapack, only: gelsd, gglse, stdlib_ilaenv | ||||||
| use stdlib_linalg_lapack_aux, only: handle_gelsd_info, handle_gglse_info | ||||||
| use stdlib_linalg_lapack, only: gelsd, gglse, ggglm, stdlib_ilaenv | ||||||
| use stdlib_linalg_lapack_aux, only: handle_gelsd_info, handle_gglse_info, handle_ggglm_info | ||||||
| use stdlib_linalg_state, only: linalg_state_type, linalg_error_handling, LINALG_ERROR, & | ||||||
| LINALG_INTERNAL_ERROR, LINALG_VALUE_ERROR | ||||||
| LINALG_INTERNAL_ERROR, LINALG_VALUE_ERROR, LINALG_SUCCESS | ||||||
| implicit none | ||||||
|
|
||||||
| character(*), parameter :: this = 'lstsq' | ||||||
|
|
@@ -564,4 +564,135 @@ submodule (stdlib_linalg) stdlib_linalg_least_squares | |||||
| end function stdlib_linalg_${ri}$_constrained_lstsq | ||||||
| #:endfor | ||||||
|
|
||||||
| !------------------------------------------------------------- | ||||||
| !----- Generalized Least-Squares Solver ----- | ||||||
| !------------------------------------------------------------- | ||||||
|
|
||||||
| #:for rk,rt,ri in RC_KINDS_TYPES | ||||||
| ! Generalized least-squares: minimize (Ax-b)^T W^{-1} (Ax-b) where W is symmetric/Hermitian positive definite | ||||||
| module function stdlib_linalg_${ri}$_generalized_lstsq(w,a,b,prefactored_w,overwrite_a,err) result(x) | ||||||
| !> Covariance matrix W[m,m] (symmetric/Hermitian positive definite) or its lower triangular Cholesky factor | ||||||
| ${rt}$, intent(in) :: w(:,:) | ||||||
| !> Input matrix a[m,n] | ||||||
| ${rt}$, intent(inout), target :: a(:,:) | ||||||
| !> Right hand side vector b[m] | ||||||
| ${rt}$, intent(in) :: b(:) | ||||||
| !> [optional] Is W already Cholesky-factored? Default: .false. | ||||||
| logical(lk), optional, intent(in) :: prefactored_w | ||||||
| !> [optional] Can A data be overwritten and destroyed? | ||||||
| logical(lk), optional, intent(in) :: overwrite_a | ||||||
| !> [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 :: x(:) | ||||||
|
|
||||||
| ! Local variables | ||||||
| type(linalg_state_type) :: err0 | ||||||
| integer(ilp) :: m, n, p, lda, ldb, lwork, info, i, j | ||||||
| logical(lk) :: copy_a, is_prefactored | ||||||
| ${rt}$, pointer :: amat(:,:) | ||||||
| ${rt}$, allocatable, target :: amat_alloc(:,:) | ||||||
| ${rt}$, allocatable :: lmat(:,:), d(:), y(:), work(:) | ||||||
| ${rt}$, parameter :: zero = 0.0_${rk}$ | ||||||
| character(*), parameter :: this = 'generalized_lstsq' | ||||||
|
|
||||||
| m = size(a, 1, kind=ilp) | ||||||
| n = size(a, 2, kind=ilp) | ||||||
| p = m ! For GLS, B is m×m | ||||||
|
|
||||||
| ! Allocate result early (prevents segfault on error return) | ||||||
| allocate(x(n)) | ||||||
|
|
||||||
| ! Validate matrix dimensions | ||||||
| if (m < 1 .or. n < 1) then | ||||||
| err0 = linalg_state_type(this, LINALG_VALUE_ERROR, 'Invalid matrix size a(m, n) =', [m, n]) | ||||||
| call linalg_error_handling(err0, err) | ||||||
| return | ||||||
| end if | ||||||
|
|
||||||
| ! Validate sizes | ||||||
| if (size(w, 1, kind=ilp) /= m .or. size(w, 2, kind=ilp) /= m) then | ||||||
| err0 = linalg_state_type(this, LINALG_VALUE_ERROR, & | ||||||
| 'Covariance matrix must be square m×m:', [size(w, 1, kind=ilp), size(w, 2, kind=ilp)]) | ||||||
|
||||||
| 'Covariance matrix must be square m×m:', [size(w, 1, kind=ilp), size(w, 2, kind=ilp)]) | |
| 'Covariance matrix must be square m×m:', shape(w, kind=ilp)) |
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.
Done
Outdated
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.
Should we provide an overwrite_w logical argument, the same way we do for overwrite_a? @loiseaujc that would allow to avoid internal allocation if the user can afford that.
We have strived to allow users to pass all necessary variables as optional arguments and avoid internal allocations wherever possible, here we have at least 4 separate allocations at every call. Perhaps this could be avoided?
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.
Yep, that would seem consistent with the rest of the stdlib_linalg stuff. As usual, overwrite_w would default to .false. just to make sure only users who know what they're doing are actually overwriting.
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.
@perazz @loiseaujc
Added overwrite_w (default .false.), matching overwrite_a pattern. Both main matrix allocations are now user controllable.
Are any other changes required?
Outdated
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.
Since you use cholesky(lmat, lower=.true., other_zeroed=.true.), you do not need to zero-out lmat again. cholesky does it for you.
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.
@loiseaujc
the do concurrent zeroing is in the else branch and it only runs when prefactored_w=.true.
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.
Went Through other stdlib functions , Should I remove the else branch and trust user provides proper lower triangular L?
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.
Two possibilities there:
- Dumb-proof the code and effectively lower the upper triangular part of the user provided matrix just to make sure.
or
- Specify clearly in the specs that, if already prefactored, the upper triangular part of
$W$ needs to be zero.
@perazz @jalvesz @jvdp1 : What would be your take on this and user-friendliness in general for such issues?
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.
Since lmat is an internal allocatable variable, the simplest solution would be:
Allocate lmat accordingly with source=zero, assign the lower symmetric values from w systematically. Just check if (.not. is_prefactored) to perform the cholesky factorization.
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.
@loiseaujc I agree: when w is a user-provided prefactored matrix, we should not modify that. It is the user's responsibility to provide a correct input matrix (perhaps we could state that in the documentation). It would be also worthwhile to check if DGGGLM actually does access the upper triangular part or not - perhaps this zeroing is not even necessary
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.
xGGGLM solves the following constrained quadratic program
!> minimize || y ||_2 subject to d = A*x + B*y
!> x
where A and B are generic matrices. The generalized least-squares problem can be recast in this form where B is a square root of the symmetric positive definite weight matrix W. Since xGGGLM works for arbitrary matrices B, we indeed do need to zero-out the upper triangular part if B is a Cholesky factor.
But I actually agree that we should not touch the user-provided matrix. One simple reason is that the Cholesky factor is not the only way to express the matrix square-root ! Maybe for some reason, user has computed it with svd. It would be a valid input and the routine should still work.
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.
@aamrindersingh : Following with previous comment, could you add a test where the matrix square root of W is computed using its svd followed by taking the non-negative square-root of the singular values ? If I'm correct, it should return the same solution as if the Cholesky factor of W was used.
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.
Done,
Removed the upper triangle zeroing and added an eigendecomposition based sqrt test. Confirms both give identical solutions as expected. Used eigh since it is equivalent to SVD for SPD matrices.
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.
Please pass
lda,ldbto this routine so theThere 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.
Done