COMPAT: Fix indent level bug preventing wrapper function rename #14620

matthagy · Nov 9, 2016

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'.

jreback · Nov 9, 2016

can u add some tests for this (and validate all functions names in the whitelist would be great)

matthagy · Nov 9, 2016

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

jreback · Nov 9, 2016

just make it 2. tests
1 for good ones
1 for failing ones
then can add a TODO for the failing ones

matthagy · Nov 9, 2016

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

jreback · Nov 9, 2016

jreback reviewed Nov 9, 2016

View changes

can u made an actual test of things in the whitelist which fail

matthagy · Nov 9, 2016

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

jreback · Nov 11, 2016

jreback reviewed Nov 11, 2016

View changes

what does this comment mean?

jreback · Nov 11, 2016

jreback reviewed Nov 11, 2016

View changes

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.

matthagy · Nov 15, 2016

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

jreback · Dec 21, 2016

@matthagy can you rebase / update.

sorry got a bit lost in the shuffle.

codecov-io · Dec 22, 2016

Codecov Report

Merging #14620 into master will decrease coverage by 0.01%.
The diff coverage is 93.87%.

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

Impacted Files	Coverage Δ
pandas/core/groupby.py	`95.03% <93.87%> (+0.05%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/types/cast.py	`85.31% <0%> (-0.29%)`	⬇️
pandas/core/frame.py	`97.56% <0%> (-0.11%)`	⬇️
pandas/core/base.py	`95.48% <0%> (-0.03%)`	⬇️
pandas/io/parsers.py	`95.51% <0%> (-0.01%)`	⬇️
pandas/io/excel.py	`79.67% <0%> (ø)`	⬆️
pandas/core/missing.py	`84.9% <0%> (+0.62%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec84ae3...db3c6e4. Read the comment docs.

matthagy · Dec 22, 2016

@jreback, rebased.

jreback · Feb 1, 2017

can you rebase?

matthagy · Feb 1, 2017

@jreback, rebased.

jreback · Feb 1, 2017

thanks @matthagy
lgtm. ping on green.

jreback · Feb 16, 2017

can you rebase / update

jreback · Mar 23, 2017

@matthagy can you rebase / ping on green.

jreback · Mar 27, 2017

@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?

jreback · Mar 27, 2017

jreback reviewed Mar 27, 2017

View changes

these here

jreback · Mar 27, 2017

jreback reviewed Mar 27, 2017

View changes

is this test superfluous? (or just a quick test)?

jreback · Mar 27, 2017

cc @mrocklin

jreback added API Design Bug Groupby labels Nov 11, 2016

jreback added this to the 0.20.0 milestone Feb 1, 2017

jreback changed the title from Fix indent level bug preventing wrapper function rename to COMPAT: Fix indent level bug preventing wrapper function rename Mar 27, 2017

jreback closed this in 2e64614 Mar 28, 2017

-    -                getattr(type(gb), m)
+    +                f = getattr(type(gb), m)
+    +                # TODO: Fix inconsistencies between attribute and method names
+    +                # self.assertEqual(f.__name__, m)

@@ -5986,11 +5986,44 @@ def test_groupby_whitelist(self):
                  # 'nlargest', 'nsmallest',
              ])
+    +        # TODO: Fix these inconsistencies between attribute and method names

+    +            'dtype',
+    +            'unique'
+    +        ])
+    +
              for obj, whitelist in zip((df, s), (df_whitelist, s_whitelist)):

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

@@ -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):

pandas-dev/pandas

COMPAT: Fix indent level bug preventing wrapper function rename #14620

matthagy commented Nov 9, 2016

jreback commented Nov 9, 2016

matthagy commented Nov 9, 2016

jreback commented Nov 9, 2016

matthagy commented Nov 9, 2016

jreback reviewed Nov 9, 2016

View changes

matthagy commented Nov 9, 2016

jreback added API Design Bug Groupby labels Nov 11, 2016

jreback reviewed Nov 11, 2016

View changes

jreback reviewed Nov 11, 2016

View changes

matthagy commented Nov 15, 2016

jreback commented Dec 21, 2016

