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

To latex position #35284

Merged
merged 15 commits into from
Aug 7, 2020
Merged

To latex position #35284

merged 15 commits into from
Aug 7, 2020

Conversation

SylvainLan
Copy link
Contributor

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Minor comment on test but otherwise I think this is OK

@@ -481,6 +482,23 @@ def test_to_latex_caption_label(self):
"""
assert result_l == expected_l

# test when only the label is provided
result_p = df.to_latex(position=the_position)
Copy link
Member

Choose a reason for hiding this comment

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

Can you create this as its own test? Same comment with one below

@WillAyd WillAyd added the IO LaTeX to_latex label Jul 21, 2020
@SylvainLan
Copy link
Contributor Author

Hello @WillAyd , thanks for the review !

I did the changes you asked for, but one test seems to have failed, I'm not sure it's related to what I've written

Would you also want tests when the caption, the label and the position is given ?

@WillAyd
Copy link
Member

WillAyd commented Jul 22, 2020

Just restarted the failure let's see

@WillAyd
Copy link
Member

WillAyd commented Jul 22, 2020

@toobaz

Copy link
Member

@toobaz toobaz left a comment

Choose a reason for hiding this comment

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

Mostly looks good, two small comments (but I wouldn't object to solving the self._table_float one in a separate PR).

# then write output in a nested table/tabular environment
buf.write(f"\\begin{{table}}")
Copy link
Member

Choose a reason for hiding this comment

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

Any obvious reason why this was moved up here? I would find buf.write(f"\\begin{{table}}{position_}") more readable that the write below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I did this was only because the line was getting too long, but I can write it as it was for sure.
Maybe we should unify the way it's done in _write_tabular_begin and _write_longtable_begin ? But it may be nitpicking

Copy link
Member

Choose a reason for hiding this comment

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

The reason I did this was only because the line was getting too long, but I can write it as it was for sure.

I do find it more readable - but if you want to break the line, you could do so at \n, so after the position.

Maybe we should unify the way it's done in _write_tabular_begin and _write_longtable_begin ?

Wow... indeed the difference between _write_tabular_begin and _write_longtable_begin seems to amount to the environment name: unless I'm mistaken, the merge is definitely worth doing (same for _end). By the way, if the function are unified feel free to move back the any([p is not None... down there.

self.caption is not None
or self.label is not None
or self.position is not None
):
Copy link
Member

Choose a reason for hiding this comment

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

As this test grows, it would be better to have self._table_float = any([p is not None for p in (caption, label, position)]) inside __init__.py, to be used both here and in _write_tabular_end.

@SylvainLan
Copy link
Contributor Author

@toobaz I tried to match the two functions _write_table_begin and _write_longtable_begin, but I don't think there is a need for the _table_float here, because:

  • There is no change in the begin line: whether or not a caption, label or position is given, it will always be \begin{longtable}...
  • The only difference would be for the line buf.write("\\\\\n") but here again the _table_float argument is not relevant because this behaviour is needed when a caption or a label is provided, but if only the position would be given it shouldn't be needed

if self.position is None:
position_ = ""
else:
position_ = f"[{self.position}]"
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right thanks !

@toobaz
Copy link
Member

toobaz commented Jul 28, 2020

I tried to match the two functions

My suggestion was actually to entirely suppress _write_longtable_begin, given that the only implications of longtable=True are that the float must be produced, and that its environment name must be longtable rather than table: all the rest is the same as _write_tabular_begin (same for *_end).

But this is not a strict necessity, at least in this PR. So if you prefer we can merge this PR after solving my last comment above.

@SylvainLan
Copy link
Contributor Author

My suggestion was actually to entirely suppress _write_longtable_begin

OK @toobaz I hadn't understood ! I tried doing something to resolve this, but I suggest doing it on a separate PR because I think it will get a bit messy.

This work is currently on a branch of mine and we can look at it once this PR is merged

@toobaz
Copy link
Member

toobaz commented Jul 31, 2020

Looks ready to merge to me. @WillAyd any idea on the two checks still failing?

@simonjayhawkins
Copy link
Member

Looks ready to merge to me. @WillAyd any idea on the two checks still failing?

see #35481. there are 20 fails here, should be only 4 now with lastest numpy-dev

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm @jreback

@WillAyd WillAyd added this to the 1.2 milestone Aug 7, 2020
@WillAyd WillAyd merged commit ce03883 into pandas-dev:master Aug 7, 2020
@WillAyd
Copy link
Member

WillAyd commented Aug 7, 2020

Thanks @SylvainLan

@SylvainLan SylvainLan deleted the to_latex_position branch August 10, 2020 06:20
@SylvainLan SylvainLan mentioned this pull request Aug 10, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO LaTeX to_latex
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: to_latex positional argument
4 participants