Overload congruence transforms for AbstractPDMat return types #222
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #222 +/- ##
==========================================
+ Coverage 93.20% 93.62% +0.42%
==========================================
Files 9 10 +1
Lines 736 769 +33
==========================================
+ Hits 686 720 +34
+ Misses 50 49 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sethaxen
changed the title
| return ScalMat(A.dim, abs2(B.value) * A.value) | ||
| end | ||
| $(f)(A::PDiagMat, B::PDiagMat) = PDiagMat(abs2.(B.diag) .* A.diag) | ||
| $(f)(A::PDiagMat, B::ScalMat) = PDiagMat(abs2(B.value) .* A.diag) |
There was a problem hiding this comment.
AFAICT we also have to check that dimensions match in this function:
| $(f)(A::PDiagMat, B::ScalMat) = PDiagMat(abs2(B.value) .* A.diag) | |
| function $(f)(A::PDiagMat, B::ScalMat) | |
| @check_argdims size(A, 1) == B.dim | |
| return PDiagMat(abs2(B.value) .* A.diag) | |
| end |
| uplo = A.chol.uplo | ||
| if uplo === 'U' | ||
| factors = A.chol.factors * B.value' | ||
| else | ||
| factors = B.value * A.chol.factors | ||
| end | ||
| chol = Cholesky(factors, uplo, A.chol.info) |
There was a problem hiding this comment.
Since ScalMat is a AbstractMatrix{<:Real}, it seems we could merge the two branches:
| uplo = A.chol.uplo | |
| if uplo === 'U' | |
| factors = A.chol.factors * B.value' | |
| else | |
| factors = B.value * A.chol.factors | |
| end | |
| chol = Cholesky(factors, uplo, A.chol.info) | |
| factors = B.value * A.chol.factors | |
| chol = Cholesky(factors, A.chol.uplo, A.chol.info) |
| chol = Cholesky(factors, uplo, A.chol.info) | ||
| return PDMat(mat, chol) | ||
| end | ||
| end |
There was a problem hiding this comment.
For completeness, probably we should also add definitions for first arguments of types PDSparseMat and Cholesky.
| @check_argdims A.dim == size(B, 1) | ||
| return PDiagMat(B.diag .* (A.value .\ B.diag)) | ||
| end | ||
| end |
There was a problem hiding this comment.
What's the reason to omit definitions for PDMat (and PDSparseMat and Cholesky)? That they wouldn't be more efficient than the generic definitions?
There was a problem hiding this comment.
For PDMat, PDMat yes, the result is a PDMat, but we can't reuse the decomposition, so if we construct a PDMat, then it requires a cholesky call. I think this is worse than outputting a Matrix.
For PDSparseMat, ScalMat, the problem is that only \ is implemented for SparseArrays.CHOLMOD.FactorComponent and only with dense vectors/matrices; we have no way to modify the entries of the factor, so it seems better not to overload it.
I can add Cholesky overloads.
As discussed in #132 (comment), this PR overloads
X_A_Xt,Xt_A_X,X_invA_Xt, andXt_invA_Xto return the appropriate PDMats type when we can guarantee the return type is anAbstractPDMatand when it doesn't require computing a new decomposition. Fixes #132.