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

Show Numbers compactly when typeinfo is a Union with Nothing or Missing #48822

Merged
merged 12 commits into from
Jan 3, 2024

Conversation

LilithHafner
Copy link
Member

@LilithHafner LilithHafner commented Feb 28, 2023

Before:

julia> show(Union{Missing, Float32}[missing, 1.0])
Union{Missing, Float32}[missing, 1.0f0]

After:

julia> show(Union{Missing, Float32}[missing, 1.0])
Union{Missing, Float32}[missing, 1.0]

This is reprable because of

convert(::Type{T}, x) where {T>:Union{Missing, Nothing}} = convert(nonmissingtype_checked(nonnothingtype_checked(T)), x)

Fixes #39590
Fixes #26847

This cannot be extended to arbitrary types because sometimes that method is overwritten (e.g. convert(Union{Nothing, Some{Int}}, 5) fails)

@LilithHafner LilithHafner added the display and printing Aesthetics and correctness of printed representations of objects. label Feb 28, 2023
@LilithHafner LilithHafner changed the title Show Bools as 0 and 1 when typeinfo is a Union of Bool with Nothing or Missing Show Numbers compactly when typeinfo is a Union with Nothing or Missing Feb 28, 2023
@LilithHafner LilithHafner added the arrays [a, r, r, a, y, s] label Feb 28, 2023
@LilithHafner
Copy link
Member Author

The function added here should be used in #45396 as well, creating a weak semantic merge conflict.

@StefanKarpinski
Copy link
Member

This feels a little ad hoc and messy. How about checking if the type info intersected with Number is the type of the value? That would handle a union with anything that isn't a number.

@LilithHafner
Copy link
Member Author

Clever!

Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem correct since only unions with Nothing or Missing have the necessary convert methods defined, so something like this will error:

julia> Union{String,Bool}[1]
ERROR: MethodError: Cannot `convert` an object of type 
  Int64 to an object of type 
  Union{Bool, String}
Closest candidates are:
  convert(::Type{T}, ::T) where T at Base.jl:61
Stacktrace:
 [1] setindex!(A::Vector{Union{Bool, String}}, x::Int64, i1::Int64)
   @ Base ./array.jl:966
 [2] getindex(#unused#::Type{Union{Bool, String}}, vals::Int64)
   @ Base ./array.jl:407
 [3] top-level scope
   @ REPL[5]:1

This check probably needs to be something like typesubtract(get(io, :typeinfo, Any), Union{Nothing,Missing}) === Bool instead

@vtjnash
Copy link
Member

vtjnash commented Mar 7, 2023

That function was deprecated a long time ago, but Base._promote_typesubtract will do what you are looking for, or nonmissingtype(nonnothingtype(T)) (those are exported)

@StefanKarpinski
Copy link
Member

Seems like for my proposal typeintersect will work:

julia> Base.typeintersect(Union{String,Bool}, Number)
Bool

julia> Base.typeintersect(Union{Int,Nothing,Missing}, Number)
Int64

@LilithHafner
Copy link
Member Author

IIUC, @StefanKarpinski want Union{String, Bool}[true] to show as Union{String, Bool}[1] because it is clear to a human that that 1 represents true and @simeonschaub want to show it as Union{String, Bool}[true] because Union{String, Bool}[1] errors when evaluated.

Alternatively, we could try to define

convert(::Type{Union{T1, NonNumber}}, x::Number) where T1 <: Number = convert(T1, x)

but this seems broken, fragile, and icky.

The implementation using nonmissingtype and nonnothingtype is 6ff4fc8, if we choose to revert the change @StefanKarpinski asked for.

@LilithHafner
Copy link
Member Author

My new response to @StefanKarpinski's comment

This feels a little ad hoc and messy. How about checking if the type info intersected with Number is the type of the value? That would handle a union with anything that isn't a number.

is, we should special case missing and nothing in show because we special case them in convert. Arbitrary unions with with non-Number types can't be printed without typeinfo because the conversion may not be defined, as @simeonschaub pointed out.

@LilithHafner
Copy link
Member Author

Bump, I think that I have addressed all the comments.

@LilithHafner
Copy link
Member Author

Bump

@LilithHafner
Copy link
Member Author

@simeonschaub, I believe I have made your requested changes, can you confirm?

@LilithHafner LilithHafner added the triage This should be discussed on a triage call label Aug 30, 2023
@LilithHafner
Copy link
Member Author

Tagging triage to get a decision on this. Do we want this abbreviation?

@LilithHafner
Copy link
Member Author

Triage says merge without the wider version and then reconsider wider version later if wanted

@LilithHafner LilithHafner removed the triage This should be discussed on a triage call label Dec 21, 2023
@LilithHafner LilithHafner merged commit 792a35b into master Jan 3, 2024
4 of 7 checks passed
@LilithHafner LilithHafner deleted the show-bool-union-typeinfo branch January 3, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
4 participants