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

Modified show, added omitted columns message #489

Merged
merged 6 commits into from
Apr 1, 2021

Conversation

r2cp
Copy link
Contributor

@r2cp r2cp commented Mar 17, 2021

Hello, this is a proposal to fix #468. It prints page 1 of TimeArray and adds the omitted columns message. I would like some feedback on it.

I have not rewritten the tests yet, but I'd gladfully do it.

@iblislin
Copy link
Collaborator

I just found that DataFrame puts the message in the right hand side and change the color. Maybe we can do that too?

src/timearray.jl Outdated
spacetime, colwidth, nrow, drow, res_row, tophalf, bothalf,
length(pages))

if length(pages) > 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to support @show for printing all pages out.

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, that would be great and it would take advantage of the paging machinery. I'm not sure how to do this, though; I tried looking at the DataFrames package and to @show df with a large DataFrame and it dumps all the data on screen (not readable) and then the same trimmed view of the df. I am not sure of what methods to override. Or maybe a separate method for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I guess DataFrames use the :limit option of IOContext to control the omitting.

You can try this:

show(IOContext(stdout, :limit => false), df)

vs

show(df)

I think we can also utilize that option.

src/timearray.jl Outdated
Comment on lines 363 to 366
Base.show(io::IO, ta::TimeArray) =
print_time_array(io, ta, get(io, :limit, false), get(io, :limit, true))
Base.show(io::IO, ::MIME"text/plain", ta::TimeArray) =
print_time_array(io, ta, false, !get(io, :limit, false))
Copy link
Contributor Author

@r2cp r2cp Mar 23, 2021

Choose a reason for hiding this comment

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

I checked the :limit in the show methods definition and added a new parameter in print_time_array to control the printing of all columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is working as follows:

  • Showing a TimeArray in the REPL by just calling it by name (say ta) prints omitted columns message if the TimeArray has lots of columns.
  • Calling @show ta prints all columns and then the compact view. Similar to DataFrames but the first output a little more ordered.

I had to modify some tests to pass the :limit option of IOContext

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm investigating why show(ta) and @show ta look different. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, I figured it out, our implementation is correct.
@show invoking sprint with IOBuffer for rendering, so the return of displaysize(io) is different from displaysize(stdout).

src/timearray.jl Outdated Show resolved Hide resolved
src/timearray.jl Outdated Show resolved Hide resolved
test/timearray.jl Outdated Show resolved Hide resolved
test/timearray.jl Outdated Show resolved Hide resolved
rafaelchp and others added 2 commits March 29, 2021 19:43
Co-authored-by: Iblis Lin <iblis.dif01@nctu.edu.tw>
- Removed last newline
- Removed individual show method
@@ -480,7 +480,7 @@ end

@testset "show methods don't throw errors" begin
io = IOBuffer()
let str = sprint(show, cl)
let str = sprint(summary, cl)
out = "500×1 TimeArray{Float64,1,Date,Array{Float64,1}} 2000-01-03 to 2001-12-31"
Copy link
Contributor Author

@r2cp r2cp Mar 30, 2021

Choose a reason for hiding this comment

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

It seems that tests are failing in 1.6 because Array{Float64,1} is now shown as Vector{Float64} and Array{Float64,2} as Matrix{Float64}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe in test/timearray.jl we can add a string to interpolate in the output depending on the value of VERSION. Something like:

dispvec = VERSION < v"1.6" ? "Array{Float64,1}" : "Vector{Float64}"

Or maybe even typed:

dispvec(T) = VERSION < v"1.6" ? "Array{$(repr(T)),1}" : "Vector{$(repr(T))}"

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, fine. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out that this TimeArray{Float64,1,Date,Array{Float64,1}} becomes TimeArray{Float64, 1, Date, Vector{Float64}} in 1.6.

@iblislin iblislin merged commit 6ad69bc into JuliaStats:master Apr 1, 2021
@iblislin
Copy link
Collaborator

iblislin commented Apr 1, 2021

Thanks for your contributions. 👍

@r2cp
Copy link
Contributor Author

r2cp commented Apr 1, 2021

Glad to help 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add omitting of columns when printing
3 participants