-
Notifications
You must be signed in to change notification settings - Fork 2
Add MatrixAlgebraKit svd
#111
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
- Coverage 75.07% 72.22% -2.85%
==========================================
Files 27 27
Lines 1063 1170 +107
==========================================
+ Hits 798 845 +47
- Misses 265 325 +60
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Besides the last comment I think this is good to go, though it needs formatting. |
Co-authored-by: Matt Fishman <[email protected]>
Very nice! |
This PR replaces the previous
svd
functionality withMatrixAlgebraKit.svd_compact
andMatrixAlgebraKit.svd_full
.The main idea is to have a
BlockDiagonalAlgorithm
wrappertype to dispatch on, which flags that we will assume the inputAbstractBlockSparseMatrix
is either blockdiagonal or can be permuted to that form.The main reason I chose this approach is that there really is no dedicated
BlockDiagonal
functionality, and I wanted to implement the factorizations without having to first fix a bunch of utility functions for that specific type.There definitely is a bunch of bookkeeping involved with this, especially for the
full
decomposition combined with the ability of empty rows and columns and rectangular inputs.I'd say the fishiest part of this PR is the added function for
GetUnstoredBlock
whenDiagonal
matrices are involved, but I feel like this is warranted since that is also howLinearAlgebra.jl
handles this.I'm curious to hear what you think about this, I'd suggest trying to figure this out first and getting this merged, and adding the truncation and the other factorizations in follow-up PRs.
The truncation should be somewhat orthogonal to this anyways, and the other factorizations should almost be copies of this one so getting this figured out first seems reasonable.
Also, I deleted the old
SVD
implementation since I don't think that is a durable approach in the long run.This does make it a breaking change, which in principle isn't entirely necessary.