Skip to content

Groupbydocs #8231

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

Closed
wants to merge 11 commits into from
Closed

Groupbydocs #8231

wants to merge 11 commits into from

Conversation

mcjcode
Copy link
Contributor

@mcjcode mcjcode commented Sep 10, 2014

Addresses #6944, by defining whitelisted methods in the DataFrameGroupBy and SeriesGroupBy class definitions. After this change, users of the whitelisted methods now get useful results from help, argument name completion, and auto-generated api docs. These DataFrameGroupBy and SeriesGroupBy methods are added to the api doc. Existing unit tests are extended to check for the presence of the whitelisted methods in the class definitions, and a new unit test verifies that old and new paths produce the same results for methods supporting level, axis, skipna arguments. (Existing methods already explicitly defined in GroupBy or derived classes are not overwritten.)

@@ -0,0 +1,63 @@
# pylint: disable-msg=W0612,E1101,W0141
Copy link
Contributor

Choose a reason for hiding this comment

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

just stick the tests in tests/test_group.py rather than adding a new test file. (you can do it in a new class like you are doing here).

@jreback
Copy link
Contributor

jreback commented Sep 11, 2014

can you indicate which of the checkboxes this will close from the issue.

@mcjcode
Copy link
Contributor Author

mcjcode commented Sep 11, 2014

This should close these checkboxes:

  • document the whitelisted methods (These are now added to api.rst)
  • More clearly document the DataFrameGroupBy and SeriesGroupBy classes at least mention them in the docs. (Both now appear in api.rst with their respective whitelist methods)
  • Add docstrings to the wrapped whitelisted functions. (Their docstrings now contain the documentation from their respective DataFrame/Series methods.)

I moved the unit test into test_groupby.py, and tweaked the exec'ed strings to fix the syntax error that the CI build was raising. These look like they are passing.

Should I submit another pull request?

@jorisvandenbossche
Copy link
Member

@mcjcode Thanks for your work on this!

+1 that all the methods now have help messages (didn't look at the implementation yet)

But I am not sure I also like adding them all to api.rst. I mean having seperate DataFrame/SeriesGroupby methods listed. It should be clearly documented that in practice you work with these objects, but listing all methods of both seperately seems also like a lot of repetition (in many cases, they are the same for both, and also the same for Series/DataFrame). Plus, some things are documented on GroupBy, other things on DataFrame/SeriesGroupby which also does not make it more clear. Another option is to also add these methods to GroupBy in the same way and document these?

@mcjcode
Copy link
Contributor Author

mcjcode commented Sep 12, 2014

@jorisvandenbossche - I agree. I put all of the additional methods under the GroupBy Computation/Descriptive functions heading in api.rst, and broke out the few cases there where a method is defined only for one of DataFrameGroupBy and SeriesGroupBy or the other.

@@ -2133,9 +2133,105 @@ def _convert_grouper(axis, grouper):
else:
return grouper

from inspect import getargspec
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 move this function inse _whitelist_method_generator? its only called from their yes (and its cleaner that way)

@mcjcode
Copy link
Contributor Author

mcjcode commented Sep 13, 2014

@jreback - I put the inspect import inside the function def. That makes sense.

The point of the additional tests were to check that ...

  1. in addition to being getattr-able from a GroupBy object, the whitelisted methods are also defined in the class, and ...
  2. you get the same answer whether you use the DataFrameGroupBy method or call the same DataFrame method with the level argument.

The dtypes exclusion was a remnant of when I hadn't wrapped it as a class member. Now that it is defined as a property in the class, this is unnecessary. I also extended the second test to nine other methods, ones that don't take a skipna parameter.

@@ -2133,9 +2133,105 @@ def _convert_grouper(axis, grouper):
else:
return grouper

def _make_signature(func) :
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 move this to pandas/utils/decorators.py? or somewhat similar. doesn't really belong in here as its generic and not related to groupby.

@jreback
Copy link
Contributor

jreback commented Sep 13, 2014

@mcjcode small change, otherwise looks ok to me.

@mcjcode
Copy link
Contributor Author

mcjcode commented Sep 14, 2014

Moved make_signature from groupby.py to decorators.py.

@jreback
Copy link
Contributor

jreback commented Sep 14, 2014

merged via 0acfa44

thanks!

@jreback
Copy link
Contributor

jreback commented Sep 14, 2014

@mcjcode I checked off some boxed in #6944 pls lmk if I missed any (or didn't do some that need)

thanks!

@mcjcode mcjcode deleted the groupbydocs branch September 15, 2014 03:43
@mcjcode
Copy link
Contributor Author

mcjcode commented Sep 15, 2014

This also "added docstrings to the wrapped whitelisted functions"

So we should check that one off as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants