paris-saclay-cds / ramp-workflow

Toolkit for building predictive workflows on top of pydata (pandas, scikit-learn, pytorch, keras, etc.).
https://paris-saclay-cds.github.io/ramp-docs/
BSD 3-Clause "New" or "Revised" License
68 stars 43 forks source link

Setting display.width to None #242

Closed gabriel-hurtado closed 3 years ago

gabriel-hurtado commented 4 years ago

Is there a reason why we print score with a fixed width of 160 here ?

Setting display.width to 0 would allow pandas to autodetects the size of terminal window, avoiding useless shortening in the printed scores

agramfort commented 4 years ago

I suspect it's also used in the web frontend. Can you check?

codecov[bot] commented 4 years ago

Codecov Report

Merging #242 into master will increase coverage by 0.04%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #242      +/-   ##
==========================================
+ Coverage   81.70%   81.75%   +0.04%     
==========================================
  Files         133      133              
  Lines        4909     4921      +12     
==========================================
+ Hits         4011     4023      +12     
  Misses        898      898              
Impacted Files Coverage Δ
rampwf/utils/pretty_print.py 100.00% <100.00%> (ø)
rampwf/utils/importing.py 100.00% <0.00%> (ø)
rampwf/tests/test_kits.py 95.55% <0.00%> (+1.61%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4d5c902...39e7365. Read the comment docs.

gabriel-hurtado commented 4 years ago

They seem to use a different technique in the fronted https://github.com/paris-saclay-cds/ramp-board/blob/4d908711ea25a977baa690292af31655887b42ff/ramp-database/ramp_database/tools/leaderboard.py#L20

agramfort commented 4 years ago

then I don't know why it's like this. git blame will tell you who wrote this

gabriel-hurtado commented 4 years ago

It seems that it was introduced with this pull request, and more specifically this comment discusses it

agramfort commented 4 years ago

ask @rth he may know then

glemaitre commented 4 years ago

I just assume that 160 characters were considered enough :) I was checking the documentation. Do you want 0 or set it to None: https://pandas.pydata.org/pandas-docs/stable/user_guide/options.html

gabriel-hurtado commented 4 years ago

Indeed, display_width should be None rather than 0 (the undocumented option of setting it to 0 works because in the end pandas does return display_width or terminal_width )

rth commented 4 years ago

As far as I remember we wanted to avoid line breaks if the score table was a bit larger than the terminal screen size. In which case scrolling horizontally might still be more readable than line breaks.. So for instance if I have a 80char wide terminal window, with None, it would break lines I think for anything larger than it?

gabriel-hurtado commented 4 years ago

Indeed, with None, uses line breaks if printing more than what can fit. However with display.width=160 and a terminal smaller, let's say 80, the current behavior is not to create scrolling, it is to truncate the output dataframe with ... Not sure what is the best way to handle it.

rth commented 4 years ago

Aww I wasn't aware of that. Might depend on the terminal. I'm not opposed to having None there as well..

kegl commented 3 years ago

Any objection to merging this PR?

rth commented 3 years ago

Let's merge it then.