Preserve masked integer dtypes in GroupBy cumsum/cumprod - #23299
Preserve masked integer dtypes in GroupBy cumsum/cumprod#23299rapids-bot[bot] merged 11 commits into
Conversation
galipremsagar
added
bug
galipremsagar
added
bug
![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 (2)
python/cudf/cudf/tests/groupby/test_cummulative.py (2)
123-131: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover NumPy
cumprodpromotion too.This regression test only exercises
cumsum; parameterize it over bothcumsumandcumprodso narrow NumPy inputs retain their expected 64-bit promotion for both scan operations.Based on the PR objective, the cumulative dtype contract applies to both
cumsumandcumprod.Suggested parameterization
-def test_groupby_cumsum_numpy_dtype_promotes(): +@pytest.mark.parametrize("op", ["cumsum", "cumprod"]) +def test_groupby_cumscan_numpy_dtype_promotes(op): ... - expected = pdf.groupby("a").cumsum() - result = gdf.groupby("a").cumsum() + expected = getattr(pdf.groupby("a"), op)() + result = getattr(gdf.groupby("a"), op)()🤖 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_cummulative.py` around lines 123 - 131, Parameterize test_groupby_cumsum_numpy_dtype_promotes over both the cumsum and cumprod groupby operations, invoking the selected operation on pandas and cuDF inputs. Preserve the narrow int8 fixture and assert each result against pandas so both scans verify expected 64-bit dtype promotion.
110-122: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise masked-integer overflow wrapping.
The current values never overflow
Int16, so this test would still pass if the final cast failed to wrap overflowing cumulative results. Add a case such as two20_000values and assert against pandas for both operators.Based on the PR objective, overflow wrapping is part of the behavior this change is intended to guarantee.
Suggested test adjustment
- pdf = pd.DataFrame({"a": [1, 1, 2], "b": [1, pd.NA, 2]}, dtype="Int16") + pdf = pd.DataFrame( + {"a": [1, 1, 2], "b": [20_000, 20_000, pd.NA]}, + dtype="Int16", + )🤖 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_cummulative.py` around lines 110 - 122, Extend test_groupby_cumscan_masked_dtype_preserved with masked Int16 input containing values such as two 20,000 entries so cumulative cumsum and cumprod results exceed Int16 range. Keep parametrization for both operators and continue comparing the cudf result with pandas via assert_eq, thereby validating wrapped overflow behavior.
🤖 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/tests/groupby/test_cummulative.py`:
- Around line 110-131: Add a unit benchmark alongside
test_groupby_cumscan_masked_dtype_preserved covering grouped nullable-integer
cumsum and cumprod, with representative group sizes and overflow-inducing
values, so the post-scan casting path is measured for both operations.
---
Nitpick comments:
In `@python/cudf/cudf/tests/groupby/test_cummulative.py`:
- Around line 123-131: Parameterize test_groupby_cumsum_numpy_dtype_promotes
over both the cumsum and cumprod groupby operations, invoking the selected
operation on pandas and cuDF inputs. Preserve the narrow int8 fixture and assert
each result against pandas so both scans verify expected 64-bit dtype promotion.
- Around line 110-122: Extend test_groupby_cumscan_masked_dtype_preserved with
masked Int16 input containing values such as two 20,000 entries so cumulative
cumsum and cumprod results exceed Int16 range. Keep parametrization for both
operators and continue comparing the cudf result with pandas via assert_eq,
thereby validating wrapped overflow behavior.
🪄 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: da0fd433-9586-4fcc-b6eb-915aaf6bd00b
📒 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_cummulative.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
| @pytest.mark.parametrize("op", ["cumsum", "cumprod"]) | ||
| def test_groupby_cumscan_masked_dtype_preserved(op): | ||
| # pandas preserves masked extension dtypes for groupby cum-scans | ||
| # (Int16 stays Int16, GH#58811) while numpy ints promote to 64-bit | ||
| pdf = pd.DataFrame({"a": [1, 1, 2], "b": [1, pd.NA, 2]}, dtype="Int16") | ||
| gdf = cudf.DataFrame(pdf) | ||
|
|
||
| expected = getattr(pdf.groupby("a")["b"], op)() | ||
| result = getattr(gdf.groupby("a")["b"], op)() | ||
|
|
||
| assert_eq(expected, result) | ||
|
|
||
|
|
||
| def test_groupby_cumsum_numpy_dtype_promotes(): | ||
| # numpy int8 promotes to int64 (pandas GH#37493) | ||
| pdf = pd.DataFrame({"a": [1, 1], "b": [111, 111]}, dtype="int8") | ||
| gdf = cudf.DataFrame(pdf) | ||
|
|
||
| expected = pdf.groupby("a").cumsum() | ||
| result = gdf.groupby("a").cumsum() | ||
|
|
||
| assert_eq(expected, result) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Add a unit benchmark for the new casting path.
The changed files add correctness tests but no benchmark for this feature/bug fix. Add a benchmark covering nullable-integer cumsum/cumprod, ideally including representative group sizes and overflow cases, to track the cost of the post-scan cast.
As per coding guidelines, “Add unit tests and unit benchmarks for feature and bug-fix contributions.”
🤖 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_cummulative.py` around lines 110 - 131,
Add a unit benchmark alongside test_groupby_cumscan_masked_dtype_preserved
covering grouped nullable-integer cumsum and cumprod, with representative group
sizes and overflow-inducing values, so the post-scan casting path is measured
for both operations.
Source: Coding guidelines
libcudf's SUM/PRODUCT scans promote narrow integers to 64-bit. pandas does the same for numpy dtypes (int8 -> int64, GH#37493) but preserves masked extension dtypes (Int16 stays Int16, GH#58811), wrapping on overflow. Cast the scan result back to the original dtype for masked integer inputs only.
64ce353 to
455b357
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGroupby cumulative scans now preserve pandas nullable integer dtypes, with tests covering masked dtypes and NumPy integer promotion. Related pandas testing known-failure mappings are updated. ChangesGroupby cumulative dtype handling
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cudf/cudf/tests/groupby/test_cummulative.py (1)
110-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd edge case coverage for overflow wrapping and unsigned masked dtypes.
The masked dtype test uses values
[1, pd.NA, 2]which don't overflowInt16, so the overflow-wrapping behavior described in thegroupby.pycomment (line 1242) is not exercised. Additionally, the code checks for kind"u"(unsigned) but no test coversUInt8/UInt16etc. Consider adding:
- A test with values that overflow the masked dtype (e.g.,
Int8with values near 127) to verify wrapping matches pandas.- A
UIntdtype case to cover the"u"branch.As per coding guidelines, "Ensure test files provide comprehensive edge case coverage (empty, all-null, single-element, mixed types)."
🧪 Suggested additional test cases
def test_groupby_cumscan_masked_dtype_preserved(op): # pandas preserves masked extension dtypes for groupby cum-scans # (Int16 stays Int16, GH#58811) while numpy ints promote to 64-bit pdf = pd.DataFrame({"a": [1, 1, 2], "b": [1, pd.NA, 2]}, dtype="Int16") gdf = cudf.DataFrame(pdf) expected = getattr(pdf.groupby("a")["b"], op)() result = getattr(gdf.groupby("a")["b"], op)() assert_eq(expected, result) +@pytest.mark.parametrize("op", ["cumsum", "cumprod"]) +@pytest.mark.parametrize("dtype", ["Int8", "UInt8"]) +def test_groupby_cumscan_masked_dtype_overflow_wrapping(op, dtype): + # Verify overflow wrapping matches pandas for masked integer dtypes + info = np.iinfo(dtype.lower()) + vals = [info.max, info.max, 1] + pdf = pd.DataFrame({"a": [1, 1, 2], "b": vals}, dtype=dtype) + gdf = cudf.DataFrame(pdf) + expected = getattr(pdf.groupby("a")["b"], op)() + result = getattr(gdf.groupby("a")["b"], op)() + assert_eq(expected, result)🤖 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_cummulative.py` around lines 110 - 120, Add edge-case coverage to test_groupby_cumscan_masked_dtype_preserved: include masked signed-integer inputs near the dtype limit that overflow during cumsum or cumprod, and verify pandas and cuDF preserve matching wrapping behavior. Add a parameterized unsigned masked dtype case such as UInt8 or UInt16 to exercise the groupby.py unsigned-kind branch, while retaining the existing Int16 coverage.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/tests/groupby/test_cummulative.py`:
- Around line 110-120: Add edge-case coverage to
test_groupby_cumscan_masked_dtype_preserved: include masked signed-integer
inputs near the dtype limit that overflow during cumsum or cumprod, and verify
pandas and cuDF preserve matching wrapping behavior. Add a parameterized
unsigned masked dtype case such as UInt8 or UInt16 to exercise the groupby.py
unsigned-kind branch, while retaining the existing Int16 coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6f06be42-47f0-4996-9d50-23481ed2efd4
📒 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_cummulative.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
|
/okay to test 5e61811 |
|
/okay to test e188265 |
Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>
|
/okay to test 7a2c200 |
galipremsagar
added
the
5 - Ready to Merge
…mscan-masked-dtype # Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
|
/ok to test 001b0ca |
…mscan-masked-dtype # Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
|
/okay to test 4f426ad |
…mscan-masked-dtype # Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
|
/okay to test f49e2e6 |
|
/merge |
|
/okay to test b45e108 |
|
/okay to test 8fc070f |
|
/merge |
908b36d
into
NVIDIA:release/26.08
4 participants
Description
libcudf's SUM/PRODUCT scans promote narrow integers to 64-bit. pandas does the same for numpy dtypes (int8 -> int64, pandas GH#37493) but preserves masked extension dtypes (Int16 stays Int16, pandas GH#58811), wrapping on overflow. Cast the scan result back to the original dtype for masked integer inputs only.
Fixes 8 pandas-tests (
test_groupby_cumsum_mask[Int*/UInt*-True-3],test_nan_in_cumsum_group_label); their xfail entries are removed.Checklist