-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
BUG-17280 to_html follows display.precision for column numbers in notebooks #25914
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25914 +/- ##
==========================================
- Coverage 91.77% 91.77% -0.01%
==========================================
Files 175 175
Lines 52607 52610 +3
==========================================
- Hits 48282 48281 -1
- Misses 4325 4329 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25914 +/- ##
==========================================
- Coverage 91.84% 91.83% -0.01%
==========================================
Files 175 175
Lines 52550 52554 +4
==========================================
- Hits 48266 48265 -1
- Misses 4284 4289 +5
Continue to review full report at Codecov.
|
As this is a display option, shouldn't it only affect the notebook display and leave the |
I have tested this fix in a notebook and it works as expected. As for whether this is the appropriate location for a fix, |
I would argue it is not a bug here. since the display options should not affect the to_html() output. compare with the display options, |
Hello @JustinZhengBC! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-04-03 15:23:23 UTC |
I agree. I have altered the fix so it only applies to |
|
||
|
||
def test_to_html_round_column_headers(): | ||
df = DataFrame([1], columns=[0.55555]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the issue number
pandas/io/formats/html.py
Outdated
@@ -491,6 +491,15 @@ class NotebookFormatter(HTMLFormatter): | |||
DataFrame._repr_html_() and DataFrame.to_html(notebook=True) | |||
""" | |||
|
|||
def __init__(self, formatter, classes=None, border=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this really need to be in __init__
, rather have this call a method in the base class which is overriden here, e.g.
self.columns = self._get_columns_formatted_values()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather have this call a method in the base class which is overriden here
in #24651, i separated out the notebook functionality from HTMLFormatter
into NotebookFormatter
using HTMLFormatter
as the base class. the base class of HTMLFormatter
is TableFormatter
which is also the base class of DataFrameFormatter
(used for to_string
and to _latex
) and LatexFormatter
.
I envisaged that at some point in the development cycle it may become desirable to also create a ToHTMLFormatter
using HTMLFormatter
as the base.
IMO HTMLFormatter
and NotebookFormatter
are not the appropriate location for non-markup related formatting issues that are common across output-formatting methods. I think this issue is in that category and should ideally be in DataFrameFormatter
or TableFormatter
.
However, if to close the open issue the fix is somewhere in io/formats/html.py
, then i think a TODO to remove the code at a later date should be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and the bug was present in
to_html(notebook=False)
as wellI would argue it is not a bug here. since the display options should not affect the to_html() output. compare with the display options,
max_rows
,max_columns
,show_dimensions
,max_colwidth
, etc. which only apply to the notebook display.
I think I'm getting a bit confused here. Doesn't this mean that this is not an issue that is common across formatters? Because to_html
should only check display preferences when used for display purposes in a notebook and not when generating HTML for a web page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. i can understand why your confused, there is a slight problem with the current class hierarchy. the workaround for max_colwidth
was to use with option_context
to ignore the display option for to_html
, see #24841
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the ideal solution would be to have say a use_display_options
attribute in DataFrameFormatter
and then in HTMLFormatter
we just have self.fmt.use_display_options = False
and in NoteBookFormatter
we have self.fmt.use_display_options = True
and then all the display formatting could be handled in io/formats/format.py
.
But this is way outside the scope of this PR. So a with option_context
work-around would be fine for now IMO.
I think if, for now, you just change pandas/pandas/io/formats/html.py Line 295 in 96a128e
to row.extend(self.columns.format()) it'll fix the open issue by making the behavior for the single level column labels consistent with the index labels and the if the behavior should be different in then in the future we probably need a |
@simonjayhawkins your fix works. It also changes columns with |
i'm getting
|
@JustinZhengBC : lgtm. #25914 (comment) still to do? |
@simonjayhawkins sorry about that, thanks for catching it. |
@simonjayhawkins this is orthogonal to your other PR? #25983 |
yes. this is ok. probably a merge conflict on the test, but just an accept both. |
thanks @JustinZhengBC |
git diff upstream/master -u -- "*.py" | flake8 --diff
When printing column labels, check if they are floats and if they are, then round according to display.precision preferences