Rework the broadcast API and document it by timholy · Pull Request #23939 · JuliaLang/julia

timholy · 2017-09-30T17:45:35Z

This is an attempt towards developing an API for broadcasting that is relatively easy to extend. Overall I'm pretty happy with it. For most custom array types, only two methods (Broadcast.rule and a broadcast-specific similar) need to be defined. Probably the best place to start reviewing this is with the documentation, or to compare the new way against the old one. ~~(I don't understand why the docs don't build locally, any ideas?)~~

There are two jobs left to do:

replace the broadcasting logic in sparse/higherorderfunctions.jl with the new API
consider how to issue deprecation warnings to packages that have been using the old (internal) API

I think it makes sense to first see whether people like this API before tackling either one. The existing sparse-broadcasting logic looks pretty complicated. I'd hope this may simplify it somewhat, but the proof will be in the pudding.

Fixes #20740, #22180 (edited).

nalimilan · 2017-09-30T18:39:30Z

Looks like this would also fix #22180 by the way?

nalimilan · 2017-09-30T18:42:26Z

Regarding the Documenter error:

567.181489  !! No doc found for reference '[`Broadcast.rule`](@ref)'. [src/manual/interfaces.md]

Maybe you need to export rule from Broadcast, or something like that?

timholy · 2017-10-03T00:29:51Z

OK, I think this is done. @Sacha0, aside from the core API changes this pretty significantly reworks broadcasting esp for sparse arrays, so I'd love it if you'd take a look. The biggest change is that it no longer needs an ambiguity-resolving layer or "second pass" at computation of output type---Broadcast.rule does it all in one pass.

timholy · 2017-10-03T01:51:50Z

~~Hmm, GitHub seems to have removed the ability to edit old posts. I wonder if that's deliberate.~~ (and now it's back...)

