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

RFC: don't show module prefix in :compact printing mode #27925

Merged
merged 1 commit into from
Jul 26, 2018

Conversation

JeffBezanson
Copy link
Member

@JeffBezanson JeffBezanson commented Jul 4, 2018

Before:

julia> fill(Foo.X(), 2, 2)
2×2 Array{Main.Foo.X,2}:
 Main.Foo.X()  Main.Foo.X()
 Main.Foo.X()  Main.Foo.X()

after:

julia> fill(Foo.X(), 2, 2)
2×2 Array{Main.Foo.X,2}:
 X()  X()
 X()  X()

I suspect some packages have added show methods just because of this, since it gets quite annoying for big arrays. This doesn't affect 1-d arrays, since they no longer use :compact, but I'd like to do something about that if possible --- either use compact printing for all array elements, or change it based on the type of each element.

@JeffBezanson JeffBezanson added the display and printing Aesthetics and correctness of printed representations of objects. label Jul 4, 2018
@nalimilan
Copy link
Member

You're aware of #23806, right? I'd tend to think that skipping the module prefix when the type is reachable from the current module is enough. The rule should probably be improved to skip the Main. part for modules which have been defined in Main (the current rule either prints all parent modules, or just the type name). Incidentally, that would also improve printing of 1D arrays.

@timholy
Copy link
Member

timholy commented Jul 4, 2018

Yep, this is a big deal:

In the Images world we try to print summaries like this:

julia> module Mine

       using FixedPointNumbers, Colors

       function makearray()
           rgb8 = rand(RGB{N0f8}, 3, 5)
           view(rgb8, 2:3, :)
       end

       end

julia> Mine.makearray()
2×5 view(::Array{RGB{N0f8},2}, 2:3, :) with eltype ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}}:
 RGB{N0f8}(0.553,0.51,0.918)   RGB{N0f8}(0.69,0.859,0.031)   RGB{N0f8}(0.306,0.82,0.518)   RGB{N0f8}(0.655,0.941,0.859)  RGB{N0f8}(0.976,0.016,0.184)
 RGB{N0f8}(0.267,0.271,0.188)  RGB{N0f8}(0.392,0.302,0.878)  RGB{N0f8}(0.141,0.533,0.482)  RGB{N0f8}(0.878,0.322,0.706)  RGB{N0f8}(0.89,0.984,0.322) 

The main point is that we scope the types with the module name (when the module is not reachable from Main) in one place, after the "with eltype"; everywhere else we use the abbreviated printing. Compare

julia> typeof(ans)
SubArray{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2,Array{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2},Tuple{UnitRange{Int64},Base.Slice{Base.OneTo{Int64}}},false}

which is much harder to read. And this is nowhere near the full complexity of what one can easily generate in real code; I've frequently seen the type (i.e., without any values) of an AbstractArray take up an entire screen.

The use of view in printing the summary is a consequence of showarg, examples here.

@nalimilan
Copy link
Member

@timholy I'm not sure what you're advocating for. Are you saying with eltype ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}}: should be with eltype RGB{Normed{UInt8,8}}:? I'm surprised this isn't the case already given that ColorTypes exports RGB. Is this on 0.7?

@JeffBezanson
Copy link
Member Author

skipping the module prefix when the type is reachable from the current module is enough

I disagree; often packages will return objects with types you're not explicitly using, and compact printing should still be compact. The fully qualified element type can still be shown in the header, so you're not really missing information.

@JeffBezanson JeffBezanson force-pushed the jb/compactpfx branch 2 times, most recently from 9c3fb6f to 4cee1e2 Compare July 4, 2018 15:50
@timholy
Copy link
Member

timholy commented Jul 4, 2018

@nalimilan, this is orthogonal to #23806. In my example I deliberately made it so that FixedPointNumbers and Colors were not reachable from Main.

The main point was that you probably don't want module scoping when printing values, but you might when printing types in summaries. Jeff's edited version is much more along the lines of what I think the behavior should be (in his original summary line I was a bit worried about the module scoping not being present). So now I approve of Jeff's proposal.

@nalimilan
Copy link
Member

Fine with me. Should we also check typeinfo, so that for e.g. Array{Any} the fully qualified name is used instead?

@rfourquet
Copy link
Member

What about checking against the :typeinfo property instead of :compact ? this would seem like a better match, as :typeinfo was created precisely to avoid redundancy in type information, and this wouldn't have the problem of not applying to 1-D arrays. But I'm not clear how easy it is to do that...

@JeffBezanson
Copy link
Member Author

This isn't type information, it's module qualification. To me :compact means the value should be printed more compactly if possible, even e.g. if that means losing precision. Dropping module qualifiers seems like an obvious way to be more compact to me.

@rfourquet
Copy link
Member

My view was maybe a bit stretched, but the idea was that if a type must be printed (like Main.Foo.X), it can be decided to not print the redundant part (Main.Floo) if the context already contains :typeinfo => Main.Foo.X, as it can then be assumed that this information has already been printed. The :compact is also not completely matching what is wanted here as for 1-D arrays we often don't want to print elements compactly.

@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Jul 26, 2018
@StefanKarpinski StefanKarpinski merged commit 2cb04e0 into master Jul 26, 2018
@StefanKarpinski StefanKarpinski deleted the jb/compactpfx branch July 26, 2018 18:29
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants