Make return type of broadcast inferrable with heterogeneous arrays #30485

nalimilan · 2018-12-21T21:40:02Z

Inference is not able to detect the element type automatically, but we can do it manually since we know promote_typejoin is used for widening.

Fixes #28382. The same changes should be applied to map.

base/broadcast.jl

nalimilan · 2018-12-21T21:44:13Z

test/broadcast.jl

@@ -360,7 +360,7 @@ end
 let f17314 = x -> x < 0 ? false : x
 @test eltype(broadcast(f17314, 1:3)) === Int
 @test eltype(broadcast(f17314, -1:1)) === Integer
- @test eltype(broadcast(f17314, Int[])) == Union{Bool,Int}
+ @test eltype(broadcast(f17314, Int[])) === Integer


This is a minor change which I think can be considered as a bug fix, in the sense that before this PR the element type when the input is empty will never be observed when the array isn't empty (we can only ever observe Int, Bool or Integer). I could change the PR to preserve the existing behavior if we want (e.g. for backports).

Oh that's interesting. Yeah, here's the current behaviors:

julia> eltype(broadcast(f17314, Int[])) Union{Bool, Int64} julia> eltype(broadcast(f17314, Int[1])) Int64 julia> eltype(broadcast(f17314, Int[-1])) Bool julia> eltype(broadcast(f17314, Int[1,-1])) Integer

The reason for using inference here in the first place is to preserve the performance in the non-empty case. Adding a fourth possible return type defeats such a purpose, so I'm in support of this change.

Exactly - we either need to change the last to match the first, or the first to match the last.

This PR seems the least breaking, and suitable for v1.x. If we ever wanted to consider the other way around maybe that should be a v2.0 change?

mbauman

I cannot meaningfully review the way in which you achieve the change in behaviors here, but I approve of the result and think this is a minor change worth making.

vtjnash · 2019-02-05T18:20:59Z

I'm still not sure. I'm not really sure that Base._return_type is reporting the right answer here, and I'm not sure that various hackery to make the answer more contorted is really the right solution that.

nalimilan · 2019-02-05T18:25:20Z

I'm still not sure. I'm not really sure that Base._return_type is reporting the right answer here, and I'm not sure that various hackery to make the answer more contorted is really the right solution that.

What do you mean? Why would return_type be incorrect? If it's imprecise in some cases, that's OK. Also it's already used, in particular when the result is empty. This PR doesn't really make things worse in that regard as it only affects inference.

Or do you have a better proposal? I agree it would be nice if the compiler did that automatically, but until it does we really need to avoid the inference failure that broadcast creates with Union{T,Missing}.

vtjnash · 2020-08-12T18:34:01Z

Upon much reflection, I now think this is actually sensible, and does actually help slightly to decouple inference from the result here, as Core.Compiler.return_types returns lists of types as a Union (possible inside a Tuple), and for usability, that shouldn't actually ever be consumed directly, but instead converted into a proper simplified type (as done here). I think there's some implication there also that users of return_types should never use anything but typejoin to widen their types, but that's just a predicability/quality improvement not correctness. And possibly return_types should do be doing this itself directly (to prevent users from depending on the specific complexities of inference in ways that can't even be seen at runtime), but this seems at least reasonable as an improvement to this consumer.

base/broadcast.jl

Inference is not able to detect the element type automatically, but we can do it manually since we know promote_typejoin is used for widening.

nalimilan · 2020-08-13T10:25:08Z

Thanks for the review. I'm amazed I didn't get more things wrong. :-p
I've added code to compute N for Vararg, let me know if it's correct.

Unfortunately, after rebasing against current master, the return type is only inferred as AbstractArray{...} while on the previous state of the PR if was inferred as a concrete Array{...} type. This is due to the fact that copyto_nonleaf! is now inferred as Any while before (on 1.5.0 it was still the case) it was inferred as Array, as indicated by e.g. @code_warntype Base.Broadcast.copyto_nonleaf!([1], Base.Broadcast.Broadcasted(+, ([1,2], [1,missing])), [1, 2], 1, 1). This PR is still an improvement so I could relax the tests for now until we find the fix.

nalimilan · 2020-09-30T07:03:49Z

Good to go @vtjnash?

base/broadcast.jl

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

JeffBezanson · 2020-10-05T21:45:45Z

IIUC, only the type assertion is needed for the inference improvement, and it seems to me that is much easier?

nalimilan · 2020-10-06T07:06:34Z

IIUC, only the type assertion is needed for the inference improvement, and it seems to me that is much easier?

AFAICT all changes are needed. promote_typejoin_union is needed to compute e.g. Integer when the function returns either Int or Bool. Otherwise the computed eltype will be Union{Int, Bool}, which isn't a supertype of Integer, so the assertion will fail.

