Skip to content

BUG/TST: Address GH28283 calling __finalize__ in pivot_table, groupby.median and groupby.mean #39473

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

Merged
merged 18 commits into from
Feb 20, 2021

Conversation

liaoaoyuan97
Copy link
Contributor

Progress towards #28283

  1. refactor pivot_table function to avoid overhead of _finalize__ calls when a list of aggfuncs passed
  2. call __finalize__ before returning groupby.mean and groupby.median
  3. add tests for 1 and 2
  4. add ignored test cases for groupby.std/var/sem/size/ohlc/describe as a reminder to fix(they are failing currently)
  • closes #xxxx
  • tests added / passed
  • Ensure all linting tests pass, see here for how to run them
  • whatsnew entry

table = concat(pieces, keys=keys, axis=1)
return table.__finalize__(data, method="pivot_table")

table = _pivot_table(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is way more complicated to read. need to simplify this or have a better name for _pivot_table -> __internal_pivot_table, and add a doc-string & types.

Copy link
Contributor

Choose a reason for hiding this comment

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

this maybe worth reverting and doing in a dedicated PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just finished reading the requirements for doc-string. Will rename the function and add a doc-string & types.

@jreback
Copy link
Contributor

jreback commented Feb 2, 2021

add ignored test cases for groupby.std/var/sem/size/ohlc/describe as a reminder to fix(they are failing currently)

where is this?

@jreback jreback added Groupby Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Feb 2, 2021
@jreback
Copy link
Contributor

jreback commented Feb 11, 2021

@liaoaoyuan97 if you can fix up would be great. (and merge master)

) -> DataFrame:
"""
Equivalent of :func:`pandas.pivot_table`, except only allowing non-list ``aggfunc``.
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback I didn't use a sharing doctring since other methods sharing it need to change. Considering it is for internal use only, devs can find reference by knowing the relation btw internal and external ones.
Please let me know your thoughts.

@liaoaoyuan97 liaoaoyuan97 requested a review from jreback February 13, 2021 00:11
@liaoaoyuan97
Copy link
Contributor Author

@jreback I think all your comments are addressed. Would you ming taking a took?

@jreback jreback added this to the 1.3 milestone Feb 19, 2021
@jreback
Copy link
Contributor

jreback commented Feb 19, 2021

@rhshadrach if you'd have a look

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

lgtm, just a small request on the docstring.

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @liaoaoyuan97

@rhshadrach rhshadrach merged commit 2e5c28f into pandas-dev:master Feb 20, 2021
@rhshadrach
Copy link
Member

Thanks @liaoaoyuan97!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Groupby Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants