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

Deprecate defaulting to scalar in broadcast (take 2) #26435

Merged
merged 3 commits into from Mar 22, 2018

Conversation

@mbauman
Copy link
Member

mbauman commented Mar 13, 2018

This PR replaces #26212. The end result is the same, but I like this implementation much better.

This PR consists of two commits — both pass tests locally and I'd like to keep them separate. The first commit changes the default behavior of broadcast to collect its arguments instead of treating them as scalars. Broadcast now calls a special helper function, broadcastable, on each argument. It ensures that the arguments support indexing and have a shape, or otherwise have defined a custom BroadcastStyle. The second commit layers on top a deprecation mechanism that makes this — almost entirely — non-breaking.

I believe this to be a compromise that will fix #18618 once deprecations are removed. Note that I kept all the types that are currently tested to behave as scalars as specializing broadcastable to return something that supports both indexing and axes, or otherwise has its own BroadcastStyle and internal implementation.

This also changes the behavior of broadcast such that if it ever wants to return a 0-dimensional array, it'll just "unwrap" that 0-dimensional array and return the contents instead.

Reasons I like this approach better than #26212:

  • We're going to be requiring folks to explicitly wrap things in a Ref to make them broadcast as scalars. In #26212, adding a Ref to an otherwise scalar expression would result in a 0-dimensional array as a result. Yes, I generally have a very strong aversion to these kinds of auto-wrapping/unwrapping, but in this case I believe it is warranted and ends up with fewer special cases. 0-dimensional arrays and "scalar" types are treated identically as far as broadcast is concerned.
  • Having an explicit broadcastable psuedo-conversion method makes it easier for other code to "work-around" broadcast and know how it'll behave. Call broadcastable on a thing and then you'll be able to know what broadcast is going to do on the basis of its axes. No digging around in BroadcastStyles or other internals — this one function makes sure that iterables and everything else can just work. (This isn't quite true, yet, as Ref isn't fully conforming with the iterable and axes spec, but I'd like to implement that separately.)
  • It's so much simpler to not need to worry about Broadcast.Scalar() styles.
@stevengj

This comment has been minimized.

Copy link
Member

stevengj commented Mar 13, 2018

Does wrapping all scalars in a Ref affect performance?

@mbauman

This comment has been minimized.

Copy link
Member Author

mbauman commented Mar 13, 2018

Numbers have both axes and indexing, so they don't need to be wrapped in a Ref. But even if you do wrap them in a ref, they're immediately unwrapped again (if all other arguments are 0-dim or similarly "scalar").

julia> f(a, b) = Ref(a) .+ Ref(b)
       g(a, b) = a .+ b
g (generic function with 1 method)

julia> @code_llvm f(1,2)

; Function f
; Location: REPL[7]:1
define i64 @julia_f_34293(i64, i64) {
top:
; Function broadcast; {
; Location: broadcast.jl:636
; Function broadcast; {
; Location: broadcast.jl:640
; Function +; {
; Location: int.jl:53
  %2 = add i64 %1, %0
;}}}
  ret i64 %2
}

julia> @code_llvm g(1,2)

; Function g
; Location: REPL[7]:2
define i64 @julia_g_34294(i64, i64) {
top:
; Function broadcast; {
; Location: broadcast.jl:228
; Function +; {
; Location: int.jl:53
  %2 = add i64 %1, %0
;}}
  ret i64 %2
}
@vchuravy
Copy link
Member

vchuravy left a comment

I like this direction. Automatically unwrapping Ref is technically a breaking change, but I don't see a clean way of deprecating this. Maybe add a deprecation warning to broadcastable(::Ref)?

"""
struct DefaultArrayStyle{N} <: AbstractArrayStyle{N} end
(::Type{<:DefaultArrayStyle})(::Val{N}) where N = DefaultArrayStyle{N}()
const DefaultVectorStyle = DefaultArrayStyle{1}
const DefaultMatrixStyle = DefaultArrayStyle{2}
BroadcastStyle(::Type{<:AbstractArray{T,N}}) where {T,N} = DefaultArrayStyle{N}()
BroadcastStyle(::Type{<:Ref}) = DefaultArrayStyle{0}()
BroadcastStyle(::Type{<:Union{Base.RefValue,Number}}) = DefaultArrayStyle{0}()

This comment has been minimized.

@vchuravy

vchuravy Mar 13, 2018

Member

This also changes RefArray (which is a good thing). Should there be a BroadcastStyle for RefArray?

This comment has been minimized.

@vtjnash

vtjnash Mar 13, 2018

Member

Why is that a good thing (I don't actually know what this return value means)? For context, RefArray is a pointer to precisely one element of an array. It also changes Ptr, if that matters?

This comment has been minimized.

@mbauman

mbauman Mar 13, 2018

Author Member

All arguments' BroadcastStyles are combined with a promote-like mechanism in order to select a broadcasting implementation. For the most part, it's largely used as a kludge around the lack of a "at least one vararg is of type T" dispatch, but it's more complicated to deal with cases that would otherwise be ambiguities.

Yes, Ptr has neither axes or indexing defined, so it cannot be — by itself — considered like a 0-dimensional array. I don't think RefArray is common enough to add special broadcasting support for it.

Before:

julia> identity.(Ref([1,2,3],1))
0-dimensional Array{Int64,0}:
1

After (edit: now out of date):

julia> identity.(Ref([1,2,3],1))
┌ Warning: broadcast will default to iterating over its arguments in the future. Wrap arguments of
│ type `x::Base.RefArray{Int64,Array{Int64,1},Nothing}` with `Ref(x)` to ensure they broadcast as "scalar" elements.
│   caller = top-level scope
└ @ Core :0
Base.RefArray{Int64,Array{Int64,1},Nothing}([1, 2, 3], 1, nothing)

In the future, it will be an error because:

julia> collect(Ref([1,2,3],1))
ERROR: MethodError: no method matching length(::Base.RefArray{Int64,Array{Int64,1},Nothing})

This comment has been minimized.

@vtjnash

vtjnash Mar 13, 2018

Member

I don't think RefArray is common enough to add special broadcasting support for it.

However, RefValue is an implementation detail, and should never be mentioned explicitly either: RefArray is simply an alternative implementation of it. We know that length(::Ref) == 1 forall Ref subtypes, we've just hesitated to add that definition to the code until needed to keep the API small.

This comment has been minimized.

@mbauman

mbauman Mar 13, 2018

Author Member

This is largely a historical accident due to how I ended up here… and trying to work around Ptr. As it stands, I can easily remove all uses of RefValue. I just pushed a change.

New after:

julia> identity.(Ref([1,2,3],1))
1
if Base.Broadcast.BroadcastStyle(typeof(x)) isa Broadcast.Unknown
depwarn("""
broadcast will default to iterating over its arguments in the future. Wrap arguments of
type `x::$(typeof(x))` with `Ref(x)` to ensure they broadcast as "scalar" elements.

This comment has been minimized.

@vchuravy

vchuravy Mar 13, 2018

Member

Maybe add:

or define broadcastable(x::MyType) = Ref(x) for the conversion to be automatic.

This comment has been minimized.

@mbauman

mbauman Mar 13, 2018

Author Member

I'm guessing that the most common reader of this deprecation will be an end-user.

broadcastable(x::AbstractArray) = x
# In the future, default to collecting arguments. TODO: uncomment once deprecations are removed
# broadcastable(x) = BroadcastStyle(typeof(x)) isa Unknown ? collect(x) : x
# broadcastable(::Dict) = error("intentionally unimplemented to allow development in 1.x")

This comment has been minimized.

@vchuravy

vchuravy Mar 13, 2018

Member

Will this be a different PR? We should deprecate this behaviour in 0.7

This comment has been minimized.

@mbauman

mbauman Mar 13, 2018

Author Member

AbstractDict hits the fallback ::Any deprecation. Next to that deprecation is a task-item to un-comment these definitions, so when that happens it will go from a warning to an error.

@inline broadcast(f, A, Bs...) =
broadcast(f, combine_styles(A, Bs...), nothing, nothing, A, Bs...)
@inline function broadcast(f, A, Bs...)
A′ = broadcastable(A)

This comment has been minimized.

@andyferris

andyferris Mar 13, 2018

Contributor

It's unfortunate that the prime looks so similar to the adjoint...

This comment has been minimized.

@mbauman

mbauman Mar 13, 2018

Author Member

¯\_(ツ)_/¯

There's more than one of us using this style in base to represent roughly-the-same-value-but-maybe-slightly-different.

This comment has been minimized.

@stevengj

stevengj Mar 14, 2018

Member

You could also use , but I agree that the \prime style is pretty common.

broadcastable(x::AbstractArray) = x
# In the future, default to collecting arguments. TODO: uncomment once deprecations are removed
# broadcastable(x) = BroadcastStyle(typeof(x)) isa Unknown ? collect(x) : x
# broadcastable(::Dict) = error("intentionally unimplemented to allow development in 1.x")

This comment has been minimized.

@andyferris

andyferris Mar 13, 2018

Contributor

::AbstractDict

This comment has been minimized.

@andyferris

andyferris Mar 13, 2018

Contributor

Also we might consider if NamedTuple should maybe broadcast together with a AbstractDict{Symbol} rather than by iteration index.

@vchuravy

This comment has been minimized.

Copy link
Member

vchuravy commented Mar 13, 2018

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

@mbauman

This comment has been minimized.

Copy link
Member Author

mbauman commented Mar 13, 2018

Nanosoldier is going to fail without that commit I just pushed.

@vchuravy

This comment has been minimized.

Copy link
Member

vchuravy commented Mar 13, 2018

Well it didn't listen to me anyway...

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

(Put I am just being overly cautious)

@@ -687,6 +687,21 @@ end
# After deprecation is removed, enable the @testset "indexing by Bool values" in test/arrayops.jl
# Also un-comment the new definition in base/indices.jl

# Broadcast no longer defaults to treating its arguments as scalar (#)
function Broadcast.broadcastable(x)

This comment has been minimized.

@vtjnash

vtjnash Mar 13, 2018

Member

should have @noinline to ensure depwarn tracking works

@nalimilan
Copy link
Contributor

nalimilan left a comment

Thanks for your efforts to improve broadcast!

Cc: @timholy

Return either `x` or an object like `x` such that it supports `axes` and indexing.
"""
broadcastable(x::Union{Symbol,String,Type,Function,UndefInitializer,Nothing,RoundingMode}) = Ref(x)

This comment has been minimized.

@nalimilan

nalimilan Mar 13, 2018

Contributor

I guess you can add Missing to this list.

@@ -457,13 +460,13 @@ as in `broadcast!(f, A, A, B)` to perform `A[:] = broadcast(f, A, B)`.
end

# Optimization for the all-Scalar case.

This comment has been minimized.

@nalimilan

nalimilan Mar 13, 2018

Contributor

Is this still the "all scalar case"?

arguments and the scalars themselves in `As`. Singleton and missing dimensions
are expanded to match the extents of the other arguments by virtually repeating
the value. By default, only a limited number of types are considered scalars,
including `Number`s, `Type`s and `Function`s; all other arguments are iterated

This comment has been minimized.

@nalimilan

nalimilan Mar 13, 2018

Contributor

Could give the full list (in particular strings).

- If all the arguments are scalars, it returns a scalar.

This comment has been minimized.

@nalimilan

nalimilan Mar 13, 2018

Contributor

I imagine this rule still applies? Maybe worth keeping for clarity even if it's covered by more general rules below.

@@ -905,6 +905,11 @@ Deprecated or removed
* `map` on dictionaries previously operated on `key=>value` pairs. This behavior is deprecated,
and in the future `map` will operate only on values ([#5794]).

* Previously, broadcast defaulted to treating its arguments as scalars if they were not

This comment has been minimized.

@nalimilan

nalimilan Mar 13, 2018

Contributor

It would be useful to add a sentence for package developers mentioning broadcastable, just like the deprecation does.

Return either `x` or an object like `x` such that it supports `axes` and indexing.
"""
broadcastable(x::Union{Symbol,AbstractString,Type,Function,UndefInitializer,Nothing,RoundingMode,Missing}) = Ref(x)

This comment has been minimized.

@JeffBezanson

JeffBezanson Mar 13, 2018

Member

Can remove Type here since it has a method below.

@mbauman mbauman force-pushed the mb/broadcastable branch from 4f2c4fe to fb59643 Mar 13, 2018

@KristofferC

This comment has been minimized.

Copy link
Contributor

KristofferC commented Mar 14, 2018

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

@nanosoldier

This comment has been minimized.

Copy link

nanosoldier commented Mar 14, 2018

Something went wrong when running your job:

IOError(EOFError() during request(https://api.github.com/repos/JuliaLang/julia/statuses/fb59643d8cfa5011d66a4d09d8e38227b977a3e5))

cc @ararslan

broadcastable(x)
Return either `x` or an object like `x` such that it supports `axes` and indexing.
"""

This comment has been minimized.

@fredrikekre

fredrikekre Mar 15, 2018

Member

Perhaps add an example?

# Examples
```jldoctest
julia> Broadcast.broadcastable([1, 2, 3])
3-element Array{Int64,1}:
 1
 2
 3

julia> Broadcast.broadcastable("Hello, world!")
Base.RefValue{String}("Hello, world!")
```
@@ -423,6 +423,23 @@ end
broadcastable(x)
Return either `x` or an object like `x` such that it supports `axes` and indexing.
If `x` supports iteration, the returned value should match [`collect(x)`](@ref).

This comment has been minimized.

@stevengj

stevengj Mar 15, 2018

Member

This isn't true. e.g. collect(3) returns an Array{Int,0}, but broadcastable just returns 3.

This comment has been minimized.

@mbauman

mbauman Mar 15, 2018

Author Member

Fair. How about I add with respect to axes and indexing?

Edit: or If `x` supports iteration, the returned value should have the same `axes` and indexing behaviors as [`collect(x)`](@ref)

@mbauman mbauman force-pushed the mb/broadcastable branch from 3272ab2 to 37bd43e Mar 16, 2018

@mbauman

This comment has been minimized.

Copy link
Member Author

mbauman commented Mar 19, 2018

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

@nanosoldier

This comment has been minimized.

Copy link

nanosoldier commented Mar 19, 2018

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

@StefanKarpinski

This comment has been minimized.

Copy link
Member

StefanKarpinski commented Mar 20, 2018

That does not look too bad – all these regressions seem like they could be in the noise.

@stevengj

This comment has been minimized.

Copy link
Member

stevengj commented Mar 20, 2018

The slowdowns for tuple broadcasting seem like they might be real. They are testing:

round.(Int, (rand(n)...,))

for various n ≤ 10. Would be worth trying that manually to verify.

@mbauman

This comment has been minimized.

Copy link
Member Author

mbauman commented Mar 20, 2018

Well that was a remarkably simple fix. Once CI passes again I'd like to merge.

@vchuravy

This comment has been minimized.

Copy link
Member

vchuravy commented Mar 20, 2018

While we wait on CI.
@nanosoldier runbenchmarks(ALL, vs = ":master")

@ararslan

This comment has been minimized.

Copy link
Member

ararslan commented Mar 20, 2018

While we wait on CI.

Except that Nanosoldier takes about 3 times longer than our longest running CI build. 😉

@ararslan

This comment has been minimized.

Copy link
Member

ararslan commented Mar 20, 2018

I had to restart the server. @nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier

This comment has been minimized.

Copy link

nanosoldier commented Mar 20, 2018

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

@vchuravy

This comment has been minimized.

Copy link
Member

vchuravy commented Mar 21, 2018

There is still a regression for ["broadcast", "typeargs", "(\"tuple\", 10)"] and a deprecation warning in the Pkg3 tests

@mbauman

This comment has been minimized.

Copy link
Member Author

mbauman commented Mar 21, 2018

I fought with that remaining allocation for quite some time with no success. It's the Ref{Type{Int}}(Int) that is getting heap-allocated in some situations, but it's not clear to me why. Everything downstream of the Ref is inlined and it doesn't escape, but I think things are just on the edge of what the optimizer can see. Nothing is obviously problematic to my eyes. My attempts at reducing this down to a MWE have failed — it works as I'd expect once I pull the relevant methods out of Base and into my own module. I'm guessing that it's because I've not copied the entire method tables.

Given that this regression is small and the optimizer is getting revamped, I'm inclined to merge.

mbauman added some commits Mar 21, 2018

Default to iterating over broadcast arguments
Broadcast now calls a special helper function, `broadcastable`, on each argument. It ensures that the arguments support indexing and have a shape, or otherwise have defined a custom `BroadcastStyle`.

@mbauman mbauman force-pushed the mb/broadcastable branch from 6c76d2b to 54f54a5 Mar 21, 2018

@StefanKarpinski

This comment has been minimized.

Copy link
Member

StefanKarpinski commented Mar 21, 2018

Would be interesting to see if the new optimizer can eliminate that allocation.

@mbauman mbauman merged commit 274f0a2 into master Mar 22, 2018

4 of 5 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
ci/circleci: build-i686 Your tests passed on CircleCI!
Details
ci/circleci: build-x86_64 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details

@mbauman mbauman deleted the mb/broadcastable branch Mar 22, 2018

@mbauman

This comment has been minimized.

Copy link
Member Author

mbauman commented on e17a2a2 Mar 22, 2018

@KristofferC this is the commit that needs to be upstreamed to Pkg3.jl.

This comment has been minimized.

Copy link
Contributor

KristofferC replied Mar 22, 2018

👍 Just fyi, there is no problem if you did everything in one commit, the cherrypick command using the subtree strategy will pick out only those changes that are relevant for Pkg3.

@fredrikekre

This comment has been minimized.

Copy link
Member

fredrikekre commented Mar 23, 2018

The manual section on the broadcasting interface still uses Broadcast.Scalar in a couple of places resulting in

WARNING: Broadcast.Scalar is deprecated, use DefaultArrayStyle{0} instead.
  likely near /home/fredrik/julia-master/doc/make.jl:149

when building the docs.

@fredrikekre

This comment has been minimized.

Copy link
Member

fredrikekre commented Mar 23, 2018

And

julia/base/broadcast.jl

Lines 593 to 596 in 7eb5d44

julia> broadcast(+, 1.0, (0, -2.0), Ref(1))
2-element Array{Float64,1}:
2.0
0.0
returns a tuple, but that seems to be intended.

julia> broadcast(+, 1.0, (0, -2.0), Ref(1))
(2.0, 0.0)

That example seem rather pointless now that Ref(1) and 1 behaves the same.

mbauman added a commit that referenced this pull request Mar 23, 2018

Simplify broadcastable a bit further and add docs
Amazing what a great exercise writing documentation is. This is a followup to #26435, and more clearly lays out the requirements for defining a custom (non-AbstractArray) type that can participate in broadcasting.  The biggest functional change here is that types that define their own BroadcastStyle must now also implement `broadcastable` to be a no-op.
@mbauman

This comment has been minimized.

Copy link
Member Author

mbauman commented Mar 23, 2018

Thank you! See #26601.

mbauman added a commit that referenced this pull request Apr 4, 2018

Simplify broadcastable a bit further and add docs (#26601)
* Make LinAlg fess up to using Base._return_type

* Simplify broadcastable a bit further and add docs

Amazing what a great exercise writing documentation is. This is a followup to #26435, and more clearly lays out the requirements for defining a custom (non-AbstractArray) type that can participate in broadcasting.  The biggest functional change here is that types that define their own BroadcastStyle must now also implement `broadcastable` to be a no-op.

* Trailing comma, doc fixes from review; more tests

@mbauman mbauman added the broadcast label Apr 24, 2018

@vtjnash vtjnash referenced this pull request May 5, 2018

Open

Notation for lazy map #19198

@GiggleLiu

This comment has been minimized.

Copy link
Contributor

GiggleLiu commented Sep 26, 2018

Thanks for the design of Ref and very explicit broadcastable interface.

The only problem is defining broadcastable for every type is too expensive for developers. We should probably provide a default broadcastable function (like before).

If broadcastable is not defined for a type, then its instance will be broadcasted by Ref, this is OK. If the program instead throws an error like length is not defined (like now), this is less intuitive. It will make people less confidense to use broadcast operations.

@fredrikekre

This comment has been minimized.

Copy link
Member

fredrikekre commented Sep 26, 2018

There is a default, which is broadcastable(x) = x.

@GiggleLiu

This comment has been minimized.

Copy link
Contributor

GiggleLiu commented Sep 26, 2018

@fredrikekre
see the discussion here,

https://discourse.julialang.org/t/broadcasting-over-structs-in-1-0-what-am-i-supposed-to-do/13408

We have to implement the interface like

julia> Broadcast.broadcastable(m::M) = Ref(m)

to make a type work as a scalar.

A minimal working example:

julia> struct A end

julia> println.(A())
ERROR: MethodError: no method matching length(::A)
Closest candidates are:
  length(::Core.SimpleVector) at essentials.jl:571
  length(::Base.MethodList) at reflection.jl:728
  length(::Core.MethodTable) at reflection.jl:802
  ...
Stacktrace:
 [1] _similar_for(::UnitRange{Int64}, ::Type, ::A, ::Base.HasLength) at ./array.jl:532
 [2] _collect(::UnitRange{Int64}, ::A, ::Base.HasEltype, ::Base.HasLength) at ./array.jl:563
 [3] collect(::A) at ./array.jl:557
 [4] broadcastable(::A) at ./broadcast.jl:609
 [5] broadcasted(::Function, ::A) at ./broadcast.jl:1134
 [6] top-level scope at none:0

@GiggleLiu GiggleLiu referenced this pull request Oct 3, 2018

Closed

Broadcastable Register #139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment