COMPAT: Fix indent level bug preventing wrapper function rename by matthagy · Pull Request #14620 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation21 Commits9 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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'.
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
1 for good ones
1 for failing ones
then can add a TODO for the failing ones
@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.
| getattr(type(gb), m) |
|---|
| f = getattr(type(gb), m) |
| # TODO: Fix inconsistencies between attribute and method names |
| # self.assertEqual(f.__name__, m) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| @@ -5986,11 +5986,44 @@ def test_groupby_whitelist(self): |
|---|
| # 'nlargest', 'nsmallest', |
| ]) |
| # TODO: Fix these inconsistencies between attribute and method names |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this comment mean?
| 'dtype', |
|---|
| 'unique' |
| ]) |
| for obj, whitelist in zip((df, s), (df_whitelist, s_whitelist)): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 __name__ is inconsistent with attribute name for these known issues. Also revised the comment explaining the inconsistencies.
@matthagy can you rebase / update.
sorry got a bit lost in the shuffle.
@matthagy I pushed a change here to make this also set __qualname__ (we do this already all Series/DataFrame anyhow, so seemed easy to add here.
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?
| @@ -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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these here
| @@ -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): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this test superfluous? (or just a quick test)?
Fix indent level bug preventing wrapper function rename COMPAT: 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.
Example:
from pandas.core.groupby import GroupBy GroupBy.sum.name 'f'
Should be 'sum'.
Matt Hagy and others added 8 commits
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
mattip pushed a commit to mattip/pandas that referenced this pull request
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 jeff@reback.net Author: Matt Hagy matt@liveramp.com Author: Matt Hagy hagy@gatech.edu
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