-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
CLN: named parameters for GroupBy.(mean|median|var|std) #31485
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
CLN: named parameters for GroupBy.(mean|median|var|std) #31485
Conversation
c6bf810 to
b6fceec
Compare
WillAyd
left a comment
There was a problem hiding this 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
|
Maybe a decorator similar to |
we don’t have the bandwidth or infrastructure to handle this type of versioning this is fine for 1.1 |
|
Ok, so I don't do a decorator solution? |
jreback
left a comment
There was a problem hiding this 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): |
There was a problem hiding this comment.
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?)
b6fceec to
92d2164
Compare
|
Ok, I've added doc strings. Also, I've found out that |
92d2164 to
3a9860a
Compare
|
thanks @topper-123 |
happy to have an issue / PR about this. |
|
I did fix that is this PR, so no issue necesary. So in short, in v1.0.0/master we'd have that |
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
TypeErrorinstead ofUnsupportedFunctionCall, so technically a API change...