Keep the dtype family in GroupBy.rank results - #23258
Conversation
pandas' groupby rank always returns floats within the value column's dtype family: numpy dtypes give float64, masked dtypes give Float64 (Float32 included), and arrow dtypes give double[pyarrow]. cuDF's blanket cast to numpy float64 flattened masked inputs; map each result column through get_dtype_of_same_kind instead. Fixes the 4 failing tests in pandas' tests/groupby/methods/test_rank.py under cudf.pandas (1548/1548 pass) and removes their plugin entries.
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 (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesGroupBy rank dtype preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/cudf/cudf/tests/groupby/test_rank.py (1)
63-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend coverage to Arrow and 2D/as_index=False results.
This test only covers NumPy/masked dtypes with default
as_index=Trueand one value column. Add cases fordouble[pyarrow],SeriesGroupBy, multi-column frames, andas_index=Falsewith a string grouping key; include null-bearing inputs to validate nullable behavior.As per coding guidelines,
python/**/test_*.pyshould provide comprehensive edge-case coverage, including empty, all-null, single-element, and mixed-type inputs.🤖 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_rank.py` around lines 63 - 84, Expand test_rank_dtype_family coverage to include double[pyarrow], SeriesGroupBy, multi-column frames, and as_index=False with a string key. Add null-bearing, empty, all-null, single-element, and mixed-type inputs while preserving validation of nullable dtype behavior and pandas/cudf result equality.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.
Inline comments:
In `@python/cudf/cudf/core/groupby/groupby.py`:
- Around line 888-897: Update the dtype mapping in the result conversion around
GroupBy.agg so grouping-key columns are excluded when as_index=False. Apply
get_dtype_of_same_kind only to value columns, while preserving grouping-key
columns’ original dtypes and leaving the existing conversion behavior for
aggregated values unchanged.
---
Nitpick comments:
In `@python/cudf/cudf/tests/groupby/test_rank.py`:
- Around line 63-84: Expand test_rank_dtype_family coverage to include
double[pyarrow], SeriesGroupBy, multi-column frames, and as_index=False with a
string key. Add null-bearing, empty, all-null, single-element, and mixed-type
inputs while preserving validation of nullable dtype behavior and pandas/cudf
result equality.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1cd89ee0-438f-4d9d-85ce-361beaf7ce60
📒 Files selected for processing (3)
python/cudf/cudf/core/groupby/groupby.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/groupby/test_rank.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
|
/okay to test d3994b1 |
|
/okay to test cc83313 |
galipremsagar
added
the
5 - Ready to Merge
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@python/cudf/cudf/core/groupby/groupby.py`:
- Line 2392: Update the UDF documentation URL in the groupby documentation
reference to use the official user_guide/guide-to-udfs/ path instead of
cudf/guide-to-udfs/, without changing the surrounding text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b509ec18-0c95-4380-9deb-5e68982186ca
📒 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
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@python/cudf/cudf/core/groupby/groupby.py`:
- Line 2392: Update the UDF documentation URL in the groupby documentation
reference to use the official user_guide/guide-to-udfs/ path instead of
cudf/guide-to-udfs/, without changing the surrounding text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b509ec18-0c95-4380-9deb-5e68982186ca
📒 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
🛑 Comments failed to post (1)
python/cudf/cudf/core/groupby/groupby.py (1)
2392-2392: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the UDF guide link.
The link points to
cudf/guide-to-udfs/, but the official page is underuser_guide/guide-to-udfs/; update the path to avoid sending users to the wrong page. (docs.rapids.ai)Proposed fix
- <https://docs.rapids.ai/api/cudf/stable/cudf/guide-to-udfs/>`__. + <https://docs.rapids.ai/api/cudf/stable/user_guide/guide-to-udfs/>`__.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<https://docs.rapids.ai/api/cudf/stable/user_guide/guide-to-udfs/>`__.🤖 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` at line 2392, Update the UDF documentation URL in the groupby documentation reference to use the official user_guide/guide-to-udfs/ path instead of cudf/guide-to-udfs/, without changing the surrounding text.
# Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
|
/okay to test e460ba1 |
|
/merge |
4 participants
Description
Running pandas' own test suite under
cudf.pandas,tests/groupby/methods/test_rank.pyhad 4 failing tests (test_rank_avg_even_valswith masked integer dtypes). This PR fixes them (1548/1548 pass) and removes the corresponding xfail entries from the pandas-testing plugin.pandas' groupby
rankalways returns floats within the value column's dtype family (verified on pandas 3.0.3):int64/float32(numpy)float64Int64/Float32(masked)Float64int64[pyarrow]double[pyarrow]cuDF's
GroupBy.rankended with a blanket cast to numpyfloat64, flattening masked inputs. Map each result column through the existingget_dtype_of_same_kindhelper instead, which reproduces the table above for bothSeriesGroupByandDataFrameGroupByresults.Validation: pandas-tests
tests/groupby/methods/test_rank.py1548/1548; the neighboringseries/methods/test_rank.pyandframe/methods/test_rank.pyshow only their pre-existing known failures; the cuDF classic groupby suite passes with a new parametrized regression test pinning the dtype-family table.Checklist