-
Notifications
You must be signed in to change notification settings - Fork 367
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
change ⍰ to ? when showing a DataFrame with missing eltype #2140
Conversation
OK - this should be good for a final check (if you agree with the description I have given for the reimplementation of |
src/abstractdataframe/show.jl
Outdated
# handle the case when after removing Missing union type name is short | ||
length(sT) ≤ 8 && return sT * suffix | ||
suffix = "?" | ||
maxwidth -= 1 # we will add "?" at the end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously we didn't include ⍰
in the count, right? I think we'd better not include it, as it means that narrow columns allowing and not allowing for missing values will print a different abbreviation for the type, e.g. DataFrame(x=BigFloat[1, 2], y=Union{BigFloat, Missing}[1, 2])
will use "BigFloa" and "BigFlo" (IIUC).
Also, by taking into account ?
and …
the number of characters used for the type is now 6 instead of 8 when it's too long and allows for missing values. Previously BigFloat
would always be printed in full, and now it isn't. Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well - it was intentional (to normalize everything to 8), but I can change it. I will switch not to count "?" when determining width (which means sometimes it will be 9 like for BigFloat?
).
src/abstractdataframe/show.jl
Outdated
maxwidth -= 1 # we will add "…" at the end | ||
|
||
if T <: Union{CategoricalString, CategoricalValue} | ||
return (maxwidth ≥ 11 ? "Categorical…" : "Cat…") * suffix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the column is wider than 17 characters, can we print CategoricalValue
/CategoricalString
in full? For simplicity, we can use the same threshold for both, anyway the latter is going away soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently there is one nuance that T
can also be Union{CategoricalString, CategoricalValue}
(probably a very improbable case). I will fix it then and we can do what you propose.
@nalimilan - your requests are fixed. As usual (and now I remember why I had problems last time) - things were more problematic than expected. I had to distinguish the phase of initial width counting and display phase to ignore "?" (the problem was when actually the type name is the widest entry in the column and it contains missing). Things should be good now. |
Yeah, sorry for derailing the PR. These things are quite tricky. I hope the choice to special-case |
Thank you! |
Fixes #2139