Add promote_typejoin() and use it instead of typejoin() to preserve Union{T, Nothing/Missing} by nalimilan · Pull Request #25553 · JuliaLang/julia

nalimilan · 2018-01-14T14:53:40Z

This is a continuation of #24332, which cannot be reopened. It's an alternative to adding a more general strict/exact promotion mechanism (#25423). It achieves the same goal, i.e. it ensures that we use Union{T, Nothing/Missing} instead of Any with collect, map, broadcast and with Tuple and NamedTuple promotion.

nalimilan · 2018-01-14T16:22:17Z

test/tuple.jl

+        @test x isa Vector{Tuple{Int,Union{Int,T}}}
+
+        y = map(v -> (v[1], v[2]), [(1, T()), (1, 2)])
+        @test y isa Vector{Tuple{Int64,Union{T,Int64}}}


These Int64 explain the 32-bit CI failures. Will fix in the next push.

StefanKarpinski · 2018-01-16T00:21:16Z

Very nearly a CI straight flush. Win32 failure was a timeout.

JeffreySarnoff · 2018-01-19T20:04:03Z

😎

JeffBezanson · 2018-01-25T20:29:36Z

base/inference.jl

    n = length(p)
    if n > lim
-        tail = reduce(typejoin, Bottom, Any[p[lim:(n-1)]..., unwrapva(p[n])])
+        tail = reduce(promote_join, Bottom, Any[p[lim:(n-1)]..., unwrapva(p[n])])


This should not be changed in inference.

Right. I thought this was needed to change the promotion of tuples, but clearly I've been overzealous. I've reverted it.

JeffBezanson · 2018-01-25T20:33:35Z

Seems basically right.

…, Void/Missing} This allows for a more efficient code and array layout than using Any.

…priate typejoin() is now used only for inference, and promote_join() everywhere else to choose the appropriate element type for a collection.

…or Nothing Reuse the typejoin() logic.

JeffBezanson · 2018-01-25T22:46:05Z

base/tuple.jl


+# promotion
+
+promote_rule(::Type{S}, ::Type{T}) where {S<:Tuple, T<:Tuple} =


I think this should be removed. Changing the type of [(1,), (1.0,)] seems to be outside of the goal here.

