Skip to content
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

promote_typejoin with abstract types #35504

Closed
nalimilan opened this issue Apr 17, 2020 · 9 comments · Fixed by #37019
Closed

promote_typejoin with abstract types #35504

nalimilan opened this issue Apr 17, 2020 · 9 comments · Fixed by #37019
Labels
missing data Base.missing and related functionality

Comments

@nalimilan
Copy link
Member

nalimilan commented Apr 17, 2020

Currently, promote_typejoin(Missing, T) returns Any when T isn't concrete. This can be annoying in cases where the type information is necessary to perform some operation (and not just for performance), like computing zero(T):

julia> nt1 = NamedTuple{(:x, :y),Tuple{Union{Missing, Int},Union{Missing, Int}}}((missing, missing));

julia> eltype(nt1)
Union{Missing, Int64}

julia> sum(skipmissing(nt1))
0

julia> nt2 = NamedTuple{(:x, :y),Tuple{Union{Missing, Int},Union{Missing, Float64}}}((missing, missing));

julia> eltype(nt2)
Any

julia> sum(skipmissing(nt2))
ERROR: MethodError: no method matching zero(::Type{Any})
[...]

This would work if promote_typejoin returned Union{Missing, Real} since zero is defined for that type.

This situation happens in practice, for example when a row or group of rows happens to contain only missing values (see JuliaData/DataFrames.jl#2196).

Would it make sense to change promote_typejoin to stop special-casing concrete types?

Cc: @JeffBezanson @vtjnash

@nalimilan nalimilan added the missing data Base.missing and related functionality label Apr 17, 2020
@pdeffebach
Copy link
Contributor

pdeffebach commented Apr 17, 2020

This would work if promote_type returned Union{Missing, Real} since zero is defined for that type.

I think you mean promote_typejoin rather than promote_type.

julia> promote_type(Union{Missing, Int}, Float64)
Union{Missing, Float64}

promote_typejoin is what's used when we get eltype(nt::NamedTuple) here. I don't understand the distinction, though.

@nalimilan
Copy link
Member Author

Woops, that was a typo. I was indeed talking about promote_typejoin everywhere.

(promote_type is very different as it's allowed to return a type which isn't a supertype of both inputs. It's used notably by vcat or by fallback methods for mathematical operators e.g. to return Float64 when the inputs mix integers and Float64.)

@pdeffebach
Copy link
Contributor

The solution seems to be altering this block here to add a method like

_promote_typejoin(a::Type{S}, b::Type{Union{T, Missing}}) = Union{typejoin(S, T), MIssing}

@nalimilan
Copy link
Member Author

The isconcretetype(T) check can simply be removed. That's mostly a design decision.

@pdeffebach
Copy link
Contributor

But we aren't joining to Missing, right? Those methods are just for joining Missing with something. Here, we are joinning Union{T, MIssing} with something that is not missing.

@pdeffebach
Copy link
Contributor

Would it be possible to get something that fixes this into 1.6?

@nalimilan
Copy link
Member Author

I've filed #36939 to get the ball rolling.

vtjnash added a commit that referenced this issue Aug 12, 2020
It is useful to have `promote_typejoin(Union{Missing, Int}, Float64}` return
`Union{Missing, Real}` instead of `Any`, in particular because `zero` is defined
on the former but not on the latter. This allows `sum(skipmissing(::NamedTuple))`
to work even when it contains only missing values.

Fixes #35504, Closes #36939

Co-authored-by: Milan Bouchet-Valat <[email protected]>
vtjnash added a commit that referenced this issue Sep 1, 2020
It is useful to have `promote_typejoin(Union{Missing, Int}, Float64}` return
`Union{Missing, Real}` instead of `Any`, in particular because `zero` is defined
on the former but not on the latter. This allows `sum(skipmissing(::NamedTuple))`
to work even when it contains only missing values.

Fixes #35504, Closes #36939

Co-authored-by: Milan Bouchet-Valat <[email protected]>
vtjnash added a commit that referenced this issue Sep 2, 2020
It is useful to have `promote_typejoin(Union{Missing, Int}, Float64}` return
`Union{Missing, Real}` instead of `Any`, in particular because `zero` is defined
on the former but not on the latter. This allows `sum(skipmissing(::NamedTuple))`
to work even when it contains only missing values.

Fixes #35504, Closes #36939

Co-authored-by: Milan Bouchet-Valat <[email protected]>
@nalimilan
Copy link
Member Author

This just got merged into Julia master, so it should be in 1.6.

@pdeffebach
Copy link
Contributor

Wonderful! Now basically all egen behaviors are easy in Julia!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing data Base.missing and related functionality
Projects
None yet
2 participants