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.

Sign up for GitHub

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

Jump to bottom

improve performance for Matrix -> SparseMatrix #26334

Merged
merged 3 commits into from
Mar 7, 2018
Merged

improve performance for Matrix -> SparseMatrix #26334

merged 3 commits into from
Mar 7, 2018

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Mar 6, 2018 •
edited
Loading

Benchmark (sparse2 is PR):

A1 = rand(3000, 3000)
A2 = Matrix(sprand(3000, 3000, 0.1))
A3 = Matrix(sprand(3000, 3000, 0.01))

@btime sparse(A1)
  114.613 ms (11 allocations: 206.04 MiB)
@btime sparse(A2)
  27.586 ms (11 allocations: 20.61 MiB)
@btime sparse(A3)
  14.071 ms (11 allocations: 2.09 MiB)

@btime sparse2(A1)
  61.808 ms (7 allocations: 137.35 MiB)
@btime sparse2(A2)
  22.060 ms (7 allocations: 13.73 MiB)
@btime sparse2(A3)
  11.622 ms (7 allocations: 1.39 MiB

@KristofferC KristofferC requested a review from andreasnoack March 6, 2018 10:38
KristofferC
KristofferC commented Mar 6, 2018
View reviewed changes
stdlib/SparseArrays/src/sparsematrix.jl Outdated
@@ -376,6 +376,27 @@ function SparseMatrixCSC{Tv,Ti}(M::AbstractMatrix) where {Tv,Ti}
eltypeTvV = convert(Vector{Tv}, V)
return sparse_IJ_sorted!(eltypeTiI, eltypeTiJ, eltypeTvV, size(M)...)
end
function SparseMatrixCSC{Tv,Ti}(M::Matrix) where {Tv,Ti}
nz = count(!equalto(0), M)
Copy link
Member Author

@KristofferC KristofferC Mar 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This count is needed to avoid using push! which unfortunately is slow due to its overhead (#24909).

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you want !equalto(zero(Tv)) here? Thinking about matrices with SVector entries.

Copy link
Member

Choose a reason for hiding this comment

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

#24790

chethega
chethega reviewed Mar 6, 2018
View reviewed changes
stdlib/SparseArrays/src/sparsematrix.jl Outdated
@@ -376,6 +376,27 @@ function SparseMatrixCSC{Tv,Ti}(M::AbstractMatrix) where {Tv,Ti}
eltypeTvV = convert(Vector{Tv}, V)
return sparse_IJ_sorted!(eltypeTiI, eltypeTiJ, eltypeTvV, size(M)...)
end
function SparseMatrixCSC{Tv,Ti}(M::Matrix) where {Tv,Ti}
nz = count(!equalto(0), M)
Copy link
Contributor

Choose a reason for hiding this comment

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

tv might not be concrete, and the matrix may contain missing or (more realistically) be have SVector entries.

Maybe nz = count(x->true===iszero(x), M)? That way we only lose type information of zeros.

Not sure about performance implications (sorry for double-posting, failed at github-ui).

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is a pure performance improvement over the current function which uses findnz which does nnzA = count(t -> t != 0, A) and if Aij != 0. Changing the sparse matrix library to work more complicated elements is out of scope for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Note that equalto uses isequal, so this will change the behavior for -0.0.

CodeLenz
CodeLenz reviewed Mar 6, 2018
View reviewed changes
stdlib/SparseArrays/src/sparsematrix.jl
end
return SparseMatrixCSC(size(M, 1), size(M, 2), colptr, rowval, nzval)
end

# converting from SparseMatrixCSC to other matrix types
function Matrix(S::SparseMatrixCSC{Tv}) where Tv
Copy link

@CodeLenz CodeLenz Mar 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hi.

You are using

size(M, 2)

3 times and

size(M,1)

two times. Would not be faster to define two variables with those values and than use them? Or the interpreter can optimize it anyway ?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a valid comment. However, in this case, even if the compiler doesn't optimize it away, the time to access these fields should be completely insignificant to the rest of the code. The question here is if it makes the code more legible to store them in m and n, for example. I'm not sure.

Copy link

Choose a reason for hiding this comment

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

Indeed! Thanks for your answer. Sometimes I wonder if the loops can be (somehow) optimized in those cases.

@mbauman mbauman added sparse Sparse arrays performance Must go faster labels Mar 6, 2018
@mbauman
Copy link
Member

mbauman commented Mar 6, 2018

Is there any reason why we wouldn't use this implementation for all AbstractMatrixes? I suppose it'd probably be slower for sparse matrices… but most every other matrix implementation should be faster like this, no?

@KristofferC
Copy link
Member Author

There are so many types of matrices I am not sure exactly what you need to fulfill the API. For example, this assumes column major and that indices starts at 1 etc. I was just playing it safe with Matrix.

@andreasnoack
Copy link
Member

Extending to StridedMatrix should be safe though.

@andreasnoack andreasnoack merged commit 6e0c299 into master Mar 7, 2018
@andreasnoack andreasnoack deleted the kc/array_to_sparse branch March 7, 2018 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chethega chethega chethega left review comments

@fredrikekre fredrikekre fredrikekre left review comments

@CodeLenz CodeLenz CodeLenz left review comments

@nalimilan nalimilan nalimilan left review comments

@andreasnoack andreasnoack andreasnoack approved these changes

Assignees
No one assigned
Labels
performance Must go faster sparse Sparse arrays
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@KristofferC @mbauman @andreasnoack @nalimilan @CodeLenz @fredrikekre @chethega