Groupby transform cleanups · Pull Request #27467 · pandas-dev/pandas (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation34 Commits47 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

ghost

Related #27389 (several issues there)
closes #27486

This was referenced

Jul 19, 2019

@pep8speaks

Hello @pilkibun! 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-07-25 18🔞14 UTC

pilkibun added 7 commits

July 19, 2019 03:06

@ghost ghost marked this pull request as ready for review

July 19, 2019 14:56

WillAyd

Choose a reason for hiding this comment

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

Thanks for the PR. Mostly stylistic comments

def test_transform_invalid_name_raises():
df = DataFrame(dict(a=[0, 1, 1, 2]))
g = df.groupby(["a", "b", "b", "c"])
with pytest.raises(ValueError, match="not a valid function name"):

Choose a reason for hiding this comment

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

Can you parametrize these instead?

Choose a reason for hiding this comment

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

I don't think that's needed. Parameterize with one case is not useful, and as a smoke test there doesn't need to be more than one. Also lots of similar tests in this file.

@TomAugspurger

Can you write release notes for each of the user-facing changes? If you merge master we should have a 0.26.0.rst now.

pilkibun added 7 commits

July 19, 2019 18:08

…leanups1

@ghost ghost mentioned this pull request

Jul 20, 2019

jreback

@jreback

this also needs a whatsnew note. I am not so concerned about the invalid case (e.g. .transform('foo')), but something that looked legitimate and maybe even did something, though it may have been wrong, is now raising

pilkibun added 3 commits

July 21, 2019 09:46

pilkibun added 12 commits

July 21, 2019 23:02

…leanups1

…leanups1

jreback

Choose a reason for hiding this comment

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

looks really good! just some small comments. ping on green.

pilkibun added 4 commits

July 24, 2019 11:10

jreback

Contributor

@jreback jreback left a comment • Loading

Choose a reason for hiding this comment

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

lgtm. not sure what is failing, merge master and ping on green. (you can ignore the codecov)

pilkibun added 2 commits

July 25, 2019 13:16

…leanups1

@ghost

@jreback

@ghost ghost deleted the groupby_transform_cleanups1 branch

July 25, 2019 22:53

@ghost ghost mentioned this pull request

Jul 25, 2019

2 tasks

@ghost

quintusdias pushed a commit to quintusdias/pandas_dev that referenced this pull request

Aug 16, 2019

@quintusdias