Skip to content

BUG: fixed issue with mixed type groupby aggregate #17003

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

BUG: fixed issue with mixed type groupby aggregate #17003

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4c9e32d
BUG: fixed issue with mixed type groupby aggregate
Jul 18, 2017
a58de54
Addressing review comments from PR issue16916
Jul 19, 2017
dbbb0d4
Merge branch 'master' into issue16916
Jul 19, 2017
7b24ee1
Merge branch 'master' of https://github.com/pandas-dev/pandas into is…
Jul 21, 2017
a7a20d5
DOC: Fix incorrect merge conflict
Jul 28, 2017
34ca8d8
used safe_sort in groupby.py
Aug 4, 2017
ba57c26
Merge branch 'master' of https://github.com/pandas-dev/pandas into is…
Aug 4, 2017
b634bc9
re-added bug fix at line 281
Aug 4, 2017
89c585b
DOC: Correct release notes
ElDeveloper Aug 6, 2017
0391dfd
Merge pull request #1 from ElDeveloper/issue16916
Aug 6, 2017
4a2dfba
Merge branch 'master' into issue16916
Sep 26, 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
7 changes: 6 additions & 1 deletion pandas/core/groupby.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2883,7 +2883,12 @@ def aggregate(self, func_or_funcs, *args, **kwargs):
except Exception:
result = self._aggregate_named(func_or_funcs, *args, **kwargs)

index = Index(sorted(result), name=self.grouper.names[0])
# mixed types fail to sort
try:
values = sorted(result)
except TypeError:
values = result
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather

# put at top
from pandas.core.agorithms

index = Index(safe_sort(result), name=self.grouper.names[0])

Copy link
Author

Choose a reason for hiding this comment

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

I tried this change and it caused 4 tests in the full test suite to fail. Those tests were TestDatetimeIndex.test_resample_categorical_data_with_timedeltaindex, TestGroupByAggregate.test_mixed_type_grouping (the test that I wrote), TestGroupBy.test_basic, and test_compressed_urls[python-explicit-gzip-.gz]
Also, I noticed that safe_sort returns all objects as strings, even if the input includes numeric types.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this is an embedded issue with safe_sort, fixed here: #17034. you can rebase on top (or wait for me to merge this).

Copy link
Author

Choose a reason for hiding this comment

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

I tried this and it resulted in the same tests failing, not sure what's going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

did u rebase

Copy link
Author

Choose a reason for hiding this comment

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

I did 'git pull upstream master'

Copy link
Contributor

Choose a reason for hiding this comment

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

you need to use safe_sort here with the latest changed.

git pull upstream origin/master

index = Index(values, name=self.grouper.names[0])
ret = Series(result, index=index)

if not self.as_index: # pragma: no cover
Expand Down
12 changes: 12 additions & 0 deletions pandas/tests/groupby/test_aggregate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,3 +892,15 @@ def test_sum_uint64_overflow(self):
expected.index.name = 0
result = df.groupby(0).sum()
tm.assert_frame_equal(result, expected)

def test_mixed_type_grouping(self):
Copy link
Member

@gfyoung gfyoung Jul 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Refer the issue number beneath the function definition (e.g. "see gh-19616")

X = pd.DataFrame(data=[[[1, 1], [2, 2], [3, 3]],
Copy link
Contributor

Choose a reason for hiding this comment

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

expected =

[[1, 1], [2, 2], [3, 3]]],
columns=['X', 'Y', 'Z'],
index=pd.Index(data=[2, 'g1'], name='grouping'))

S = pd.DataFrame(data=[[1, 2, 3], [1, 2, 3], [1, 2, 3], [1, 2, 3]],
Copy link
Contributor

Choose a reason for hiding this comment

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

df =

columns=list('XYZ'), index=list('qwer'))
S['grouping'] = ['g1', 'g1', 2, 2]
T = S.groupby('grouping').aggregate(lambda x: x.tolist())
Copy link
Contributor

Choose a reason for hiding this comment

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

result = df.groupby(....)

tm.assert_frame_equal(T, X)
Copy link
Contributor

Choose a reason for hiding this comment

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

tm.assert_frame_equal(result, expected)