Skip to content

implemented additionally functionality of formatters_col in to_latex #32666

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

Closed
wants to merge 15 commits into from

Conversation

wgranados
Copy link

Not sure what I should do for the whatsnew entry, bit confused about versioning.

@pep8speaks
Copy link

pep8speaks commented Mar 12, 2020

Hello @wlgranados! 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 2020-10-06 01:25:04 UTC

@@ -2809,6 +2810,10 @@ def to_latex(
Formatter functions to apply to columns' elements by position or
name. The result of each function must be a unicode string.
List must be of length equal to the number of columns.
formatter_col: list of str, optional
Copy link
Member

@mroeschke mroeschke Mar 12, 2020

Choose a reason for hiding this comment

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

Can you specify the default argument? default None

@@ -2762,6 +2762,7 @@ def to_latex(
index=True,
na_rep="NaN",
formatters=None,
formatters_col=None,
Copy link
Member

Choose a reason for hiding this comment

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

To not break API compatibility for user who are calling this function positionally, this kwarg should go at the end.

@@ -2897,6 +2902,10 @@ def to_latex(
if multirow is None:
multirow = config.get_option("display.latex.multirow")

# formatters is a list
if formatters_col:
formatters = [lambda x: style % x for style in formatters_col]
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to use an f-string here instead?

Copy link
Member

Choose a reason for hiding this comment

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

Also could you add a check to validate that formatters_col is a list (or an iterable)?

Copy link
Author

Choose a reason for hiding this comment

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

I've added a check that silently fails if it's not a list, not sure if this is the best way to do this (feels un-pythonic but I digress)

Copy link
Author

Choose a reason for hiding this comment

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

I attempted an f-string approach as such:
formatters = [lambda x: f"{style}{x}" for style in formatters_col]
But I was failing the test suite (broke functionality), maybe there's a better approach I'm not seeing?

"""
assert result == expected

def test_to_latex_with_formatters_col(self):
Copy link
Member

Choose a reason for hiding this comment

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

If you could put the issue number as a comment below this line e.g. #GH ...

@jreback
Copy link
Contributor

jreback commented Mar 12, 2020

i am not sure the proposed api is in line how we format for example
i’m to_csv is there a reason you are diverging?

@wgranados
Copy link
Author

i am not sure the proposed api is in line how we format for example
i’m to_csv is there a reason you are diverging?

No particular reason, just wasn't paying attention, I believe I have addressed this issue with the latest commit.

@@ -2776,6 +2776,7 @@ def to_latex(
multirow=None,
caption=None,
label=None,
formatters_col=None,
Copy link
Member

Choose a reason for hiding this comment

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

In line what @jreback was referring to, this should probably be float_format that mirrors the kwarg in to_csv

Copy link
Author

Choose a reason for hiding this comment

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

I think I've addressed with the name change of the variable? If not let me know.

@@ -2897,6 +2903,10 @@ def to_latex(
if multirow is None:
multirow = config.get_option("display.latex.multirow")

# formatters is a list
if formatters_col and type(formatters_col) == list:
Copy link
Member

Choose a reason for hiding this comment

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

Use is_list_like here and add raise a ValueError if the use has passed something other than a list.

@mroeschke
Copy link
Member

@wlgranados do you have time to merge in master and to address the review comments?

@wgranados
Copy link
Author

@mroeschke yes, I have some time.
I believe I addressed some comments on my local branch, just forgot to mention it here.
I'll update this shortly.

@@ -2776,6 +2776,7 @@ def to_latex(
multirow=None,
caption=None,
label=None,
multifloat_format=None,
Copy link
Member

Choose a reason for hiding this comment

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

How about float_format so it matches the to_csv signature

@WillAyd
Copy link
Member

WillAyd commented Jun 26, 2020

@wlgranados is this still active? Can you address latest comments?

@wgranados
Copy link
Author

@WillAyd @mroeschke Yeah still active, I found that the test case I included was incorrect. There is an issue with the lambda function generation inside the list comprehension, so attempting to fix that currently. Also I read in the original thread that it might be possible to use the formatters variable instead, I think this should address the api usage comments then what I had before.

@wgranados
Copy link
Author

@WillAyd @mroeschke this should be code complete, so any comments would be appreciated

@mroeschke
Copy link
Member

Looks pretty good. Just needs a whatsnew entry for 1.2.0

for style in formatters
]
elif not formatter_elems_is_lambda:
raise ValueError("Formatters elements should be strings")
Copy link
Member

Choose a reason for hiding this comment

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

So if a user passes a mix of callable and strings this error gets thrown no? Is there a way to more generically support that case?

@github-actions github-actions bot added the Stale label Sep 30, 2020
@WillAyd
Copy link
Member

WillAyd commented Sep 30, 2020

@wlgranados looks like some merge conflicts have piled up and there are some outstanding CI failures - can you take a look and try to get things green?

@arw2019
Copy link
Member

arw2019 commented Nov 1, 2020

Closing in favor of #37552

@arw2019 arw2019 closed this Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: easier digit format in to_latex()
6 participants