Skip to content

BUG: Fix remaining cases of groupby(...).transform with dropna=True #46367

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.

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 24 commits into from
Mar 22, 2022
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
2143c59
BUG: Correct results for groupby(...).transform with null keys
rhshadrach Feb 4, 2022
deb3edb
Merge branch 'main' of https://github.com/pandas-dev/pandas into tran…
rhshadrach Feb 5, 2022
04a2a6b
Series tests
rhshadrach Feb 9, 2022
ad7bf3e
Remove reliance on Series
rhshadrach Feb 10, 2022
39e1438
Merge branch 'main' of https://github.com/pandas-dev/pandas into tran…
rhshadrach Feb 10, 2022
7a4a99b
Merge branch 'main' of https://github.com/pandas-dev/pandas into tran…
rhshadrach Feb 11, 2022
f234a89
Merge branch 'main' of https://github.com/pandas-dev/pandas into tran…
rhshadrach Feb 28, 2022
d1eeb65
Merge cleanup - WIP
rhshadrach Feb 28, 2022
e57025d
Merge branch 'transform_dropna' of https://github.com/rhshadrach/pand…
rhshadrach Mar 2, 2022
be2f45c
Fixes for ngroup
rhshadrach Mar 2, 2022
8e5df01
Merge branch 'main' of https://github.com/pandas-dev/pandas into tran…
rhshadrach Mar 2, 2022
2d79a0f
name in some cases
rhshadrach Mar 2, 2022
e2f3080
Merge branch 'main' of https://github.com/pandas-dev/pandas into tran…
rhshadrach Mar 15, 2022
d9da864
Fixups for ngroup
rhshadrach Mar 15, 2022
b973af0
Fixups
rhshadrach Mar 15, 2022
67ffb60
Fixups
rhshadrach Mar 15, 2022
e31041d
Fixups
rhshadrach Mar 15, 2022
60e60b0
Merge branch 'main' of https://github.com/pandas-dev/pandas into tran…
rhshadrach Mar 18, 2022
04293a7
PR feedback
rhshadrach Mar 18, 2022
9a6dc3c
whatsnew improvements
rhshadrach Mar 19, 2022
d7bb828
Merge branch 'main' of https://github.com/pandas-dev/pandas into tran…
rhshadrach Mar 19, 2022
6571291
merge cleanup
rhshadrach Mar 19, 2022
a8f524b
type-hint fixup
rhshadrach Mar 22, 2022
2a02de0
Merge branch 'main' of https://github.com/pandas-dev/pandas into tran…
rhshadrach Mar 22, 2022
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
24 changes: 16 additions & 8 deletions doc/source/whatsnew/v1.5.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -82,32 +82,40 @@ did not have the same index as the input.

.. code-block:: ipython

In [3]: df.groupby('a', dropna=True).transform('sum')
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe be worth commenting on each of these what is changing (e.g. like you did for [3])

Out[3]:
b
0 5
1 5
2 5

In [3]: df.groupby('a', dropna=True).transform(lambda x: x.sum())
Out[3]:
b
0 5
1 5

In [3]: df.groupby('a', dropna=True).transform('ffill')
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment here on the casting of the fill value

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're saying to explain why the old result comes out as -9223372036854775808, which is np.nan when interpreted as an integer. Will do.

Out[3]:
b
0 2
1 3
2 -9223372036854775808

In [3]: df.groupby('a', dropna=True).transform(lambda x: x)
Out[3]:
b
0 2
1 3

In [3]: df.groupby('a', dropna=True).transform('sum')
Out[3]:
b
0 5
1 5
2 5

*New behavior*:

.. ipython:: python

df.groupby('a', dropna=True).transform('sum')
df.groupby('a', dropna=True).transform(lambda x: x.sum())
df.groupby('a', dropna=True).transform('ffill')
df.groupby('a', dropna=True).transform(lambda x: x)
df.groupby('a', dropna=True).transform('sum')

.. _whatsnew_150.notable_bug_fixes.notable_bug_fix2:

Expand Down
2 changes: 1 addition & 1 deletion pandas/core/groupby/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ class OutputKey:
"mean",
"median",
"min",
"ngroup",
"nth",
"nunique",
"prod",
Expand Down Expand Up @@ -113,6 +112,7 @@ def maybe_normalize_deprecated_kernels(kernel):
"diff",
"ffill",
"fillna",
"ngroup",
"pad",
"pct_change",
"rank",
Expand Down
30 changes: 25 additions & 5 deletions pandas/core/groupby/groupby.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class providing the base-class of operations.
)
from pandas.util._exceptions import find_stack_level

