Skip to content

Fix nonmissingtype with unspecified type parameters#31016

Merged
vtjnash merged 2 commits intomasterfrom
nl/nonmissingtype
Apr 2, 2019
Merged

Fix nonmissingtype with unspecified type parameters#31016
vtjnash merged 2 commits intomasterfrom
nl/nonmissingtype

Conversation

@nalimilan
Copy link
Copy Markdown
Member

Fixes #31013.

@nalimilan nalimilan added the missing data Base.missing and related functionality label Feb 8, 2019
@nalimilan nalimilan requested a review from vtjnash February 8, 2019 23:15
@quinnj
Copy link
Copy Markdown
Member

quinnj commented Feb 9, 2019

I don't understand why the regular definition didn't work here? Specifically, nonmissingtype(::Type{Union{T, ,Missing}}) where {T} = T should have taken Union{Rational, Missing} and produced Rational, right? Any idea why that doesn't work?

@nalimilan
Copy link
Copy Markdown
Member Author

No idea, but that's may be related to #26135. Anyway using typesubtract sounds cleaner to me (only a single method definition is needed, and it avoids introducing exceptions in ambiguous.jl), and that's what we do in Missings.T (JuliaData/Missings.jl#82).

BTW, what about exporting the following function?

typesubtract(::Type{S}, ::Type{T}) where {S, T} = Core.Compiler.typesubtract(S, T)

Then we can recommend writing typesubtract(T, Missing) instead of the undocumented Base.nonmissingtype(T) or of Missings.T(T).

@nalimilan
Copy link
Copy Markdown
Member Author

Merge?

@bkamins
Copy link
Copy Markdown
Member

bkamins commented Feb 20, 2019

I think that typesubtract could be improved to better handle cases like Union{Missings.Missing, <:Number}`. This can be a separate PR if you prefer.

@nalimilan
Copy link
Copy Markdown
Member Author

Indeed. If the fix is in typesubtract, maybe better apply it separately.

@bkamins
Copy link
Copy Markdown
Member

bkamins commented Feb 20, 2019

This typesubtract part is tricky so it is really for a separate PR, e.g. you would have to go from something like:

Union{Missing, <:AbstractString, <:Number}

to something like:

Union{<:AbstractString, <:Number}

and this is a nested structure, but on the other hand, surprisingly:

julia> Union{Missing, Union{<:AbstractString, <:Number}}
Union{Missing, Union{#s1, #s2} where #s1<:Number where #s2<:AbstractString}

does not get reduced into a single union. On the other hand:

julia> Union{Missing, Union{<:AbstractString}}
Union{Missing, AbstractString}

gets simplified nicely.

@JeffBezanson - do you thing it worth to try to explore such special cases, or we leave them as I think in normal coding practice they are not very relevant.

@nalimilan nalimilan requested a review from JeffBezanson March 3, 2019 20:53
@nalimilan
Copy link
Copy Markdown
Member Author

Bump.

@vtjnash vtjnash merged commit 98673b5 into master Apr 2, 2019
@vtjnash vtjnash deleted the nl/nonmissingtype branch April 2, 2019 17:10
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

Development

Successfully merging this pull request may close these issues.

4 participants