Restructure of the promotion mechanism for broadcast #18642

pabloferz · 2016-09-23T02:13:47Z

This addresses #18622 and replaces #18623. This PR is an overhaul of the promotion mechanism for broadcast and gets rid off some promote_eltype_op and _promote_op methods. The later is somewhat replaced by the already existing _default_eltype which is used by map and comprehensions.

pabloferz · 2016-09-23T02:35:00Z

promote_op is left only for unary and binary elementwise operators and matrix multiplication and it is decoupled from the rest of broadcast methods which will rely on _default_eltype.

TotalVerb · 2016-09-23T03:06:37Z

Will this mean that broadcast(sqrt, Integer[1.0, 2.0, 3.0]) will return Vector{Float64} now, consistent with maps and comprehensions? Currently it returns the strange Vector{Real} type, due to promote_op.

nalimilan · 2016-09-23T06:32:43Z

base/broadcast.jl

@@ -15,7 +16,7 @@ export broadcast_getindex, broadcast_setindex!
 broadcast(f) = f()
 @inline broadcast(f, x::Number...) = f(x...)
 @inline broadcast{N}(f, t::NTuple{N}, ts::Vararg{NTuple{N}}) = map(f, t, ts...)
-@inline broadcast(f, As::AbstractArray...) = broadcast_t(f, promote_eltype_op(f, As...), As...)
+@inline broadcast(f, As::AbstractArray...) = _broadcast(f, As...)


Is there still a point in calling this _broadcast instead of broadcast? Isn't multiple dispatch enough here?

Calling broadcast now first checks the containers types to determine the resulting container. In this case I was trying to avoid that check for the case were we know all are Array like containers.

martinholters · 2016-09-23T07:24:19Z

test/broadcast.jl

@@ -300,7 +300,6 @@ import Base.Meta: isexpr
 # PR 16988
 @test Base.promote_op(+, Bool) === Int
 @test isa(broadcast(+, [true]), Array{Int,1})
-@test Base.promote_op(Float64, Bool) === Float64


Are you dropping this test because it would fail now? That would be unfortunate in case something outside Base relies on it...

tkelman · 2016-09-23T10:08:51Z

base/broadcast.jl

@@ -253,17 +250,41 @@ function broadcast_t(f, ::Type{Any}, As...)
    B[I] = val
    return _broadcast!(f, B, keeps, Idefaults, As, Val{nargs}, iter, st, 1)
 end
-
-@inline broadcast_t(f, T, As...) = broadcast!(f, similar(Array{T}, broadcast_indices(As...)), As...)
+@inline function _broadcast_t(f, T, shape, iter, As...)


add some comments here and below describing what these are for

tkelman · 2016-09-23T10:09:43Z

base/broadcast.jl


 function broadcast_c(f, ::Type{Tuple}, As...)
    shape = broadcast_indices(As...)
-    check_broadcast_indices(shape, As...)


why is this being removed?

broadcast_indices already fails when the sizes are not compatible. The only places where we really need it is for broadcast! where we don't know the size of the supplied container.

tkelman · 2016-09-23T10:11:17Z

test/broadcast.jl

@@ -300,7 +300,6 @@ import Base.Meta: isexpr
 # PR 16988
 @test Base.promote_op(+, Bool) === Int
 @test isa(broadcast(+, [true]), Array{Int,1})
-@test Base.promote_op(Float64, Bool) === Float64


does this now return something different, or is it a method error?

I returns Any because I remove the specialized method when the first argument is a type. promote_op would only be used by the unary and binary elementwise operations in base, as well as by the matrix multiplication methods, where we know that the operator is not a type.

I guess I could leave the definition for uses outside Base addressing @martinholters concern above.

should it be deprecated if base won't need it going forward?

Yeah, I guess so, will do.

FWIW

promote_op(op::Type, T) = Core.Inference.return_type(op, Tuple{T}) promote_op(op::Type, T1, T2) = Core.Inference.return_type(op, Tuple{T1,T2})

seems to be inferable and handles cases where the constructor specializes, e.g.
promote_op(Array{Int64}, Int) then correctly returns Array{Int64,1}, not just Array{Int64}.

Not that I'm too worried about improving the obsolete promote_op, but maybe you can employ this somehow to increase the chances of getting a leaftype in _broadcast?

pabloferz · 2016-09-23T10:31:28Z

Will this mean that broadcast(sqrt, Integer[1.0, 2.0, 3.0]) will return Vector{Float64} now, consistent with maps and comprehensions? Currently it returns the strange Vector{Real} type, due to promote_op.

