[WIP] Make it easier to extend broadcast! #24992

tkoolen · 2017-12-08T20:37:53Z

This implements @timholy's proposal from #24914 (comment). This passes all tests locally except some @inferred/@allocated tests in sparse/higherorderfns.jl, specifically:

julia/test/sparse/higherorderfns.jl

Line 239 in 5f929a4

@test (@allocated broadcast!(+, Q, X, Y, Z)) == 0
julia/test/sparse/higherorderfns.jl

Line 244 in 5f929a4

@test (@allocated broadcast!(*, Q, X, Y, Z)) == 0
julia/test/sparse/higherorderfns.jl

Line 329 in 5f929a4

@test isa(@inferred(broadcast!(*, X, sparseargs...)), SparseMatrixCSC{elT})

when the number of arguments is higher than some number
julia/test/sparse/higherorderfns.jl

Line 363 in 5f929a4

@test isa(@inferred(broadcast!(*, X, sparseargs...)), SparseMatrixCSC{elT})

I'm not exactly sure why this is the case, and could use some help.

To do:

get sparse/higherorderfns.jl inference/allocation tests to pass
documentation
decide what to do with broadcast!(f, X::AbstractArray, x::Number...)

I'll add some line comments to better explain what I did.

tkoolen · 2017-12-08T20:42:13Z

base/broadcast.jl

-
-# special cases for "X .= ..." (broadcast!) assignments
-broadcast!(::typeof(identity), X::AbstractArray, x::Number) = fill!(X, x)
-broadcast!(f, X::AbstractArray, x::Number...) = (@inbounds for I in eachindex(X); X[I] = f(x...); end; X)


I wasn't quite sure how to handle this case, as it specializes on both the destination and the source args. What is the goal of this special case? Reducing compilation time or runtime?

tkoolen · 2017-12-08T20:44:02Z

base/sparse/higherorderfns.jl

@@ -93,7 +93,8 @@ end
 # (3) broadcast[!] entry points
 broadcast(f::Tf, A::SparseVector) where {Tf} = _noshapecheck_map(f, A)
 broadcast(f::Tf, A::SparseMatrixCSC) where {Tf} = _noshapecheck_map(f, A)
-function broadcast!(f::Tf, C::SparseVecOrMat) where Tf
+
+function broadcast!(f::Tf, C::SparseVecOrMat, ::Void) where Tf


Use ::Voidfor the broadcast style here and in the next broadcast! method since we're specializing on the destination only, as per the rules at the end of #24914 (comment).

tkoolen · 2017-12-08T20:45:41Z

base/sparse/higherorderfns.jl

@@ -106,14 +107,13 @@ function broadcast!(f::Tf, C::SparseVecOrMat) where Tf
    end
    return C
 end
-function broadcast!(f::Tf, C::SparseVecOrMat, A::SparseVecOrMat, Bs::Vararg{SparseVecOrMat,N}) where {Tf,N}


Turned this into a spbroadcast_args! method.

tkoolen · 2017-12-08T20:48:24Z

base/sparse/higherorderfns.jl

+function broadcast!(f, dest::SparseVecOrMat, ::Void, A, Bs::Vararg{Any,N}) where N
+    if isa(f, typeof(identity)) && N == 0 && isa(A, Number)
+        return fill!(dest, A)
+    end


I wasn't sure whether to also implement the copy! optimization when the indices match here.

I think that method was mostly about ambiguity resolution, so prob not necessary.

tkoolen · 2017-12-08T20:53:36Z

I tried to mimic the previous behavior of sparse broadcast!ing (only active when the destination is a SparseVecOrMat, whether or not the source args have SPVM BroadcastStyle is ignored). Is that a good idea?

timholy

Looks good overall. Will need to update the docs too.

timholy · 2017-12-10T16:31:55Z

base/sparse/higherorderfns.jl

+function broadcast!(f, dest::SparseVecOrMat, ::Void, A, Bs::Vararg{Any,N}) where N
+    if isa(f, typeof(identity)) && N == 0 && isa(A, Number)
+        return fill!(dest, A)
+    end


