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

Dr-Irv · 2017-01-11T15:17:59Z

closes BUG: DataFrame.groupby() on tuple column works only when column name is "key" #14848
[x ] tests added / passed
[x ] passes git diff upstream/master | flake8 --diff
[x ] whatsnew entry

This isn't the most elegant fix for the bug, but all the tests pass. There is probably a bigger issue to deal with here, which is to support tuples as an Index (as opposed to a MultiIndex) throughout, but at least this fix clears the bug.

jreback · 2017-01-11T18:59:12Z

this is not going to be acceptable as a fix. way too hacky and special cased. I don't know where the actual issue is, but this needs more investigation.

Dr-Irv · 2017-01-11T19:12:57Z

@jreback I had a feeling you'd say that. I will see if there is a better fix.

jreback · 2017-01-11T19:16:19Z

@Dr-Irv yeah, sorry about that....just want to find the root cause here. thanks!

codecov-io · 2017-01-11T22:22:42Z

Current coverage is 84.76% (diff: 100%)

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

@@             master     #15110   diff @@
==========================================
  Files           145        145          
  Lines         51232      51274    +42   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43420      43460    +40   
- Misses         7812       7814     +2   
  Partials          0          0

Powered by Codecov. Last update 0fe491d...c18c6cb

…ining all tuples

Dr-Irv · 2017-01-12T12:01:17Z

@jreback Found the root cause, so this should be good to go.

jreback · 2017-01-12T13:25:00Z

pandas/tests/groupby/test_groupby.py

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


comment inside the test

jreback · 2017-01-12T13:25:09Z

pandas/tests/groupby/test_groupby.py

+        df2 = df1.rename(columns={'k': 'key'})
+        des1 = df1.groupby('k').describe()
+        des2 = df2.groupby('key').describe()
+        if len(des1) > 0:


why are you doing this conditional?

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

jreback · 2017-01-12T13:25:37Z

pandas/tests/groupby/test_groupby.py

+                         '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()


result = expected =

jreback · 2017-01-12T13:29:41Z

pandas/tools/merge.py

-            keys = Index(clean_keys, name=name)
+            # GH 14848
+            # Don't pass name when creating index (# GH 14252)
+            # So that if keys are tuples, name isn't checked


then this should be fixed inside Index

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

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

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

jreback · 2017-01-12T17:02:28Z

you can prob fix it for this issue is fine

jreback · 2017-01-12T20:35:16Z

pandas/indexes/multi.py

@@ -490,6 +491,11 @@ def _set_names(self, names, level=None, validate=True):
        that it only acts on copies
        """

+        # GH 15110
+        # Don't allow a single string for names in a MultiIndex
+        if names is not None and is_string_like(names):


I would use not is_list_like here (which excludes things like TImestamps and strings), but allows other list-like things

jreback · 2017-01-12T20:35:55Z

pandas/tools/merge.py

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


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

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

hmm

maybe this should be handled in the _get_grouper logic then

where you first figure out what the name actually is

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

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

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

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.

jreback · 2017-01-13T13:52:48Z

doc/source/whatsnew/v0.20.0.txt

@@ -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. Raise `ValueError` if creating an `Index` with tuples and not passing a list of names (:issue:`14848`)


make this in to 2 lines in the whatsnew (first one is the issue number, second is the PR number)

jreback · 2017-01-13T16:52:56Z

ok, lgtm. ping on green.

jreback · 2017-01-13T17:38:18Z

thanks @Dr-Irv !

… as the Index closes pandas-dev#14848 Author: Dr-Irv <[email protected]> Closes pandas-dev#15110 from Dr-Irv/Issue14848 and squashes the following commits: c18c6cb [Dr-Irv] Undo change to merge.py and make whatsnew a 2 line comment. db13c3b [Dr-Irv] Use not is_list_like fbd20f5 [Dr-Irv] Raise error when creating index of tuples with name parameter a string f3a7a21 [Dr-Irv] Changes per jreback requests 9489cb2 [Dr-Irv] BUG: Fix issue pandas-dev#14848 groupby().describe() on indices containing all tuples

jreback added the Groupby label Jan 11, 2017

BUG: Fix issue pandas-dev#14848 groupby().describe() on indices conta…

9489cb2

…ining all tuples

Dr-Irv force-pushed the Issue14848 branch from dfc17d8 to 9489cb2 Compare January 12, 2017 04:38

jreback reviewed Jan 12, 2017

View reviewed changes

jreback requested changes Jan 12, 2017

View reviewed changes

Changes per jreback requests

f3a7a21

Raise error when creating index of tuples with name parameter a string

fbd20f5

jreback reviewed Jan 12, 2017

View reviewed changes

jreback added Bug Indexing Related to indexing on series/frames, not to indexes themselves labels Jan 12, 2017

Use not is_list_like

db13c3b

jreback reviewed Jan 13, 2017

View reviewed changes

Undo change to merge.py and make whatsnew a 2 line comment.

c18c6cb

jreback approved these changes Jan 13, 2017

View reviewed changes

jreback added this to the 0.20.0 milestone Jan 13, 2017

jreback closed this in ab0d236 Jan 13, 2017

Dr-Irv deleted the Issue14848 branch January 23, 2017 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Dr-Irv commented Jan 11, 2017

jreback commented Jan 11, 2017

Dr-Irv commented Jan 11, 2017

jreback commented Jan 11, 2017

codecov-io commented Jan 11, 2017 •

edited

Loading

Dr-Irv commented Jan 12, 2017

jreback Jan 12, 2017

Dr-Irv Jan 12, 2017

jreback Jan 12, 2017 •

edited

Loading

Dr-Irv Jan 12, 2017

jreback Jan 12, 2017

Dr-Irv Jan 12, 2017

jreback Jan 12, 2017

Dr-Irv Jan 12, 2017

jreback Jan 12, 2017

Dr-Irv Jan 12, 2017

jreback commented Jan 12, 2017

jreback Jan 12, 2017

jreback Jan 12, 2017

Dr-Irv Jan 12, 2017

jreback Jan 12, 2017

Dr-Irv Jan 12, 2017

jreback Jan 12, 2017

Dr-Irv Jan 12, 2017

jreback Jan 13, 2017

jreback Jan 13, 2017

jreback commented Jan 13, 2017

jreback commented Jan 13, 2017

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

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

Conversation

Dr-Irv commented Jan 11, 2017

jreback commented Jan 11, 2017

Dr-Irv commented Jan 11, 2017

jreback commented Jan 11, 2017

codecov-io commented Jan 11, 2017 • edited Loading

Current coverage is 84.76% (diff: 100%)

Dr-Irv commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jan 13, 2017

jreback commented Jan 13, 2017

codecov-io commented Jan 11, 2017 •

edited

Loading

jreback Jan 12, 2017 •

edited

Loading