Skip to content

Commit

Permalink
replace broadcast with map in sparse binary op definitions (#19527)
Browse files Browse the repository at this point in the history
* Make some sparse elementwise binary operations short children of map rather than {broadcast + shape check}.

* Test shape checks for sparse elementwise binary operations equivalent to map.
  • Loading branch information
Sacha0 authored and tkelman committed Dec 19, 2016
1 parent 071c60a commit c604d05
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 11 deletions.
19 changes: 8 additions & 11 deletions base/sparse/sparsematrix.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2278,17 +2278,14 @@ round{To}(::Type{To}, A::SparseMatrixCSC) = round.(To, A)

## Binary arithmetic and boolean operators

# TODO: These seven functions should probably be reimplemented in terms of sparse map
# when a better sparse map exists. (And vectorized min, max, &, |, and xor should be
# deprecated in favor of compact-broadcast syntax.)
_checksameshape(A, B) = size(A) == size(B) || throw(DimensionMismatch("size(A) must match size(B)"))
(+)(A::SparseMatrixCSC, B::SparseMatrixCSC) = (_checksameshape(A, B); broadcast(+, A, B))
(-)(A::SparseMatrixCSC, B::SparseMatrixCSC) = (_checksameshape(A, B); broadcast(-, A, B))
min(A::SparseMatrixCSC, B::SparseMatrixCSC) = (_checksameshape(A, B); broadcast(min, A, B))
max(A::SparseMatrixCSC, B::SparseMatrixCSC) = (_checksameshape(A, B); broadcast(max, A, B))
(&)(A::SparseMatrixCSC, B::SparseMatrixCSC) = (_checksameshape(A, B); broadcast(&, A, B))
(|)(A::SparseMatrixCSC, B::SparseMatrixCSC) = (_checksameshape(A, B); broadcast(|, A, B))
xor(A::SparseMatrixCSC, B::SparseMatrixCSC) = (_checksameshape(A, B); broadcast(xor, A, B))
(+)(A::SparseMatrixCSC, B::SparseMatrixCSC) = map(+, A, B)
(-)(A::SparseMatrixCSC, B::SparseMatrixCSC) = map(-, A, B)
# TODO: Vectorized min, max, |, and xor should be deprecated in favor of compact-broadcast syntax.
min(A::SparseMatrixCSC, B::SparseMatrixCSC) = map(min, A, B)
max(A::SparseMatrixCSC, B::SparseMatrixCSC) = map(max, A, B)
(&)(A::SparseMatrixCSC, B::SparseMatrixCSC) = map(&, A, B)
(|)(A::SparseMatrixCSC, B::SparseMatrixCSC) = map(|, A, B)
xor(A::SparseMatrixCSC, B::SparseMatrixCSC) = map(xor, A, B)

(.+)(A::SparseMatrixCSC, B::Number) = Array(A) .+ B
( +)(A::SparseMatrixCSC, B::Array ) = Array(A) + B
Expand Down
11 changes: 11 additions & 0 deletions test/sparse/sparse.jl
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@ do33 = ones(3)
# check sparse binary op
@test all(Array(se33 + convert(SparseMatrixCSC{Float32,Int32}, se33)) == 2*eye(3))
@test all(Array(se33 * convert(SparseMatrixCSC{Float32,Int32}, se33)) == eye(3))
@testset "Shape checks for sparse elementwise binary operations equivalent to map" begin
sqrfloatmat, colfloatmat = sprand(4, 4, 0.5), sprand(4, 1, 0.5)
@test_throws DimensionMismatch (+)(sqrfloatmat, colfloatmat)
@test_throws DimensionMismatch (-)(sqrfloatmat, colfloatmat)
@test_throws DimensionMismatch min(sqrfloatmat, colfloatmat)
@test_throws DimensionMismatch max(sqrfloatmat, colfloatmat)
sqrboolmat, colboolmat = sprand(Bool, 4, 4, 0.5), sprand(Bool, 4, 1, 0.5)
@test_throws DimensionMismatch (&)(sqrboolmat, colboolmat)
@test_throws DimensionMismatch (|)(sqrboolmat, colboolmat)
@test_throws DimensionMismatch xor(sqrboolmat, colboolmat)
end

# check horiz concatenation
@test all([se33 se33] == sparse([1, 2, 3, 1, 2, 3], [1, 2, 3, 4, 5, 6], ones(6)))
Expand Down

2 comments on commit c604d05

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

Please sign in to comment.