Skip to content

Groupby getitem works with all index types #13731

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

Conversation

jcrist
Copy link
Contributor

@jcrist jcrist commented Jul 20, 2016

  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
  • whatsnew entry

Previously df.groupby(0)[df.columns] would fail if all column names
were integers (meaning df.columns was an Int64Index). This was
because the implementation of __getitem__ in SelectionMixin was
checking for ABCIndex when it probably should have checked for
ABCIndexClass.

@jcrist jcrist force-pushed the groupby-getitem-int64index branch from a4a8d41 to 29185e4 Compare July 20, 2016 23:51
@jcrist jcrist mentioned this pull request Jul 20, 2016
@codecov-io
Copy link

codecov-io commented Jul 21, 2016

Current coverage is 84.56% (diff: 100%)

Merging #13731 into master will increase coverage by <.01%

@@             master     #13731   diff @@
==========================================
  Files           141        141          
  Lines         51195      51196     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43295      43296     +1   
  Misses         7900       7900          
  Partials          0          0          

Powered by Codecov. Last update e357ea1...b901322

@@ -3769,6 +3769,21 @@ def test_getitem_list_of_columns(self):
assert_frame_equal(result2, expected)
assert_frame_equal(result3, expected)

def test_getitem_numeric_column_names(self):
df = DataFrame({0: list('abcd') * 2,
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the issue number (or in this case PR number) as a comment on the first line?

@jorisvandenbossche
Copy link
Member

Looks good to me, some minor comments

@jreback
Copy link
Contributor

jreback commented Jul 21, 2016

yep lgtm.

Previously `df.groupby(0)[df.columns]` would fail if all column names
were integers (meaning `df.columns` was an `Int64Index`). This was
because the implementation of `__getitem__` in `SelectionMixin` was
checking for `ABCIndex` when it probably should have checked for
`ABCIndexClass`.
@jcrist jcrist force-pushed the groupby-getitem-int64index branch from 29185e4 to b901322 Compare July 21, 2016 14:06
@jcrist
Copy link
Contributor Author

jcrist commented Jul 21, 2016

Thanks, fixed.

@jreback
Copy link
Contributor

jreback commented Jul 22, 2016

lgtm

@jorisvandenbossche jorisvandenbossche merged commit 1cd1026 into pandas-dev:master Jul 23, 2016
@jorisvandenbossche
Copy link
Member

@jcrist Thanks!

@jcrist jcrist deleted the groupby-getitem-int64index branch July 23, 2016 15:59
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.

4 participants