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

Fix dispatch of SparseMatrixCSC*Diagonal multiplication #29045

Merged
merged 4 commits into from
Sep 5, 2018

Conversation

Pbellive
Copy link
Contributor

@Pbellive Pbellive commented Sep 4, 2018

This fixes #28934. In short, multiplication of a SparseMatrixCSC with a Diagonal matrix was getting dispatched to a generic matmul routine rather than the specialized methods defined at

function mul!(C::SparseMatrixCSC, A::SparseMatrixCSC, D::Diagonal{<:Vector})

because the type signature of the diagonal matrix input in those methods was wrong, as pointed out by @GiggleLiu.

Correctness of results is already tested at

@testset "matrix multiplication" begin

I'm not sure how to write a test that checks that calling, say, A*D for compatible sparse and diagonal matrices dispatches to the right method. Is that doable/necessary?

…ith Diagonal matrices. Type signature for diagonal matrices was wrong, causing fallback to generic Matmul.
@KristofferC
Copy link
Member

KristofferC commented Sep 4, 2018

Some sort of dispatch test could be done with e.g. (B is SparseMatrixCSC and D is Diagonal).

m = @which mul!(B, B, D);
@test m.module == SparseArrays

@Pbellive
Copy link
Contributor Author

Pbellive commented Sep 4, 2018

That dispatch test seems reasonable @KristofferC. Added dispatch tests to the PR.

@andreasnoack andreasnoack merged commit 8d99356 into JuliaLang:master Sep 5, 2018
KristofferC pushed a commit that referenced this pull request Sep 6, 2018
* Fix type signature of mul! methods for multiplying SparseMatrixCSCs with Diagonal matrices. Type signature for diagonal matrices was wrong, causing fallback to generic Matmul.

* Add SparseMatrixCSC*Diagonal dispatch test

* Fix trailing whitespace

* Don't copy with deepcopy

(cherry picked from commit 8d99356)
@KristofferC KristofferC mentioned this pull request Sep 6, 2018
KristofferC pushed a commit that referenced this pull request Sep 8, 2018
* Fix type signature of mul! methods for multiplying SparseMatrixCSCs with Diagonal matrices. Type signature for diagonal matrices was wrong, causing fallback to generic Matmul.

* Add SparseMatrixCSC*Diagonal dispatch test

* Fix trailing whitespace

* Don't copy with deepcopy

(cherry picked from commit 8d99356)
KristofferC pushed a commit that referenced this pull request Sep 8, 2018
* Fix type signature of mul! methods for multiplying SparseMatrixCSCs with Diagonal matrices. Type signature for diagonal matrices was wrong, causing fallback to generic Matmul.

* Add SparseMatrixCSC*Diagonal dispatch test

* Fix trailing whitespace

* Don't copy with deepcopy

(cherry picked from commit 8d99356)
KristofferC pushed a commit that referenced this pull request Feb 11, 2019
* Fix type signature of mul! methods for multiplying SparseMatrixCSCs with Diagonal matrices. Type signature for diagonal matrices was wrong, causing fallback to generic Matmul.

* Add SparseMatrixCSC*Diagonal dispatch test

* Fix trailing whitespace

* Don't copy with deepcopy

(cherry picked from commit 8d99356)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiplication of sparse matrices with diagonal matrices is slow (fix suggested)
3 participants