-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
REF: Use Styler
implementation for DataFrame.to_latex
#47970
REF: Use Styler
implementation for DataFrame.to_latex
#47970
Conversation
@ivanovmg @rhshadrach you both had input to the underlying issue so I think your opinion on the output here is very welcome. |
@@ -351,6 +350,8 @@ def test_read_fspath_all(self, reader, module, path, datapath): | |||
], | |||
) | |||
def test_write_fspath_all(self, writer_name, writer_kwargs, module): | |||
if writer_name in ["to_latex"]: # uses Styler implementation | |||
pytest.importorskip("jinja2") |
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.
So this new implementation will require jinja2
to be installed first?
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.
correct. @jreback was fine for this to be the case.
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.
No objection in having that as a requirement, but we regard this as a breaking change, yes?
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.
it is a breaking chang, yes. Current implemtations of dataframe.to_latex will fail after this PR if the user does not have jinja2.
fallback option: since the pr does not remove the LatexFormatter, code could be redirected on undetected jinja2. this would then be non-breaking.
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.
If the fallback behavior was implemented then I think this PR would be okay for 1.5. If not, this could be one of the "breaking without deprecation" behaviors for 2.0 possibly if this change is "too large" #44823
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.
When thinking about doing this I realised the tests that I have altered will be a nightmare for dual implementation, so if this needs to go to 2.0, I don't oppose that.
But then for 2.0 I will probably look to change the arg signature of all of this anyway.
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.
What would the args be changed to? The current arguments should be deprecated.
@@ -3377,14 +3376,16 @@ def test_filepath_or_buffer_arg( | |||
filepath_or_buffer_id, | |||
): | |||
df = DataFrame([data]) | |||
if method in ["to_latex"]: # uses styler implementation |
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 it make sense to simplify it to method == 'to_latex'
?
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.
if other methods move over to Styler, (the intention being to_html
) we will need a list here anyway
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.
Some of my comments so far.
82340d1
to
c9c61c3
Compare
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.
Need to take a more detailed look, but definitely +1 on the direction this is going. I have concerns on merging this prior to 2.0 though.
@@ -351,6 +350,8 @@ def test_read_fspath_all(self, reader, module, path, datapath): | |||
], | |||
) | |||
def test_write_fspath_all(self, writer_name, writer_kwargs, module): | |||
if writer_name in ["to_latex"]: # uses Styler implementation | |||
pytest.importorskip("jinja2") |
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.
No objection in having that as a requirement, but we regard this as a breaking change, yes?
Can you share those. Im abivalent as towards 1.5.0 and 2.0, but here are some good reasons for 1.5.0.
Arguments for including in 2.0
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
Apologies @attack68 on not getting back to you here.
The concerns are the breaking changes this introduces, highlighted in my comments above. I think it would be okay to have the added requirement of jinja2 in 2.0 (#47970 (comment)). #47970 (comment) is still outstanding. |
…lement # Conflicts: # pandas/tests/frame/test_repr_info.py
doc/source/whatsnew/v2.0.0.rst
Outdated
.. _whatsnew_200.api_breaking.to_latex: | ||
|
||
DataFrame to LaTeX has a new render engine | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
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.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
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.
Sphinx allows underline to be exact or longer than the text so this isn't strictly necessary, but committed anyway.
doc/source/whatsnew/v2.0.0.rst
Outdated
|
||
The pandas options below are no longer used and will be removed in future releases. | ||
The alternative options giving similar functionality are indicated below: | ||
- ``display.latex.escape``: replaced with ``styler.format.escape``, |
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 this indentation is causing this docbuild error:
2023-01-05T18:42:10.0685513Z /home/runner/work/pandas/pandas/doc/source/whatsnew/v2.0.0.rst:435: WARNING: Block quote ends without a blank line; unexpected unindent.
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.
lets see if the pushed change fixes that error
Also as a reminder. Could you remove any potential |
Just noting that during yesterday's dev call that we decided that this PR isn't necessarily a blocker for releasing 2.0 i.e. this FutureWarning will persist until 3.0 if not ready. That being said this PR seems close |
Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>
Yes, if I can just get this to green I dont think there is more to do. Post PR clean up, such as removal of redundant code and checking the filters can be done after 2.0. WHilst not a blocker I still it would be good to get in if poss. |
@ivanovmg @rhshadrach @mroeschke this is greenish now. I believe the http doc build is unrelated. |
Thanks for the great work here @attack68! So to clarify the follow ups?
|
Yes and remove the redundant LatexFormatter code |
…#47970) * Base implementation * Base implementation * test fix up * test fix up * test fix up * doc change * doc change * doc change * mypy fixes * ivanov doc comment * ivanov doc comment * rhshadrach reduction * change text from 1.5.0 to 2.0.0 * remove argument col_space and add whatsnew * mroeschke requests * mroeschke requests * pylint fix * Whats new text improvements and description added * Update doc/source/whatsnew/v2.0.0.rst Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com> * Update doc/source/whatsnew/v2.0.0.rst * remove trailing whitespace * remove trailing whitespace * Whats new linting fixes * mroeschke requests Co-authored-by: JHM Darbyshire (iMac) <attack68@users.noreply.github.com> Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>
After a year of patching up things in #41649, and @jreback merge of #47864 I can finally propose this for review.
Objective
DataFrame.to_latex
with its existing arguments, adding no argumentsStyler.to_latex
LatexFormatter
(code removal not part of this PR) and dual pandas code systems.Styler
implementation for forward developmentOutcome
DataFrame.to_latex
were replicable, with the exception ofcol_space
which has no impact upon latex render and,I personally don't like anyway.col_space
is deprecated with warning and test.col_space
is removed.Styler
is marginally better, although for the table sizes that one would like to render in latex is neglible, anyway.No whats_new: awaiting feedback.New docs
The key new section of the docs..