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

Add showarg(io, ::Slices) printing for eachslice #45355

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mcabbott
Copy link
Contributor

@mcabbott mcabbott commented May 18, 2022

This proposes to print the Slices objects from #32310 in a similar way to SubArray, ReshapedArray, etc. These compose nicely:

julia> view(rand(10)', 2:3)
2-element view(reshape(adjoint(::Vector{Float64}), 10), 2:3) with eltype Float64:
 0.09114060604932506
 0.6108851444289344

julia> eachcol(ans)
1-element eachcol(reshape(view(reshape(adjoint(::Vector{Float64}), 10), 2:3), 2, 1)) of 2-element slices with eltype Float64:
 [0.09114060604932506, 0.6108851444289344]

julia> eltype(ans)
SubArray{Float64, 1, Base.ReshapedArray{Float64, 2, SubArray{Float64, 1, Base.ReshapedArray{Float64, 1, Adjoint{Float64, Vector{Float64}}, Tuple{}}, Tuple{UnitRange{Int64}}, true}, Tuple{}}, Tuple{Base.Slice{Base.OneTo{Int64}}, Int64}, true}

Most showarg methods have the outermost one print the eltype of the object. Here this is a long and not especially illuminating SubArray type, so instead it prints the size of the elements, and eltype(eltype(s)). Examples (edit: focusing on the empty case, as initially this needed special treatment):

julia> eachrow(Int[])
0-element eachrow(::Matrix{Int64}) of 1-element slices with eltype Int64

julia> eachcol(Int[])
1-element eachcol(::Matrix{Int64}) of 0-element slices with eltype Int64:
 0-element view(::Matrix{Int64}, :, 1) with eltype Int64

Finally, it also hides this SubArray type in compact show. Not completely sure that's a good idea: (removed for now)

julia> eachcol([1 2 3; 44 55 66]) |> repr
"[[1, 44], [2, 55], [3, 66]]"

julia> (0, eachcol(Float32[1 2; 3 4]), 5)  # array within a tuple uses 2-arg show
(0, [Float32[1.0, 3.0], Float32[2.0, 4.0]], 5)

@oscardssmith oscardssmith added the display and printing Aesthetics and correctness of printed representations of objects. label May 18, 2022
@nalimilan
Copy link
Member

Most showarg methods have the outermost one print the eltype of the object. Here this is a long and not especially illuminating SubArray type, so instead it prints the size of the elements, and eltype(eltype(s)):

julia> eachrow(Int[])
0-element eachrow(::Matrix{Int64}) of 1-element slices with eltype Int64

julia> eachcol(Int[])
1-element eachcol(::Matrix{Int64}) of 0-element slices with eltype Int64:
 0-element view(::Matrix{Int64}, :, 1) with eltype Int64

Is there a copy/paste bug here? I don't understand where the "0-element view(::Matrix{Int64}, :, 1) with eltype Int64" part comes from, and it doesn't appear in the examples in the diff.

base/slicearray.jl Show resolved Hide resolved
base/slicearray.jl Show resolved Hide resolved
base/slicearray.jl Show resolved Hide resolved
return nothing
end

# This hides the SubArray type in compact printing, e.g. (0, eachcol(Float32[1 2; 3 4]), 5)
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this comment. Maybe develop a bit more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear what this is doing, this is the effect:

julia> (0, eachcol(Float32[1 2; 3 4]), 5)  # master
(0, SubArray{Float32, 1, Matrix{Float32}, Tuple{Base.Slice{Base.OneTo{Int64}}, Int64}, true}[[1.0, 3.0], [2.0, 4.0]], 5)

julia> (0, eachcol(Float32[1 2; 3 4]), 5)  # this PR
(0, [Float32[1.0, 3.0], Float32[2.0, 4.0]], 5)

The default 2-arg show for an array of arrays prints the whole type of the inner arrays as the eltype of the outer. This line removes that. But also notifies show that the eltype of the inner arrays has not been printed anywhere, hence they become Float32[...].

Will expand the comment a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expanded in f37584d

Copy link
Member

Choose a reason for hiding this comment

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

OK. The new printing is certainly clearer, but I wonder it's consistent with what we do elsewhere. Are there any equivalent constructs that could benefit from the same simplification? My concern is that if it's the only case where we omit the actual eltype then people could be misled into thinking they really have a Vector{Vector{Float32}}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm not sure. Compact printing is sometimes lossy, e.g. Transpose is not marked here (but matters):

julia> ([1 2], transpose([3,4]), [[5,6], [6,7]]')
([1 2], [3 4], Adjoint{Int64, Vector{Int64}}[[5 6] [6 7]])

julia> Adjoint{Int64, Vector{Int64}}[[5 6] [6 7]]
ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type Adjoint{Int64, Vector{Int64}}

Also, the Adjoint printing is not copy-pastable, it does not re-create such elements. That's also true of the SubArray types which will be shown for eachcol. Perhaps both should be abbreviated somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f3869c9 reverts this for now.

@mcabbott
Copy link
Contributor Author

Is there a copy/paste bug here?

I don't think so. eachcol reshapes a vector, thus size (0,) becomes size (0,1) which has 1 column.

But it's a confusing example now, sorry. Initially I treated empty cases differently, to use size(first(s)) when non-empty. But not anymore.

@mcabbott
Copy link
Contributor Author

mcabbott commented Jun 1, 2022

Last commit removes the changes to 2-arg show, keeping only showarg which is called by 3-arg show.

@mcabbott mcabbott requested a review from nalimilan October 5, 2022 13:05
@mcabbott mcabbott force-pushed the show_eachcol branch 2 times, most recently from 63ac0c1 to 353d759 Compare November 1, 2022 18:26
base/slicearray.jl Outdated Show resolved Hide resolved
Comment on lines -149 to +167
julia> a = [1 2; 3 4]
2 Matrix{Int64}:
1 2
3 4
julia> a = [1 2 3; 40 50 60]
3 Matrix{Int64}:
1 2 3
40 50 60
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing these examples from 2x2 to 2x3 will, I hope, make it more obvious which dimension is which.

Comment on lines +171 to +181
julia> s isa RowSlices, s[1] isa SubArray
(true, true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added because the type alias RowSlices is no longer printed.

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