I think that method was mostly about ambiguity resolution, so prob not necessary.

timholy · 2017-12-10T16:36:13Z

base/broadcast.jl

-    _broadcast!(f, C, A, Bs...)
+broadcast!(f, dest, As...) = broadcast!(f, dest, combine_styles(As...), As...)
+broadcast!(f, dest, ::BroadcastStyle, As...) = broadcast!(f, dest, nothing, As...)
+@inline function broadcast!(f, C, ::Void, A, Bs::Vararg{Any,N}) where N


I didn't think about this before, but I wonder if we should consider the implications of specializing A, Bs::Vararg{Any,N} vs just As::Vararg{Any,N}. I believe those two have separate precedence for dispatch. Whatever we choose should be documented that way.

timholy · 2017-12-10T16:42:35Z

I'm not exactly sure why this is the case, and could use some help.

Might need some @inline on the "dispatch chain" of broadcast!.

tkoolen · 2017-12-11T15:41:20Z

OK, thanks for your comments. I'll have more time to work on this after Wednesday.

Sacha0 · 2017-12-15T05:01:17Z

I tried to mimic the previous behavior of sparse broadcast!ing (only active when the destination is a SparseVecOrMat, whether or not the source args have SPVM BroadcastStyle is ignored). Is that a good idea?

Not sure I follow --- sparse broadcast should only capture argument combinations where both the destination is SparseVecOrMat and the source args are SPVM if memory serves? Thanks for working on this! :)

timholy · 2017-12-15T09:38:08Z

Tests don't run due to a merge conflict.

tkoolen · 2017-12-15T17:56:06Z

@Sacha0, that may be the case for broadcast, but for broadcast! there was no notion of BroadcastStyle in the dispatch chain before this PR, so dispatch really only happened on the type of the destination:

julia/base/sparse/higherorderfns.jl

Line 96 in 193f763

function broadcast!(f::Tf, C::SparseVecOrMat) where Tf

julia/base/sparse/higherorderfns.jl

Line 109 in 193f763

    
           function broadcast!(f::Tf, C::SparseVecOrMat, A::SparseVecOrMat, Bs::Vararg{SparseVecOrMat,N}) where {Tf,N}

julia/base/sparse/higherorderfns.jl

Line 1017 in 193f763

broadcast!(f, dest::SparseVecOrMat, mixedsrcargs::Vararg{Any,N}) where N =

Updates so far (sorry, should have commented as I pushed my latest commit):

I'm now handling the optimization of all-Number source arguments and an AbstractArray destination (and actually generalized it a bit) by adding a specialization for the Scalar BroadcastStyle.
changed all method signatures to take As... as opposed to A, Bs... in some cases.
got almost all sparse/higherorderfns.jl inference/allocation tests to pass by:
1. changing broadcast!(f, dest::SparseVecOrMat, style::BroadcastStyle, A, Bs::Vararg{Any,N}) where N into broadcast!(f::Tf, dest::SparseVecOrMat, style::BroadcastStyle, As::Vararg{Any,N}) where {Tf,N}, note the Tf; I thought that didn't matter anymore, but apparently it does.
2. making specializations be of the form broadcast!(f, dest, ::BroadcastStyle, As...) instead of broadcast!(f, dest, ::Void, As...), as it enables reuse of the result of combine_styles for further dispatch instead of recomputing it. I know, I know, this should be done at compile time anyway, but the previous implementation seemed to trigger inference limits earlier (as witnessed by higher allocations in test/sparse/higherorderfns.jl:239 and test/sparse/higherorderfns.jl:244). ~~I think this should still be fine in terms of ambiguities, correct?~~ I think this is actually not OK.
3. perhaps one @inline annotation mattered, can't remember. Just @inlineing everything didn't resolve the last allocation test failures.

I'll rebase and keep trying things today to get the allocation tests to pass. Any additional pointers on staying within the current limits of inference would be greatly appreciated.

Sacha0 · 2017-12-15T18:07:16Z

