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/WIP] Handle :compact context in 3-arg show for arrays #34733

Closed
wants to merge 3 commits into from

Conversation

tkf
Copy link
Member

@tkf tkf commented Feb 12, 2020

Edit: now this PR fixes only permutedims([Int[], Int[]]) case #34733 (comment)


Before:

julia> [Int[]]
1-element Array{Array{Int64,1},1}:
 0-element Array{Int64,1}

After:

julia> [Int[]]
1-element Array{Array{Int64,1},1}:
 []

I'm sending this as a WIP I was meant to send this as a WIP PR before fixing the tests as I don't know if the core devs think this is the way to go.

fix (a related bug of) #34659

@tkf tkf changed the title Handle :compact context in 3-arg show for arrays [RFC/WIP] Handle :compact context in 3-arg show for arrays Feb 12, 2020
# 0) show summary before setting :compact
summary(io, X)
isempty(X) && return
print(io, ":")
show_circular(io, X) && return

# 1) compute new IOContext
if !haskey(io, :compact) && length(axes(X, 2)) > 1
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if people are happy with removing length(axes(X, 2)) > 1.

(BTW I think this is a shortcoming of the way :compact is used. It's not possible to declare compactness of the width and height separately. This is why I've been suggesting to use :displaysize instead of :compact #33260 (comment))

Copy link
Member

Choose a reason for hiding this comment

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

This is a separate issue and will probably be controversial, so I wouldn't change this for now.

@fredrikekre fredrikekre added the display and printing Aesthetics and correctness of printed representations of objects. label Feb 12, 2020
@@ -320,14 +320,21 @@ print_array(io::IO, X::AbstractArray) = show_nd(io, X, print_matrix, true)
# typeinfo aware
# implements: show(io::IO, ::MIME"text/plain", X::AbstractArray)
function show(io::IO, ::MIME"text/plain", X::AbstractArray)
# -1) handle arrays-in-array (recursive call)
if get(io, :compact, false) === true
# Using `show` of method with the most abstract type to avoid infinite recursion.
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary? I don't think it's common for 2-arg show methods to call 3-arg ones.

@tkf
Copy link
Member Author

tkf commented Feb 19, 2020

@JeffBezanson Note that both changes are required if you want to fix the bug #34659 this way. The problem is that 3-arg show is used for the inner array when calling 3-arg show for the outer array show(io, "text/plain", [Int[]]) due to

julia/base/arrayshow.jl

Lines 105 to 111 in b0d1c1a

# First try 3-arg show
sx = sprint(show, "text/plain", x, context=io, sizehint=0)
# If the output contains line breaks, try 2-arg show instead.
if occursin('\n', sx)
sx = sprint(show, x, context=io, sizehint=0)
end

That is to say, since 3-arg show of the inner array show(io, "text/plain", Int[]) does not print multiple lines, we don't use the if occursin('\n', sx) branch. This is why we need if get(io, :compact, false) === true #34733 (comment). However, for the inner array to be able to check that it is invoked via 3-arg show, we need to set :compat always even with vectors #34733 (comment).

Of course, there may be another solution for fixing #34659. Please close this PR if there is a better way.

@JeffBezanson
Copy link
Member

You're right; it would only fix this case:

julia> permutedims([Int[], Int[]])
1×2 Array{Array{Int64,1},2}:
 0-element Array{Int64,1}  0-element Array{Int64,1}

but I think that's worth doing.
I don't see any way to fix the 1-column case except adding a showelement function and overloading it for arrays.

@tkf
Copy link
Member Author

tkf commented Feb 19, 2020

OK, I put length(axes(X, 2)) > 1 back. Let's see how many tests fail...

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.

3 participants