The goal is to have [(1,), (missing,)] isa Vector{Tuple{Union{Int,Missing}} rather than Tuple{Any}. I agree that's not totally required in this PR, but it makes a lot of sense to avoid an inconsistency with map(identity, [(1,), (missing,)]). That's also consistent with [[1], [missing]]. In general I don't see why tuples shoudn't implement promotion, like other containers.

Another solution is to use promote_join rather than promote_type. This could be done either here (i.e. special-case tuples promotion to just use promote_join), or more generally when concatenating arrays. I would prefer a solution which applies consistently to all types.

Anyway I've added a commit to revert this for now, so that we can address these issues separately.

JeffBezanson · 2018-01-25T22:47:34Z

base/namedtuple.jl

 isempty(::NamedTuple{()}) = true
 isempty(::NamedTuple) = false

+promote_rule(::Type{NamedTuple{n, S}}, ::Type{NamedTuple{n, T}}) where {n, S, T} =


I would also remove this for now. Can be raised as a separate issue if necessary.

Remove promotion tests, revert unneeded Any[ specifications. Also add @_pure_meta to two methods to preserve current behavior (without it, vcat((1,), (1.0,)) is not inferred as Vector{Tuple{Real}}, which is caught by a test which previously would not detect this problem).

nalimilan · 2018-01-26T15:32:58Z

Thanks for the review. I must say I'm not very happy with the name promote_join, since promote operates on values but promote_join operates on types. Should it be promote_typejoin? Anyway it's internal so we can change it later.

JeffreySarnoff · 2018-01-26T15:41:25Z

promote_jointype?

vtjnash · 2018-01-26T18:08:09Z

base/promotion.jl

+typejoin(@nospecialize(a), @nospecialize(b)) = (@_pure_meta; join_types(a, b, typejoin))
+
+function join_types(@nospecialize(a), @nospecialize(b), f::Function)
    @_pure_meta


not pure anymore (due to the abstractly typed f::Function parameter)

Ah, I always get this wrong... Fixed.

Don't we need this function to be pure though?

I have no idea. We could use a loop inside @eval to define two separate functions which would be pure, without duplicating the code.

nalimilan · 2018-01-26T22:20:09Z

I've renamed promote_join to promote_typejoin. I find this more consistent with both promote_type and typejoin (promote_jointype would have had the drawback of being slightly different from the latter for no clear reason). Better suggestions welcome (typejoin_promote?...).

JeffreySarnoff · 2018-01-26T22:33:10Z

🆗

JeffBezanson · 2018-01-27T08:06:17Z

base/twiceprecision.jl

 promote_rule(::Type{TwicePrecision{R}}, ::Type{TwicePrecision{S}}) where {R,S} =
    TwicePrecision{promote_type(R,S)}
-promote_rule(::Type{TwicePrecision{R}}, ::Type{S}) where {R,S} =
+promote_rule(::Type{TwicePrecision{R}}, ::Type{S}) where {R,S<:Number} =


I'm not sure this is right. For some reason TwicePrecision{T}'s parameter does not need to be a Number. @timholy Why is that?

andreasnoack · 2019-01-01T15:56:59Z

base/promotion.jl


-Compute a type that contains both `T` and `S`.
+
+Return the closest common ancestor of `T` and `S`, i.e. the narrowest type from which


I think this docstring could be made clearer. I'm trying to figure out exactly what typejoin and promote_typejoin are doing and I don't think the current descriptions are complete. The current docstring for typejoin states that it returns

the narrowest type from which they both [T and S] inherit

but that must always be Union{T,S}. However, my understanding is that typejoin doesn't return unions and that promote_typejoin was introduced exactly because of that. So what is this function returning? My guess would be something like

T if S<:T
S if T<:S
else min(TS) for which T<:TS and S<:TS and isabstract(TS)

I think I just copied "closest common ancestor" from a comment and tried to make it a little more explicit. You're right that typejoin returns an abstract supertype and never a Union AFAIK.

If the definition I stated above is correct then it might be fine to just define ancestor to cover only declared abstract types (and concrete types), i.e. exclude unions. It probably makes sense the declared subset of the type lattice is a tree so ancestor feels natural.

The name of the function would be slightly a misnomer as the lattice join is the least upper bound, but I guess some upper bound is fine for our use.

Examples would then also be helpful in the docstring. I wouldn't mind preparing the PR but I would need a confirmation that my understanding of the function is correct.

nalimilan requested a review from JeffBezanson January 14, 2018 14:53

nalimilan force-pushed the nl/typejoin branch from 6d9edf6 to bfcd16d Compare January 14, 2018 14:54

This was referenced Jan 14, 2018

Add special case for Void in typejoin() to return Union{T, Void} #24332

Closed

WIP: Add promote_strict mechanism and use it instead of typejoin() #25423

Closed

nalimilan commented Jan 14, 2018

View reviewed changes

nalimilan force-pushed the nl/typejoin branch from bfcd16d to 8d90c23 Compare January 14, 2018 16:34

This was referenced Jan 19, 2018

new columns via transform and missing values JuliaData/DataFramesMeta.jl#84

Closed

Operations on Array{Union{T,Missing},N} return Array{Any,N} JuliaData/Missings.jl#74

Open

nalimilan mentioned this pull request Jan 20, 2018

RFC: Change indexin sentinel to nothing #25662

Merged

nalimilan added the triage This should be discussed on a triage call label Jan 25, 2018

JeffBezanson reviewed Jan 25, 2018

View reviewed changes

JeffBezanson added this to the 1.0 milestone Jan 25, 2018

JeffBezanson removed the triage This should be discussed on a triage call label Jan 25, 2018

nalimilan added 4 commits January 25, 2018 22:23

Add special case for Void and Missing in typejoin() to return Union{T…

664cb99

…, Void/Missing} This allows for a more efficient code and array layout than using Any.

Introduce promote_join() and use it instead of typejoin() where appro…

3ae9f50

…priate typejoin() is now used only for inference, and promote_join() everywhere else to choose the appropriate element type for a collection.

Support promoting parameters of Tuple and NamedTuple, add promotion f…

d7c501c

…or Nothing Reuse the typejoin() logic.

Rebase fixes

39ceccb

nalimilan force-pushed the nl/typejoin branch from 8d90c23 to 39ceccb Compare January 25, 2018 21:50

JeffBezanson reviewed Jan 25, 2018

View reviewed changes

vtjnash reviewed Jan 26, 2018

View reviewed changes

nalimilan added 2 commits January 26, 2018 23:04

Remove incorrect @_pure_meta annotation

5c8deed

Rename promote_join to promote_typejoin

556e416

JeffBezanson reviewed Jan 27, 2018

View reviewed changes

JeffBezanson merged commit f58d2cf into master Jan 27, 2018

JeffBezanson deleted the nl/typejoin branch January 27, 2018 17:17

nalimilan changed the title ~~Add promote_join() and use it instead of typejoin() to preserve Union{T, Nothing/Missing}~~ Add promote_typejoin() and use it instead of typejoin() to preserve Union{T, Nothing/Missing} Jan 29, 2018

nalimilan mentioned this pull request Jan 29, 2018

map() is slow with Union{T, Missing/Nothing} element type #25799

Closed

This was referenced Feb 7, 2018

Use promote_typejoin for Tuple and NamedTuple promotion #25924

Closed

hcat not type-stable with union types #26004

Open

nalimilan mentioned this pull request Feb 20, 2018

ensure promotion rules don't alter eltype values #26109

Merged

nalimilan mentioned this pull request Mar 17, 2018

Make promote_typejoin recursive #26501

Closed

nalimilan mentioned this pull request May 15, 2018

broadcasting over arrays of small unions #27106

Open

andreasnoack reviewed Jan 1, 2019

View reviewed changes

bramtayl mentioned this pull request Jul 11, 2019

Base.unzip() #13942

Open

devmotion mentioned this pull request Mar 24, 2020

Catch Array{Any} return value TuringLang/Turing.jl#1175

Closed


		# promotion

		promote_rule(::Type{S}, ::Type{T}) where {S<:Tuple, T<:Tuple} =


		Compute a type that contains both `T` and `S`.

		Return the closest common ancestor of `T` and `S`, i.e. the narrowest type from which

Uh oh!

Conversation

nalimilan commented Jan 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanKarpinski commented Jan 16, 2018

Uh oh!

JeffreySarnoff commented Jan 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeffBezanson commented Jan 25, 2018

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!

nalimilan commented Jan 26, 2018

Uh oh!

JeffreySarnoff commented Jan 26, 2018

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!

nalimilan commented Jan 26, 2018

Uh oh!

JeffreySarnoff commented Jan 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreasnoack Jan 1, 2019 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andreasnoack Jan 1, 2019 •

edited

Loading