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.

Sign up for GitHub

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

Jump to bottom

CLN: named parameters for GroupBy.(mean|median|var|std) #31485

Merged
merged 2 commits into from
Feb 2, 2020
Merged

CLN: named parameters for GroupBy.(mean|median|var|std) #31485

merged 2 commits into from
Feb 2, 2020

Conversation

topper-123
Copy link
Contributor

@topper-123 topper-123 commented Jan 31, 2020 •
edited
Loading

Drops *args & **kwargs, replace with named parameters for groupby methods mean, median, var & std. Similar to #31473.

This PR has the side effect that the raised error when a parameter is not allowed, is now TypeError instead of UnsupportedFunctionCall, so technically a API change...

@topper-123 topper-123 force-pushed the cln_groupby_mean_median_std_var branch from c6bf810 to b6fceec Compare January 31, 2020 09:42
@topper-123 topper-123 changed the title CLN: paramters for GroupBy.(mean|median|var|std) CLN: named parameters for GroupBy.(mean|median|var|std) Jan 31, 2020
WillAyd
WillAyd reviewed Jan 31, 2020
View reviewed changes
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.

looks good to me. I'd be ok with 1.1 but maybe worth a 2.0 target as well

@topper-123
Copy link
Contributor Author

topper-123 commented Jan 31, 2020 •
edited
Loading

Maybe a decorator similar to deprecate_kwargs could be added. That way we'd get the correct signature, and UnsupportedFunctionCall would be raised in the decorator.

@jreback
Copy link
Contributor

jreback commented Jan 31, 2020

looks good to me. I'd be ok with 1.1 but maybe worth a 2.0 target as well

we don’t have the bandwidth or infrastructure to handle this type of versioning
if it’s not targeted then would rather close

this is fine for 1.1

@topper-123
Copy link
Contributor Author

Ok, so I don't do a decorator solution?

jreback
jreback approved these changes Feb 1, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm. a question.

pandas/core/groupby/groupby.py Outdated
)

@Substitution(name="groupby")
@Appender(_common_see_also)
def median(self, **kwargs):
def median(self, numeric_only=True, min_count=-1):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you update doc-strings for these parameters (or are they already ok with the Appenders?)

@jreback jreback added this to the 1.1 milestone Feb 1, 2020
@topper-123 topper-123 force-pushed the cln_groupby_mean_median_std_var branch from b6fceec to 92d2164 Compare February 2, 2020 08:09
@topper-123
Copy link
Contributor Author

topper-123 commented Feb 2, 2020 •
edited
Loading

Ok, I've added doc strings.

Also, I've found out that Groupby.median actually didn't accept min_count in master, but raised an AssertionError. So I removed the parameter from that method signature.

@topper-123 topper-123 force-pushed the cln_groupby_mean_median_std_var branch from 92d2164 to 3a9860a Compare February 2, 2020 08:24
@jreback jreback merged commit f6146a5 into pandas-dev:master Feb 2, 2020
@jreback
Copy link
Contributor

jreback commented Feb 2, 2020

thanks @topper-123

@jreback
Copy link
Contributor

jreback commented Feb 2, 2020

Ok, I've added doc strings.

Also, I've found out that Groupby.median actually didn't accept min_count in master, but raised an AssertionError. So I removed the parameter from that method signature.

happy to have an issue / PR about this.

@topper-123 topper-123 deleted the cln_groupby_mean_median_std_var branch February 4, 2020 16:21
@topper-123
Copy link
Contributor Author

I did fix that is this PR, so no issue necesary.

So in short, in v1.0.0/master we'd have that grp.mean(min_count=1) raised a AssertionError, while now it raised a TypeError, which is the same error code as we'd get for df.mean(min_count=1), so is the correct error type, IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd left review comments

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Groupby
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.
3 participants
@topper-123 @jreback @WillAyd