-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Add det(Symmetric/Hermitian) based on bkfact. #13166
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
Add det(Symmetric/Hermitian) based on bkfact.
@@ -117,8 +117,16 @@ A_mul_B!{T<:BlasComplex,S<:StridedMatrix}(C::StridedMatrix{T}, A::StridedMatrix{ | |||
*(A::HermOrSym, B::HermOrSym) = full(A)*full(B) | |||
*(A::StridedMatrix, B::HermOrSym) = A*full(B) | |||
|
|||
factorize(A::HermOrSym) = bkfact(A.data, symbol(A.uplo), issym(A)) | |||
bkfact(A::HermOrSym) = bkfact(A.data, symbol(A.uplo), issym(A)) | |||
factorize(A::HermOrSym) = bkfact(A) |
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 this not try cholfact
first @andreasnoack ? Since this is the generic factorization method. Currently:
julia> A = rand(Complex128, 4, 4); A = A'A; H = Hermitian(A);
julia> typeof(factorize(A))
Base.LinAlg.Cholesky{Complex{Float64},Array{Complex{Float64},2}}
julia> typeof(factorize(H))
Base.LinAlg.BunchKaufman{Complex{Float64},Array{Complex{Float64},2}}
and then change this and this to bkfact(Hermitian(A))
and bkfact(Symmetric(A))
?
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.
cholfact can fail, bunch kaufman won't, so why would starting with cholfact be better?
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.
This type of check is already happening in factorize
? Since factorize
is a general method which essentially should return the "best" factorization I think the same thing should apply if you have a Hermitian
input matrix?
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.
(And since #21976 cholfact
does not throw)
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.
it doesn't throw, but it gives you an answer that you can't use for many indefinite inputs
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.
That is already what is happening (unless the matrix is annotated with Hermitian
/Symmetric
). Are you suggesting we should not even try the Cholesky factorization in factorize and do this:
diff --git a/base/linalg/dense.jl b/base/linalg/dense.jl
index ab2dac2..c39940f 100644
--- a/base/linalg/dense.jl
+++ b/base/linalg/dense.jl
@@ -768,12 +768,7 @@ function factorize(A::StridedMatrix{T}) where T
return UpperTriangular(A)
end
if herm
- cf = cholfact(A)
- if cf.info == 0
- return cf
- else
- return factorize(Hermitian(A))
- end
+ return factorize(Hermitian(A))
end
if sym
return factorize(Symmetric(A))
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.
it's a bit of a judgement call on how frequent you expect indefinite input to be and worth benchmarking the cost of trying cholesky before bunch kaufman vs going to bunch kaufman first. The latter has a faster worst case but can be a bit slower on inputs that cholesky works for, which may be more common in some fields
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.
Okay, what I am really after is that my interpretation of the documentation is that this should return a Cholesky
factorization since H
is positive definite:
julia> A = rand(3, 3); A = A'A; H = Hermitian(A);
julia> isposdef(H)
true
julia> typeof(factorize(H))
Base.LinAlg.BunchKaufman{Float64,Array{Float64,2}}
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.
My original suggestion here would also make it consistent with sparse matrices https://github.com/JuliaLang/julia/blob/master/base/sparse/linalg.jl#L927-L942
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.
yeah, part of the problem is cholmod can't do sparse bkfact, there are other libraries but it would take some thought to figure out how to make them sanely coexist
Fixes JuliaLang/LinearAlgebra.jl#252.