COMPAT: Fix indent level bug preventing wrapper function rename #14620
Conversation
|
can u add some tests for this (and validate all functions names in the whitelist would be great) |
|
@jreback, tried adding a test for all whitelist function names and discovered there are additional places where attribute and method names are inconsistent. Replaced that with a test that only covers the names corresponding to this current bug fix. If you think it makes sense to hold off until all whitelist function names are properly handled then I can close this PR for now and open an issue. |
|
just make it 2. tests |
|
@jreback, added back failing test, commented out, and marked w/ TODO. Does reuse a an existing test to prevent duplicating whitelists or refactoring them to be class members. Let me know if you had something else in mind. |
pandas/tests/test_groupby.py
Outdated
| getattr(type(gb), m) | ||
| f = getattr(type(gb), m) | ||
| # TODO: Fix inconsistencies between attribute and method names | ||
| # self.assertEqual(f.__name__, m) |
There was a problem hiding this comment.
can u made an actual test of things in the whitelist which fail
|
@jreback, revised test to show things in whitelist that currently fail. Test now also handles all of the other whitelist cases that aren't known issues. |
pandas/tests/test_groupby.py
Outdated
| @@ -5986,11 +5986,44 @@ def test_groupby_whitelist(self): | |||
| # 'nlargest', 'nsmallest', | |||
| ]) | |||
|
|
|||
| # TODO: Fix these inconsistencies between attribute and method names | |||
There was a problem hiding this comment.
what does this comment mean?
pandas/tests/test_groupby.py
Outdated
| 'dtype', | ||
| 'unique' | ||
| ]) | ||
|
|
||
| for obj, whitelist in zip((df, s), (df_whitelist, s_whitelist)): |
There was a problem hiding this comment.
what I mean is create another function where you assert that these are NOT equal (with the sub-list of those that are broken), so that when they are fixed this tests will break (and the list will need to be updated). IOW, create a list like the white-list of the broken ones, have the existing test just skip on them (as you do below), but then create another function that asserts that the broken ones are not equal.
|
@jreback, created the additional test to assert that method |
|
@matthagy can you rebase / update. sorry got a bit lost in the shuffle. |
Codecov Report
@@ Coverage Diff @@
## master #14620 +/- ##
==========================================
- Coverage 90.99% 90.98% -0.02%
==========================================
Files 143 143
Lines 49420 49403 -17
==========================================
- Hits 44972 44949 -23
- Misses 4448 4454 +6
Continue to review full report at Codecov.
|
|
@jreback, rebased. |
|
can you rebase? |
|
@jreback, rebased. |
|
thanks @matthagy |
|
can you rebase / update |
|
@matthagy can you rebase / ping on green. |
|
@matthagy I pushed a change here to make this also set Can you push a whatsnew note, otherwise this lgtm. I think you had several TODO's in the tests for functions which don't set things properly, can you have a look at those? |
pandas/tests/groupby/test_groupby.py
Outdated
| @@ -3753,6 +3753,19 @@ def test_groupby_selection_with_methods(self): | |||
| assert_frame_equal(g.filter(lambda x: len(x) == 3), | |||
| g_exp.filter(lambda x: len(x) == 3)) | |||
|
|
|||
| # The methods returned by these attributes don't have a __name__ attribute | |||
| # that matches that attribute. | |||
pandas/tests/groupby/test_groupby.py
Outdated
| @@ -3929,6 +3986,12 @@ def test_tab_completion(self): | |||
| 'ffill', 'bfill', 'pad', 'backfill', 'rolling', 'expanding']) | |||
| self.assertEqual(results, expected) | |||
|
|
|||
| def test_groupby_function_rename(self): | |||
There was a problem hiding this comment.
is this test superfluous? (or just a quick test)?
jreback
changed the title
|
cc @mrocklin |
Original code intends to rename the wrapper function f using the provided name, but this isn't happening because code is incorrectly indented an extra level. Example: >>> from pandas.core.groupby import GroupBy >>> GroupBy.sum.__name__ 'f' Should be 'sum'.
Commented out and marked with a TODO since some are currently inconsistent and not immediately obvious how to fix all of them.
Add support for __qualname__
Original code intends to rename the wrapper function f using the provided name, but this isn't happening because code is incorrectly indented an extra level. from pandas.core.groupby import GroupBy GroupBy.sum.__name__ Should be 'sum'. Author: Jeff Reback <[email protected]> Author: Matt Hagy <[email protected]> Author: Matt Hagy <[email protected]> Closes pandas-dev#14620 from matthagy/patch-1 and squashes the following commits: db3c6e4 [Jeff Reback] clean/reorg tests 205489b [Jeff Reback] doc 8b185b4 [Jeff Reback] PEP 781b9b3 [Jeff Reback] Move _groupby_function inside GroupBy 68013bf [Matt Hagy] Added a test for known inconsistent attribute/method names 3bf8993 [Matt Hagy] Revise attribute/method consistency check to skip known inconsistencies 033e42d [Matt Hagy] Test for consistency of attribute and method names 2a54b77 [Matt Hagy] Test renaming of _groupby_function wrapper function a492b5a [Matt Hagy] Fix indent level bug preventing wrapper function rename
Original code intends to rename the wrapper function f using the provided name, but this isn't happening because code is incorrectly indented an extra level.
Should be 'sum'.