What could be avoided is the minor change of eltype from Union{Int, Bool} to Integer when the input is empty (#30485 (comment)), but that doesn't make the PR more complex.

nalimilan · 2020-10-22T16:32:13Z

Bump. This is going to miss 1.6 (meaning it will reach its second anniversary without being released...).

nalimilan · 2020-10-25T14:53:49Z

Thanks @vtjnash and @JeffBezanson. I'll merge tomorrow if nobody objects (FreeBSD failure seems unrelated).

simeonschaub · 2021-02-12T13:27:14Z

test/broadcast.jl

+ @test_broken Core.Compiler.return_type(broadcast, Tuple{typeof(+), Vector{Int},
+ Vector{Union{Float64, Missing}}}) ==
+ Vector{<:Union{Float64, Missing}}
+ @test Core.Compiler.return_type(broadcast, Tuple{typeof(+), Vector{Int},
+ Vector{Union{Float64, Missing}}}) ==
+ AbstractVector{<:Union{Float64, Missing}}
+ @test isequal([1, 2] + [3.0, missing], [4.0, missing])
+ @test_broken Core.Compiler.return_type(+, Tuple{Vector{Int},
+ Vector{Union{Float64, Missing}}}) ==
+ Vector{<:Union{Float64, Missing}}
+ @test Core.Compiler.return_type(+, Tuple{Vector{Int},
+ Vector{Union{Float64, Missing}}}) ==
+ AbstractVector{<:Union{Float64, Missing}}
+ @test_broken Core.Compiler.return_type(+, Tuple{Vector{Int},
+ Vector{Union{Float64, Missing}}}) ==
+ Vector{<:Union{Float64, Missing}}
+ @test isequal(tuple.([1, 2], [3.0, missing]), [(1, 3.0), (2, missing)])
+ @test_broken Core.Compiler.return_type(broadcast, Tuple{typeof(tuple), Vector{Int},
+ Vector{Union{Float64, Missing}}}) ==
+ Vector{<:Tuple{Int, Any}}
+ @test Core.Compiler.return_type(broadcast, Tuple{typeof(tuple), Vector{Int},
+ Vector{Union{Float64, Missing}}}) ==
+ AbstractVector{<:Tuple{Int, Any}}


#39618 seems to fix these tests. @nalimilan Could that mean that this workaround might not even be needed anymore? Or does it maybe just fix this particular example?

Ah then it's great! Actually all the work I did in this PR had not effect for now due to this inference failure (a regression due to changes to reduce compile time introduced since 1.5). So if you can replace these @test_broken with @test then it's perfect!

Ah, cool. Thanks for checking!

Inference is not able to detect the element type automatically, but we can do it manually since we know promote_typejoin is used for widening. This is similar to the approach used for `broadcast` at #30485.

Inference is not able to detect the element type automatically, but we can do it manually since we know promote_typejoin is used for widening. This is similar to the approach used for `broadcast` at #30485. (cherry picked from commit 49e3aec)

…ng#42046) Inference is not able to detect the element type automatically, but we can do it manually since we know promote_typejoin is used for widening. This is similar to the approach used for `broadcast` at JuliaLang#30485.

nalimilan requested review from mbauman and vtjnash December 21, 2018 21:40

nalimilan commented Dec 21, 2018

View reviewed changes

base/broadcast.jl Outdated Show resolved Hide resolved

nalimilan commented Dec 21, 2018

View reviewed changes

nalimilan mentioned this pull request Dec 21, 2018

Poor inference of [1, 2] + [3.0, missing]` #28382

Closed

nalimilan added performance Must go faster broadcast Applying a function over a collection missing data Base.missing and related functionality labels Dec 21, 2018

mbauman added needs news A NEWS entry is required for this change minor change Marginal behavior change acceptable for a minor release labels Dec 21, 2018

mbauman reviewed Dec 21, 2018

View reviewed changes

nalimilan mentioned this pull request Feb 5, 2019

Fix getindex and add groupvars and groupindices JuliaData/DataFrames.jl#1709

Merged

bkamins mentioned this pull request Feb 5, 2019

Replace replace with a comprehension JuliaData/DataFrames.jl#1713

Open

bkamins mentioned this pull request Feb 6, 2019

Improve quantile in corner cases of collection eltype #30938

Merged

nalimilan mentioned this pull request Feb 8, 2019

Lazy broadcasting macro JuliaArrays/LazyArrays.jl#21

Merged

vtjnash added the forget me not PRs that one wants to make sure aren't forgotten label Dec 17, 2019

vtjnash reviewed Aug 12, 2020

View reviewed changes

base/broadcast.jl Outdated Show resolved Hide resolved

base/broadcast.jl Outdated Show resolved Hide resolved

base/broadcast.jl Outdated Show resolved Hide resolved

base/broadcast.jl Show resolved Hide resolved

base/broadcast.jl Outdated Show resolved Hide resolved

nalimilan added 5 commits August 13, 2020 10:59

Make return type of broadcast inferrable with heterogeneous arrays

d487b1d

Inference is not able to detect the element type automatically, but we can do it manually since we know promote_typejoin is used for widening.

Add var and std

3f1a1f1

Don't use @pure

8a1e65f

Stop using @pure with promote_typejoin

999f17f

Review fixes

707e0fd

nalimilan force-pushed the nl/inference branch from eb61cce to 707e0fd Compare August 13, 2020 10:21

nalimilan mentioned this pull request Aug 13, 2020

Fix type of allocated array when broadcasting type unstable function #37028

Merged

nalimilan requested a review from vtjnash September 29, 2020 12:33

Merge branch 'master' into nl/inference

f32e2a3

vtjnash reviewed Sep 30, 2020

View reviewed changes

base/broadcast.jl Outdated Show resolved Hide resolved

vtjnash reviewed Sep 30, 2020

View reviewed changes

base/broadcast.jl Outdated Show resolved Hide resolved

vtjnash reviewed Sep 30, 2020

View reviewed changes

base/broadcast.jl Outdated Show resolved Hide resolved

vtjnash requested a review from JeffBezanson September 30, 2020 14:37

Apply suggestions from code review

56d37ff

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

nalimilan added 2 commits October 22, 2020 18:32

Merge branch 'master' into nl/inference

5b55e06

Use typejoin

0c78029

nalimilan merged commit 65c7bf5 into master Oct 27, 2020

nalimilan deleted the nl/inference branch October 27, 2020 17:11

nalimilan mentioned this pull request Oct 28, 2020

set default max_methods to 3 #36208

Merged

KristofferC mentioned this pull request Nov 13, 2020

Type assert error in broadcasting on nightly #38422

Closed

simeonschaub reviewed Feb 12, 2021

View reviewed changes

simeonschaub removed the forget me not PRs that one wants to make sure aren't forgotten label May 29, 2021

nalimilan mentioned this pull request Aug 19, 2021

add pure kwarg to map JuliaData/PooledArrays.jl#71

Merged

nalimilan mentioned this pull request Aug 29, 2021

Make return type of map inferrable with heterogeneous arrays #42046

Merged

nalimilan mentioned this pull request Nov 17, 2021

Fix regression in map and collect #43120

Merged

nalimilan mentioned this pull request Mar 4, 2022

Fix pairwise for type-unstable corner case function JuliaStats/StatsBase.jl#772

Merged

vtjnash mentioned this pull request May 1, 2024

Create a function to use type inference for eltype #54157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make return type of broadcast inferrable with heterogeneous arrays #30485

Make return type of broadcast inferrable with heterogeneous arrays #30485

nalimilan commented Dec 21, 2018 •

edited

Loading

nalimilan Dec 21, 2018

mbauman Dec 21, 2018

andyferris Jan 2, 2019

mbauman left a comment

vtjnash commented Feb 5, 2019

nalimilan commented Feb 5, 2019

vtjnash commented Aug 12, 2020

nalimilan commented Aug 13, 2020 •

edited

Loading

nalimilan commented Sep 30, 2020

JeffBezanson commented Oct 5, 2020

nalimilan commented Oct 6, 2020

nalimilan commented Oct 22, 2020

nalimilan commented Oct 25, 2020

simeonschaub Feb 12, 2021 •

edited

Loading

nalimilan Feb 12, 2021

simeonschaub Feb 12, 2021

Make return type of broadcast inferrable with heterogeneous arrays #30485

Make return type of broadcast inferrable with heterogeneous arrays #30485

Conversation

nalimilan commented Dec 21, 2018 • edited Loading

nalimilan Dec 21, 2018

Choose a reason for hiding this comment

mbauman Dec 21, 2018

Choose a reason for hiding this comment

andyferris Jan 2, 2019

Choose a reason for hiding this comment

mbauman left a comment

Choose a reason for hiding this comment

vtjnash commented Feb 5, 2019

nalimilan commented Feb 5, 2019

vtjnash commented Aug 12, 2020

nalimilan commented Aug 13, 2020 • edited Loading

nalimilan commented Sep 30, 2020

JeffBezanson commented Oct 5, 2020

nalimilan commented Oct 6, 2020

nalimilan commented Oct 22, 2020

nalimilan commented Oct 25, 2020

simeonschaub Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

nalimilan Feb 12, 2021

Choose a reason for hiding this comment

simeonschaub Feb 12, 2021

Choose a reason for hiding this comment

nalimilan commented Dec 21, 2018 •

edited

Loading

nalimilan commented Aug 13, 2020 •

edited

Loading

simeonschaub Feb 12, 2021 •

edited

Loading