Make broadcast over sparse and rowvecs sparse #20959
Conversation
base/sparse/higherorderfns.jl
Outdated
| @@ -1061,8 +1062,8 @@ promote_containertype(::Type{Array}, ::Type{AbstractSparseArray}) = PromoteToSpa | |||
| # mostly just disambiguates Array from the main containertype promotion mechanism | |||
| # AbstractArray serves as a marker to shunt to the generic AbstractArray broadcast code | |||
| _spcontainertype(x) = _containertype(x) | |||
| _spcontainertype(::Type{<:Vector}) = Vector | |||
| _spcontainertype(::Type{<:Matrix}) = Matrix | |||
| _spcontainertype(::Type{<:AbstractVector}) = Vector | |||
There was a problem hiding this comment.
Loosening these signatures would cause sparse broadcast[!] to capture a much broader array of type combinations than it does now. A conservative approach seems wiser. Instead just add a method for row vectors?
There was a problem hiding this comment.
In the future I'd love if we could have some kind of out-of-the-box functionality being guaranteed for types that implement some interfaces, e.g. the AbstractArray interface. That is, it would be cool IMO if you could properly define the interface methods and the you automatically you get the same behavior as for other subtypes of AbstractArray when broadcasting with other type of containers, and only have to worry about implementing more methods if you want a specific behavior.
This change and the general idea in #20102 are some proposals in that direction, but of course, that needs more careful design. So I could go with a more conservative approach for now, but I really hope we can come up with a better structure for all of these.
kshyatt
added
sparse
| @inline _sparsifystructured(M::StructuredMatrix) = SparseMatrixCSC(M) | ||
| @inline _sparsifystructured(M::Matrix) = SparseMatrixCSC(M) | ||
| @inline _sparsifystructured(V::Vector) = SparseVector(V) | ||
| @inline _sparsifystructured(M::AbstractMatrix) = SparseMatrixCSC(M) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'd also prefer having this and if necessary, then being more conservative over _spcontainertype.
| @test broadcast!(*, Z, s, V, A, X) == sparse(broadcast(*, s, fV, fA, X)) | ||
| # Issue #20954 combinations of sparse arrays and RowVectors |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
pabloferz
force-pushed
the
pz/bcsprowvec
branch
from
eada11e to
3b0a5dc
Compare
|
All right, I went with a more conservative approach for |
|
I'll merge this by the end of the day (CST) if there are no objections. |
Part of #20102, fixes #20954.