@Sacha0, that may be the case for broadcast, but for broadcast! there was no notion of BroadcastStyle in the dispatch chain before this PR, so dispatch really only happened on the type of the destination...

Prior to #23939, IIRC the entry points to sparse broadcast! either constrained both the destination and input arguments as in the first two signatures you link, or directly constrained the destination argument and then checked the input arguments internally (redispatching to generic AbstractArray broadcast! for unhandled input arguments). The merged iteration of #23939 may have changed this behavior (I remain much behind on reviews, apologies), but if so the former behavior should somehow be restored rather than further eroded :). I will try to look closer tonight. Best!

Sacha0 · 2017-12-15T18:10:52Z

changed all method signatures to take As... as opposed to A, Bs... in some cases.

IIRC, this signature structure might've been necessary to correctly handle first arguments that are types.

Sacha0 · 2017-12-15T18:11:56Z

Again thanks for your efforts on this front @tkoolen! :)

timholy · 2017-12-15T19:08:39Z

I think those implementations that retain the BroadcastStyle are ambiguous. You'll probably need to do the Void variant and recompute the style. Or, specialize on the BroadcastStyle too so that it doesn't intersect e.g., Scalar.

timholy · 2017-12-15T19:09:50Z

If you want to test for ambiguities, you can do this:

using Test
detect_ambiguities(Base, Core)

Sacha0 · 2017-12-15T19:14:36Z

If you want to test for ambiguities, you can do this:

Might want Test.detect_ambiguities(Core, Base; imported = true, recursive=true, ambiguous_bottom = false) to match CI :).

timholy · 2017-12-15T20:17:35Z

@tkoolen also don't hesitate to ask for help. We definitely want this done for 1.0, and today is the deadline.

Sacha0 · 2017-12-15T20:49:26Z

We definitely want this done for 1.0, and today is the deadline.

Last I heard broadcast internals will be exempt from the stability guarantee in 1.0? Additionally, stretching this by a weekend should be alright (what with all the other work in progress that is yet to merge) :).

tkoolen · 2017-12-15T21:12:08Z

I was aware of the deadline. I could certainly use help understanding what inference/compiler limits I'm triggering in the last cases where there are allocations, i.e.

julia/test/sparse/higherorderfns.jl

Line 239 in 5f929a4

@test (@allocated broadcast!(+, Q, X, Y, Z)) == 0
julia/test/sparse/higherorderfns.jl

Line 244 in 5f929a4

@test (@allocated broadcast!(*, Q, X, Y, Z)) == 0

I've just pushed my rebase (was hacking away at my local branch earlier). Feel free to open a PR against my branch if anybody has found a solution to the allocation problems. Or just give me some ideas for things to try.

I had also come to the conclusion that passing along the BroadcastStyle would result in ambiguities (not necessarily directly within Base, but certainly when package authors try to further extend broadcast!).

tkoolen · 2017-12-15T21:32:03Z

Here's a smaller test (reduced from test/sparse/higherorderfns.jl) to play around with:

using Test
@testset begin
    N, M, p = 10, 12, 0.3
    mats = (sprand(N, M, p), sprand(N, 1, p), sprand(1, M, p), sprand(1, 1, 1.0), spzeros(1, 1))
    vecs = (sprand(N, p), sprand(1, 1.0), spzeros(1))
    tens = (mats..., vecs...)
    Xo = tens[1]
    X = ndims(Xo) == 1 ? SparseVector{Float32,Int32}(Xo) : SparseMatrixCSC{Float32,Int32}(Xo)
    shapeX, fX = size(X), Array(X)
    Y = tens[1]
    Z = tens[1]
    fY, fZ = Array(Y), Array(Z)
    fQ = broadcast(+, fX, fY, fZ); Q = sparse(fQ)
    broadcast!(+, Q, X, Y, Z); Q = sparse(fQ) # warmup for @allocated
    @test (@allocated broadcast!(+, Q, X, Y, Z)) == 0
end

