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

Make broadcast over sparse and rowvecs sparse #20959

Merged
merged 1 commit into from
Mar 17, 2017
Merged

Make broadcast over sparse and rowvecs sparse #20959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions base/sparse/higherorderfns.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ module HigherOrderFns
# particularly map[!]/broadcast[!] for SparseVectors and SparseMatrixCSCs at present.
import Base: map, map!, broadcast, broadcast!
import Base.Broadcast: _containertype, promote_containertype,
broadcast_indices, broadcast_c, broadcast_c!
broadcast_indices, broadcast_c, broadcast_c!

using Base: front, tail, to_shape
using ..SparseArrays: SparseVector, SparseMatrixCSC, AbstractSparseArray, indtype
using ..SparseArrays: SparseVector, SparseMatrixCSC, AbstractSparseVector,
AbstractSparseMatrix, AbstractSparseArray, indtype

# This module is organized as follows:
# (1) Define a common interface to SparseVectors and SparseMatrixCSCs sufficient for
Expand Down Expand Up @@ -1063,6 +1064,7 @@ promote_containertype(::Type{Array}, ::Type{AbstractSparseArray}) = PromoteToSpa
_spcontainertype(x) = _containertype(x)
_spcontainertype(::Type{<:Vector}) = Vector
_spcontainertype(::Type{<:Matrix}) = Matrix
_spcontainertype(::Type{<:RowVector}) = Matrix
_spcontainertype(::Type{<:Ref}) = AbstractArray
_spcontainertype(::Type{<:AbstractArray}) = AbstractArray
# need the following two methods to override the immediately preceding method
Expand Down Expand Up @@ -1105,10 +1107,11 @@ promote_spcontainertype(::FunnelToSparseBC, ::FunnelToSparseBC) = PromoteToSpars
@inline spbroadcast_c!{N}(f, ::Type{AbstractSparseArray}, ::Type{AbstractArray}, C, B, As::Vararg{Any,N}) =
broadcast_c!(f, Array, Array, C, B, As...)

@inline _sparsifystructured(A::AbstractSparseArray) = A
@inline _sparsifystructured(M::StructuredMatrix) = SparseMatrixCSC(M)
@inline _sparsifystructured(M::Matrix) = SparseMatrixCSC(M)
@inline _sparsifystructured(V::Vector) = SparseVector(V)
@inline _sparsifystructured(M::AbstractMatrix) = SparseMatrixCSC(M)
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned in #20102, I like these changes: These definitions are safer if we at some point broaden the set of type combinations sparse broadcast[!] captures. On the other hand, absent that broadening, being explicit about the types sparse broadcast[!] handles and what it does with them is also nice. But then being explicit in _spcontainertype should be sufficient, so I favor keeping these changes. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd also prefer having this and if necessary, then being more conservative over _spcontainertype.

@inline _sparsifystructured(V::AbstractVector) = SparseVector(V)
@inline _sparsifystructured(M::AbstractSparseMatrix) = SparseMatrixCSC(M)
@inline _sparsifystructured(V::AbstractSparseVector) = SparseVector(V)
@inline _sparsifystructured(S::SparseVecOrMat) = S
@inline _sparsifystructured(x) = x


Expand Down
13 changes: 8 additions & 5 deletions test/sparse/higherorderfns.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,16 +403,19 @@ end
densearrays = (C, M)
fD, fB = Array(D), Array(B)
for X in densearrays
@test (Q = broadcast(+, D, X); Q isa SparseMatrixCSC && Q == sparse(broadcast(+, fD, X)))
@test broadcast(+, D, X)::SparseMatrixCSC == sparse(broadcast(+, fD, X))
@test broadcast!(+, Z, D, X) == sparse(broadcast(+, fD, X))
@test (Q = broadcast(*, s, B, X); Q isa SparseMatrixCSC && Q == sparse(broadcast(*, s, fB, X)))
@test broadcast(*, s, B, X)::SparseMatrixCSC == sparse(broadcast(*, s, fB, X))
@test broadcast!(*, Z, s, B, X) == sparse(broadcast(*, s, fB, X))
@test (Q = broadcast(+, V, B, X); Q isa SparseMatrixCSC && Q == sparse(broadcast(+, fV, fB, X)))
@test broadcast(+, V, B, X)::SparseMatrixCSC == sparse(broadcast(+, fV, fB, X))
@test broadcast!(+, Z, V, B, X) == sparse(broadcast(+, fV, fB, X))
@test (Q = broadcast(+, V, A, X); Q isa SparseMatrixCSC && Q == sparse(broadcast(+, fV, fA, X)))
@test broadcast(+, V, A, X)::SparseMatrixCSC == sparse(broadcast(+, fV, fA, X))
@test broadcast!(+, Z, V, A, X) == sparse(broadcast(+, fV, fA, X))
@test (Q = broadcast(*, s, V, A, X); Q isa SparseMatrixCSC && Q == sparse(broadcast(*, s, fV, fA, X)))
@test broadcast(*, s, V, A, X)::SparseMatrixCSC == sparse(broadcast(*, s, fV, fA, X))
@test broadcast!(*, Z, s, V, A, X) == sparse(broadcast(*, s, fV, fA, X))
# Issue #20954 combinations of sparse arrays and RowVectors
Copy link
Member

Choose a reason for hiding this comment

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

X runs over both vectors and matrices in these two tests. If you intended to test only vectors (transposing matrix X not testing anything additional at the moment), move these below the loop? Or did you place these inside the loop in anticipation of lazy transposition of matrices appearing in the next dev cycle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentional. It would work now, so this would make sure it doesn't regress in the future if/when the lazy transposition of matrices land.

@test broadcast(+, A, X')::SparseMatrixCSC == sparse(broadcast(+, fA, X'))
@test broadcast(*, V, X')::SparseMatrixCSC == sparse(broadcast(*, fV, X'))
end
end

Expand Down