IOContext: introduce the :typeinfo property #24651

rfourquet · 2017-11-19T15:57:46Z

Before, the :compact property was conflating 2 concepts:

are we low on screen space?
can we skip printing individual type information for elements in a collection?

Cf. #22981 for context and discussion. Credit to Stefan Karpinski for the
formulation of the design implemented here.

</end commit message>

So this tackle only the redundancy in type information, not the number of digits of array elements to print (refered to as :sigdigits in the linked PR).
Here is a short demo:

julia> a = rand(Float16, 2) # BEFORE:
2-element Array{Float16,1}:
 Float16(0.1699)
 Float16(0.127)

julia> show(a)
Float16[0.16992, 0.12695] # why the need to print more digits here?

julia> reshape(a, 1, 1, 2)
1×1×2 Array{Float16,3}:
[:, :, 1] =
 Float16(0.1699)

[:, :, 2] =
 Float16(0.127)

julia> a = rand(Float16, 2) # AFTER
2-element Array{Float16,1}:
 0.1699
 0.127

julia> show(a)
Float16[0.1699, 0.127]

julia> reshape(a, 1, 1, 2)
1×1×2 Array{Float16,3}:
[:, :, 1] =
 0.1699

[:, :, 2] =
 0.127

julia> show(ModInt[ModInt{3}(2), ModInt{4}(2)])  # BEFORE
Main.ModInts.ModInt[2, 2]     # ouch, not quite unambiguous

julia> [ModInt{3}(2)]  # but quite redundant here
1-element Array{Main.ModInts.ModInt{3},1}:
 2 mod 3

julia> show(ModInt[ModInt{3}(2), ModInt{4}(2)]) # AFTER
Main.ModInts.ModInt[2 mod 3, 2 mod 4]

julia> [ModInt{3}(2)]
1-element Array{Main.ModInts.ModInt{3},1}:
 2

julia> [Set([Set([Float16(1)])])]  # just because I can! # BEFORE
1-element Array{Set{Set{Float16}},1}:
 Set(Set{Float16}[Set(Float16[1.0])])

julia> [Set([Set([Float16(1)])])] # AFTER
1-element Array{Set{Set{Float16}},1}:
 Set([Set([1.0])])

I didn't update here the show method for every object which could take advantage of this new attribute, but this can be done incrementally.

Also, as my editor (emacs) had trouble indenting the "show.jl" file because it's too big, editing this file was a nightmare, so I put the method related to showing arrays in a new file, "arrayshow.jl", which I believe is a better organization anyway, regardless of my editing comfort. The code was quite entangled, with non-consistent function names, so I had also to refactor to have a chance to understand the interaction between the functions and to include the new feature (I'm fine of course with suggestions for naming).

