carry out (c)transpose in Ax_mul_Bx methods for Hermitian and Symmetric #22396
Conversation
fredrikekre
force-pushed
the
fe/symmetric-hermitian
branch
from
94d543e to
696a123
Compare
|
This introduced a lot of ambiguous methods so tests will fail... Will have a look at that tomorrow. |
base/linalg/symmetric.jl
Outdated
|
|
||
| # Fallbacks to avoid generic_matvecmul!/generic_matmatmul! | ||
| ## Symmetric and Hermitian{<:Real} are invariant to transpose and for | ||
| ## Hermitian{<:Real} it is faster to carry out the transposition |
fredrikekre
force-pushed
the
fe/symmetric-hermitian
branch
from
696a123 to
19e766a
Compare
|
Rebased and fixed ambiguities. The first iteration of this PR was not very well thought through, so I decided to only include the trivial cases in this PR and think a bit more on what to do with the remaining methods. I updated the benchmarks in the top post. |
base/linalg/symmetric.jl
Outdated
| @@ -301,6 +302,25 @@ A_mul_B!(C::StridedMatrix{T}, A::StridedMatrix{T}, B::Hermitian{T,<:StridedMatri | |||
|
|
|||
| *(A::HermOrSym, B::HermOrSym) = A*full(B) | |||
|
|
|||
| # Fallbacks to avoid generic_matvecmul!/generic_matmatmul! | |||
| ## Symmetric and Hermitian{<:Real} are invariant to transpose; peel of the t | |||
fredrikekre
force-pushed
the
fe/symmetric-hermitian
branch
from
19e766a to
ba78532
Compare
base/linalg/diagonal.jl
Outdated
| A_mul_Bt(A::Diagonal, B::RealHermSymComplexSym) = A*B | ||
| At_mul_B(A::RealHermSymComplexSym, D::Diagonal) = A*B | ||
| A_mul_Bc(A::Diagonal, B::RealHermSymComplexHerm) = A*B | ||
| Ac_mul_B(A::RealHermSymComplexHerm, D::Diagonal) = A*B |
There was a problem hiding this comment.
Are these definitions correct (considering the recursive nature of [c]transpose)?
There was a problem hiding this comment.
and given the second input was named D, not B - this method must not have been covered in tests?
There was a problem hiding this comment.
Are these definitions correct (considering the recursive nature of
[c]transpose)?
I didn't think about that... The t in At_mul_B methods are also considered to be recursive, right?
and given the second input was named D, not B - this method must not have been covered in tests?
Should never do changes like that in the last minute... Will add some tests.
There was a problem hiding this comment.
Are these definitions correct (considering the recursive nature of
[c]transpose)?
Turns out it is correct after all -- definition of the alias RealHermSymComplexHerm only allows element types <:Number so we will never dispatch here when the element type is a matrix for example (and thus this should not affect the behavior of block matrices). Probably worth adding a comment.
|
Is this expected beavior: julia> A = [rand(2, 2) for i in 1:2, j in 1:2]; S = Symmetric(A);
julia> issymmetric(S)
true
julia> issymmetric(Matrix(S))
false |
|
So interaction with |
fredrikekre
force-pushed
the
fe/symmetric-hermitian
branch
2 times, most recently
from
9cadce7 to
8e81d79
Compare
|
Rebased after #22474 so I think this is good to go now. |
base/linalg/diagonal.jl
Outdated
| A_mul_Bt(A::Diagonal, B::RealHermSymComplexSym) = A*B | ||
| At_mul_B(A::RealHermSymComplexSym, D::Diagonal) = A*B | ||
| A_mul_Bc(A::Diagonal, B::RealHermSymComplexHerm) = A*B | ||
| Ac_mul_B(A::RealHermSymComplexHerm, D::Diagonal) = A*B |
There was a problem hiding this comment.
Great... forgot to change D -> B....
Good thing we have tests!
…mitian and Symmetric
- Symmetric and Hermitian{<:Real} are invariant to transpose so it is a no-op
- Hermitian and Symmetric{<:Real} are invariant to ctranspose so it is a no-op
fredrikekre
force-pushed
the
fe/symmetric-hermitian
branch
from
8e81d79 to
2f95f42
Compare
|
Timeout on travis 32bit |
|
should some benchmarks be added to track that this continues dispatching the way it should? |
KristofferC
added
the
potential benchmark
Benchmarks: https://gist.github.com/fredrikekre/03f31913dd322f4c245740f3f6894436
transpose(A::Hermitian{<:Complex})andctranspose(A::Symmetric{<:Complex})will allocate more here. (But that amount is comparable to element type promotion so I think it is okay?). It is still faster to be able to use BLAS instead of the generic methods.(This change only makes sense after #22373 so will rebase after that is merged)