Skip to content

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

Merged
merged 3 commits into from
Mar 25, 2021

Conversation

vladu
Copy link
Contributor

@vladu 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.

@jreback jreback added this to the 1.3 milestone Mar 23, 2021
@jreback
Copy link
Contributor

jreback commented Mar 23, 2021

looks fine. @rhshadrach if any comments.

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.

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
Copy link
Member

@rhshadrach rhshadrach Mar 24, 2021

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

@rhshadrach rhshadrach added the Apply Apply, Aggregate, Transform, Map label Mar 25, 2021
@rhshadrach rhshadrach merged commit 3ab4f8d into pandas-dev:master Mar 25, 2021
@rhshadrach
Copy link
Member

Thanks @vladu!

@vladu vladu deleted the vladu-doc-fixes-1 branch March 25, 2021 01:20
@vladu
Copy link
Contributor Author

vladu commented Mar 25, 2021

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

@rhshadrach
Copy link
Member

@vladu - I wouldn't recommend changing a thing. To make sure - my complaint about the current docs for DataFrame.transform was very much outside the scope of this PR. This was well done.

vladu added a commit to vladu/pandas that referenced this pull request Apr 5, 2021
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Apply Apply, Aggregate, Transform, Map Docs Groupby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants