Skip to content

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