-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
inv of Symmetric/Hermitian with more eltypes than BlasFloat #22882
Conversation
this makes #22686 (comment) more of an issue |
I find it unlikely that a matrix which is a result of Lines 317 to 318 in 8cd3c3f
|
test/linalg/symmetric.jl
Outdated
@test inv(Symmetric(asym))::Symmetric ≈ inv(asym) | ||
@test inv(Hermitian(aherm))::Hermitian ≈ inv(aherm) | ||
for uplo in (:U, :L) | ||
@test inv(Symmetric(asym, uplo))::Symmetric ≈ inv(full(Symmetric(asym, uplo))) |
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.
Perhaps Array
instead of full
? :)
The previous fallback assumes that the inverse will have the same element type as the factorization, so this should not be a problem. |
56c5122
to
b324d0d
Compare
b324d0d
to
b23fc3e
Compare
Do you still think this is problematic @tkelman, or anyone else? |
Main concern for me is whether this requires additional methods to be defined for it to work on inv of symmetric/hermitian wrappers of user defined array and/or element types |
Okay, yea, I did some testing, and for a user defined number type, this does not change anything; if you define all that's needed for |
@@ -364,8 +364,8 @@ function _inv(A::HermOrSym) | |||
end | |||
B | |||
end | |||
inv(A::Hermitian{T,S}) where {T<:BlasFloat,S<:StridedMatrix} = Hermitian{T,S}(_inv(A), A.uplo) | |||
inv(A::Symmetric{T,S}) where {T<:BlasFloat,S<:StridedMatrix} = Symmetric{T,S}(_inv(A), A.uplo) | |||
inv(A::Hermitian{<:Any,<:StridedMatrix}) = Hermitian(_inv(A), Symbol(A.uplo)) |
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.
Re my comment above regarding user define array types, should this be changed to:
inv(A::Hermitian{<:Any,S}) where {S<:StridedMatrix} = (Ai = _inv(A); Hermitian{eltype(Ai), S}(Ai, A.uplo)
such that we enforce a conversion back to the same array type? I noticed that if you don't implement getindex
to return your user defined array you get back an Array
from the backsolve.
Edit: No, this does not work, it is not always possible to convert back to the same type, e.g. with Int
s. So I think the current implementation is the best.
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.
👍! :)
Restarting Travis since there was some time this went through. Hoping to get more than 0 out of 3 this time |
Nope. None your fault though. |
#22686 and #22767 implemented the infrastructure to invert
Symmetric
/Hermitian
matrices with any element type that supports LU factorization (and backsolve). The relaxations here are necessary as well. For instance, before thisinv(::Symmetric{Int})
returned aMatrix
rather thanSymmetric
andinv(::Symmetric{Int})
MethodError
d.