Match pandas semantics in groupby.rolling and groupby.apply result construction - #23122
Merged
Conversation
github-actions
Bot
added
Python
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR preserves inner MultiIndex names in ChangesGroupBy.apply inner index name preservation
Rolling groupby enhancements
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
mroeschke
approved these changes
Jul 6, 2026
Contributor
Author
|
/merge |
galipremsagar
added
the
5 - Ready to Merge
![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/core/window/rolling.py (1)
676-702: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid computing
_get_sorted_inds()when it will be discarded.Line 683 always computes
groupby.grouping.keys._get_sorted_inds()(a full sort), but whennot groupby._sort, the result is immediately overwritten at line 700 by the cupy-based reordering. This wastes a sort pass on everysort=Falsecall.♻️ Proposed fix to skip the unused sort
self._as_index = groupby._as_index - sort_inds = groupby.grouping.keys._get_sorted_inds() - if not groupby._sort: + if groupby._sort: + sort_inds = groupby.grouping.keys._get_sorted_inds() + else: # With sort=False pandas keeps groups in order of first # appearance; reorder the key-sorted blocks accordingly while # keeping the original row order within each block. offsets, _, (positions,) = groupby._groups( [groupby._range_column_from_obj] ) pos = cupy.asarray(positions.values) off = cupy.asarray(offsets) sizes = off[1:] - off[:-1] row_first_pos = cupy.repeat(pos[off[:-1]], sizes) order = cupy.lexsort( cupy.stack([cupy.arange(len(pos)), row_first_pos]) ) sort_inds = as_column(pos[order])🤖 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/window/rolling.py` around lines 676 - 702, Avoid computing groupby.grouping.keys._get_sorted_inds() when sort=False in the rolling groupby path. In the groupby.rolling logic, move the initial sort_inds assignment so it only runs when groupby._sort is true, and let the existing cupy-based reorder path provide sort_inds for the false case. This removes the wasted full sort while preserving the current behavior in the BaseIndexer and GatherMap.from_column_unchecked flow.python/cudf/cudf/tests/window/test_rolling.py (1)
550-591: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood coverage for the three new behaviors (as_index=False, sort=False ordering, BaseIndexer rejection).
Consider also adding a case with multiple groups per first-appearance test (e.g., 3+ distinct groups interleaved) to more thoroughly exercise the cupy lexsort reordering logic beyond the 2-group case, but this isn't blocking.
🤖 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/window/test_rolling.py` around lines 550 - 591, The new sort=False coverage in test_groupby_rolling_no_sort_first_appearance_order only exercises two groups, so extend that test to use 3+ distinct interleaved groups to better cover first-appearance ordering and the cupy lexsort reordering path. Update the existing pandas/cudf comparison in test_groupby_rolling_no_sort_first_appearance_order so it still validates the same rolling().min() behavior with a richer group pattern.
🤖 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/window/rolling.py`:
- Around line 676-702: Avoid computing groupby.grouping.keys._get_sorted_inds()
when sort=False in the rolling groupby path. In the groupby.rolling logic, move
the initial sort_inds assignment so it only runs when groupby._sort is true, and
let the existing cupy-based reorder path provide sort_inds for the false case.
This removes the wasted full sort while preserving the current behavior in the
BaseIndexer and GatherMap.from_column_unchecked flow.
In `@python/cudf/cudf/tests/window/test_rolling.py`:
- Around line 550-591: The new sort=False coverage in
test_groupby_rolling_no_sort_first_appearance_order only exercises two groups,
so extend that test to use 3+ distinct interleaved groups to better cover
first-appearance ordering and the cupy lexsort reordering path. Update the
existing pandas/cudf comparison in
test_groupby_rolling_no_sort_first_appearance_order so it still validates the
same rolling().min() behavior with a richer group pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4fb647a0-fbab-4ed7-a4dd-7e5b93e8b180
📒 Files selected for processing (5)
python/cudf/cudf/core/groupby/groupby.pypython/cudf/cudf/core/window/rolling.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/groupby/test_apply.pypython/cudf/cudf/tests/window/test_rolling.py
Older cupy (oldest-deps CI) does not accept an ndarray as the repeats argument; broadcast each group's first-appearance position via searchsorted instead.
Contributor
Author
|
/okay to test 45e04eb |
Contributor
Author
|
/okay to test 3ae476c |
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
3 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
Fixes 12 of the 14
tests/window/test_groupby.pyfailures undercudf.pandas:RollingGroupbynow honorsas_index=False(group keys as leading columns) andsort=False(first-appearance group order), raises forBaseIndexerwindows whose bounds were silently computed across group boundaries, andgroupby.applypreserves the UDF result's index name in the concatenated MultiIndex. The tworolling.corrtuple-index tests are inherent (tuple values are stored as list rows and do not round-trip), so their xfail entries get a real reason; the 13 fixed entries are removed.Checklist