From e0d6b657d5b53fc5ae340c5e8877c490e59d4e9f Mon Sep 17 00:00:00 2001 From: galipremsagar Date: Thu, 18 Jun 2026 21:19:04 +0000 Subject: [PATCH 1/5] fix --- .../cudf/cudf/core/_internals/aggregation.py | 20 +++- python/cudf/cudf/core/groupby/groupby.py | 31 ++++++- .../pandas/scripts/pandas-testing-plugin.py | 92 ------------------- .../cudf/tests/groupby/test_reductions.py | 42 ++++++++- 4 files changed, 86 insertions(+), 99 deletions(-) diff --git a/python/cudf/cudf/core/_internals/aggregation.py b/python/cudf/cudf/core/_internals/aggregation.py index ef2c7818620e..54260a571bb5 100644 --- a/python/cudf/cudf/core/_internals/aggregation.py +++ b/python/cudf/cudf/core/_internals/aggregation.py @@ -1,4 +1,4 @@ -# SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION. +# SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 from __future__ import annotations @@ -152,15 +152,25 @@ def unique(cls) -> Self: ) @classmethod - def first(cls) -> Self: + def first(cls, skipna: bool = True) -> Self: return cls( - plc.aggregation.nth_element(0, plc.types.NullPolicy.EXCLUDE) + plc.aggregation.nth_element( + 0, + plc.types.NullPolicy.EXCLUDE + if skipna + else plc.types.NullPolicy.INCLUDE, + ) ) @classmethod - def last(cls) -> Self: + def last(cls, skipna: bool = True) -> Self: return cls( - plc.aggregation.nth_element(-1, plc.types.NullPolicy.EXCLUDE) + plc.aggregation.nth_element( + -1, + plc.types.NullPolicy.EXCLUDE + if skipna + else plc.types.NullPolicy.INCLUDE, + ) ) @classmethod diff --git a/python/cudf/cudf/core/groupby/groupby.py b/python/cudf/cudf/core/groupby/groupby.py index df78d00c82cf..c3d69ba7c2c5 100644 --- a/python/cudf/cudf/core/groupby/groupby.py +++ b/python/cudf/cudf/core/groupby/groupby.py @@ -1412,7 +1412,14 @@ def _reduce( skipna=kwargs.get("skipna", True), min_count=min_count ) - result = self.agg(op) + agg_op: str | _FirstLastAggSpec = op + if op in {"first", "last"} and not kwargs.get("skipna", True): + # ``first``/``last`` default to dropping nulls (skipna=True). With + # ``skipna=False`` the actual first/last element of each group is + # returned even when it is null, matching pandas. + agg_op = _FirstLastAggSpec(op, skipna=False) + + result = self.agg(agg_op) if min_count and min_count > 0: counts = self.agg("count") result = result.where(counts >= min_count, None) @@ -4138,6 +4145,28 @@ def copy(self, deep=True): return out +class _FirstLastAggSpec: + """Callable aggregation spec for groupby ``first``/``last``. + + Lets :meth:`GroupBy._reduce` thread ``skipna`` to + :meth:`Aggregation.first`/:meth:`Aggregation.last` through + ``make_aggregation``'s callable path. ``__str__``/``__name__`` report the + op name so aggregation-validity checks and result-column naming behave + exactly as they do for the plain ``"first"``/``"last"`` string specs. + """ + + def __init__(self, op: str, skipna: bool) -> None: + self._op = op + self._skipna = skipna + self.__name__ = op + + def __call__(self, agg): + return getattr(agg, self._op)(skipna=self._skipna) + + def __str__(self) -> str: + return self._op + + def _is_multi_agg(aggs): """ Returns True if more than one aggregation is performed diff --git a/python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py b/python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py index 05b67c2d2b92..20ccf07783fe 100644 --- a/python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py +++ b/python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py @@ -2428,98 +2428,6 @@ def pytest_unconfigure(config): "tests/groupby/test_reductions.py::test_any": 'AssertionError: Column name="B" are different', "tests/groupby/test_reductions.py::test_basic_aggregations[float32]": "AssertionError: Attributes of Series are different", "tests/groupby/test_reductions.py::test_basic_aggregations[int32]": "AssertionError: Attributes of Series are different", - "tests/groupby/test_reductions.py::test_first_last_skipna[Float32-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Float32-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Float32-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Float32-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Float64-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Float64-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Float64-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Float64-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int16-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int16-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int16-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int16-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int32-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int32-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int32-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int32-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int64-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int64-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int64-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int64-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int8-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int8-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int8-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[Int8-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt16-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt16-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt16-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt16-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt32-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt32-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt32-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt32-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt64-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt64-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt64-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt64-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt8-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt8-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt8-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[UInt8-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[double[pyarrow]-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[double[pyarrow]-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[double[pyarrow]-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[double[pyarrow]-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float32-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float32-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float32-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float32-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float64-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float64-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float64-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float64-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float[pyarrow]-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float[pyarrow]-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float[pyarrow]-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[float[pyarrow]-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int16[pyarrow]-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int16[pyarrow]-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int16[pyarrow]-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int16[pyarrow]-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int32[pyarrow]-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int32[pyarrow]-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int32[pyarrow]-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int32[pyarrow]-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int64[pyarrow]-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int64[pyarrow]-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int64[pyarrow]-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int64[pyarrow]-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int8[pyarrow]-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int8[pyarrow]-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int8[pyarrow]-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[int8[pyarrow]-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint16[pyarrow]-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint16[pyarrow]-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint16[pyarrow]-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint16[pyarrow]-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint32[pyarrow]-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint32[pyarrow]-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint32[pyarrow]-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint32[pyarrow]-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint64[pyarrow]-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint64[pyarrow]-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint64[pyarrow]-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint64[pyarrow]-True-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint8[pyarrow]-False-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint8[pyarrow]-False-False-last]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint8[pyarrow]-True-False-first]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_first_last_skipna[uint8[pyarrow]-True-False-last]": "TODO: Add a reason for failure", "tests/groupby/test_reductions.py::test_groupby_bool_aggs[True-any-vals12]": 'AssertionError: Column name="val" are different', "tests/groupby/test_reductions.py::test_groupby_mean_no_overflow": "TODO: Add a reason for failure", "tests/groupby/test_reductions.py::test_groupby_sum_mincount_boolean[0]": "TODO: Add a reason for failure", diff --git a/python/cudf/cudf/tests/groupby/test_reductions.py b/python/cudf/cudf/tests/groupby/test_reductions.py index d0fdac83934a..618b684cb734 100644 --- a/python/cudf/cudf/tests/groupby/test_reductions.py +++ b/python/cudf/cudf/tests/groupby/test_reductions.py @@ -1,4 +1,4 @@ -# SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION. +# SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 import numpy as np @@ -1387,6 +1387,46 @@ def test_groupby_reduce_min_count(op, min_count): assert_eq(expect, got) +@pytest.mark.parametrize( + "dtype", + ["Int64", "UInt32", "Float64", "int64[pyarrow]", "double[pyarrow]"], +) +@pytest.mark.parametrize("op", ["first", "last"]) +@pytest.mark.parametrize("skipna", [True, False]) +@pytest.mark.parametrize("sort", [True, False]) +def test_groupby_first_last_skipna(dtype, op, skipna, sort): + # first/last honor skipna: with skipna=True nulls are dropped, with + # skipna=False the actual first/last element of each group is returned + # even when it is null (previously skipna=False was ignored). + pdf = pd.DataFrame( + { + "a": [2, 1, 1, 2, 3, 3], + "b": pd.array([None, 3, None, 4, None, None], dtype=dtype), + "c": pd.array([None, 30, None, 40, None, None], dtype=dtype), + } + ) + gdf = cudf.from_pandas(pdf) + with cudf.option_context("mode.pandas_compatible", True): + got = getattr(gdf.groupby("a", sort=sort), op)(skipna=skipna) + expect = getattr(pdf.groupby("a", sort=sort), op)(skipna=skipna) + assert_eq(expect, got) + + +@pytest.mark.parametrize("op", ["first", "last"]) +@pytest.mark.parametrize("skipna", [True, False]) +def test_groupby_series_first_last_skipna(op, skipna): + psr = pd.Series( + pd.array([None, 3, None, 4, None, None], dtype="Float64"), name="b" + ) + pkeys = pd.Series([2, 1, 1, 2, 3, 3], name="a") + gsr = cudf.from_pandas(psr) + gkeys = cudf.from_pandas(pkeys) + with cudf.option_context("mode.pandas_compatible", True): + got = getattr(gsr.groupby(gkeys), op)(skipna=skipna) + expect = getattr(psr.groupby(pkeys), op)(skipna=skipna) + assert_eq(expect, got) + + @pytest.mark.parametrize("min_count", [0, 2, 3]) def test_groupby_series_reduce_min_count(min_count): psr = pd.Series([1.0, 2.0, 3.0, 4.0, 5.0]) From 1b20000caf8075edec18a85236919f8e064b3cb6 Mon Sep 17 00:00:00 2001 From: galipremsagar Date: Thu, 18 Jun 2026 21:52:12 +0000 Subject: [PATCH 2/5] fix --- python/cudf/cudf/core/groupby/groupby.py | 55 +++++++++++++++++-- .../pandas/scripts/pandas-testing-plugin.py | 22 +------- .../cudf/tests/groupby/test_reductions.py | 33 +++++++++++ 3 files changed, 84 insertions(+), 26 deletions(-) diff --git a/python/cudf/cudf/core/groupby/groupby.py b/python/cudf/cudf/core/groupby/groupby.py index c3d69ba7c2c5..8308c0bde22b 100644 --- a/python/cudf/cudf/core/groupby/groupby.py +++ b/python/cudf/cudf/core/groupby/groupby.py @@ -77,6 +77,20 @@ # different dtypes. These strings must be elements of the AggregationKind enum. # The libcudf infrastructure exists for "COLLECT" support on # categoricals, but the dtype support in python does not. +# Reductions whose result for a group becomes null when that group contains +# any null value and ``skipna=False`` (libcudf otherwise always drops nulls). +_NULL_PROPAGATING_REDUCTIONS = { + "sum", + "prod", + "product", + "mean", + "median", + "var", + "std", + "min", + "max", +} + _CATEGORICAL_AGGS = {"COUNT", "NUNIQUE", "SIZE", "UNIQUE"} _STRING_AGGS = { "COLLECT", @@ -1317,7 +1331,9 @@ def agg(self, func=None, *args, engine=None, engine_kwargs=None, **kwargs): return result - def _wrap_idxmin_idxmax(self, result: DataFrame | Series, *, skipna: bool): + def _wrap_idxmin_idxmax( + self, result: DataFrame | Series, *, skipna: bool, how: str + ): # libcudf's idxmin/idxmax return the integer row-position of the # min/max element within each group (null if the group's values were # all NA). pandas instead returns the *label* of that row taken from @@ -1326,6 +1342,11 @@ def _wrap_idxmin_idxmax(self, result: DataFrame | Series, *, skipna: bool): from cudf.core.multiindex import MultiIndex from cudf.core.series import Series + if not skipna: + # pandas does not support positional idxmin/idxmax with + # skipna=False (it cannot represent "the label of a NA"). + raise ValueError(f"{how} with skipna=False") + key_names = set(self.grouping.names) if result.ndim == 2: value_items = [ @@ -1412,14 +1433,36 @@ def _reduce( skipna=kwargs.get("skipna", True), min_count=min_count ) + skipna = kwargs.get("skipna", True) agg_op: str | _FirstLastAggSpec = op - if op in {"first", "last"} and not kwargs.get("skipna", True): + if op in {"first", "last"} and not skipna: # ``first``/``last`` default to dropping nulls (skipna=True). With # ``skipna=False`` the actual first/last element of each group is # returned even when it is null, matching pandas. agg_op = _FirstLastAggSpec(op, skipna=False) result = self.agg(agg_op) + if op in _NULL_PROPAGATING_REDUCTIONS and not skipna: + # libcudf reductions always drop nulls. With ``skipna=False`` a + # group containing any null in a column yields a null result for + # that (group, column), matching pandas. A (group, column) is + # all-non-null when its non-null count equals the group size + # (``size()`` is used instead of the ``size`` aggregation because + # the latter is unsupported for string columns). + from cudf.core.dataframe import DataFrame + + non_null_counts = self.agg("count") + group_sizes = self.size() + if isinstance(result, DataFrame): + all_non_null = DataFrame( + { + name: non_null_counts[name] == group_sizes + for name in non_null_counts._column_names + } + ) + else: + all_non_null = non_null_counts == group_sizes + result = result.where(all_non_null, None) if min_count and min_count > 0: counts = self.agg("count") result = result.where(counts >= min_count, None) @@ -3502,7 +3545,7 @@ def idxmin( **kwargs: Any, ) -> DataFrame: result = self._reduce("idxmin", numeric_only=numeric_only) - return self._wrap_idxmin_idxmax(result, skipna=skipna) + return self._wrap_idxmin_idxmax(result, skipna=skipna, how="idxmin") def idxmax( self, @@ -3512,7 +3555,7 @@ def idxmax( **kwargs: Any, ) -> DataFrame: result = self._reduce("idxmax", numeric_only=numeric_only) - return self._wrap_idxmin_idxmax(result, skipna=skipna) + return self._wrap_idxmin_idxmax(result, skipna=skipna, how="idxmax") def value_counts( self, @@ -3827,13 +3870,13 @@ def idxmin( self, skipna: bool = True, min_count: int = 0, **kwargs: Any ) -> Series: result = self._reduce("idxmin") - return self._wrap_idxmin_idxmax(result, skipna=skipna) + return self._wrap_idxmin_idxmax(result, skipna=skipna, how="idxmin") def idxmax( self, skipna: bool = True, min_count: int = 0, **kwargs: Any ) -> Series: result = self._reduce("idxmax") - return self._wrap_idxmin_idxmax(result, skipna=skipna) + return self._wrap_idxmin_idxmax(result, skipna=skipna, how="idxmax") @property def dtype(self) -> pd.Series: diff --git a/python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py b/python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py index 20ccf07783fe..d593227cc155 100644 --- a/python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py +++ b/python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py @@ -2431,26 +2431,10 @@ def pytest_unconfigure(config): "tests/groupby/test_reductions.py::test_groupby_bool_aggs[True-any-vals12]": 'AssertionError: Column name="val" are different', "tests/groupby/test_reductions.py::test_groupby_mean_no_overflow": "TODO: Add a reason for failure", "tests/groupby/test_reductions.py::test_groupby_sum_mincount_boolean[0]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_idxmin_idxmax_extremes_skipna[False-float-idxmax]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_idxmin_idxmax_extremes_skipna[False-float-idxmin]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_idxmin_idxmax_extremes_skipna[False-float32-idxmax]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_idxmin_idxmax_extremes_skipna[False-float32-idxmin]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_idxmin_idxmax_extremes_skipna[False-float64-idxmax]": "TODO: Add a reason for failure", - "tests/groupby/test_reductions.py::test_idxmin_idxmax_extremes_skipna[False-float64-idxmin]": "TODO: Add a reason for failure", "tests/groupby/test_reductions.py::test_masked_kleene_logic[any-False-data2]": "AssertionError: Series NA mask are different", "tests/groupby/test_reductions.py::test_masked_kleene_logic[any-False-data3]": "AssertionError: Series NA mask are different", "tests/groupby/test_reductions.py::test_mean_numeric_only_validates_bool": "Failed: DID NOT RAISE ", - "tests/groupby/test_reductions.py::test_mean_skipna[False-values0-float64-float64]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_mean_skipna[False-values3-timedelta64[ns]-timedelta64[ns]]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_multifunc_skipna[False-max-values32-float64-float64]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_multifunc_skipna[False-max-values35-timedelta64[ns]-timedelta64[ns]]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_multifunc_skipna[False-max-values36-datetime64[ns]-datetime64[ns]]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_multifunc_skipna[False-median-values40-float64-float64]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_multifunc_skipna[False-min-values24-float64-float64]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_multifunc_skipna[False-min-values27-timedelta64[ns]-timedelta64[ns]]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_multifunc_skipna[False-min-values28-datetime64[ns]-datetime64[ns]]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_multifunc_skipna[False-prod-values0-float64-float64]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_multifunc_skipna[True-prod-values3-float64-float64]": "AssertionError: Series are different", + "tests/groupby/test_reductions.py::test_multifunc_skipna[True-prod-values3-float64-float64]": "cudf returns NA for an all-null prod; pandas returns the empty-product identity 1.0 (min_count/empty-reduction semantics, not skipna)", "tests/groupby/test_reductions.py::test_nunique_with_NaT[key1-data1-True-expected1]": "TODO: Add a reason for failure", "tests/groupby/test_reductions.py::test_nunique_with_timegrouper": "TODO: Add a reason for failure", "tests/groupby/test_reductions.py::test_string_dtype_all_na[string=str[pyarrow]-all-False-False-0]": 'AssertionError: DataFrame.iloc[:, 0] (column name="b") values are different', @@ -2493,9 +2477,7 @@ def pytest_unconfigure(config): "tests/groupby/test_reductions.py::test_string_dtype_all_na[string=string[python]-any-False-False-1]": "AssertionError: DataFrame shape mismatch", "tests/groupby/test_reductions.py::test_string_dtype_all_na[string=string[python]-any-True-False-0]": "AssertionError: DataFrame shape mismatch", "tests/groupby/test_reductions.py::test_string_dtype_all_na[string=string[python]-any-True-False-1]": "AssertionError: DataFrame shape mismatch", - "tests/groupby/test_reductions.py::test_sum_skipna[False-values0-float64]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_sum_skipna[False-values3-timedelta64[ns]]": "AssertionError: Series are different", - "tests/groupby/test_reductions.py::test_sum_skipna_object[False]": "AssertionError: Series are different", + "tests/groupby/test_reductions.py::test_sum_skipna_object[False]": "Inherent cudf.pandas None-vs-NaN difference for object-dtype null (skipna logic is correct)", "tests/groupby/test_timegrouper.py::TestGroupBy::test_groupby_datetime64_32_bit": "TODO: Add a reason for failure", "tests/groupby/test_timegrouper.py::TestGroupBy::test_groupby_with_timegrouper": "TODO: Add a reason for failure", "tests/groupby/test_timegrouper.py::TestGroupBy::test_scalar_call_versus_list_call": "TODO: Add a reason for failure", diff --git a/python/cudf/cudf/tests/groupby/test_reductions.py b/python/cudf/cudf/tests/groupby/test_reductions.py index 618b684cb734..e7580c50e07b 100644 --- a/python/cudf/cudf/tests/groupby/test_reductions.py +++ b/python/cudf/cudf/tests/groupby/test_reductions.py @@ -1427,6 +1427,39 @@ def test_groupby_series_first_last_skipna(op, skipna): assert_eq(expect, got) +@pytest.mark.parametrize("op", ["sum", "prod", "mean", "median", "min", "max"]) +@pytest.mark.parametrize("dtype", ["float64", "Int64", "Float64"]) +def test_groupby_reduction_skipna_false(op, dtype): + # With skipna=False a group containing any null yields a null result for + # that group/column (libcudf reductions otherwise always drop nulls). + pdf = pd.DataFrame( + { + "a": [1, 1, 2, 2, 2], + "b": pd.array([1, None, 3, 4, 5], dtype=dtype), + "c": pd.array([10, 20, None, 40, 50], dtype=dtype), + } + ) + gdf = cudf.from_pandas(pdf) + with cudf.option_context("mode.pandas_compatible", True): + got = getattr(gdf.groupby("a"), op)(skipna=False) + expect = getattr(pdf.groupby("a"), op)(skipna=False) + assert_eq(expect, got) + + +@pytest.mark.parametrize("how", ["idxmin", "idxmax"]) +@pytest.mark.parametrize("as_series", [False, True]) +def test_groupby_idxmin_idxmax_skipna_false_raises(how, as_series): + # pandas does not support positional idxmin/idxmax with skipna=False. + pdf = pd.DataFrame({"a": [1, 1, 2], "b": [1.0, None, 3.0]}) + gdf = cudf.from_pandas(pdf) + pobj = pdf["b"].groupby(pdf["a"]) if as_series else pdf.groupby("a") + gobj = gdf["b"].groupby(gdf["a"]) if as_series else gdf.groupby("a") + assert_exceptions_equal( + lfunc=lambda: getattr(pobj, how)(skipna=False), + rfunc=lambda: getattr(gobj, how)(skipna=False), + ) + + @pytest.mark.parametrize("min_count", [0, 2, 3]) def test_groupby_series_reduce_min_count(min_count): psr = pd.Series([1.0, 2.0, 3.0, 4.0, 5.0]) From 8740118cc2d6cc0674b2ad80b2249cf6541b31ca Mon Sep 17 00:00:00 2001 From: galipremsagar Date: Wed, 24 Jun 2026 00:15:16 +0000 Subject: [PATCH 3/5] Address reviews --- python/cudf/cudf/core/groupby/groupby.py | 24 ++++++++++++------ .../cudf/tests/groupby/test_reductions.py | 25 +++++++++++++++++++ 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/python/cudf/cudf/core/groupby/groupby.py b/python/cudf/cudf/core/groupby/groupby.py index 1326c8412487..8333cacb8198 100644 --- a/python/cudf/cudf/core/groupby/groupby.py +++ b/python/cudf/cudf/core/groupby/groupby.py @@ -1454,16 +1454,24 @@ def _reduce( non_null_counts = self.agg("count") group_sizes = self.size() + if isinstance(group_sizes, DataFrame): + # With ``as_index=False`` the per-group counts are returned as + # the "size" column of a DataFrame; reduce it to a Series so it + # aligns with each value column below. + group_sizes = group_sizes["size"] if isinstance(result, DataFrame): - all_non_null = DataFrame( - { - name: non_null_counts[name] == group_sizes - for name in non_null_counts._column_names - } - ) + # ``as_index=False`` keeps the grouping keys as columns of + # ``result``; they must never be nulled out, so mask only the + # value columns. + key_names = set(self.grouping.names) + for name in result._column_names: + if name in key_names: + continue + result[name] = result[name].where( + non_null_counts[name] == group_sizes, None + ) else: - all_non_null = non_null_counts == group_sizes - result = result.where(all_non_null, None) + result = result.where(non_null_counts == group_sizes, None) if min_count and min_count > 0: counts = self.agg("count") result = result.where(counts >= min_count, None) diff --git a/python/cudf/cudf/tests/groupby/test_reductions.py b/python/cudf/cudf/tests/groupby/test_reductions.py index 28123d5f0f8f..35914e5a791f 100644 --- a/python/cudf/cudf/tests/groupby/test_reductions.py +++ b/python/cudf/cudf/tests/groupby/test_reductions.py @@ -1495,6 +1495,31 @@ def test_groupby_reduction_skipna_false(op, dtype): assert_eq(expect, got) +@pytest.mark.parametrize("op", ["sum", "prod", "mean", "median", "min", "max"]) +@pytest.mark.parametrize("dtype", ["float64", "Int64", "Float64"]) +def test_groupby_reduction_skipna_false_as_index_false(op, dtype): + # Regression: with as_index=False the grouping key is a column of the + # result and ``size()`` returns a DataFrame. The skipna=False + # null-propagation must mask only the value columns (never the key + # column) and must not raise on the Series/DataFrame comparison. + pdf = pd.DataFrame( + { + "a": [1, 1, 2, 2, 2], + "b": pd.array([1, None, 3, 4, 5], dtype=dtype), + "c": pd.array([10, 20, None, 40, 50], dtype=dtype), + } + ) + gdf = cudf.from_pandas(pdf) + with cudf.option_context("mode.pandas_compatible", True): + got = getattr(gdf.groupby("a", as_index=False, sort=True), op)( + skipna=False + ) + expect = getattr(pdf.groupby("a", as_index=False, sort=True), op)( + skipna=False + ) + assert_eq(expect, got) + + @pytest.mark.parametrize("how", ["idxmin", "idxmax"]) @pytest.mark.parametrize("as_series", [False, True]) def test_groupby_idxmin_idxmax_skipna_false_raises(how, as_series): From 3f9b71f5800040b07545ab45761d76a609c16ee7 Mon Sep 17 00:00:00 2001 From: galipremsagar Date: Thu, 25 Jun 2026 18:54:20 +0000 Subject: [PATCH 4/5] address reviews --- .../cudf/tests/groupby/test_reductions.py | 49 ++++++------------- 1 file changed, 14 insertions(+), 35 deletions(-) diff --git a/python/cudf/cudf/tests/groupby/test_reductions.py b/python/cudf/cudf/tests/groupby/test_reductions.py index 35914e5a791f..e5697961a644 100644 --- a/python/cudf/cudf/tests/groupby/test_reductions.py +++ b/python/cudf/cudf/tests/groupby/test_reductions.py @@ -1442,11 +1442,11 @@ def test_groupby_reduce_min_count(op, min_count): ) @pytest.mark.parametrize("op", ["first", "last"]) @pytest.mark.parametrize("skipna", [True, False]) -@pytest.mark.parametrize("sort", [True, False]) -def test_groupby_first_last_skipna(dtype, op, skipna, sort): +def test_groupby_first_last_skipna(dtype, op, skipna): # first/last honor skipna: with skipna=True nulls are dropped, with # skipna=False the actual first/last element of each group is returned - # even when it is null (previously skipna=False was ignored). + # even when it is null (previously skipna=False was ignored). sort=True so + # the group order matches pandas (first/last values are independent of it). pdf = pd.DataFrame( { "a": [2, 1, 1, 2, 3, 3], @@ -1455,9 +1455,8 @@ def test_groupby_first_last_skipna(dtype, op, skipna, sort): } ) gdf = cudf.from_pandas(pdf) - with cudf.option_context("mode.pandas_compatible", True): - got = getattr(gdf.groupby("a", sort=sort), op)(skipna=skipna) - expect = getattr(pdf.groupby("a", sort=sort), op)(skipna=skipna) + got = getattr(gdf.groupby("a", sort=True), op)(skipna=skipna) + expect = getattr(pdf.groupby("a", sort=True), op)(skipna=skipna) assert_eq(expect, got) @@ -1470,17 +1469,19 @@ def test_groupby_series_first_last_skipna(op, skipna): pkeys = pd.Series([2, 1, 1, 2, 3, 3], name="a") gsr = cudf.from_pandas(psr) gkeys = cudf.from_pandas(pkeys) - with cudf.option_context("mode.pandas_compatible", True): - got = getattr(gsr.groupby(gkeys), op)(skipna=skipna) - expect = getattr(psr.groupby(pkeys), op)(skipna=skipna) + got = getattr(gsr.groupby(gkeys, sort=True), op)(skipna=skipna) + expect = getattr(psr.groupby(pkeys, sort=True), op)(skipna=skipna) assert_eq(expect, got) @pytest.mark.parametrize("op", ["sum", "prod", "mean", "median", "min", "max"]) @pytest.mark.parametrize("dtype", ["float64", "Int64", "Float64"]) -def test_groupby_reduction_skipna_false(op, dtype): +def test_groupby_reduction_skipna_false(op, dtype, as_index): # With skipna=False a group containing any null yields a null result for # that group/column (libcudf reductions otherwise always drop nulls). + # With as_index=False the grouping key is also a column of the result (and + # ``size()`` returns a DataFrame), so the null-propagation must mask only + # the value columns, never the key column. pdf = pd.DataFrame( { "a": [1, 1, 2, 2, 2], @@ -1489,32 +1490,10 @@ def test_groupby_reduction_skipna_false(op, dtype): } ) gdf = cudf.from_pandas(pdf) - with cudf.option_context("mode.pandas_compatible", True): - got = getattr(gdf.groupby("a"), op)(skipna=False) - expect = getattr(pdf.groupby("a"), op)(skipna=False) - assert_eq(expect, got) - - -@pytest.mark.parametrize("op", ["sum", "prod", "mean", "median", "min", "max"]) -@pytest.mark.parametrize("dtype", ["float64", "Int64", "Float64"]) -def test_groupby_reduction_skipna_false_as_index_false(op, dtype): - # Regression: with as_index=False the grouping key is a column of the - # result and ``size()`` returns a DataFrame. The skipna=False - # null-propagation must mask only the value columns (never the key - # column) and must not raise on the Series/DataFrame comparison. - pdf = pd.DataFrame( - { - "a": [1, 1, 2, 2, 2], - "b": pd.array([1, None, 3, 4, 5], dtype=dtype), - "c": pd.array([10, 20, None, 40, 50], dtype=dtype), - } + got = getattr(gdf.groupby("a", as_index=as_index, sort=True), op)( + skipna=False ) - gdf = cudf.from_pandas(pdf) - with cudf.option_context("mode.pandas_compatible", True): - got = getattr(gdf.groupby("a", as_index=False, sort=True), op)( - skipna=False - ) - expect = getattr(pdf.groupby("a", as_index=False, sort=True), op)( + expect = getattr(pdf.groupby("a", as_index=as_index, sort=True), op)( skipna=False ) assert_eq(expect, got) From a660d5ed4be9f59adf1590da8583e0f261a6cf7e Mon Sep 17 00:00:00 2001 From: galipremsagar Date: Thu, 25 Jun 2026 19:26:08 +0000 Subject: [PATCH 5/5] update --- python/cudf/cudf/tests/groupby/test_reductions.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/python/cudf/cudf/tests/groupby/test_reductions.py b/python/cudf/cudf/tests/groupby/test_reductions.py index e5697961a644..6252bb7402dd 100644 --- a/python/cudf/cudf/tests/groupby/test_reductions.py +++ b/python/cudf/cudf/tests/groupby/test_reductions.py @@ -1441,7 +1441,6 @@ def test_groupby_reduce_min_count(op, min_count): ["Int64", "UInt32", "Float64", "int64[pyarrow]", "double[pyarrow]"], ) @pytest.mark.parametrize("op", ["first", "last"]) -@pytest.mark.parametrize("skipna", [True, False]) def test_groupby_first_last_skipna(dtype, op, skipna): # first/last honor skipna: with skipna=True nulls are dropped, with # skipna=False the actual first/last element of each group is returned @@ -1461,7 +1460,6 @@ def test_groupby_first_last_skipna(dtype, op, skipna): @pytest.mark.parametrize("op", ["first", "last"]) -@pytest.mark.parametrize("skipna", [True, False]) def test_groupby_series_first_last_skipna(op, skipna): psr = pd.Series( pd.array([None, 3, None, 4, None, None], dtype="Float64"), name="b"