codecov-io commented Dec 22, 2016 • edited by codecov

matthagy commented Dec 22, 2016

jreback commented Feb 1, 2017

matthagy commented Feb 1, 2017

jreback commented Feb 1, 2017

jreback added this to the 0.20.0 milestone Feb 1, 2017

jreback commented Feb 16, 2017

jreback commented Mar 23, 2017

jreback commented Mar 27, 2017

jreback reviewed Mar 27, 2017

View changes

jreback reviewed Mar 27, 2017

View changes

jreback changed the title from Fix indent level bug preventing wrapper function rename to COMPAT: Fix indent level bug preventing wrapper function rename Mar 27, 2017

jreback commented Mar 27, 2017

matthagy and others added some commits Nov 9, 2016

jreback closed this in `2e64614` Mar 28, 2017

mattip added a commit to mattip/pandas that referenced this pull request Apr 3, 2017

linebp added a commit to linebp/pandas that referenced this pull request Apr 17, 2017

matthagy	`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'.		`a492b5a`
Matt Hagy	`Test renaming of _groupby_function wrapper function`		`2a54b77`
Matt Hagy	`Test for consistency of attribute and method names` Commented out and marked with a TODO since some are currently inconsistent and not immediately obvious how to fix all of them.		`033e42d`
Matt Hagy	`Revise attribute/method consistency check to skip known inconsistencies`		`3bf8993`
Matt Hagy	`Added a test for known inconsistent attribute/method names`		`68013bf`
jreback	`Move _groupby_function inside GroupBy` Add support for __qualname__		`781b9b3`
jreback	`PEP`		`8b185b4`
jreback	`doc`		`205489b`
jreback	`clean/reorg tests`	Some checks were not successful 1 errored, 1 failing, and 1 successful checks continuous-integration/appveyor/pr — AppVeyor build cancelled Details continuous-integration/travis-ci/pr — The Travis CI build failed Details ci/circleci — Your tests passed on CircleCI! Details	`db3c6e4`

pandas-dev/pandas

Join GitHub today

COMPAT: Fix indent level bug preventing wrapper function rename #14620

Conversation

matthagy commented Nov 9, 2016

jreback commented Nov 9, 2016

matthagy commented Nov 9, 2016

jreback commented Nov 9, 2016

matthagy commented Nov 9, 2016

jreback reviewed Nov 9, 2016 View changes

jreback Nov 9, 2016

matthagy commented Nov 9, 2016

jreback added API Design Bug Groupby labels Nov 11, 2016

jreback reviewed Nov 11, 2016 View changes

jreback Nov 11, 2016

jreback reviewed Nov 11, 2016 View changes

jreback Nov 11, 2016

matthagy commented Nov 15, 2016

jreback commented Dec 21, 2016

codecov-io commented Dec 22, 2016 • edited by codecov

Codecov Report

matthagy commented Dec 22, 2016

jreback commented Feb 1, 2017

matthagy commented Feb 1, 2017

jreback commented Feb 1, 2017

jreback added this to the 0.20.0 milestone Feb 1, 2017

jreback commented Feb 16, 2017

jreback commented Mar 23, 2017

jreback commented Mar 27, 2017

jreback reviewed Mar 27, 2017 View changes

jreback Mar 27, 2017

jreback reviewed Mar 27, 2017 View changes

jreback Mar 27, 2017

jreback changed the title from Fix indent level bug preventing wrapper function rename to COMPAT: Fix indent level bug preventing wrapper function rename Mar 27, 2017

jreback commented Mar 27, 2017

matthagy and others added some commits Nov 9, 2016

Some checks were not successful

jreback closed this in 2e64614 Mar 28, 2017

mattip added a commit to mattip/pandas that referenced this pull request Apr 3, 2017

linebp added a commit to linebp/pandas that referenced this pull request Apr 17, 2017

jreback reviewed Nov 9, 2016

View changes

jreback reviewed Nov 11, 2016

View changes

jreback reviewed Nov 11, 2016

View changes

jreback reviewed Mar 27, 2017

View changes

jreback reviewed Mar 27, 2017

View changes

jreback closed this in `2e64614` Mar 28, 2017