Do note that if I use a let block, i.e.

    let Q = Q, X = X, Y = Y, Z = Z
        broadcast!(+, Q, X, Y, Z); Q = sparse(fQ) # warmup for @allocated
        @test (@allocated broadcast!(+, Q, X, Y, Z)) == 0
    end

then the test passes.

I'm honestly kind of surprised that this passed before by the way, given these lines:

julia/test/sparse/higherorderfns.jl

Lines 216 to 220 in 9315ca0

    
           mats = (sprand(N, M, p), sprand(N, 1, p), sprand(1, M, p), sprand(1, 1, 1.0), spzeros(1, 1)) 
        
           vecs = (sprand(N, p), sprand(1, 1.0), spzeros(1)) 
        
           tens = (mats..., vecs...) 
        
           for Xo in tens 
        
               X = ndims(Xo) == 1 ? SparseVector{Float32,Int32}(Xo) : SparseMatrixCSC{Float32,Int32}(Xo)

tkoolen · 2017-12-15T22:24:19Z

Ran out of time for today. I'll take another look tomorrow.

timholy · 2017-12-16T12:40:11Z

It seems to be a failure to specialize. I can get that particular test to pass with the following changes (I'm not sure they all are necessary, but they are sufficient):

diff --git a/base/broadcast.jl b/base/broadcast.jl
index 8576824..f40f526 100644
--- a/base/broadcast.jl
+++ b/base/broadcast.jl
@@ -258,7 +258,7 @@ longest(::Tuple{}, ::Tuple{}) = ()
 # combine_styles operates on values (arbitrarily many)
 combine_styles(c) = result_style(BroadcastStyle(typeof(c)))
 combine_styles(c1, c2) = result_style(combine_styles(c1), combine_styles(c2))
-combine_styles(c1, c2, cs...) = result_style(combine_styles(c1), combine_styles(c2, cs...))
+@inline combine_styles(c1, c2, cs...) = result_style(combine_styles(c1), combine_styles(c2, cs...))
 
 # result_style works on types (singletons and pairs), and leverages `BroadcastStyle`
 result_style(s::BroadcastStyle) = s
@@ -442,8 +442,8 @@ Note that `dest` is only used to store the result, and does not supply
 arguments to `f` unless it is also listed in the `As`,
 as in `broadcast!(f, A, A, B)` to perform `A[:] = broadcast(f, A, B)`.
 """
-@inline broadcast!(f, dest, As...) = broadcast!(f, dest, combine_styles(As...), As...)
-@inline broadcast!(f, dest, ::BroadcastStyle, As...) = broadcast!(f, dest, nothing, As...)
+@inline broadcast!(f::Tf, dest, As::Vararg{Any,N}) where {Tf,N} = broadcast!(f, dest, combine_styles(As...), As...)
+@inline broadcast!(f::Tf, dest, ::BroadcastStyle, As::Vararg{Any,N}) where {Tf,N} = broadcast!(f, dest, nothing, As...)
 
 # Default behavior (separated out so that it can be called by users who want to extend broadcast!).
 @inline function broadcast!(f, dest, ::Void, As::Vararg{Any, N}) where N
diff --git a/base/sparse/higherorderfns.jl b/base/sparse/higherorderfns.jl
index ba89f04..423ce10 100644
--- a/base/sparse/higherorderfns.jl
+++ b/base/sparse/higherorderfns.jl
@@ -1021,15 +1021,15 @@ function spbroadcast_args!(f::Tf, C, ::SPVM, A::SparseVecOrMat, Bs::Vararg{Spars
     return fpreszeros ? _broadcast_zeropres!(f, C, A, Bs...) :
                         _broadcast_notzeropres!(f, fofzeros, C, A, Bs...)
 end
-function spbroadcast_args!(f, dest, ::SPVM, mixedsrcargs::Vararg{Any,N}) where N
+function spbroadcast_args!(f::Tf, dest, ::SPVM, mixedsrcargs::Vararg{Any,N}) where {Tf,N}
     # mixedsrcargs contains nothing but SparseVecOrMat and scalars
     parevalf, passedsrcargstup = capturescalars(f, mixedsrcargs)
     return broadcast!(parevalf, dest, passedsrcargstup...)
 end
-function spbroadcast_args!(f, dest, ::PromoteToSparse, mixedsrcargs::Vararg{Any,N}) where N
+function spbroadcast_args!(f::Tf, dest, ::PromoteToSparse, mixedsrcargs::Vararg{Any,N}) where {Tf,N}
     broadcast!(f, dest, map(_sparsifystructured, mixedsrcargs)...)
 end
-function spbroadcast_args!(f, dest, ::Any, mixedsrcargs::Vararg{Any,N}) where N
+function spbroadcast_args!(f::Tf, dest, ::Any, mixedsrcargs::Vararg{Any,N}) where {Tf,N}
     # Fallback. From a performance perspective would it be best to densify?
     Broadcast._broadcast!(f, dest, mixedsrcargs...)
 end

timholy · 2017-12-16T13:41:08Z

There's still a really strange inference failure though. Let's get a branch pushed and then see if we can get Jameson to take a look.

tkoolen · 2017-12-16T14:36:25Z

Thanks, I pushed commit with those changes.

timholy · 2017-12-16T15:54:19Z

Great. Any chance anyone not swamped with other things can take a look at the inference failure? (I'm moving today...bad timing.) Here's a test:

julia> A = sprand(5, 5, 0.4);

julia> a = sprand(5, 0.4);

julia> X = similar(A);

julia> @code_warntype broadcast!(*, X, A, a, A, 1.0f0, 1.0f0, 1.0f0, A, a)
Variables:
  f<optimized out>
  dest::SparseMatrixCSC{Float64,Int64}
  As::Tuple{SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64},SparseMatrixCSC{Float64,Int64},Float32,Float32,Float32,SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64}}

Body:
  begin
      SSAValue(14) = (Core.getfield)(As::Tuple{SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64},SparseMatrixCSC{Float64,Int64},Float32,Float32,Float32,SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64}}, 1)::SparseMatrixCSC{Float64,Int64}
      SSAValue(15) = (Core.getfield)(As::Tuple{SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64},SparseMatrixCSC{Float64,Int64},Float32,Float32,Float32,SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64}}, 2)::SparseVector{Float64,Int64}
      SSAValue(16) = (Core.getfield)(As::Tuple{SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64},SparseMatrixCSC{Float64,Int64},Float32,Float32,Float32,SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64}}, 3)::SparseMatrixCSC{Float64,Int64}
      SSAValue(17) = (Core.getfield)(As::Tuple{SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64},SparseMatrixCSC{Float64,Int64},Float32,Float32,Float32,SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64}}, 4)::Float32
      SSAValue(18) = (Core.getfield)(As::Tuple{SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64},SparseMatrixCSC{Float64,Int64},Float32,Float32,Float32,SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64}}, 5)::Float32
      SSAValue(19) = (Core.getfield)(As::Tuple{SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64},SparseMatrixCSC{Float64,Int64},Float32,Float32,Float32,SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64}}, 6)::Float32
      SSAValue(20) = (Core.getfield)(As::Tuple{SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64},SparseMatrixCSC{Float64,Int64},Float32,Float32,Float32,SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64}}, 7)::SparseMatrixCSC{Float64,Int64}
      SSAValue(21) = (Core.getfield)(As::Tuple{SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64},SparseMatrixCSC{Float64,Int64},Float32,Float32,Float32,SparseMatrixCSC{Float64,Int64},SparseVector{Float64,Int64}}, 8)::SparseVector{Float64,Int64}
      # meta: location broadcast.jl broadcast! 443
      SSAValue(24) = $(Expr(:invoke, MethodInstance for broadcast!(::typeof(*), ::SparseMatrixCSC{Float64,Int64}, ::Void, ::SparseMatrixCSC{Float64,Int64}, ::SparseVector{Float64,Int64}, ::SparseMatrixCSC{Float64,Int64}, ::Float32, ::Float32, ::Float32, ::SparseMatrixCSC{Float64,Int64}, ::SparseVector{Float64,Int64}), :(Base.Broadcast.broadcast!), *, :(dest), nothing, SSAValue(14), SSAValue(15), SSAValue(16), SSAValue(17), SSAValue(18), SSAValue(19), SSAValue(20), SSAValue(21)))::Any
      # meta: pop location
      return SSAValue(24)
  end::Any

The funny thing about it is that it knows which method it will call, and that method is inferrable:

@code_warntype broadcast!(*, X, nothing, A, a, A, 1.0f0, 1.0f0, 1.0f0, A, a) # just adds the `nothing`

Worst-case scenario I think we should mark those tests as broken and merge anyway, then fix.

@tkoolen, can you add docs? See the doc/src/manual/interfaces.md.

Sacha0 · 2017-12-17T03:53:31Z

Just reviewed more extensively. Overall these changes look great; thanks @tkoolen and @timholy! :)

Re. the inference failure, my best guess is that the additional argument (the BroadcastStyle) to broadcast! pushes the eight-source-argument call over one or another tuple limit beyond which the system gives up. (Getting as much as possible inferred and allocation-free the first time around was challenging, so I wrote the tests to exercise the limits I hit, and also consequently much appreciate the patience and effort you both are putting in to retain those characteristics). If the inference failure is just a matter of handling maximally one fewer source argument, I agree that marking the associated tests broken and moving ahead for now seems reasonable.

One broad comment: I imagined this mechanism enabling flattening of the dispatch layers in sparse broadcast!. That is, sparse broadcast! presently constrains both the destination and source argument types by: (1) specializing broadcast! on the destination type (SparseVecOrMat); and then (2) dispatching to spbroadcast_args!, which further sorts calls on the basis of the source types and dispatches to the appropriate machinery. (This logic was a hack around the absence of a mechanism like that implemented in this pull request.) Eliminating that hack (the additional spbroadcast_args! layer) by, e.g., specializing broadcast! on both the destination type (SparseVecOrMat) and source types (BroadcastStyle) would be lovely. Thoughts? Thanks!

timholy · 2017-12-21T08:36:44Z

As long as you're dispatching on a specific BroadcastStyle as well as on typeof(dest), then there should be no problem from ambiguity---the method you're defining is even more specific, so it's even less "surface area" for ambiguities. You just don't want to define broadcast!(f, ::DestType, ::BroadcastStyle, ...) for generic BroadcastStyle.

tkoolen · 2017-12-21T08:54:40Z

Yep, agree. FYI: I'll be AFK for the next 48 hours or so.

Sacha0 · 2017-12-21T21:53:46Z

One argument against the change is that we'd be breaking the conventions we just added to the docs right off the bat. Then again, maybe those need to be refined. There should probably be a sentence about how to handle the case of specializing on both dest type and BroadcastStyle simultaneously anyway.

On this end, enabling graceful dispatch on both the destination and source arguments (edit: simultaneously, that is) was the perhaps primary impetus for this direction :).

timholy · 2017-12-22T10:17:53Z

This passes tests locally for me. Let's get some performance data: @nanosoldier runbenchmarks(ALL, vs = ":master")

nanosoldier · 2017-12-22T14:53:28Z

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

timholy · 2017-12-22T15:56:17Z

LGTM. Do people want to see this go through CI? If so I'll squash on a separate branch and push as a new PR.

The API will change further because of #23692 and its successor, but I think this is making progress in the right direction. So I'd say it's worth merging.

tkoolen · 2017-12-22T16:20:50Z

I can squash as well if that's easier.

timholy · 2017-12-22T16:25:25Z

If you can (and get rid of the ci skips), that would be great.

tkoolen · 2017-12-22T16:44:14Z

Done.

Sacha0 · 2017-12-22T21:32:46Z

base/broadcast.jl

+@inline broadcast!(f::Tf, dest, ::BroadcastStyle, As::Vararg{Any,N}) where {Tf,N} = broadcast!(f, dest, nothing, As...)
+
+# Default behavior (separated out so that it can be called by users who want to extend broadcast!).
+@inline function broadcast!(f, dest, ::Nothing, As::Vararg{Any, N}) where N


@StefanKarpinski, thanks again for seeing the Void -> Nothing change through! So much more intuitive :).

Sacha0 · 2017-12-22T21:39:05Z

base/sparse/higherorderfns.jl

-    # Fallback. From a performance perspective would it be best to densify?
-    Broadcast._broadcast!(f, dest, mixedsrcargs...)
+function broadcast!(f::Tf, dest::SparseVecOrMat, ::PromoteToSparse, mixedsrcargs::Vararg{Any,N}) where {Tf,N}
+    broadcast!(f, dest, nothing, map(_sparsifystructured, mixedsrcargs)...)


The nothing for BroadcastStyle causes dispatch to the generic AbstractArray broadcast implementation, correct? If so, this nothing should disappear, as PromoteToSparse argument combinations should go to sparse broadcast after promotion IIRC?

Hmm, when I originally suggested this there was a Nothing method at the "top" of the sparse dispatch hierarchy.

Sacha0 · 2017-12-22T21:41:45Z

base/sparse/higherorderfns.jl

-end
-function spbroadcast_args!(f, dest, ::PromoteToSparse, mixedsrcargs::Vararg{Any,N}) where N
-    broadcast!(f, dest, map(_sparsifystructured, mixedsrcargs)...)
+    broadcast!(parevalf, dest, nothing, passedsrcargstup...)


Likewise (https://github.com/JuliaLang/julia/pull/24992/files#r158561090) here, does this nothing cause dispatch specifically to the generic AbstractArray code?

@timholy

Reimplement and generalize all-scalar optimization. Add documentation. Explicitly return dest in various broadcast!-related methods. This is to make things easier on inference. Found by @timholy. Collapse spbroadcast_args! into broadcast! as suggested by @Sacha0.

Sacha0

Fantastic! Much thanks for seeing this through @tkoolen! :)

StefanKarpinski · 2017-12-23T21:03:40Z

Who else's sign-off do we need in order to merge this? Anyone? @timholy perhaps?

timholy · 2017-12-24T11:44:49Z

Thanks, @tkoolen!

tkoolen · 2017-12-24T14:54:37Z

Thank you very much for your help, @timholy.

stevengj · 2017-12-29T15:58:18Z

doc/src/manual/interfaces.md

+Note that no bounds should be placed on the types of `f` and `As...`.
+
+Second, if specialized `broadcast!` behavior is desired depending on the input types,
+you should write [binary broadcasting rules](@ref writing-binary-broadcasting-rules) to


Where is the writing-binary-broadcasting-rules section?

https://docs.julialang.org/en/latest/manual/interfaces/#writing-binary-broadcasting-rules-1 (link seems to be working).

andyferris · 2018-02-17T10:44:45Z

Not sure if I'm commenting on the right PR, but I think I am.

I note that now I can't use nothing as the first input for broadcast / broadcast! as a scalar value. For instance I can do tuple.([1,2,3], nothing) but I can't do tuple.(nothing, [1,2,3]). This strikes me as odd.

I think maybe we need a different sentinel for the "default" broadcast!, one that users wouldn't normally use except for the purpose of extending the broadcast API. Or move that to _broadcast! or something.

tkoolen · 2018-02-17T17:32:47Z

Yes, this was noted in #25377 (comment), but if that doesn't get merged soon it should be fixed in the old code. I opened #26097 as a reminder.

tkoolen commented Dec 8, 2017

View reviewed changes

nalimilan requested a review from timholy December 8, 2017 20:56

nalimilan added the broadcast label Dec 8, 2017

timholy reviewed Dec 10, 2017

View reviewed changes

timholy added this to the 1.0 milestone Dec 15, 2017

timholy added the needs docs Documentation for this change is required label Dec 15, 2017

tkoolen force-pushed the tk/in-place-broadcast branch from 84bb5d9 to 35638c7 Compare December 15, 2017 21:08

tkoolen force-pushed the tk/in-place-broadcast branch from 780d5b5 to 26769f7 Compare December 17, 2017 20:46

tkoolen force-pushed the tk/in-place-broadcast branch from bd93724 to 761ca80 Compare December 21, 2017 07:48

tkoolen force-pushed the tk/in-place-broadcast branch from 140de09 to 94dcd82 Compare December 22, 2017 16:43

Sacha0 reviewed Dec 22, 2017

View reviewed changes

Fix JuliaLang#24914.

fa3fe32

Reimplement and generalize all-scalar optimization. Add documentation. Explicitly return dest in various broadcast!-related methods. This is to make things easier on inference. Found by @timholy. Collapse spbroadcast_args! into broadcast! as suggested by @Sacha0.

tkoolen force-pushed the tk/in-place-broadcast branch from 94dcd82 to fa3fe32 Compare December 23, 2017 04:22

Sacha0 approved these changes Dec 23, 2017

View reviewed changes

timholy merged commit fa2ae8c into JuliaLang:master Dec 24, 2017

tkoolen deleted the tk/in-place-broadcast branch December 24, 2017 14:54

stevengj mentioned this pull request Dec 29, 2017

RFC: introduce runtime representation of broadcast fusion #23692

Closed

stevengj reviewed Dec 29, 2017

View reviewed changes

tkoolen mentioned this pull request Jan 10, 2018

Use new broadcast API JuliaArrays/StaticArrays.jl#348

Closed

tkoolen mentioned this pull request Jan 27, 2018

RFC: Customizable lazy broadcasting with options for pure-Julia fusion and eager evaluation #25377

Closed

tkoolen mentioned this pull request Feb 17, 2018

Bug in broadcast! involving nothing #26097

Closed

mbauman added the broadcast Applying a function over a collection label Apr 24, 2018

[WIP] Make it easier to extend broadcast! #24992

[WIP] Make it easier to extend broadcast! #24992

Conversation

tkoolen commented Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkoolen commented Dec 8, 2017

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented Dec 10, 2017

tkoolen commented Dec 11, 2017

Sacha0 commented Dec 15, 2017

timholy commented Dec 15, 2017

tkoolen commented Dec 15, 2017 • edited Loading

Sacha0 commented Dec 15, 2017

Sacha0 commented Dec 15, 2017

Sacha0 commented Dec 15, 2017

timholy commented Dec 15, 2017

timholy commented Dec 15, 2017

Sacha0 commented Dec 15, 2017

timholy commented Dec 15, 2017

Sacha0 commented Dec 15, 2017

tkoolen commented Dec 15, 2017

tkoolen commented Dec 15, 2017 • edited Loading

tkoolen commented Dec 15, 2017

timholy commented Dec 16, 2017

timholy commented Dec 16, 2017

tkoolen commented Dec 16, 2017

timholy commented Dec 16, 2017

Sacha0 commented Dec 17, 2017 • edited Loading

timholy commented Dec 21, 2017

tkoolen commented Dec 21, 2017

Sacha0 commented Dec 21, 2017 • edited Loading

timholy commented Dec 22, 2017

nanosoldier commented Dec 22, 2017

timholy commented Dec 22, 2017

tkoolen commented Dec 22, 2017

timholy commented Dec 22, 2017

tkoolen commented Dec 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

StefanKarpinski commented Dec 23, 2017

timholy commented Dec 24, 2017

tkoolen commented Dec 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyferris commented Feb 17, 2018 • edited Loading

tkoolen commented Feb 17, 2018

tkoolen commented Dec 8, 2017 •

edited

Loading

tkoolen commented Dec 15, 2017 •

edited

Loading

tkoolen commented Dec 15, 2017 •

edited

Loading

Sacha0 commented Dec 17, 2017 •

edited

Loading

Sacha0 commented Dec 21, 2017 •

edited

Loading

andyferris commented Feb 17, 2018 •

edited

Loading