Skip to content
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

Kron of transpose/adjoint of sparse matrix with other sparse matrix becomes dense #30271

Closed
jebej opened this issue Dec 4, 2018 · 8 comments
Closed
Labels

Comments

@jebej
Copy link
Contributor

jebej commented Dec 4, 2018

This is because of the kron fallback for AbstractMatrix in LinearAlgebra catching the Transpose{Float64,SparseMatrixCSC{Float64,Int64}} matrix type.

This is a manifestation of the issue noted here #27586 (comment). I had tried a bunch of different signatures in that PR in order to switch to an abstract type instead of VecOrMat, but it caused an ambiguity with the LinearAlgebra method.

Note that this is a regression since 0.6 properly returned a sparse matrix by materializing the transpose/adjoint immediately instead of wrapping.

@jebej
Copy link
Contributor Author

jebej commented Dec 4, 2018

Example for experimentation: kron(adjoint(sprand(4,4,0.2)), sprand(4,4,0.2))

@fredrikekre
Copy link
Member

kron(copy(adjoint(sprand(4,4,0.2))), sprand(4,4,0.2)) should work (and be equivlent to the 0.6 behavior).

@jebej
Copy link
Contributor Author

jebej commented Dec 4, 2018

kron(copy(adjoint(sprand(4,4,0.2))), sprand(4,4,0.2)) should work (and be equivlent to the 0.6 behavior).

Won't this copy unnecessarily if the matrix is dense?

@fredrikekre
Copy link
Member

No? copy is how you materialize an adjoint/transpose wrapped matrix.

@jebej
Copy link
Contributor Author

jebej commented Dec 4, 2018

Ok I did the test, and because sparse ends up being called to create the sparse matrix for kron, it is more efficient to always copy an Adjoint or Transpose.

@jebej
Copy link
Contributor Author

jebej commented Dec 4, 2018

I guess that copy could be done automatically in the SparseMatrixCSC constructor, because calling the constructor directly on Adjoint or Transpose is slow.

@ViralBShah
Copy link
Member

Anyone want to submit a PR for this one?

@ViralBShah
Copy link
Member

It seems like #24980 could be revived.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants