-
-
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
Fast conversion of wrapped types to SparseMatrixCSC #30552
Conversation
I think, this PR is important, because it allows to avoid the "abstractarray fallback" for wrapped sparse matrices by transforming them to unwrapped sparse matrices. |
Bump ??! |
Just want to say that I am not against this PR, but I have been hesitating merging it because it feels a bit brute force. Maybe that shouldn't be a reason to merge it now and have something better in the future. |
I am not sure I understand. What do you mean by "brute force"? Is that targeted to the coding style? In my eyes, it feels quite good. Maybe it is not appropriate to argument against feelings, but let me give some explanations, why my feeling is positive:
|
Sorry, my comment about brute force is not related to your PR or coding style, but on the need to enumerate all these cases as a shortcoming of Julia itself. I will try to review this and get this merged. |
Thanks, sorry for my misunderstanding. |
I just ran into a slowdown that might be related and wanted to check if it is — I had an Is that kind of indexing performance addressed by this PR? |
This is a related problem, which is not covered by this PR. Instead of |
Can we put these in a separate file, say, sparseconvert.jl, as a reminder that there should be a better way to do this in the future and avoid the combinatorial explosion of things to define? |
Yes, that is a good idea and is more or less what I did. My actual use case is indexing with
Edit: Now I think about it, I should probably use knowledge of the array being a transpose to optimize the row lookup case. |
done. |
Can we avoid exporting |
This PR addresses the issue of slow conversions of wrapped sparse matrices to sparse.
For example see also issue 29353 by @KristofferC.
The case
SparseMatrixCSC(Symmetric(A))
for sparseA
is generalized to wrappersSymmetric, Hermitian, [Unit](Upper|Lower)Triangular, Adjoint, Transpose, SubArray
and arbitrarily nested combinations of those.The operation time is
O(nnz(A) * O(depth(A))
, wheredepth
is the wrapping depth ofA
. All method specializations are type-stable.Additional functions of the type of
A
are defined, which are evaluated at compile time.(
SparseWrappers: depth, iswrsparse
).Another useful function
SparseWrappers.unwrap
is defined to convert a wrapped matrix to aSparseMatrixCSC
or aMatrix
depending on the sparsity status ofA
.Following benchmark results were observed:
Symmetric(A)
Adjoint(A)
UpperTriangular(A)
UnitUpperTriangular(A)
Adjoint(UpperTriangular(A))
Adjoint(UnitUpperTriangular(A))
Symmetric(Adjoint(UnitUpperTriangular(A)))
Symmetric(Adjoint(UnitUpperTriangular(A')))
The complete benchmark listing is attached here for completeness: