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

Propose increasing number of columns to 800 #1040

Closed
wants to merge 3 commits into from

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Jun 13, 2022

80 columns in HTML is a very small number given current standards. Especially that browser creates horizontal scroll bar when displaying an object. This should not have a big impact on performance either.

I think leaving 30 lines is OK.

@bkamins
Copy link
Member Author

bkamins commented Jun 13, 2022

CC @nalimilan @ronisbr

@bkamins
Copy link
Member Author

bkamins commented Jun 15, 2022

@stevengj - do you have an opinion on this proposal? Thank you!

@bkamins
Copy link
Member Author

bkamins commented Jun 19, 2022

It would be good to have a decision on this PR before JuliaCon so that Jupyter based materials presented there could take advantage of this change (if it were approved). Thank you!

@fredrikekre
Copy link
Member

I'm happy to merge it, but do you have a MWE just so I can try it out?

@fredrikekre
Copy link
Member

For something like a rand(1000, 1000) this doesn't look very nice (text/plain output) and doesn't give a scrollbar. For a DataFrame it looks ok though (text/html I suppose?)

@bkamins
Copy link
Member Author

bkamins commented Jun 19, 2022

A good point - we checked the same thing in parallel.

The MWE is rand(100, 100) and it shows we have a problem.

The issue is that if I e.g. do:

using DataFrames
DataFrame(rand(100, 100), :auto)

all is good and horizontal scroll bar is automatically added.

However, if I do:

rand(100, 100)

I get (after setting "COLUMNS" to 800):
image

This is bad and the problem is that for some reason in this case Jupyter Notebook does not insert a horizontal scroll bar. Most likely (I am not 100% about the reason) is that when we do rand(100, 100) the display is always plain text.

This means that before merging this PR we should first understand in what circumstances Jupyter Notebook inserts a horizontal scroll bar.

This would probably also affect display of things like:
image
which currently use whole window width and "wrap around".

The issue is discussed on SO.


In summary: things are more complex than they seemed :( and this PR should not be merged as is.

@nalimilan @ronisbr : what do you think we should do for DataFrames.jl given this context?

In essence the problem is:

user cannot have both wide Matrix and DataFrame nicely displayed. If we set "COLUMNS" to a large value DataFrame will be displayed nicely, but Matrix will be a mess. If we keep "COLUMNS" as it is now (80) then Matrix is OK, but DataFrame is not good

So it seems that the only solution is to have our own configuration parameter for HTML display width/height?

@bkamins bkamins marked this pull request as draft June 19, 2022 10:12
@fredrikekre
Copy link
Member

fredrikekre commented Jun 19, 2022

You could perhaps insert this change more surgically for just for text/html

@bkamins
Copy link
Member Author

bkamins commented Jun 19, 2022

My feeling was that if we anyway need two separate values then the least problematic approach is to have a custom DataFrames.jl specific configuration option. In DataFrames.jl we could just use get(ENV, "TABLE_COLUMNS", 800) instead of using ENV["COLUMNS"]. Or maybe this should be PrettyTables.jl setting? @ronisbr - what do you think?

@nalimilan
Copy link
Member

nalimilan commented Jun 19, 2022

AFAICT the option should be HTML-specific rather than DataFrames/PrettyTables-specific? Or maybe let's just hardcode the value of 800 in DataFrames? I mean, scrolling until the 800th column is totally impractical anyway. Does IJulia set the :limit=>true IOContext property when printing HTML?

It's hard to find a good solution because none of these choices is ideal IMO. What should ideally happen is that only the columns that fit on screen should be visible, but users would be able to see other columns by clicking on scroll buttons that would load the next block of columns (or by scrolling indefinitely). Other approaches are just stopgaps.

@bkamins
Copy link
Member Author

bkamins commented Jun 19, 2022

Other approaches are just stopgaps.

I agree, but this would be a big project to do it right (if practically possible at all).
What you propose is roughly the target functionality of VSCode extension for viewing tables AFAICT.

Or maybe let's just hardcode the value of 800 in DataFrames?

So this is my proposal essentially. But I want to allow this 800 to be changed by setting a proper ENV entry.
I would name property (in order from more specific to more general):

  • "DATAFRAME_COLUMNS" if we want it to be DataFrames.jl specific (then only DataFrames.jl needs to be aware of it)
  • "TABLE_COLUMNS" if we want it to be PrettyTables.jl specific (then PrettyTables.jl has to be aware of it and in consequence also DataFrames.jl)
  • "HTML_COLUMNS" if we want it to be any HTML output specific (then it should be mentioned in IJulia.jl developer documentation)

@nalimilan
Copy link
Member

OK. Then could IJulia set the displaysize IOContext property for HTML output?

@ronisbr
Copy link
Member

ronisbr commented Jun 21, 2022

Hi! Sorry for the delay.

In PrettyTables, the maximum number of rows or columns that will be printed is just two keyword parameters. Hence, DataFrames can select an environment variable and we use this value when calling pretty_table.

@bkamins
Copy link
Member Author

bkamins commented Jun 21, 2022

Hence, DataFrames can select an environment variable and we use this value when calling pretty_table.

OK - let us discuss it then when you open a PR to DataFrames.jl. Thank you!

@bkamins
Copy link
Member Author

bkamins commented Sep 3, 2022

Closing as this solution is problematic per the comments above. In DataFrames.jl we will use separate values to control display size.

@bkamins bkamins closed this Sep 3, 2022
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.

4 participants