I was going to strike out the "docs don't build" comment above, since that works now too. (I'd just forgotten to add those functions to the stdlib section of the manual.)

andyferris

Nice design, as usual @timholy !

E.g. I think I can see this working fine with StaticArrays given I implement a static unit range (I'd have to try).

andyferris · 2017-10-03T03:25:31Z

base/broadcast.jl

+See [`Broadcast.Result`](@ref) and [`Broadcast.rule`](@ref).
+"""
+Base.similar(f, r::Result{BottomArray{N}}, As...) where N  = similar(Array{eltype(r)}, indices(r))
+# In cases of conflict we fall back on Array


andyferris · 2017-10-03T03:36:09Z

base/broadcast.jl

+
+# When ElType is not concrete, use narrowing. Use the first element of each input to determine
+# the starting output eltype; the _broadcast! method will widen `dest` as needed to
+# accomodate later values.


Silly question: Is it possible that this ends up in the situation where the element type is too narrow? E.g. For eltypes like Union{T,Null} might the final array end up with eltype T or Null? (This might prevent push! on a vector when you have broadcasted it - no problem in the functional case, though).

If I understand your question correctly, you can definitely get that, but it's not unique to this PR. (I think this PR doesn't alter the element-type behavior whatsoever.) julia 0.6:

julia> a = Real[1, 2] 2-element Array{Real,1}: 1 2 julia> b = map(identity, a) 2-element Array{Int64,1}: 1 2 julia> push!(b, 3.2) ERROR: InexactError() Stacktrace: [1] convert(::Type{Int64}, ::Float64) at ./float.jl:679 [2] push!(::Array{Int64,1}, ::Float64) at ./array.jl:617 julia> b = a .+ 1 2-element Array{Int64,1}: 2 3

The reasoning here is that inference is unfortunately likely to give Any occasionally for something that doesn't have to be that wide. So it's good to narrow the outputs where possible to keep instability from propagating.

Ok thanks for clarifying.

I'm wondering if we'll need to be "smarter" about union types in the future, now that they are faster.

Yes, that's a common behavior already. That could be problematic if you expected to set a value to null in the resulting array. I'm not sure what could be done about this, as we can't know what are the possible types without relying on inference, which we want to avoid. And for things like map(isnull, x) we definitely don't want an Array{Union{T, Null}} result.

See also #23940.

The good news is that there's broadcast!, so in the end you don't give anything up by forced narrowing.

timholy · 2017-10-03T12:05:52Z

doc/src/manual/interfaces.md

+    Broadcast.rule(::Type{<:AbstractArray{T,N}}) where {T,N} = BottomArray{N}
+    Broadcast.rule(::Type{A}, ::Type{BottomArray{N}}) where {A<:AbstractArray,N} = A
+
+If you want to leverage this `AbstractArray` types, it makes sense to have the unary `rule` return an


Just noticed I left a sentence unfinished here. Will fix before merging, but I'll wait to see if there are any more requested changes, just to avoid jamming CI or erasing the record of the successful tests.

fredrikekre · 2017-10-03T12:05:54Z

@nanosoldier runbenchmarks(ALL, vs=":master")

vchuravy · 2017-10-03T12:09:53Z

This looks like a very nice improvement, but if I understand this correctly this is orthogonal to the work @vtjnash has been doing in #23692 since we don't preserve the semantic information of the broadcast and can't desugar f.

timholy · 2017-10-03T12:18:42Z

Yes, completely orthogonal.

nanosoldier · 2017-10-03T15:49:51Z

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

timholy · 2017-10-03T15:58:00Z

OK, looks likely that some of those @inlines I removed were important, or that there's something borked about this. Can't fix today, but will try to get to it soon.

vtjnash · 2017-10-03T16:12:18Z

Yes, completely orthogonal.

Maybe to that PR, but I started another (currently secret) branch that is better than it and may depend quite heavily on this 🙂 (I don't mean to be cryptic, the branch just triggered some work on other issues first).

vtjnash · 2017-10-03T16:15:33Z

base/broadcast.jl

+longest(V1::Val, V2::Val) = longest(ntuple(identity, V1), ntuple(identity, V2))
+longest(t1::Tuple, t2::Tuple) = (true, longest(Base.tail(t1), Base.tail(t2))...)
+longest(::Tuple{}, t2::Tuple) = (true, longest((), Base.tail(t2))...)
+longest(t1::Tuple, ::Tuple{}) = (true, longest(Base.tail(t1), ())...)


use nothing as a sentinel value

Can you explain? I need to check for Tuple{} anyway when one tuple is longer than the other. I could add nothing as a new sentinel, but I'm not sure I understand why.

p-zubieta · 2017-10-04T00:39:01Z

base/broadcast.jl

+# we defer to any case where the result of `rule` is known.
+result_join(::Type{S}, ::Type{T}, ::Type{Unknown}, ::Type{Unknown}) where {S,T} = Unknown
+result_join(::Type{S}, ::Type{T}, ::Type{Unknown}, ::Type{U}) where {S,T,U}     = U
+result_join(::Type{S}, ::Type{T}, ::Type{U}, ::Type{Unknown}) where {S,T,U}     = U


result_join(::Type, ::Type, ::Type{U}, ::Type{Unknown}) where {U} = U and similarly for the ones above?

Sacha0 · 2017-10-04T03:33:27Z

base/sparse/higherorderfns.jl

-# dense vectors/matrices, and PromoteToSparse collections continue in the promote-to-sparse funnel
-FunnelToSparseBC = Union{Type{Any},Type{Vector},Type{Matrix},Type{PromoteToSparse},Type{AbstractSparseArray}}
-promote_spcontainertype(::FunnelToSparseBC, ::FunnelToSparseBC) = PromoteToSparse
+Broadcast.rule(::Type{SPVM}, ::Type{Tuple})                             = PromoteToSparse


Does this rule result in combinations involving tuples being sent to sparse broadcast?

Yes, somehow I misinterpreted the current rules as diverting them to your "second stage" 1-2 dimensional analysis. Will fix (and add a test).

In my view, sparse vectors should have precedence over tuples for broadcasting purposes. The problem IIUC is that the sparse broadcast implementations work only (for understandable practicality reasons) with combinations of sparse structures that share the same storage behavior. A possible way around this would be to create a wrapper for tuples that behaves as an SparseVector for sparse broadcasting purposes so this works.

Of course that can be done in another PR and with this new API in place, should much easier than before.

... create a wrapper ... that behaves as an SparseVector for sparse broadcasting purposes so this works.

Yes, writing a common iteration interface for all types that participate in sparse broadcast is the direction I have in mind broadly. Such an interface would enable removal of the sparsifystructured layer and associated allocation. Very much a post-1.0 project though :).

The problem IIUC is that the sparse broadcast implementations work only (for understandable practicality reasons) with combinations of sparse structures that share the same storage behavior.

Rather, including tuples in sparse broadcast is as simple as adding an appropriate sparsifystructured method and changing the couple of rules that Tim inadvertently changed in this pull request. tuples do not presently participate because the utility is not clear. Though given Vectors participate, perhaps tuples should as well :).

tuples do not presently participate because the utility is not clear. Though given Vectors participate, perhaps tuples should as well

Given they "behave" as vectors in combination with arrays, I think they should behave as Vectors also in combination with sparse arrays. I imagine it would be unexpected for some people to see them not working with sparse structures.

There have been requests asking even for general iterables with shapes to participate broadcast and I don't see why general iterables with shape, including tuples, should be excluded (given a clear broadcasting precedence is well established).

Sacha0 · 2017-10-04T03:35:01Z

base/sparse/higherorderfns.jl

+Broadcast.rule(::Type{SPVM}, ::Type{Broadcast.BottomVector})            = PromoteToSparse
+Broadcast.rule(::Type{SPVM}, ::Type{Broadcast.BottomMatrix})            = PromoteToSparse
+Broadcast.rule(::Type{PromoteToSparse}, ::Type{Broadcast.Scalar})       = PromoteToSparse
+Broadcast.rule(::Type{PromoteToSparse}, ::Type{Tuple})                  = PromoteToSparse


Likewise here, does this rule result in combinations involving tuples being sent to sparse broadcast?

Sacha0 · 2017-10-04T04:06:41Z

doc/src/manual/interfaces.md

+Base.similar(f, r::Broadcast.Result{ContainerType}, As...)
+```
+
+`f` is the operation being performed, `ContainerType` signals the resulting container type


Should there be an "and" following the comma, or should the period terminating this sentence instead be a comma, or am I misreading these sentences? :)

timholy · 2017-10-04T08:33:39Z

@nanosoldier runbenchmarks(ALL, vs=":master")

timholy · 2017-11-24T11:22:23Z

The problem turned out to be an unused type parameter in lines like

julia/base/broadcast.jl

Lines 176 to 177 in c94e502

    
           broadcast_similar(f, ::DefaultArrayStyle{N}, ::Type{Bool}, inds::Indices{N}, As...) where {N,ElType} = 
        
               similar(BitArray, inds)

due to a copy/paste from the lines above, detected thanks to @vchuravy's #24706 (though this is not dependent upon that PR).

I'm still a little reluctant to do the more general rename: what if it turns out that for purposes such as concatenation, etc, we want slightly different rules? I'd feel happier about generalizing the name if there were a proof-of-principle demonstrating that we can leverage this machinery for novel purposes without modification---but until then, I think it's better to use a too-narrow name than a too-broad one. However, I am willing to make the change if there is a strong consensus.

In any event, I think this is ready.

vchuravy · 2017-11-24T13:21:50Z

Build error on Travis is unrelated (libopenblas couldn't be loaded)

timholy · 2017-11-24T18:33:25Z

The Appveyor failure also looks unrelated.

I may merge this early next week in case it paves the way for #23692.

Sacha0 · 2017-11-26T04:46:12Z

doc/src/manual/interfaces.md

+It is worth noting that you do not need to (and should not) define both argument orders
+of this call; defining one is sufficient no matter what order the user supplies the arguments in.
+
+For `AbstractArray` types, defining a BroadcastStyle supersedes the fallback choice,


"a BroadcastStyle" -> "a BroadcastStyle"?

Sacha0 · 2017-11-26T05:01:29Z

doc/src/manual/interfaces.md

+```
+
+Whenever you subtype `AbstractArrayStyle`, you also need to define rules for combining
+dimensionalities, by creating a constructor for your style that takes a `Val(N)` argument.


Nicely done! :)

Sacha0 · 2017-11-26T05:07:35Z

test/broadcast.jl

+Base.setindex!(A::ArrayData, v::Any, i::Integer...) = setindex!(A.data, v, i...)
+Base.size(A::ArrayData) = size(A.data)
+Base.broadcast_similar(f, ::Broadcast.ArrayStyle{A}, ::Type{T}, inds::Tuple, As...) where {A,T} =
+    A(Array{T}(length.(inds)))


Array{T}(length.(inds)) -> Array{T}(uninitialized, length.(inds)) in anticipation of conflict? :)

This should be reverted someday

These broadcast styles were introduced in response to #23939 (comment) as a way to limit the "greediness" of Sparse's broadcasting implementation -- sparse only wanted to allow known combinations of array types (including Array but not any AbstractArray). The idea was to allow us to gradually improve the sparse broadcast implementation over 1.x in a non-breaking manner. Unfortunately, these special styles for Array make defining new styles in the heirarchy a bit of a pain (ref. #23939 (comment)), and it was making my life harder in getting the 1.0 breaking changes in. This commit removes these special broadcast styles in favor of just having Sparse identify the cases itself and re-dispatch back into the default implementation in the cases it doesn't know how to handle.

* Remove kludgy VectorStyle and MatrixStyle These broadcast styles were introduced in response to #23939 (comment) as a way to limit the "greediness" of Sparse's broadcasting implementation -- sparse only wanted to allow known combinations of array types (including Array but not any AbstractArray). The idea was to allow us to gradually improve the sparse broadcast implementation over 1.x in a non-breaking manner. Unfortunately, these special styles for Array make defining new styles in the heirarchy a bit of a pain (ref. #23939 (comment)), and it was making my life harder in getting the 1.0 breaking changes in. This commit removes these special broadcast styles in favor of just having Sparse identify the cases itself and re-dispatch back into the default implementation in the cases it doesn't know how to handle. * Add a graceful deprecation

* Remove kludgy VectorStyle and MatrixStyle These broadcast styles were introduced in response to JuliaLang/julia#23939 (comment) as a way to limit the "greediness" of Sparse's broadcasting implementation -- sparse only wanted to allow known combinations of array types (including Array but not any AbstractArray). The idea was to allow us to gradually improve the sparse broadcast implementation over 1.x in a non-breaking manner. Unfortunately, these special styles for Array make defining new styles in the heirarchy a bit of a pain (ref. JuliaLang/julia#23939 (comment)), and it was making my life harder in getting the 1.0 breaking changes in. This commit removes these special broadcast styles in favor of just having Sparse identify the cases itself and re-dispatch back into the default implementation in the cases it doesn't know how to handle. * Add a graceful deprecation

ararslan added design Design of APIs or of the language itself broadcast Applying a function over a collection labels Sep 30, 2017

ararslan added this to the 1.0 milestone Sep 30, 2017

timholy force-pushed the teh/extensible_broadcast branch from 3007181 to 4ac2424 Compare October 1, 2017 12:29

StefanKarpinski assigned mbauman Oct 2, 2017

StefanKarpinski requested a review from andyferris October 2, 2017 17:08

timholy changed the title ~~WIP/RFC: rework the broadcast API and document it~~ Rework the broadcast API and document it Oct 3, 2017

timholy force-pushed the teh/extensible_broadcast branch from 4ac2424 to 85ec999 Compare October 3, 2017 00:26

This was referenced Oct 3, 2017

Fix length calculation of broadcast over tuples #23887

Merged

Revert "Fix broadcast_indices (#22130)" & add test for issue #22180

Closed

andyferris approved these changes Oct 3, 2017

View reviewed changes

timholy commented Oct 3, 2017

View reviewed changes

vtjnash reviewed Oct 3, 2017

View reviewed changes

p-zubieta mentioned this pull request Oct 3, 2017

Extend sparse broadcast to VecOrMats #20102

Closed

p-zubieta reviewed Oct 4, 2017

View reviewed changes

Sacha0 reviewed Oct 4, 2017

View reviewed changes

timholy force-pushed the teh/extensible_broadcast branch from 85ec999 to 47a906c Compare October 4, 2017 08:33

This was referenced Nov 13, 2017

UnitRange broacasting with literals gives a Vector #24586

Closed

Assertion failure in jl_gf_invoke_lookup (detect_unbound_args-related) #24460

Closed

timholy force-pushed the teh/extensible_broadcast branch from 3ce4cb0 to f875cf9 Compare November 24, 2017 11:14

Sacha0 reviewed Nov 26, 2017

View reviewed changes

timholy added 3 commits November 26, 2017 05:12

Rework the broadcast API and document it (fixes #20740)

7c7f6ef

Improve the docstrings for broadcast_getindex and broadcast_setindex!

7f487b6

Restrict sparse broadcast promotion to Array

776139b

This should be reverted someday

timholy force-pushed the teh/extensible_broadcast branch from f875cf9 to 776139b Compare November 26, 2017 11:12

timholy merged commit d4a0e27 into master Nov 26, 2017

timholy deleted the teh/extensible_broadcast branch November 26, 2017 12:42

iblislin mentioned this pull request Nov 26, 2017

New Broadcast API in 0.7 dmlc/MXNet.jl#348

Closed

This was referenced Dec 4, 2017

drop dispatch rule that all method type parameters must be captured #21026

Closed

Extending in-place broadcast (broadcast!) for user types #24914

Closed

maleadt mentioned this pull request Dec 6, 2017

Broadcast regression #24944

Closed

timholy mentioned this pull request Dec 10, 2017

Use new broadcast API JuliaArrays/StaticArrays.jl#348

Closed

Sacha0 mentioned this pull request Dec 15, 2017

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

Merged

3 tasks

mbauman mentioned this pull request Mar 12, 2018

Remove kludgy VectorStyle and MatrixStyle #26430

Merged

timholy mentioned this pull request Jul 8, 2018

inference failure for broadcast implementations that use flatten (eg SparseArrays) #27988

Closed

timholy mentioned this pull request Aug 27, 2019

RFC: Refactor LinearAlgebra to use isa instead of method dispatch to avoid ambiguity hell #33072

Closed

Uh oh!

Conversation

timholy commented Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nalimilan commented Sep 30, 2017

Uh oh!

nalimilan commented Sep 30, 2017

Uh oh!

timholy commented Oct 3, 2017

Uh oh!

timholy commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andyferris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredrikekre commented Oct 3, 2017

Uh oh!

vchuravy commented Oct 3, 2017

Uh oh!

timholy commented Oct 3, 2017

Uh oh!

nanosoldier commented Oct 3, 2017

Uh oh!

timholy commented Oct 3, 2017

Uh oh!

vtjnash commented Oct 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-zubieta Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timholy commented Oct 4, 2017

Uh oh!

timholy commented Nov 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vchuravy commented Nov 24, 2017

Uh oh!

timholy commented Nov 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timholy commented Sep 30, 2017 •

edited

Loading

timholy commented Oct 3, 2017 •

edited

Loading

p-zubieta Oct 6, 2017 •

edited

Loading

timholy commented Nov 24, 2017 •

edited

Loading