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

SparseMatrixCSC constructors for structured matrices - Diagonal, Tridiagonal, etc. #32466

Merged
merged 5 commits into from
Jul 3, 2019
Merged

SparseMatrixCSC constructors for structured matrices - Diagonal, Tridiagonal, etc. #32466

merged 5 commits into from
Jul 3, 2019

Conversation

dkarrasch
Copy link
Member

This keeps identical functionality, but makes the specialized constructors for structured matrices such as Diagonal, Bidiagonal etc. available to the broadcast machinery.

Fixes #31770.

@ViralBShah ViralBShah added the sparse Sparse arrays label Jul 1, 2019
Copy link
Member

@ViralBShah ViralBShah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. It would be good to get some tests in and merge.

@dkarrasch
Copy link
Member Author

dkarrasch commented Jul 3, 2019

There exist these:

@testset "issue #10837, sparse constructors from special matrices" begin
T = Tridiagonal(randn(4),randn(5),randn(4))
S = sparse(T)
@test norm(Array(T) - Array(S)) == 0.0
T = SymTridiagonal(randn(5),rand(4))
S = sparse(T)
@test norm(Array(T) - Array(S)) == 0.0
B = Bidiagonal(randn(5),randn(4),:U)
S = sparse(B)
@test norm(Array(B) - Array(S)) == 0.0
B = Bidiagonal(randn(5),randn(4),:L)
S = sparse(B)
@test norm(Array(B) - Array(S)) == 0.0
D = Diagonal(randn(5))
S = sparse(D)
@test norm(Array(D) - Array(S)) == 0.0
end

Since sparse calls SparseMatrixCSC they are tested already. Should I simply duplicate these tests and replace each sparse by SparseMatrixCSC or is it enough what we have.

On a different note, I have code that does what was asked for in #10843 (comment). I could add it here and leave the individual commits for review, or open another PR.

Edit: It may make sense to merge this as is and then have another PR in which the SparseMatrixCSC constructors are changed. The speed-up is about a factor of 10, so worth it, I'd say.

@ViralBShah ViralBShah changed the title Move "sparse" code for structured matrices to "SparseMatrixCSC" SparseMatrixCSC constructors for structured matrices - Diagonal, Tridiagonal, etc. Jul 3, 2019
@ViralBShah ViralBShah merged commit a17d9ae into JuliaLang:master Jul 3, 2019
@ViralBShah
Copy link
Member

I agree. Merged this one in - but we can have further optimizations if you already have them in a separate PR.

@Roger-luo
Copy link
Contributor

Will this be backported?

@ViralBShah
Copy link
Member

Triage should decide whether this goes into 1.2.

@StefanKarpinski StefanKarpinski added triage This should be discussed on a triage call and removed triage This should be discussed on a triage call labels Jul 8, 2019
@StefanKarpinski
Copy link
Member

This is a feature, so no.

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

Successfully merging this pull request may close these issues.

Very Poor Performance of Diagonal + SparseMatrixCSC
4 participants