-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix ambiguity of broadcast!(identity, ::SparseVector, ::SparseVector) #19895
Conversation
@@ -148,7 +148,7 @@ ambiguityfunnel{Tf}(f::Tf, x, y) = _aresameshape(x, y) ? _noshapecheck_map(f, x, | |||
broadcast(::typeof(+), x::SparseVector, y::SparseVector) = ambiguityfunnel(+, x, y) # base/sparse/sparsevectors.jl:1266 | |||
broadcast(::typeof(-), x::SparseVector, y::SparseVector) = ambiguityfunnel(-, x, y) # base/sparse/sparsevectors.jl:1266 | |||
broadcast(::typeof(*), x::SparseVector, y::SparseVector) = ambiguityfunnel(*, x, y) # base/sparse/sparsevectors.jl:1266 | |||
function broadcast!(::typeof(identity), C::SparseMatrixCSC, A::SparseMatrixCSC) # from #17623, loc? | |||
function broadcast!{T<:SparseVecOrMat}(::typeof(identity), C::T, A::T) # from #17623, base/broadcast.jl:26 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this won't handle the same cases as the previous one. Maybe
broadcast!{T<:SparseVecOrMat}(::typeof(identity), C::T, A::SparseVecOrMat)
or
broadcast!{T<:SparseVecOrMat,S<:SparseVecOrMat}(::typeof(identity), C::S, A::T)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Will update after the weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggested forms might allow e.g. C <: SparseVector
while A <: SparseMatrixCSC
, no? Best!
With something along the lines of |
As Experimenting here, I noted something else: julia> zeros(3,1) .= sin.(zeros(3))
3×1 Array{Float64,2}:
0.0
0.0
0.0
julia> spzeros(3,1) .= sin.(spzeros(3))
ERROR: DimensionMismatch("argument shapes must match")
Stacktrace:
[1] _checksameshape at ./sparse/higherorderfns.jl:124 [inlined]
[2] map!(::Base.#sin, ::SparseMatrixCSC{Float64,Int64}, ::SparseVector{Float64,Int64}) at ./sparse/higherorderfns.jl:64
[3] broadcast!(::Base.#sin, ::SparseMatrixCSC{Float64,Int64}, ::SparseVector{Float64,Int64}) at ./sparse/higherorderfns.jl:84 @Sacha0 Is that being fixed by something you're currently working on (I lost track, I admit), or should I open an issue so that we don't forget? |
…ray{T,N}, ::AbstractArray{S,N})
992de28
to
b0ae001
Compare
Updated the PR to just remove the specialization. I realized, however, that the specialized version for |
@tkelman is that a 👍 to the updated PR or to keeping |
both, mostly the latter for now if it makes a noticeable difference |
I'd keep the |
@martinholters, re. #19895 (comment), you've found a bug --- thanks! :) The signature of a specialization in the sparse |
IIRC the |
@@ -148,10 +148,13 @@ ambiguityfunnel{Tf}(f::Tf, x, y) = _aresameshape(x, y) ? _noshapecheck_map(f, x, | |||
broadcast(::typeof(+), x::SparseVector, y::SparseVector) = ambiguityfunnel(+, x, y) # base/sparse/sparsevectors.jl:1266 | |||
broadcast(::typeof(-), x::SparseVector, y::SparseVector) = ambiguityfunnel(-, x, y) # base/sparse/sparsevectors.jl:1266 | |||
broadcast(::typeof(*), x::SparseVector, y::SparseVector) = ambiguityfunnel(*, x, y) # base/sparse/sparsevectors.jl:1266 | |||
function broadcast!(::typeof(identity), C::SparseMatrixCSC, A::SparseMatrixCSC) # from #17623, loc? | |||
# specializations to improve performance for C .= A | |||
function broadcast!(::typeof(identity), C::SparseVector, A::SparseVector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps compact these?
broadcast!(::typeof(identity), C::SparseVector, A::SparseVector) = (_checksameshape(C,A); copy!(C, A))
broadcast!(::typeof(identity), C::SparseMatrixCSC, A::SparseMatrixCSC) = (_checksameshape(C,A); copy!(C, A))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add an @boundscheck
so people can remove shape checking when they already checked that before hand or know in advance the arrays are of the same size? E.g.
broadcast!(::typeof(identity), C::SparseVector, A::SparseVector) = (@boundscheck _checksameshape(C,A); copy!(C, A))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated; also added @inline
.
Right. The |
Right, ambiguities. Carry on then. |
a5ae045
to
6f2f739
Compare
Huh, the specializations for the sparse case actually don't quite match the unspecialized versions as they don't remove 0-entries: julia> a=sprand(5, 1.0); a.nzval .= 0.0; a
Sparse vector of length 5 with 5 Float64 nonzero entries:
[1] = 0.0
[2] = 0.0
[3] = 0.0
[4] = 0.0
[5] = 0.0
julia> broadcast!(identity, copy(a), a)
Sparse vector of length 5 with 5 Float64 nonzero entries:
[1] = 0.0
[2] = 0.0
[3] = 0.0
[4] = 0.0
[5] = 0.0
julia> broadcast!(x->x, copy(a), a)
Sparse vector of length 5 with 0 Float64 nonzero entries: Adding a |
My present tendency is to remove the specialized versions, i.e. go back to just b0ae001. If we want to treat performance of |
Favoring consistency, SGTM.
If you are suggesting changing the zero-storing behavior of generic sparse |
6f2f739
to
b0ae001
Compare
Ok, went back to just removing the specializations. Any objections? |
I'm fine with this. Better general performance seems like a more appropriate path to follow here. |
I just don't think that if be want both |
I also don't think they can be as fast as |
Looks like we have reached agreement to just remove the specializations and CI is green. Merge? |
How does this affect performance for |
On reflection the former specializations were incorrect, failing to handle cases where the source and destination shapes differ. Best! (Edit: Tests added for the sparse case in #19986.) |
Might still be nice to do |
To clarify, the specialization that |
To substantiate the incorrectness: # this is before this PR, still including the specialization now removed
julia> x=zeros(5,1); y=ones(5,5);
julia> y .= x # equivalent to y .= identity.(x), only does a partial copy!
5×5 Array{Float64,2}:
0.0 1.0 1.0 1.0 1.0
0.0 1.0 1.0 1.0 1.0
0.0 1.0 1.0 1.0 1.0
0.0 1.0 1.0 1.0 1.0
0.0 1.0 1.0 1.0 1.0
julia> y .= (a->a).(x) # does correct broadcast with anonymous identity function
5×5 Array{Float64,2}:
0.0 0.0 0.0 0.0 0.0
0.0 0.0 0.0 0.0 0.0
0.0 0.0 0.0 0.0 0.0
0.0 0.0 0.0 0.0 0.0
0.0 0.0 0.0 0.0 0.0 However, I have to withdraw my previous statement about the generic Should we add something like function broadcast!{T,S,N}(::typeof(identity), x::Array{T,N}, y::Array{S,N})
if size(x) == size(y)
copy!(x, y)
else
broadcast_c!(identity, Array, x, y)
end
end ? |
Interesting! I wonder whether some measure of unrolling might be useful? Best! |
Would be good to have these cases in BaseBenchmarks so that nanosoldier can check them. |
Can someone open an issue to restore the use of |
|
Without this:
BTW, should
test/ambiguous.jl
have caught this?