-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Printing of time-related types in containers is inconsistent #30901
Comments
Matrix printing sets What we could also do is check the Cc: @fchorney |
We should remove the branch that switches to |
@tkf Would you make a pull request? |
I think I can make a PR, but I need to understand the desired output. If we check Branching on Likewise, Line 104 in 62c411d
It seems to me that it should be using 3-arg (All my above comments are based on the assumption that changing |
I guess we can defer the Regarding the two-argument vs. three-argument |
I already asked him this in #30757. His answer (#30757 (comment)) was:
So maybe
I think there are three concepts and only two methods. Ideally, it's nice to have:
3-arg |
TBH I'd rather not discuss the redesign of the printing system here as it's really beyond the scope of this issue. Feel free to file another issue if you have a detailed proposal.
Why not. Though as a data point, Unitful.jl represents arrays of values with units like this (which is not parseable): julia> show(fill(1Unitful.m, 2))
Quantity{Int64,𝐋,Unitful.FreeUnits{(m,),𝐋,nothing}}[1 m, 1 m] So the equivalent would be |
I personally don't like this as it appears to be valid Julia code but it isn't. Since we already decided that
I like this idea. Can we implement the fix for DataFrames first? |
Agree. Actually I will partly retract my previous comment --- with the printing system as it is now, using |
@nalimilan |
We can't print
What do you want to fix in DataFrames? AFAICT it uses |
At the moment nothing needs to change. If we decide to move away from using the compact show to display the human-readable output then it would be good to adjust DataFrames behaviour so it can continue to show the human-readable output. |
I don't think DataFrames should diverge from vector printing. |
@JeffBezanson So
@nalimilan In principle I think we can add an API (say) |
Yes, that's the best we can do. I guess |
Is |
@JeffBezanson Regarding:
("2-arg" added by me) A downside of this approach is that Lines 592 to 601 in 0ec1727
|
🤷♂️ |
Seems much more readable to me. |
@JeffBezanson So do you think we need to change something here? Better not release 1.2 with a new behavior if we want to change it again in 1.3. In particular, it seems weird to me that the 3-argument julia> show(stdout, "text/plain", Day(1))
1 day
julia> show(IOContext(stdout, :compact=>false), Day(1))
Day(1)
julia> show(IOContext(stdout, :compact=>true), Day(1))
1 day
julia> show(stdout, "text/plain", Date(2018, 12, 7))
2018-12-07
julia> show(IOContext(stdout, :compact=>false), Date(2018, 12, 7))
Date(2018, 12, 7)
julia> show(IOContext(stdout, :compact=>true), Date(2018, 12, 7))
2018-12-07 Maybe |
One thing happening here is that printing like |
Having something like |
I think using a new MIME like See also #30757 |
A new MIME type might be ok but I think it solves a different problem. The problem here is that this output is weird:
The context (REPL display) does not require parseable output, it's just that |
OK I think I get caught up the name
|
That's an interesting idea. I believe the main downside is that it won't give us this output back:
if that's what we want. |
Would something like this at least feel consistent? That being said, I am still unsure what we would want the solution for the
|
Isn't that exactly what was implemented before --- using the |
For that specific type yes. I thought the main issue was that the printing of things wasn't necessarily consistent, so maybe I misunderstood. My example above also changes how Date and DateTime is printed in various situations, which seemed to be consistent among the 3 types (as far as printing a "compact" vs "not compact" output goes). From what I understand, perhaps there are two issues.
Please correct me if I am misunderstanding some aspect of this. I'm trying to wrap my head around what we want the end goal to be, so that I can jump in and start working on the solution. |
Just a small point:
I don't think this difference is justified: we have |
IMO, the goal should be for This issue also complains about the difference in printing between vectors and matrices. Ideally they should be the same; that can be done by either (1) turning The only contexts I can think of where |
Should |
ok I think I'm starting to understand. Thanks! julia> d = Day(1)
1 day
julia> show(d)
Day(1)
julia> showelement(d)
1 day
julia> show([d])
Day[1]
julia> show([d d])
Day[1 1]
julia> show(IOContext(stdout, :compact=>true), [d])
Day[1]
julia> show(IOContext(stdout, :compact=>false), [d])
Day[1]
julia> [d, d][1]
1 day
julia> fill(d, 2)
2-element Array{Day,1}:
Day(1)
Day(1)
julia> fill(d, 2, 2)
2×2 Array{Day,2}:
Day(1) Day(1)
Day(1) Day(1)
julia> print(d)
1 day
julia> string(d)
"1 day"
julia> repr(d)
"Day(1)"
julia> fill(d => d, 2)
2-element Array{Pair{Day,Day},1}:
Day(1) => Day(1)
Day(1) => Day(1)
julia> fill(d => d, 2, 3)
2×3 Array{Pair{Day,Day},2}:
Day(1)=>Day(1) Day(1)=>Day(1) Day(1)=>Day(1)
Day(1)=>Day(1) Day(1)=>Day(1) Day(1)=>Day(1) |
I guess Im having an issue with the whole julia> show([d])
Day[1] since it seems like above people don't really like this as it looks like parsable julia code but isn't (yet). In the same vain, it seems like people also don't like:
since it duplicates the type. |
Those would call |
We could allow overloading another function (Base calls it |
What about |
I think that would look like
|
Would that be handled by 2-arg 1-element Array{Array{Day,1},1}:
[1, 1, …, 1, 1] or even 1-element Array{Array{Day,1},1}:
[1 day, 1 day, …, 1 day, 1 day] is possible.
I suppose |
Currently array elements use 2-argument show, so I think I think
No, |
Wasn't this the reason why we don't have parsable If you say that only the call signature is the public API of
Thanks for pointing it out. I totally missed this point. |
Interesting, I think I see what you mean --- for values like days, 3-argument show would print the desired In fact, 3-arg show is never(?) called recursively, so we could potentially use |
That's exactly what I'm thinking! |
So I'm continuing to work on this. I think this would be a nice improvement. I am unsure how to overload the function If I add something such as function Base.typeinfo_implicit(x::Day)
return true
end Doing that seems to not do anything as calling |
|
OK, I'm on board with trying switching array elements to The other discussion item is what to show in Vectors. I'm inclined to just remove the inconsistency and use the same printing as matrices. Printing vector elements non-compactly is nice for showing all digits of numbers, but I'm not sure it's really so important or useful outside of that. |
ok cool, this is my current working output. I'm thinking this satisfies all if not most of the discussion here. Hopefully if this makes people happy, I can move on to getting the other time/date types feeling consistent with this, and I'll open up a PR. julia> using Dates
julia> d = Day(1)
1 day
julia> show(d)
Day(1)
julia> show([d])
[Day(1)]
julia> show([d d])
[Day(1) Day(1)]
julia> show(IOContext(stdout, :compact=>true), [d])
[Day(1)]
julia> show(IOContext(stdout, :compact=>false), [d])
[Day(1)]
julia> [d, d][1]
1 day
julia> fill(d, 2)
2-element Array{Day,1}:
1 day
1 day
julia> fill(d, 2, 2)
2×2 Array{Day,2}:
1 day 1 day
1 day 1 day
julia> print(d)
1 day
julia> string(d)
"1 day"
julia> repr(d)
"Day(1)"
julia> fill(d => d, 2)
2-element Array{Pair{Day,Day},1}:
Day(1) => Day(1)
Day(1) => Day(1)
julia> fill(d => d, 2, 3)
2×3 Array{Pair{Day,Day},2}:
Day(1)=>Day(1) Day(1)=>Day(1) Day(1)=>Day(1)
Day(1)=>Day(1) Day(1)=>Day(1) Day(1)=>Day(1) |
Looks perfect to me! |
- Use 3 arg compact show for array elements. - Add show/print methods for periods - Periods imply their typeinfo, so set typeinfo_implicit to true for all period types fixes #30901
I think #30817 changed
show(io, "text/plain", ::Array{Day})
etc. On cb7a569 (and in Julia 1.0 and 1.1) it was:But now (c0e9182) it's:
Furthermore, confusingly (to me),
Matrix{Day}
still prints human readable form:However, @omus and others approved the following behavior in #30200:
So maybe the current behavior of
Vector{Day}
andMatrix{Day}
is intentional? (But, to be honest, I was confused whyndims
of an array changes how elements are printed.)Another confusing part is that
show(io, ::Array{Day})
still shows human-readable form. Shouldn't it closer torepr
than 3-argumentshow
?Note that the output of
Dict{Day,Day}
andVector{Pair{Day,Day}}
are not changed. It has been (and is):What is the intended printing behavior of
Day
,Date
, and alike in the containers?See also: #30683
The text was updated successfully, but these errors were encountered: