Skip to content

BUG: Fix for GH #14848 for groupby().describe() with tuples as the Index #15110

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.

Sign up for GitHub

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 5 commits into from
Closed

BUG: Fix for GH #14848 for groupby().describe() with tuples as the Index #15110

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9489cb2
BUG: Fix issue #14848 groupby().describe() on indices containing all …
Dr-Irv Jan 12, 2017
f3a7a21
Changes per jreback requests
Dr-Irv Jan 12, 2017
fbd20f5
Raise error when creating index of tuples with name parameter a string
Dr-Irv Jan 12, 2017
db13c3b
Use not is_list_like
Dr-Irv Jan 12, 2017
c18c6cb
Undo change to merge.py and make whatsnew a 2 line comment.
Dr-Irv Jan 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.20.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,7 @@ Bug Fixes
- Bug in converting object elements of array-like objects to unsigned 64-bit integers (:issue:`4471`, :issue:`14982`)
- Bug in ``pd.pivot_table()`` where no error was raised when values argument was not in the columns (:issue:`14938`)

- Bug in ``DataFrame.groupby().describe()`` when grouping on ``Index`` containing tuples (:issue:`14848`)



Expand Down
13 changes: 13 additions & 0 deletions pandas/tests/groupby/test_groupby.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1490,6 +1490,19 @@ def test_frame_describe_multikey(self):
for name, group in groupedT:
assert_frame_equal(result[name], group.describe())

# GH #14848
def test_frame_describe_tupleindex(self):
df1 = DataFrame({'x': [1, 2, 3, 4, 5] * 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

comment inside the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

'y': [10, 20, 30, 40, 50] * 3,
'z': [100, 200, 300, 400, 500] * 3})
df1['k'] = [(0, 0, 1), (0, 1, 0), (1, 0, 0)] * 5
df2 = df1.rename(columns={'k': 'key'})
des1 = df1.groupby('k').describe()
Copy link
Contributor

Choose a reason for hiding this comment

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

result = 
expected =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

des2 = df2.groupby('key').describe()
if len(des1) > 0:
Copy link
Contributor

@jreback jreback Jan 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

why are you doing this conditional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure the index would exist if the DataFrame was empty. Retested and see that I don't need the test.

des2.index.set_names(des1.index.names, inplace=True)
assert_frame_equal(des1, des2)

def test_frame_groupby(self):
grouped = self.tsframe.groupby(lambda x: x.weekday())

Expand Down
6 changes: 5 additions & 1 deletion pandas/tools/merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1626,7 +1626,11 @@ def __init__(self, objs, axis=0, join='outer', join_axes=None,
clean_objs.append(v)
objs = clean_objs
name = getattr(keys, 'name', None)
keys = Index(clean_keys, name=name)
# GH 14848
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need the check here? the Index already has a fast path for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback Because in the original issue, if keys is an Index containing all tuples, and clean_keys then becomes a list of tuples, and name is a single string, that's where things go wrong. What the fix does is avoid creating an Index (and let the name propagate) because the Index has already been created.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm

maybe this should be handled in the _get_grouper logic then

where you first figure out what the name actually is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback It's not a grouper issue. Here's code that fails with 0.19.2 when passing an Index of tuples into concat.

df1 = pd.DataFrame({"x" : [1,2,3], "k" : [(0,0,1),(0,1,0),(1,0,0)]}).set_index('k')
df2 = pd.DataFrame({"x" : [4,5,6]}, index = pd.Index(['a','b','c']))
ic2 = df1.index
ic1 = pd.Index(['g','h','i'], name = 'foobar')
c1 = pd.concat([df2, df2, df2], axis = 0, keys=ic1, levels=[ic1], names=[ic1.name])
print('c1 is ok')
c2 = pd.concat([df2, df2, df2], axis = 0, keys=ic2, levels=[ic2], names=[ic2.name])
print('c2 is ok')

The value of c1 is computed fine. The value of c2 raises an error because the tuples were passed as the index into concat. But if you use the code that I put in above in merge.py, then this example works.

I know the example is a bit far-fetched, but it is equivalent to what is going on when the grouper calls concat, and I'd rather not mess with the grouper when the simple fix handles this particular bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

In [1]: li = [(0, 0, 1), (0, 1, 0), (1, 0, 0)]

In [2]: pd.Index(li)
Out[2]: 
MultiIndex(levels=[[0, 1], [0, 1], [0, 1]],
           labels=[[0, 0, 1], [0, 1, 0], [1, 0, 0]])

In [3]: pd.Index(li, name='foo')
Out[3]: 
MultiIndex(levels=[[0, 1], [0, 1], [0, 1]],
           labels=[[0, 0, 1], [0, 1, 0], [1, 0, 0]],
           names=['f', 'o', 'o'])

this is with this PR, something wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback Looks like I didn't push the last set of changes. Sorry about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

so I remove the change in this file and everything still works. Please do that and push up again, the only fix here needed was in MultiIndex.

# Don't pass name when creating index (# GH 14252)
# So that if keys are tuples, name isn't checked
Copy link
Contributor

Choose a reason for hiding this comment

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

then this should be fixed inside Index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback I'm not sure this can be fixed there, because the goal of the code in the section above was to fix an issue with the propagation of names in Index when using concat(). (Issue #14252). So what the fix I proposed does is allow the Index to be created, and then it copies the names. If you consider the following code:

tl = [(0,0,1),(0,1,0),(1,0,0)]
i1 = pd.Index(tl, name='a')
i1

This code will raise an exception because it tries to compute a MultiIndex and there is only one name (single character 'a') supplied. But if the name argument is a 3 character string, i.e.,

i1 = pd.Index(tl, name='abc')

then the above snippet will work because the name is 3 characters long. In that case, it creates a MultiIndex with the names 'a', 'b' and 'c'.

I could modify Index to see if a single string is passed and not have that treated as a sequence for names for a MultiIndex, but I'd be concerned that would break code for others.

Copy link
Contributor

Choose a reason for hiding this comment

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

i c
well that definitely is a bug; a string is NOT a list_like so it SHOULD raise on your example (and not try treat the string as a list)

t

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback Just to be clear, are you suggesting that the following code should raise exceptions on both calls to Index? (Right now, the second call succeeds, the first raises)

tl = [(0,0,1),(0,1,0),(1,0,0)]
i1 = pd.Index(tl, name='a')
i1 = pd.Index(tl, name='abc')

If an exception should be raised in both cases, should I create a separate issue for that (and work on a fix as a separate pull request)? (Note - I think I have a fix for the current issue that is clean that doesn't require modifying Index).

keys = Index(clean_keys)
keys.name = name

if len(objs) == 0:
raise ValueError('All objects passed were None')
Expand Down