Skip to content

REGR: Prevent indexes that aren't directly backed by numpy from entering libreduction code paths #31238

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

Merged
merged 5 commits into from
Jan 28, 2020
Merged

REGR: Prevent indexes that aren't directly backed by numpy from entering libreduction code paths #31238

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
dea76c1
REGR: Fix GroupBy aggregation with ExtensionArray backed index
jschendel Jan 23, 2020
ed3a9a3
add back _has_complex_internals
jschendel Jan 26, 2020
2efd22b
Merge remote-tracking branch 'upstream/master' into regr-groupby-exte…
TomAugspurger Jan 27, 2020
1018ca9
prevent fast_apply case for _has_complex_internals
jschendel Jan 27, 2020
cc400cd
update comment
jschendel Jan 28, 2020
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
6 changes: 3 additions & 3 deletions pandas/_libs/reduction.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ cdef class _BaseGrouper:
if util.is_array(values) and not values.flags.contiguous:
# e.g. Categorical has no `flags` attribute
values = values.copy()
index = dummy.index.values
index = dummy.index.to_numpy()
Copy link
Member

Choose a reason for hiding this comment

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

for EA cases, we want to avoid going through libreduction altogether

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean? We shouldn't be hitting this code path in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, yeah what does this do to the categorical grouping case. This should be a highly optimized path, is this still?

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean? We shouldn't be hitting this code path in the first place?

Correct. This is called from core.apply.FrameApply.apply_standard after a check:

        if (
            self.result_type in ["reduce", None]
            and not self.dtypes.apply(is_extension_array_dtype).any()
            # Disallow complex_internals since libreduction shortcut
            #  cannot handle MultiIndex
            and not isinstance(self.agg_axis, ABCMultiIndex)
        ):

The MultiIndex check may need to be expanded to include EAs.

I think this is called from one other place in non-test code, so the missing check may belong there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I vaguely recall a _has_complex_... property that indicated with the Index was backed by an ndarray. Was that removed?

Copy link
Member

Choose a reason for hiding this comment

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

self._has_complex_internals was equivalent to to isinstance(self, MultiIndex)

Copy link
Member Author

@jschendel jschendel Jan 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The issue in question doesn't actually go through FrameApply.apply_standard, but rather _aggregate_series_fast, which dispatches through libreduction. The point still applies that we want to avoid EA backed indexes in FrameApply.apply_standard, so I've modified the check to use _has_complex_internals.

if not index.flags.contiguous:
index = index.copy()

Expand Down Expand Up @@ -229,7 +229,7 @@ cdef class SeriesBinGrouper(_BaseGrouper):
self.arr = values
self.typ = series._constructor
self.ityp = series.index._constructor
self.index = series.index.values
self.index = series.index.to_numpy()
self.name = series.name

self.dummy_arr, self.dummy_index = self._check_dummy(dummy)
Expand Down Expand Up @@ -326,7 +326,7 @@ cdef class SeriesGrouper(_BaseGrouper):
self.arr = values
self.typ = series._constructor
self.ityp = series.index._constructor
self.index = series.index.values
self.index = series.index.to_numpy()
self.name = series.name

self.dummy_arr, self.dummy_index = self._check_dummy(dummy)
Expand Down
11 changes: 11 additions & 0 deletions pandas/tests/groupby/aggregate/test_aggregate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,17 @@ def test_func_duplicates_raises():
df.groupby("A").agg(["min", "min"])


@pytest.mark.parametrize(
"index", [pd.CategoricalIndex(list("abc")), pd.interval_range(0, 3)]
)
def test_agg_with_ea_backed_index(index):
# GH 31223
df = DataFrame({"group": [1, 1, 2], "value": [0, 1, 0]}, index=index)
result = df.groupby("group").agg({"value": Series.nunique})
expected = DataFrame({"group": [1, 2], "value": [2, 1]}).set_index("group")
tm.assert_frame_equal(result, expected)


class TestNamedAggregationSeries:
def test_series_named_agg(self):
df = pd.Series([1, 2, 3, 4])
Expand Down