@TotalVerb: yes. The only case where it would work as before is for elementwise unary and binary operators in Base, where it would still preserve the more general abstract type.

pabloferz · 2016-09-24T11:16:12Z

Comments so far addressed. Can we check performance?

tkelman · 2016-09-24T11:18:45Z

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

nanosoldier · 2016-09-24T14:00:52Z

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

pabloferz · 2016-09-28T03:28:25Z

Can we ask nanosoldier again?

vchuravy · 2016-09-28T03:29:48Z

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

nanosoldier · 2016-09-28T06:16:22Z

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

pabloferz · 2016-09-28T17:03:18Z

Ok. I believe there should be no more performance regressions.

mbauman · 2016-09-28T17:04:27Z

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

nanosoldier · 2016-09-28T19:40:30Z

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

pabloferz · 2016-09-28T19:59:13Z

All right, there are no more regressions (just the usual noisy benchmarks). But feel free to review or make more suggestions.

tkelman · 2016-09-29T05:43:16Z

base/deprecated.jl

@@ -1009,4 +1009,15 @@ export @vectorize_1arg, @vectorize_2arg
 @deprecate abs(M::SymTridiagonal) abs.(M)
 @deprecate abs(x::AbstractSparseVector) abs.(x)

+# promote_op method where the operator is also a type
+function promote_op(op::Type, Ts::Type...)
+    depwarn("promote_op(op::Type, ::Type...) is deprecated as is no longer " *


stevengj · 2016-11-04T19:10:17Z

Best to run the benchmarks again, since broadcast benchmarks were only added recently (JuliaCI/BaseBenchmarks.jl#30), although I don't think any of them will be affected by the changes here.

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

nanosoldier · 2016-11-04T22:23:03Z

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

pabloferz · 2016-11-04T23:20:01Z

That's odd, some of those performance regressions weren't present before the last rebase (in which there are no changes, just the rebase). Probably other PRs touching broadcast or inference are causing the performance hit?

Anyway, I'll see what I can do.

stevengj · 2016-11-05T02:41:29Z

@pabloferz, the previous time the benchmark was run on this PR was September 28, which was before the broadcast benchmarks were added to BaseBenchmarks. ... oh wait, nanosoldier is showing an improvement in the broadcast fusion benchmarks. The regressions are in linalg?

pabloferz · 2016-11-05T03:20:07Z

The regressions in linalg may come from the fact that some matrix products, e.g. *(A::Diagonal, B::Diagonal) rely on broadcast. So the problem is probably with some element-wise binary operations.

pabloferz · 2016-11-05T14:51:54Z

The problem was with promote_op, it was being inlined before, but some changes here (and possibly elsewhere) were preventing inlining.

Should be better now.

martinholters · 2016-11-07T09:17:54Z

Let's see: @nanosoldier runbenchmarks(ALL, vs = ":master")

nanosoldier · 2016-11-07T12:27:25Z

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

martinholters · 2016-11-07T13:33:19Z

Ah yes, much better. What about the sparse transposes? Unrelated/noise?

pabloferz · 2016-11-07T16:23:38Z

I believe that the sparse transposes are either unrelated to the PR or noise.

stevengj · 2016-11-07T17:21:19Z

Yeah, it doesn't look like the sparse-transpose code uses anything touched by this PR.

The travis failure seems to be an unrelated network problem with apt-get.

I think this is okay to merge.

JeffBezanson · 2016-11-08T21:32:17Z

base/broadcast.jl

+@inline broadcast_elwise_op(f, As...) =
+    broadcast!(f, similar(Array{promote_eltype_op(f, As...)}, broadcast_indices(As...)), As...)
+
+ftype(f, A) = typeof(a -> f(a))


Can this just be typeof(f)?

Yeah, I think so, I don't know why I put it like this (same for the one below).

JeffBezanson · 2016-11-08T21:33:26Z

base/promotion.jl

-    T = _promote_op(f, _default_type(S))
+    @_pure_meta
+    Z = Tuple{_default_type(S)}
+    T = _default_eltype(Generator{Z, typeof(a -> f(a))})


* Restructure the promotion mechanism for broadcast * More broadcast tests * Use broadcast for element wise operators where appropriate

…g#18642).

…#21931)

…#21931) (cherry picked from commit 907afec)

pabloferz force-pushed the pz/betterbroadcast branch from 58796f1 to 87b0cb7 Compare September 23, 2016 02:57

nalimilan reviewed Sep 23, 2016

View reviewed changes

martinholters reviewed Sep 23, 2016

View reviewed changes

tkelman reviewed Sep 23, 2016

View reviewed changes

kshyatt added types and dispatch Types, subtyping and method dispatch broadcast Applying a function over a collection labels Sep 23, 2016

pabloferz force-pushed the pz/betterbroadcast branch 2 times, most recently from 6b55f4a to 0b7e17f Compare September 24, 2016 09:31

pabloferz force-pushed the pz/betterbroadcast branch from 0b7e17f to 5d4cd87 Compare September 27, 2016 19:07

pabloferz force-pushed the pz/betterbroadcast branch 2 times, most recently from 20f5ba0 to fa03b1d Compare September 28, 2016 14:36

pabloferz changed the title ~~WIP: Restructure promotion mechanism for broadcast~~ Restructure of the promotion mechanism for broadcast Sep 28, 2016

tkelman reviewed Sep 29, 2016

View reviewed changes

pabloferz force-pushed the pz/betterbroadcast branch 2 times, most recently from 7cca21a to 6bd6840 Compare October 4, 2016 15:19

pabloferz added 3 commits November 5, 2016 09:58

Restructure the promotion mechanism for broadcast

62e0225

More broadcast tests

403b32e

Use broadcast for element wise operators where appropriate

a174718

pabloferz force-pushed the pz/betterbroadcast branch from 8e78b26 to a174718 Compare November 5, 2016 15:59

stevengj merged commit d16d994 into JuliaLang:master Nov 7, 2016

This was referenced Nov 7, 2016

Broadcast promotion machinery assumes functions are binary #18622

Closed

Improve promotion for constructors... #18623

Closed

pabloferz deleted the pz/betterbroadcast branch November 7, 2016 17:44

JeffBezanson reviewed Nov 8, 2016

View reviewed changes

pabloferz mentioned this pull request Nov 8, 2016

Inference-based promote_op is not pure (fix #19270) #19273

Merged

timholy mentioned this pull request Feb 11, 2017

Broadcasting breakage for non-1 indices #20569

Closed

Sacha0 added a commit to Sacha0/julia that referenced this pull request May 17, 2017

Add NEWS.md entry for promote_op(::Type, Ts...) deprecation (JuliaLan…

2d5eb9e

…g#18642).

Sacha0 added needs news A NEWS entry is required for this change deprecation This change introduces or involves a deprecation and removed needs news A NEWS entry is required for this change labels May 17, 2017

ararslan pushed a commit that referenced this pull request May 19, 2017

Add NEWS.md entry for promote_op(::Type, Ts...) deprecation (#18642). (…

907afec

…#21931)

tkelman pushed a commit that referenced this pull request Jun 3, 2017

Add NEWS.md entry for promote_op(::Type, Ts...) deprecation (#18642). (…

2ec983d

…#21931) (cherry picked from commit 907afec)

Restructure of the promotion mechanism for broadcast #18642

Restructure of the promotion mechanism for broadcast #18642

Conversation

pabloferz commented Sep 23, 2016

pabloferz commented Sep 23, 2016

TotalVerb commented Sep 23, 2016 • 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

pabloferz Sep 23, 2016 • 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

pabloferz commented Sep 23, 2016 • edited Loading

pabloferz commented Sep 24, 2016

tkelman commented Sep 24, 2016

nanosoldier commented Sep 24, 2016

pabloferz commented Sep 28, 2016 • edited Loading

vchuravy commented Sep 28, 2016

nanosoldier commented Sep 28, 2016

pabloferz commented Sep 28, 2016

mbauman commented Sep 28, 2016

nanosoldier commented Sep 28, 2016

pabloferz commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Nov 4, 2016

nanosoldier commented Nov 4, 2016

pabloferz commented Nov 4, 2016 • edited Loading

stevengj commented Nov 5, 2016 • edited Loading

pabloferz commented Nov 5, 2016 • edited Loading

pabloferz commented Nov 5, 2016 • edited Loading

martinholters commented Nov 7, 2016

nanosoldier commented Nov 7, 2016

martinholters commented Nov 7, 2016

pabloferz commented Nov 7, 2016

stevengj commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotalVerb commented Sep 23, 2016 •

edited

Loading

pabloferz Sep 23, 2016 •

edited

Loading

pabloferz commented Sep 23, 2016 •

edited

Loading

pabloferz commented Sep 28, 2016 •

edited

Loading

pabloferz commented Nov 4, 2016 •

edited

Loading

stevengj commented Nov 5, 2016 •

edited

Loading

pabloferz commented Nov 5, 2016 •

edited

Loading

pabloferz commented Nov 5, 2016 •

edited

Loading