Rewrite GroupBy.nth as a pandas-compatible positional row filter - #23257
Merged
Conversation
Fixes all 43 failures in pandas' tests/groupby/methods/test_nth.py under cudf.pandas (203/203 pass) and removes the 42 corresponding plugin entries. GroupBy.nth is now, like pandas, a property returning a selector that supports both the call form gb.nth(n, dropna=...) and the index form gb.nth[n] with ints, list-likes of ints and non-negative-step slices. The implementation is a positional-mask row filter over the grouped offsets (same machinery as head/tail): it validates arguments with pandas' exceptions, never mutates the grouped object (the old implementation temporarily inserted a column into the user's frame and crashed on SeriesGroupBy), honors the groupby's dropna for NaN keys, and gathers the surviving rows from the pre-nans_to_nulls object so values (NaN vs null), dtypes, the index, and the original row order are those of the original rows. In the cudf.pandas layer, pandas' nth is a class-level property (a GroupByNthSelector), which the generic attribute machinery cannot wrap: resolving gb.nth leaked the raw cudf attribute with no call-time fallback, so cudf exceptions (e.g. for dropna) escaped instead of falling back to pandas. Register a dispatching selector on both GroupBy proxies that routes call and index forms through the fast-slow machinery. Also reattach the timezone on aggregated tz-aware datetime columns (libcudf returns tz-naive timestamps holding the same UTC instants) and keep object dtype for string-producing aggregations on object-dtype columns, both matching pandas.
github-actions
Bot
added
Python
galipremsagar
added
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughGroupBy gains pandas-style ChangesGroupBy nth compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/cudf/cudf/tests/groupby/test_nth.py (1)
63-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd empty/all-null-group edge cases to
test_nth_selector_indexing.The parametrized cases cover a single-element group and several slice/list forms, but there's no case for an empty
DataFrameor a group whose key is entirely null (with the defaultdropna=Truegroupby behavior).🧪 Suggested additional cases
def test_nth_selector_indexing(arg): ... assert_groupby_results_equal( pdf.groupby("a").nth[arg], gdf.groupby("a").nth[arg], ) if not isinstance(arg, slice): assert_groupby_results_equal( pdf.groupby("a").nth(arg), gdf.groupby("a").nth(arg), ) + + +def test_nth_selector_empty_and_all_null_groups(): + pdf = pd.DataFrame({"a": [1, 1, None, None], "b": [10, 20, 30, 40]}) + gdf = cudf.from_pandas(pdf) + assert_groupby_results_equal( + pdf.groupby("a").nth[0], gdf.groupby("a").nth[0] + ) + + empty_pdf = pdf.iloc[:0] + empty_gdf = gdf.iloc[:0] + assert_groupby_results_equal( + empty_pdf.groupby("a").nth[0], empty_gdf.groupby("a").nth[0] + )As per path instructions, "Ensure test files provide comprehensive edge case coverage (empty, all-null, single-element, mixed types)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf/cudf/tests/groupby/test_nth.py` around lines 63 - 96, Add empty-DataFrame and entirely null-group cases to test_nth_selector_indexing, covering the existing indexed selector forms and preserving pandas/cuDF result parity. Use the default dropna=True groupby behavior so null-key groups are excluded, and retain the current single-element and slice/list coverage.Source: Path instructions
python/cudf/cudf/core/groupby/groupby.py (1)
1796-1806: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring for the new public
nthproperty lacks Parameters/Returns sections.Sibling public methods in this class (
head,tail,sample,rank, etc.) documentParameters/Returnsin numpydoc style. Thenthdocstring only prose-describes supported forms and omits thedropnaparameter (which currently always raisesNotImplementedErrorin native cuDF) and the return type.📝 Suggested docstring expansion
`@property` def nth(self) -> GroupByNthSelector: # type: ignore[override] """ Take the nth row from each group if n is an int, otherwise a subset of rows. Like pandas, supports both the call form ``gb.nth(n, dropna=...)`` and the index form ``gb.nth[n]`` (with ints, list-likes of ints and non-negative-step slices). + + Parameters + ---------- + n : int, list-like of int, or slice + Position(s) to select within each group. Slices must have a + non-negative step. + dropna : {"any", "all", None}, default None + Not currently supported; raises ``NotImplementedError`` if set. + + Returns + ------- + Series or DataFrame + The selected rows, preserving original row order and index. """ return GroupByNthSelector(self)As per coding guidelines, "Ensure all public API methods have complete docstrings documenting parameters, return values, and behavior."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf/cudf/core/groupby/groupby.py` around lines 1796 - 1806, Expand the public GroupBy.nth property docstring for GroupByNthSelector to use numpydoc-style Parameters and Returns sections. Document the supported n/index forms, the dropna parameter and its current NotImplementedError behavior in native cuDF, and the returned grouped selection type while preserving the existing behavior description.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/cudf/cudf/core/groupby/groupby.py`:
- Around line 1796-1806: Expand the public GroupBy.nth property docstring for
GroupByNthSelector to use numpydoc-style Parameters and Returns sections.
Document the supported n/index forms, the dropna parameter and its current
NotImplementedError behavior in native cuDF, and the returned grouped selection
type while preserving the existing behavior description.
In `@python/cudf/cudf/tests/groupby/test_nth.py`:
- Around line 63-96: Add empty-DataFrame and entirely null-group cases to
test_nth_selector_indexing, covering the existing indexed selector forms and
preserving pandas/cuDF result parity. Use the default dropna=True groupby
behavior so null-key groups are excluded, and retain the current single-element
and slice/list coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 29e0486e-b044-436c-9e0e-7071edee0849
📒 Files selected for processing (5)
python/cudf/cudf/core/groupby/groupby.pypython/cudf/cudf/pandas/_wrappers/pandas.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/groupby/test_nth.pypython/cudf/cudf/utils/dtypes.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Contributor
Author
|
/okay to test d02fd94 |
3 tasks
…eference * Move the object-vs-StringDtype preservation out of the global get_dtype_of_same_kind helper into GroupBy.agg's dtype branch: the helper's other callers (e.g. merge key coalescing) re-infer str for object inputs, and the global change regressed three TestMergeCategorical::test_dtype_on_merged_different params in CI. String-producing aggregations on object columns still stay object. * Drop the nth property's return annotation: sphinx (warnings as errors) cannot resolve the GroupByNthSelector class reference that autodoc generates from it. * Prune five plugin entries that strict-XPASSed in CI: the tz reattachment fixes test_groupby_agg_extension, test_agg_timezone_round_trip and test_pivot_tz_in_values, and the nth selector fixes test_groupby_with_single_column and test_indexing.py::test_multiindex (all verified passing in isolation).
Contributor
Author
|
/okay to test edff70b |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
python/cudf/cudf/core/groupby/groupby.py (4)
597-600: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist
_obj_originalduring GroupBy serialization.
serialize()still persistsself.obj(Line [2158]), which is thenans_to_nulls()result in pandas-compatible mode. After deserialization (Line [2182]),_nthcan no longer restore the original NaN-versus-null values. Include the pre-conversion object in the serialized state and add a round-trip regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf/cudf/core/groupby/groupby.py` around lines 597 - 600, Update GroupBy serialization and deserialization to persist and restore the pre-conversion object stored in _obj_original, rather than retaining only self.obj after nans_to_nulls(). Ensure _nth uses the restored original values so NaN-versus-null distinctions survive round trips, and add a regression test covering serialization followed by deserialization and nth.
1888-1898: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor GroupBy column selections in
_nth.
_nthalways gathers fromself._obj_originaland returns the full object. The class tracks selected columns through_selection(Line [610]), so calls such asdf.groupby("key")["value"].nth(0)can return the entire original frame instead of the selected result. Route the gathered object through the existing selection-aware result construction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf/cudf/core/groupby/groupby.py` around lines 1888 - 1898, Update the return path in _nth to apply the tracked _selection when constructing the gathered result, using the existing selection-aware result construction instead of returning self._obj_original.take(original_positions) directly. Preserve the current original-row ordering and ensure selected-column calls such as grouped Series selections return only the requested columns.
1840-1843: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject zero-step slices.
arg.step or 1turnsstep=0into1, sogb.nth[::0]silently returns rows instead of raising. Handlestep == 0explicitly in both the slice validation and mask construction paths (python/cudf/cudf/core/groupby/groupby.py:1841-1843, 1865).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf/cudf/core/groupby/groupby.py` around lines 1840 - 1843, Update the slice handling in the groupby nth validation and mask-construction paths to reject arg.step == 0 explicitly, alongside negative steps, raising the existing ValueError instead of treating zero as the default step. Apply this consistently near the validation block and the mask logic around the visible slice handling.
1831-1832: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAccept all supported list-like indexers. This branch only handles
list,tuple, andnp.ndarray, so other list-like inputs still raiseTypeErrorbefore element validation. Usecudf.api.types.is_list_likehere, while excluding scalars and strings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf/cudf/core/groupby/groupby.py` around lines 1831 - 1832, Update the indexer normalization branch in the groupby logic to use cudf.api.types.is_list_like, while explicitly excluding scalar values and strings. Preserve converting accepted list-like inputs to args before element validation, and ensure unsupported scalars or string inputs still follow the TypeError path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@python/cudf/cudf/core/groupby/groupby.py`:
- Around line 597-600: Update GroupBy serialization and deserialization to
persist and restore the pre-conversion object stored in _obj_original, rather
than retaining only self.obj after nans_to_nulls(). Ensure _nth uses the
restored original values so NaN-versus-null distinctions survive round trips,
and add a regression test covering serialization followed by deserialization and
nth.
- Around line 1888-1898: Update the return path in _nth to apply the tracked
_selection when constructing the gathered result, using the existing
selection-aware result construction instead of returning
self._obj_original.take(original_positions) directly. Preserve the current
original-row ordering and ensure selected-column calls such as grouped Series
selections return only the requested columns.
- Around line 1840-1843: Update the slice handling in the groupby nth validation
and mask-construction paths to reject arg.step == 0 explicitly, alongside
negative steps, raising the existing ValueError instead of treating zero as the
default step. Apply this consistently near the validation block and the mask
logic around the visible slice handling.
- Around line 1831-1832: Update the indexer normalization branch in the groupby
logic to use cudf.api.types.is_list_like, while explicitly excluding scalar
values and strings. Preserve converting accepted list-like inputs to args before
element validation, and ensure unsupported scalars or string inputs still follow
the TypeError path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 29321660-d654-46c9-a395-e0bee99c727d
📒 Files selected for processing (2)
python/cudf/cudf/core/groupby/groupby.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
# Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Contributor
Author
|
/okay to test b0caa07 |
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jul 15, 2026
…rt=False (#23260) Running pandas' own test suite under `cudf.pandas`, `tests/groupby/methods/test_size.py` had 6 failing tests. This PR fixes them (22/22 pass) and removes the corresponding xfail entries from the pandas-testing plugin. Two independent root causes: ### `SeriesGroupBy.size()` dtype for masked series pandas returns `Int64` for `size()` on masked (`Int*`/`UInt*`/`Float*`/`boolean`) series (pandas GH#54132). Verified matrix on pandas 3.0.3: | `.size()` on | result dtype | |---|---| | masked series (`Int64`, `Float64`, `boolean`, ...) | `Int64` | | `string[pyarrow]` series | `Int64` | | `ArrowDtype` series | `int64[pyarrow]` | | `object` / `str` / `string[python]` series | `int64` | | any `DataFrameGroupBy` | `int64` | cuDF already special-cased the Arrow and `string[pyarrow]` rows; this adds the missing masked branch (Series-only, `StringDtype` excluded so `string[python]` stays `int64`). ### `GroupBy.apply` group order with `sort=False` pandas processes groups in order of first appearance when `sort=False`. libcudf returns groups sorted by key, and `apply` consumed that layout directly, so results came out key-sorted. (`size()` was already correct — the failing tests compare `size()` against `apply(lambda a: a.shape[0])`, and the `apply` side was the broken one.) The grouped layout (group names, offsets, grouped keys and values) is now permuted into first-appearance order before either engine runs — the same device-side first-position/argsort technique `GroupBy.__iter__` already uses — so every result shape (scalar-per-group, frame-per-group, transforms) sees pandas' iteration order. ### Validation - pandas-tests `tests/groupby/methods/test_size.py`: 6 failed → 22/22 pass. - pandas-tests `tests/groupby/test_apply.py` + the full `tests/groupby/methods/` directory: remaining failures are exactly the pre-existing known-failure entries (plus one pre-existing local-only `test_nth` failure that reproduces identically on `main` and is fixed by #23257). - cuDF classic groupby suite passes; new classic regression tests pin both behaviors. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Matthew Roeschke (https://github.com/mroeschke) URL: #23260
mroeschke
reviewed
Jul 15, 2026
…_dtype_obj_string, numpydoc nth docstring - Replace the hand-rolled _GroupByNthSelector dispatching stand-in with a registered intermediate proxy pair (cudf GroupByNthSelector / pandas GroupByNthSelector): the attribute machinery wraps gb.nth via the recorded getattr provenance and __call__/__getitem__ get the standard call-time fast/slow dispatch, including the dropna fallback. - Use is_dtype_obj_string for the string-result check in agg dtype preservation so it also covers arrow string dtypes should dtype_from_pylibcudf_column ever produce them. - Expand the GroupBy.nth docstring to numpydoc style with Parameters, Returns and doctested Examples.
Contributor
Author
|
/okay to test c5e4b9f |
mroeschke
approved these changes
Jul 15, 2026
Contributor
Author
|
/merge |
galipremsagar
added
5 - Ready to Merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Running pandas' own test suite under
cudf.pandas,tests/groupby/methods/test_nth.pyhad 43 failing tests. This PR fixes all of them (203/203 pass) and removes the 42 corresponding xfail entries from the pandas-testing plugin.Why the failures could not fall back to pandas
pandas'
GroupBy.nthis a class-level property returning aGroupByNthSelector(supporting bothgb.nth(n, dropna=...)andgb.nth[n]), not a method.cudf.pandas' attribute machinery only builds fallback-capable method proxies for functions, so resolvinggb.nthleaked cuDF's raw bound method: cuDF exceptions (e.g.NotImplementedErrorfordropna=) escaped directly to user code instead of triggering the transparent pandas fallback, and the index formgb.nth[...]failed with "method object is not subscriptable".GroupBy.nthrewrite (cuDF classic)nthis now, like pandas, a property returning a selector supporting both call and index forms, implemented as a positional-mask row filter over the grouped offsets (the same machinery ashead/tail):TypeError: Invalid index ...,ValueError: Invalid step ...);__groupbynth_order__column into the user's frame, and crashed onSeriesGroupBy);dropnafor NaN group keys;nans_to_nullsobject, so a NaN in the original data stays NaN instead of becoming null.dropna="any"/"all"still raisesNotImplementedErrorin cuDF classic; undercudf.pandasthis now correctly falls back to pandas via a dispatching selector registered on both GroupBy proxies (scalarnthstays on the GPU).Aggregation dtype fixes
datetime64[ns, tz]columns lost the tz.objectdtype for string-producing aggregations on object-dtype columns instead of re-typing tostr, matching pandas.Validation
tests/groupby/methods/test_nth.py: 43 failed → 203/203 pass (stable across test orderings).tests/groupby/methods/(full directory): remaining failures are exactly the pre-existing known-failure entries; zero new failures.Checklist