from pandas.core.dtypes.cast import ensure_dtype_can_hold_na
from pandas.core.dtypes.common import (
is_bool_dtype,
is_datetime64_dtype,
Expand Down Expand Up @@ -950,7 +951,18 @@ def curried(x):
if name in base.plotting_methods:
return self.apply(curried)

return self._python_apply_general(curried, self._obj_with_exclusions)
result = self._python_apply_general(curried, self._obj_with_exclusions)

if result.ndim == 1 and self.obj.ndim == 1 and result.name != self.obj.name:
Copy link
Member

Choose a reason for hiding this comment

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

result.name != self.obj.name will be wrong with np.nan (and will raise with pd.NA)

Copy link
Member Author

@rhshadrach rhshadrach Mar 18, 2022

Choose a reason for hiding this comment

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

Doh, thanks. This highlights the fact that we shouldn't be fixing this here, but rather the logic in wrapping the apply results. I didn't want this PR spreading out to that method though. I've opened #46369 for this; I'm thinking here we ignore testing the name, and fix properly.

# apply sets the name on each group as the key; if there is one
# group this name will come through on Series results
result.name = None

if self.grouper.has_dropped_na and name in base.transformation_kernels:
# result will have dropped rows due to nans, fill with null
# and ensure index is ordered same as the input
result = self._set_result_index_ordered(result)
return result

wrapper.__name__ = name
return wrapper
Expand Down Expand Up @@ -2608,7 +2620,11 @@ def blk_func(values: ArrayLike) -> ArrayLike:
# then there will be no -1s in indexer, so we can use
# the original dtype (no need to ensure_dtype_can_hold_na)
if isinstance(values, np.ndarray):
out = np.empty(values.shape, dtype=values.dtype)
dtype = values.dtype
if self.grouper.has_dropped_na:
# dropped null groups give rise to nan in the result
dtype = ensure_dtype_can_hold_na(values.dtype)
out = np.empty(values.shape, dtype=dtype)
else:
out = type(values)._empty(values.shape, dtype=values.dtype)

Expand Down Expand Up @@ -3114,9 +3130,13 @@ def ngroup(self, ascending: bool = True):
"""
with self._group_selection_context():
index = self._selected_obj.index
result = self._obj_1d_constructor(
self.grouper.group_info[0], index, dtype=np.int64
)
comp_ids = self.grouper.group_info[0]
if self.grouper.has_dropped_na:
comp_ids = np.where(comp_ids == -1, np.nan, comp_ids)
dtype = np.float64
else:
dtype = np.int64
result = self._obj_1d_constructor(comp_ids, index, dtype=dtype)
if not ascending:
result = self.ngroups - 1 - result
return result
Expand Down
22 changes: 19 additions & 3 deletions pandas/core/groupby/ops.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
from pandas.util._decorators import cache_readonly

from pandas.core.dtypes.cast import (
ensure_dtype_can_hold_na,
maybe_cast_pointwise_result,
maybe_downcast_to_dtype,
)
Expand Down Expand Up @@ -104,7 +105,8 @@ class WrappedCythonOp:
# back to the original dtype.
cast_blocklist = frozenset(["rank", "count", "size", "idxmin", "idxmax"])

def __init__(self, kind: str, how: str):
def __init__(self, grouper: BaseGrouper, kind: str, how: str):
self.grouper = grouper
self.kind = kind
self.how = how

Expand Down Expand Up @@ -194,7 +196,9 @@ def _get_cython_vals(self, values: np.ndarray) -> np.ndarray:
values = ensure_float64(values)

elif values.dtype.kind in ["i", "u"]:
if how in ["add", "var", "prod", "mean", "ohlc"]:
if how in ["add", "var", "prod", "mean", "ohlc"] or (
self.kind == "transform" and self.grouper.has_dropped_na
):
# result may still include NaN, so we have to cast
values = ensure_float64(values)

Expand Down Expand Up @@ -260,6 +264,9 @@ def _get_output_shape(self, ngroups: int, values: np.ndarray) -> Shape:
def _get_out_dtype(self, dtype: np.dtype) -> np.dtype:
how = self.how

if self.kind == "transform" and self.grouper.has_dropped_na:
dtype = ensure_dtype_can_hold_na(dtype)

Copy link
Member

Choose a reason for hiding this comment

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

ATM we do something similar to this L578-L591. could this be rolled into that? (admittedly thats a bit messy so this may just b A Better Solution)

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems better to me to determine the result dtype upfront, rather than cast after, as much as possible.

Copy link
Member

Choose a reason for hiding this comment

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

i dont have an opinion on before vs after so am happy to defer to you, but do strongly prefer doing things Just One Way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense - I didn't realize that my change to _get_cython_vals actually makes values float, and so this is indeed not necessary at all.

if how == "rank":
out_dtype = "float64"
else:
Expand Down Expand Up @@ -462,6 +469,11 @@ def _cython_op_ndim_compat(
# otherwise we have OHLC
return res.T

if self.kind == "transform" and self.grouper.has_dropped_na:
mask = comp_ids == -1
# make mask 2d
Copy link
Member

Choose a reason for hiding this comment

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

can this be rolled into the mask-reshaping done above?

Copy link
Member Author

@rhshadrach rhshadrach Mar 16, 2022

Choose a reason for hiding this comment

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

This block turned out to not be necessary. It will be removed.

mask = mask[None, :]

return self._call_cython_op(
values,
min_count=min_count,
Expand Down Expand Up @@ -592,6 +604,10 @@ def _call_cython_op(

result = result.T

if self.how == "rank" and self.grouper.has_dropped_na:
# TODO: Wouldn't need this if group_rank supported mask
Copy link
Member

Choose a reason for hiding this comment

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

would this be a matter of supporting a mask arg in the libgroupby function, or would this be just for Nullable dtype? bc i have a branch on deck that does the former

Copy link
Member Author

Choose a reason for hiding this comment

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

The former - I believe rank had to be singled out here because it was the one transform that didn't support a mask arg.

Copy link
Member

Choose a reason for hiding this comment

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

@rhshadrach mask just got added to group_rank, but commenting this out causes a few failures. is more needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This comment was incorrect; opened #46953

result = np.where(comp_ids < 0, np.nan, result)

if self.how not in self.cast_blocklist:
# e.g. if we are int64 and need to restore to datetime64/timedelta64
# "rank" is the only member of cast_blocklist we get here
Expand Down Expand Up @@ -969,7 +985,7 @@ def _cython_operation(
"""
assert kind in ["transform", "aggregate"]

cy_op = WrappedCythonOp(kind=kind, how=how)
cy_op = WrappedCythonOp(grouper=self, kind=kind, how=how)
Copy link
Member Author

Choose a reason for hiding this comment

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

@jbrockmendel - It would suffice to pass self.has_dropped_na instead of the full grouper here, wasn't sure if there was a preference one way or the others.

Copy link
Member

Choose a reason for hiding this comment

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

I'd much rather just has_dropped_na be part of WrappedCythonOop's state


ids, _, _ = self.group_info
ngroups = self.ngroups
Expand Down
11 changes: 10 additions & 1 deletion pandas/tests/apply/test_frame_transform.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ def test_transform_bad_dtype(op, frame_or_series, request):
raises=ValueError, reason="GH 40418: rank does not raise a TypeError"
)
)
elif op == "ngroup":
request.node.add_marker(
pytest.mark.xfail(raises=ValueError, reason="ngroup not valid for NDFrame")
)

