-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
DOC: Fix documentation for DataFrame.groupby.transform #40506
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
Conversation
vladu
commented
Mar 18, 2021
- The doc template should no longer reference "Series" transforms explicitly.
- Make consistent the formatting for references to function argument names.
This should no longer reference "Series" transforms.
looks fine. @rhshadrach if any comments. |
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.
Thanks for the PR! Looks to be in good shape, just one request below.
%(klass)s.groupby.aggregate : Aggregate using one or more | ||
operations over the specified axis. | ||
%(klass)s.transform : Transforms the Series on each group | ||
based on the given function. | ||
%(klass)s.transform : Call ``func`` on self producing a %(klass)s with |
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.
I would think that this should be about DataFrame.groupby.transform, rather than DataFrame.transform.
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.
So this is the doc string for DataFrame.groupby.transform. So in the "See Also" section, it makes sense to me that it references the ungrouped DataFrame.transform.
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.
Ahh, whoops, thanks! In that case, may want to specify that transform operates on the columns rather than self.
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.
Sorry, let me be more specific. This is a template doc-string that is reused for both [DataFrame|Series].groupby.transform. Hence the "klass" templating. In master right now, the "see also" section specifically talks about Series, which doesn't make sense for DataFrame. So my PR here is to fix that. The text I inserted, is taken verbatim from the template for [DataFrame|Series].tansform. I think that's the reason it's being a little vague, and avoiding verbiage that would imply dimensionality.
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.
Thanks - make sense. It does seem to me desirable to improve this (namely, it seems to me to be misleading in the DataFrame case), but okay for this PR.
Thanks @vladu! |
Thanks, Richard. I appreciate your feedback. This was my first PR to pandas, and I was trying to do something low impact / low controversy. Will be more comprehensive going forward. -- Vlad |
@vladu - I wouldn't recommend changing a thing. To make sure - my complaint about the current docs for |