The main refactoring change is that I deleted the labyrinthic showarray method, which had 2 options: repr and header. By tracing how it was called in base, it appeared that basically 2 independant code paths were used depending on the combination of those 2 options, so the most sane action was to split it into 2 distinct methods, which implement show and display. The result is a bit less powerful (not possible to call showarray(io, X, false, header=false), but in the case were this was needed in base (for printing invalid strings), this was equivalent to print_array(io, X) (which doesn't know about :typeinfo, but this is fine for UInt8 elements).

TODO:

write basic documentation to IOContext's docstring (but there are already some explanatory comments in the new file)
tests
remove TODO in the commit message
remove duplicated alignment in the first commit

JeffBezanson · 2017-11-19T17:50:10Z

Nice work! 👍

why the need to print more digits here?

I think we should always print all digits, except in a context that's clearly for convenient interactive use (i.e. display).

rfourquet · 2017-11-20T11:20:13Z

I think we should always print all digits, except in a context that's clearly for convenient interactive use (i.e. display).

Ah OK. So none of the current IOContext attributes seem like a good fit for this choice of number of digits, so this looks like a good motivating example for introducing the :sigdigits attribute (which could be also modified in REPL.Options). As this is beyond the scope of this current PR, I will change back to printing more digits here in the meantime.

nalimilan · 2017-11-20T13:39:22Z

base/arrayshow.jl

+    end
+end
+
+typeinfo_context(io::IO) = get(io, :typeinfo, Any)::Union{DataType,UnionAll}


Is this wrapper really useful? It makes it actually harder to read the code for people who don't know that it's just a call to get.

I'm fine with removing it, it's mainly a leftover from my successive re-factorization of the implementation of the new feature. But one thing which I will was addressed is that the default value of attributes (Any in this case) was set once and for all, instead of repeated at each call site. I have been thinking of defining getindex(io::IO, ::Symbol) which would take care of that, but it's not clear that it would be the best way.

nalimilan · 2017-11-20T13:42:29Z

Thanks for this!

Would it be possible to start with a commit which doesn't move code around? It's really hard to review with this large diff...

rfourquet · 2017-11-20T15:15:03Z

Would it be possible to start with a commit which doesn't move code around?

I was expecting such request... If you don't mind, I think it would be easier to first move code around and then introduce the functionality.

rfourquet · 2017-11-20T19:10:24Z

This is now split: the first commit is very basic cut-n-paste from one file to another, and the second implements the new stuff. It's indeed a much easier diff. I realize that I didn't move all the printing-array related functions to the new file (like summary or alignment), I think it would be cleaner to move them, so will probably do so in subsequent commits.

rfourquet · 2017-11-21T14:19:49Z

I now added tests and documentation. I still wish to review the whole thing myself at least once, but I think this is in pretty good shape (I will just have to rebase and edit a commit message before merge).

I realize that I didn't move all the printing-array related functions to the new file (like summary or alignment),

I now just moved one more method of alignment which is clearly related to array printing, but otherwise I think other methods are not sufficiently specific to the core implementation of array printing, so I left them in "show.jl".

nalimilan

Looks quite good in general! Here are my comments.

nalimilan · 2017-11-23T10:46:06Z

base/arrayshow.jl

+
+# 0-dimensional arrays
+print_array(io::IO, X::AbstractArray{T,0} where T) = isassigned(X) ?
+    show(io, X[]) :


Unusual indentation.

nalimilan · 2017-11-23T10:47:05Z

base/arrayshow.jl

+print_array(io::IO, X::AbstractArray) = show_nd(io, X, print_matrix, true)
+
+# typeinfo aware
+# implements: show(io::IO, ::MIME"text/plain", X::AbstractArray)


Why is this called _display if it implements show?

Because basically this method of show is what is invoked by display, whereas calling show(object) prints something else.

Call it _showtextplain, then? It's not only called by display, nor is it necessarily called by display (depends on the Display), and confusing display overloading with show overloading is a perennial problem.

Not sure what was wrong with the old showarray, though… can't we leave the name as-is?

@rfourquet Bump!

IIRC, the old showarray was doing mainly two distinct things, which have been separated out into two distinct functions here, so it made sense to use new names: otherwise which one should inherit the old name? But maybe there is an easy answer (which also doesn't break code which may have used this function)? (I can't look at this right now).

Was already fixed in #25775

nalimilan · 2017-11-23T10:50:36Z

base/arrayshow.jl

+    io = IOContext(io, :typeinfo => eltype(X))
+
+    # 1) print summary info
+    print(io, summary(X))


Not your code, but this should use summary(io, X) to allow summary to use IOContext information.

FWIW, I had to fix that in #23806 too.

nalimilan · 2017-11-23T10:52:20Z

base/arrayshow.jl

+
+### non-Vector arrays
+
+# _show & _show_empty: main helper function for show(io, X)


"functions"

nalimilan · 2017-11-23T10:56:57Z

base/arrayshow.jl

+
+# NOTE: it's not clear how this method could use the :typeinfo attribute
+_show_empty(io::IO, X::Array{T}) where {T} = print(io, "Array{$T}(", join(size(X),','), ')')
+_show_empty(io, X) = nothing # by default, we don't know this constructor


Maybe better print an error?

You may be right, but I'm not sure. I think I prefer to not change this behavior here (which is the current one in master), and tackle this in a separate PR if the right thing to do is found.

nalimilan · 2017-11-23T12:40:06Z

test/nullable.jl

@@ -102,6 +102,8 @@ for (i, T) in enumerate(types)
    showcompact(io2, get(x3))
    @test String(take!(io1)) == @sprintf("Nullable{%s}(%s)", T, String(take!(io2)))

+    # these tests are disabled rather than fixed, because Nullable is to be removed from base


Just leaving a note so that we don't forget it.

What do you mean?

Just that I left that comment so that it's obvious to everybody that this shouldn't be merged until Nullable has been removed so that we don't leave these commented out tests.

nalimilan · 2017-11-23T12:40:53Z

test/sets.jl

@@ -70,7 +70,7 @@ end
    @test ===(eltype(s3), Float32)
 end
 @testset "show" begin
-    @test sprint(show, Set()) == "Set{Any}()"
+    @test sprint(show, Set()) == "Set(Any[])"


I'm not sure that change is an improvement. Maybe that allows simplifying the code, but Set{Any} is really the information that should be printed AFAICT.

Set{Any} is really the information that should be printed AFAICT.

Why do you think so? I find it more regular (and preferable) to always print Set(array-form) than resorting to Set{T}() only when it's empty, and as you said it simplifies the code.

Ah, OK. I thought we printed e.g. Set{Real}([1, 2]}, but since that's not the case I guess it's fine.

I think the printing may predate being able to write it that way, maybe we should change it to that instead?

Indeed. Though probably better left for a separate PR.

I got used to the printing Set(array) ! and on a not so serious note, it's more compact, we save 2 characters! But yes I would prefer to leave that to anther PR.

nalimilan · 2017-11-23T12:43:38Z

test/show.jl

+    @test showstr(Set([[Int16(1)]])) == "Set(Array{Int16,1}[[1]])"
+    @test showstr([Float16(1)]) == "Float16[1.0]"
+    @test showstr([[Float16(1)]]) == "Array{Float16}[[1.0]]"
+    @testset "nested Any eltype" begin


Would be nice to test with other abstract types like Real.

The specifity of Any (compared to an abstract type in general) is that a collection is isa Any and its elements are also isa Any, and I don't know which other type would satisfy this. But I can of course add a test for Real.

OK. I was thinking about the Any special case (see my comment).

nalimilan · 2017-11-23T12:46:37Z

base/show.jl

@@ -61,6 +61,11 @@ The following properties are in common use:
 - `:displaysize`: A `Tuple{Int,Int}` giving the size in rows and columns to use for text
   output. This can be used to override the display size for called functions, but to
   get the size of the screen use the `displaysize` function.
+ - `:typeinfo`: a type specification characterizing the information already expressed


"type specification" is a bit vague. Maybe "A Type"? "expressed" could be "printed".

nalimilan · 2017-11-23T12:48:09Z

base/show.jl

@@ -61,6 +61,11 @@ The following properties are in common use:
 - `:displaysize`: A `Tuple{Int,Int}` giving the size in rows and columns to use for text
   output. This can be used to override the display size for called functions, but to
   get the size of the screen use the `displaysize` function.
+ - `:typeinfo`: a type specification characterizing the information already expressed
+   concerning the type of the object about to be displayed. This is mainly useful when
+   displaying a collection of object of the same type, so that redundant type information


"objects". Close the parentheses.

Maybe a simpler example would be easier to understand, e.g. with [Float16(1)]? The text should say what's the value of the argument in that case.

rfourquet · 2017-11-23T16:57:47Z

Thanks @nalimilan for your review! I updated accordingly, except for the "compact printing" `Float16" which is not sorted out yet.

nalimilan · 2017-12-06T09:35:05Z

Looks like this PR has been kind of forgotten. Could you rebase it and squash the commits, except the one which just moves code around? That will make it easier to review and ready for merging.

rfourquet · 2017-12-06T10:32:09Z

Thanks for reviving it! but I had understood your comment as meaning that this depends on your PR which replaces Nullable by Union{...} to be merged first, did I misunderstand? if yes, what do you suggest that I do with the Nullable tests?

nalimilan · 2017-12-06T10:39:14Z

Yeah, I guess I hoped it would take less time to merge my PR. You could just remove Nullable tests in this one.

StefanKarpinski · 2017-12-06T15:51:59Z

Would it be possible to separate the commits into code motion versus changes? I find it a bit hard to review in the current state with code motion and changes combined. Either order is fine.

rfourquet · 2017-12-06T16:38:17Z

Squashed all but the first commit, which strictly only moves some code to the new "arrayshow.jl" file, should be easier to review now.

StefanKarpinski

This is very nice – thanks for making the change. It's great to have type info only displayed when it's not redundant!

rfourquet · 2017-12-07T11:56:55Z

Thanks!
I triggered by accident the @assert error from typeinfo_prefix (https://github.com/JuliaLang/julia/pull/24651/files#diff-6f3c3cea343e12203f3635ea6504d6b3R474), while displaying Base.active_repl. It turned out that show_default, which prints its fields, must reset the :typeinfo property, because there is a priori no link between the displayed type of an object and that of its fields.
Now, I can't guaranty this assertion won't be triggered again, but it just proved it was nonetheless useful for catching incomplete integration of the :typeinfo feature, so it seems fine for me to keep the @assert.

StefanKarpinski · 2017-12-07T16:16:02Z

Agree, this code is not performance-critical so no harm in leaving the assertion in there. Better to know about our bugs than remain clueless.

StefanKarpinski · 2017-12-07T16:16:37Z

From my perspective this is good to go, so please feel free to merge this when you see fit.

rfourquet · 2017-12-07T17:10:18Z

Ok, I will keep it open a couple of days before re-squashing and merging.

StefanKarpinski · 2017-12-07T17:21:07Z

Why wait that long? It's been open for comment for a while already. It's not like the idea is controversial – it's pretty clearly the right way to go.

rfourquet · 2017-12-07T17:29:54Z

Why wait that long?

Mainly to give @nalimilan the time to review if he wanted, as he is the one having requested to squash... but he had reviewed already, so you are right, I won't wait so long! (at least just to have again CI green).

nalimilan · 2017-12-07T17:34:38Z

base/arrayshow.jl

+
+### non-Vector arrays
+
+# _show_nonemtpy & _show_empty: main helper functions for show(io, X)


"_show_nonempty". Same below.

Good catches! Thank you

nalimilan

Looks good to me apart from the few details I just noted. BTW, there's also a typo in the commit title: "IOContex".

nalimilan · 2017-12-07T17:35:22Z

base/arrayshow.jl

-    end
-end
+# a specific call path is used to show vectors (show_vector)
+_show_nonemtpy(::IO, ::AbstractVector, ::String) = error("_show_nonemtpy(::IO, ::AbstractVector, ::String) is not implemented")


Break at 92 characters?

nalimilan · 2017-12-07T17:36:37Z

base/arrayshow.jl

+# similar to `eltype`, but in some cases this would lead to incomplete
+# information: assume we are at the top level, and no typeinfo is set,
+# and that it is deduced to be typeinfo=Any by default, and consider
+# printing X = Any[1]; to know if the eltype of X is already diplayed,


"displayed"

nalimilan · 2017-12-07T17:38:20Z

base/set.jl

-    show_vector(io, s, "[", "]")
-    print(io, ")")
+    print(io, "Set(")
+    show_vector(io, s)


Is there a reason why we have print_array but show_vector?

I'm not good at naming, and I forged them on top of what was existing, but the print_array work for any array, while show_vector is a special method implemented only for 1-D collections, which is meant (as this example show) to be used beyond the array case. I just didn't change the original name for this one, and I can't think of a clearly better one.

I mean that the discrepancy between "show" and "print" is weird, unless there's a reason for it.

You mean in those two lines? there I just kept it how it was, and showing a String will print the quotes. But you refered to print_array, which is just an internal name basically implementing the logic of display, so it's not so important (print_array comes from the pre-existing print_matrix function). Sorry I'm still not clear exactly what you think should be changed!

OK, I thought you had renamed one of them. Carry on...

Before, the :compact property was conflating 2 concepts: 1) are we low on screen space? 2) can we skip printing individual type information for elements in a collection? Now, :compact controls only 1), while :typeinfo controls 2). Cf. #22981 for context and discussion. Credit to Stefan Karpinski for the formulation of the design implemented here.

rfourquet · 2017-12-08T10:08:24Z

Wow all green, rare anough to be worth mentioning :D
Thanks for your reviews!

nalimilan · 2017-12-29T11:43:32Z

Looks like the docstring for showcompact should be updated? The part about "repeating type information" isn't correct anymore, right?

  Show a compact representation of a value to io. If io is not specified, the
  default is to print to STDOUT.

  This is used for printing array elements without repeating type information
  (which would be redundant with that printed once for the whole array), and
  without line breaks inside the representation of an element.

rfourquet added display and printing Aesthetics and correctness of printed representations of objects. needs tests Unit tests are required for this change labels Nov 19, 2017

JeffBezanson mentioned this pull request Nov 19, 2017

Printing vectors of NamedTuples as a markdown table #24627

Closed

nalimilan reviewed Nov 20, 2017

View reviewed changes

rfourquet force-pushed the rf/typeinfo branch from c7c03dc to ae52c98 Compare November 20, 2017 19:04

rfourquet force-pushed the rf/typeinfo branch from a7df592 to 3aa10c9 Compare November 21, 2017 14:13

rfourquet removed the needs tests Unit tests are required for this change label Nov 21, 2017

rfourquet added the collections Data structures holding multiple items, e.g. sets label Nov 21, 2017

nalimilan reviewed Nov 23, 2017

View reviewed changes

nalimilan changed the title ~~WIP: IOContex: introduce the :typeinfo property~~ WIP: IOContext: introduce the :typeinfo property Nov 23, 2017

rfourquet mentioned this pull request Nov 24, 2017

Replace Nullable{T} with Union{Some{T}, Void} #23642

Merged

nalimilan mentioned this pull request Nov 25, 2017

Change find() to return the same index type as pairs() #24774

Merged

rfourquet force-pushed the rf/typeinfo branch 2 times, most recently from 3c95afb to 2cb6a3d Compare December 6, 2017 16:36

rfourquet changed the title ~~WIP: IOContext: introduce the :typeinfo property~~ IOContext: introduce the :typeinfo property Dec 6, 2017

StefanKarpinski approved these changes Dec 6, 2017

View reviewed changes

move array printing functions to arrayshow.jl

9b72a2c

rfourquet force-pushed the rf/typeinfo branch from 6c92664 to 76b83f0 Compare December 7, 2017 17:31

nalimilan reviewed Dec 7, 2017

View reviewed changes

rfourquet force-pushed the rf/typeinfo branch from 76b83f0 to 8a0c710 Compare December 7, 2017 18:36

rfourquet force-pushed the rf/typeinfo branch from 8a0c710 to fe6709b Compare December 7, 2017 20:10

rfourquet merged commit 6eec805 into master Dec 8, 2017

rfourquet deleted the rf/typeinfo branch December 8, 2017 10:09

timholy mentioned this pull request Dec 12, 2017

IOContext :typeinfo setting breaks display of SubArrays #25038

Closed

nalimilan mentioned this pull request Jan 26, 2018

showcompact() docstring outdated? #25765

Closed

stevengj mentioned this pull request Jan 27, 2018

eliminate unnecessary and misnamed Base._display function #25775

Merged

nalimilan mentioned this pull request Apr 18, 2018

Redundant type information printed for arrays with Union eltype #26847

Closed

rfourquet mentioned this pull request Jun 28, 2018

fix printing of arrays when :compact => false is specified #27850

Merged

simonbyrne mentioned this pull request Oct 13, 2019

Floating point printing bug whack-a-mole #33463

Closed


		### non-Vector arrays

		# _show & _show_empty: main helper function for show(io, X)


		### non-Vector arrays

		# _show_nonemtpy & _show_empty: main helper functions for show(io, X)

IOContext: introduce the :typeinfo property #24651

IOContext: introduce the :typeinfo property #24651

Conversation

rfourquet commented Nov 19, 2017 • edited Loading

JeffBezanson commented Nov 19, 2017

rfourquet commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Nov 20, 2017

rfourquet commented Nov 20, 2017

rfourquet commented Nov 20, 2017

rfourquet commented Nov 21, 2017

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfourquet commented Nov 23, 2017

nalimilan commented Dec 6, 2017

rfourquet commented Dec 6, 2017

nalimilan commented Dec 6, 2017

StefanKarpinski commented Dec 6, 2017

rfourquet commented Dec 6, 2017

StefanKarpinski left a comment

Choose a reason for hiding this comment

rfourquet commented Dec 7, 2017

StefanKarpinski commented Dec 7, 2017

StefanKarpinski commented Dec 7, 2017

rfourquet commented Dec 7, 2017

StefanKarpinski commented Dec 7, 2017

rfourquet commented Dec 7, 2017

Choose a reason for hiding this comment

rfourquet Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfourquet commented Dec 8, 2017

nalimilan commented Dec 29, 2017

rfourquet commented Nov 19, 2017 •

edited

Loading

stevengj Jan 27, 2018 •

edited

Loading

rfourquet Dec 7, 2017 •

edited

Loading