Skip to content
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

Fix handling of values=None in pylibcudf GroupBy.get_groups #14998

Merged
merged 5 commits into from
Feb 8, 2024
Merged

Fix handling of values=None in pylibcudf GroupBy.get_groups #14998

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
950713e
Fix handling of values=None in pylibcudf GroupBy.get_groups
shwina Feb 7, 2024
0f4849d
Doc fixes
shwina Feb 7, 2024
e60a054
Review
shwina Feb 7, 2024
2088742
Reorder grouped outputs
shwina Feb 8, 2024
b2cc741
Fixes
shwina Feb 8, 2024
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
21 changes: 13 additions & 8 deletions python/cudf/cudf/_lib/pylibcudf/groupby.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,8 @@ cdef class GroupBy:
Parameters
----------
values : Table, optional
The columns to get group labels for. If not specified, the group
labels for the group keys are returned.
The columns to get group labels for. If not specified,
an empty table is returned for the group values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we return an empty table or should we not return anything at all? On one hand I tend to prefer the return type not changing based on arguments, on the other I would prefer not creating an extra object for no reason and there is precedent for this kind of thing in numpy APIs (like np.unique).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No preference. I changed t oreturn None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also saved a reference to keys table.


Returns
-------
Expand All @@ -268,11 +268,16 @@ cdef class GroupBy:
cdef groups c_groups
if values:
c_groups = dereference(self.c_obj).get_groups(values.view())
return (
Table.from_libcudf(move(c_groups.keys)),
Table.from_libcudf(move(c_groups.values)),
c_groups.offsets,
)
else:
# c_groups.values is nullptr
c_groups = dereference(self.c_obj).get_groups()

return (
Table.from_libcudf(move(c_groups.keys)),
Table.from_libcudf(move(c_groups.values)),
c_groups.offsets,
)
return (
Table.from_libcudf(move(c_groups.keys)),
Table([]),
c_groups.offsets,
)
7 changes: 7 additions & 0 deletions python/cudf/cudf/tests/test_groupby.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3756,3 +3756,10 @@ def test_group_by_value_counts_with_count_column():
df = cudf.DataFrame({"a": [1, 5, 3], "count": [2, 5, 2]})
with pytest.raises(ValueError):
df.groupby("a", as_index=False).value_counts()


def test_groupby_internal_groups_empty(gdf):
# test that we don't segfault when calling the internal
# .groups() method with an empty list:
gb = gdf.groupby("y")._groupby
gb.groups([])
Loading