obj = DataFrame({"A": 3 * [object]}) # DataFrame that will fail on most transforms
obj = tm.get_obj(obj, frame_or_series)
Expand All @@ -157,9 +161,14 @@ def test_transform_bad_dtype(op, frame_or_series, request):


@pytest.mark.parametrize("op", frame_kernels_raise)
def test_transform_partial_failure_typeerror(op):
def test_transform_partial_failure_typeerror(request, op):
# GH 35964

if op == "ngroup":
request.node.add_marker(
pytest.mark.xfail(raises=ValueError, reason="ngroup not valid for NDFrame")
)

# Using object makes most transform kernels fail
df = DataFrame({"A": 3 * [object], "B": [1, 2, 3]})

Expand Down
14 changes: 12 additions & 2 deletions pandas/tests/apply/test_str.py
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,12 @@ def test_agg_cython_table_transform_frame(df, func, expected, axis):


@pytest.mark.parametrize("op", series_transform_kernels)
def test_transform_groupby_kernel_series(string_series, op):
def test_transform_groupby_kernel_series(request, string_series, op):
# GH 35964
if op == "ngroup":
request.node.add_marker(
pytest.mark.xfail(raises=ValueError, reason="ngroup not valid for NDFrame")
)
# TODO(2.0) Remove after pad/backfill deprecation enforced
op = maybe_normalize_deprecated_kernels(op)
args = [0.0] if op == "fillna" else []
Expand All @@ -255,9 +259,15 @@ def test_transform_groupby_kernel_series(string_series, op):


@pytest.mark.parametrize("op", frame_transform_kernels)
def test_transform_groupby_kernel_frame(axis, float_frame, op):
def test_transform_groupby_kernel_frame(request, axis, float_frame, op):
# TODO(2.0) Remove after pad/backfill deprecation enforced
op = maybe_normalize_deprecated_kernels(op)

if op == "ngroup":
request.node.add_marker(
pytest.mark.xfail(raises=ValueError, reason="ngroup not valid for NDFrame")
)

# GH 35964

args = [0.0] if op == "fillna" else []
Expand Down
4 changes: 3 additions & 1 deletion pandas/tests/groupby/test_rank.py
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,8 @@ def test_non_unique_index():
)
result = df.groupby([df.index, "A"]).value.rank(ascending=True, pct=True)
expected = Series(
[1.0] * 4, index=[pd.Timestamp("20170101", tz="US/Eastern")] * 4, name="value"
[1.0, 1.0, 1.0, np.nan],
index=[pd.Timestamp("20170101", tz="US/Eastern")] * 4,
name="value",
)
tm.assert_series_equal(result, expected)
Loading