-
Notifications
You must be signed in to change notification settings - Fork 197
Pivoting QR decomposition #1045
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?
Conversation
|
I think this PR is almost ready to be reviewed. I have the specs left to write but this should pretty quick as it mainly is a small modification of the |
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.
Pull Request Overview
This PR adds support for QR factorization with column pivoting to the linear algebra library. The implementation leverages the LAPACK geqp3 routine and extends the existing qr interface to support pivot tracking.
- Adds pivoting QR factorization via
geqp3LAPACK routine - Extends
qr()andqr_space()interfaces to support column pivoting with pivot output - Includes comprehensive test coverage for tall, wide, and rank-deficient matrices
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/linalg/test_linalg_pivoting_qr.fypp | Comprehensive test suite for pivoting QR factorization across different matrix types and configurations |
| test/linalg/CMakeLists.txt | Adds pivoting QR test to build system |
| src/stdlib_linalg_qr.fypp | Implements pivoting QR factorization routines and workspace calculation |
| src/stdlib_linalg_lapack.fypp | Adds LAPACK geqp3 interface for QR with column pivoting |
| src/stdlib_linalg.fypp | Extends public interface with pivoting QR subroutines |
| src/lapack/stdlib_linalg_lapack_aux.fypp | Adds error handler for geqp3 LAPACK routine |
| example/linalg/example_pivoting_qr_space.f90 | Example demonstrating pivoting QR with pre-allocated storage |
| example/linalg/example_pivoting_qr.f90 | Basic example demonstrating pivoting QR factorization |
| example/linalg/CMakeLists.txt | Adds pivoting QR examples to build system |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1045 +/- ##
=========================================
Coverage ? 25.12%
=========================================
Files ? 570
Lines ? 234201
Branches ? 41275
=========================================
Hits ? 58834
Misses ? 175367
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Modulo the issue with the |
| ### Syntax | ||
|
|
||
| `call ` [[stdlib_linalg(module):qr(interface)]] `(a, q, r, [, storage] [, overwrite_a] [, err])` | ||
| `call ` [[stdlib_linalg(module):qr(interface)]] `(a, q, r [, pivots] [, overwrite_a] [, storage] [, err])` |
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.
| `call ` [[stdlib_linalg(module):qr(interface)]] `(a, q, r [, pivots] [, overwrite_a] [, storage] [, err])` | |
| `call ` [[stdlib_linalg(module):qr(interface)]] `(a, q, r [, storage] [, overwrite_a] [, pivots] [, err])` |
Excellent work @loiseaujc -
Just a preliminary comment - I will have more time this weekend. I would try to not change the previous API. Adding pivots after overwrite_a instead of swapping it still allows running (no keywords)
call qr(a, q, r, pivots)
because storage is kind(A), so, there is no ambiguity.
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.
I made the change in the docs because the actual interface in stdlib_linalg.fypp reads
pure module subroutine stdlib_linalg_${ri}$_qr(a, q, r, overwrite_a, storage, err)
I am not sure why there would be ambiguity though. pivots is an integer array. The call qr(a, q, r, pivots) is exactly the one used in the example.
program example_pivoting_qr
use stdlib_linalg, only: qr
implicit none
real :: A(104, 32), Q(104, 32), R(32, 32)
integer :: pivots(32)
! Create a random matrix
call random_number(A)
! Compute its QR factorization (reduced)
call qr(A, Q, R, pivots)
! Test factorization: Q*R = A
print *, maxval(abs(matmul(Q, R) - A(:, pivots)))
end program example_pivoting_qr
Following #930, this PR extends the
qrinterface to enable the computation of the QR factorization with column pivoting. It is based on thexGEQP3driver fromlapack.Proposed interfaces
call qr(a, q, r, pivots [, overwrite_a, storage, err])where
ais the matrix to be factorized,qthe orthonormal basis forcolspan(a),rthe upper triangular matrix andpivotsan integer array with indices of the pivoted columns.Progress
Ping: @perazz, @jvdp1, @jalvesz