Fix various pandas issues - #22705
Merged
Merged
Conversation
GroupBy._grouped passed internal storage keys (self.obj._index_names) to _from_columns_like_self, which for a nameless MultiIndex produced [0, 1] instead of the correct public names [None, None]. This caused GroupBy.apply to return results with wrong index level names, making the cudf.pandas accelerated test_cython_transform_frame_column fail when comparing apply() expected values against transform() results. Use self.obj.index.names (public metadata) instead. Fixes the following failing pandas tests: - tests/groupby/transform/test_transform.py::test_cython_transform_frame_column[*-frame_mi-*]
These tests assert that pandas raises NotImplementedError when using engine='numba' with as_index=False for groupby reductions. cuDF computes reductions on GPU without numba, so the pandas-specific limitation does not apply and the operation succeeds. The divergence is intentional. Skips the following pandas tests: - tests/groupby/test_numba.py::TestEngine::test_as_index_false_unsupported[*]
vyasr
added
improvement
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 (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds IntervalColumn equality/inequality support with correct null semantics, switches GroupBy to use public index names, refines Series.dtype wrapping for certain dtypes and adds an intermediate proxy type, updates pandas-testing expected-fail mappings, adjusts Narwhals CI exclusions, and adds numexpr to Conda/dependency manifests. ChangesInterval equality operators and cuDF/pandas compatibility fixes
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🤖 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/column/interval.py`:
- Around line 169-180: The equality branch in the interval column binary op
(method handling op values "__eq__", "__ne__", "NULL_EQUALS", "NULL_NOT_EQUALS")
currently compares only endpoints (self.left/right vs other.left/right) and
ignores the interval `closed` attribute; update the logic in that branch (the
block using self.left._binaryop, self.right._binaryop, and binaryop.binaryop) to
include a `closed` equality check—either short-circuit to a
False/NULL-equivalent when self.closed != other.closed for equality ops, or
incorporate a `closed_equal` predicate and AND it with the existing
endpoint-equality `result` (and invert for "__ne__"/"NULL_NOT_EQUALS"); also add
a regression test asserting intervals with identical bounds but differing
`closed` values are not equal.
In `@python/cudf/cudf/pandas/_wrappers/pandas.py`:
- Around line 337-350: Series.dtype currently reads self._fsproxy_slow.dtype in
a way that can mutate the proxy and pin it to slow mode; update _Series_dtype to
detect the complex dtypes (cudf.ListDtype, cudf.StructDtype, Decimal*Dtypes) and
then obtain the dtype from a non-mutating accessor so you don't swap
_fsproxy_wrapped to slow. Concretely, inside _Series_dtype use a
read-only/metadata-only call (or an existing non-mutating helper) to peek the
dtype from the slow-side representation (referencing _fsproxy_slow) without
assigning or replacing _fsproxy_wrapped, and return that value via
_maybe_wrap_result; do not perform any assignment to _fsproxy_wrapped or other
state changes.
In `@python/cudf/cudf/tests/groupby/test_apply.py`:
- Around line 857-873: The test
test_groupby_apply_series_preserves_multiindex_names should explicitly set
MultiIndex level names on pdf (e.g., use MultiIndex.from_product(...,
names=[...]) rather than anonymous levels) so the test verifies public level
names are preserved; update the pdf creation (and gdf from_pandas) to use named
levels and add an assertion that the resulting indexes' .names on expect and got
are equal (or ensure assert_eq compares index names) after the groupby apply,
referencing pdf, gdf, expect, and got to locate the changes.
🪄 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: 670005e7-cb14-4c0b-9264-604d055bc9ce
📒 Files selected for processing (6)
python/cudf/cudf/core/column/interval.pypython/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_apply.pypython/cudf/cudf/tests/indexes/test_interval.py
Comment on lines
+169
to
+180
| if op in {"__eq__", "__ne__", "NULL_EQUALS", "NULL_NOT_EQUALS"}: | ||
| lefts_equal = self.left._binaryop(other.left, "NULL_EQUALS") | ||
| rights_equal = self.right._binaryop(other.right, "NULL_EQUALS") | ||
| return binaryop.binaryop( | ||
| result = binaryop.binaryop( | ||
| lefts_equal, | ||
| rights_equal, | ||
| "__and__", | ||
| get_dtype_of_same_kind(self.dtype, lefts_equal.dtype), | ||
| ) | ||
| if op in {"__ne__", "NULL_NOT_EQUALS"}: | ||
| result = ~result | ||
| return result |
There was a problem hiding this comment.
Equality logic ignores interval closed semantics.
__eq__/__ne__ currently compare only endpoints, so intervals with identical bounds but different closed values can be misclassified as equal. Please include self.closed == other.closed in the equality predicate (or short-circuit unequal-closed before endpoint comparison), and add a regression test for comparing same breaks with different closed settings.
As per coding guidelines: "Validate algorithm correctness - detect logic errors producing wrong results, silent data corruption from type coercion, and incorrect null/NA handling (cuDF uses nullable dtypes throughout)".
🤖 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/column/interval.py` around lines 169 - 180, The
equality branch in the interval column binary op (method handling op values
"__eq__", "__ne__", "NULL_EQUALS", "NULL_NOT_EQUALS") currently compares only
endpoints (self.left/right vs other.left/right) and ignores the interval
`closed` attribute; update the logic in that branch (the block using
self.left._binaryop, self.right._binaryop, and binaryop.binaryop) to include a
`closed` equality check—either short-circuit to a False/NULL-equivalent when
self.closed != other.closed for equality ops, or incorporate a `closed_equal`
predicate and AND it with the existing endpoint-equality `result` (and invert
for "__ne__"/"NULL_NOT_EQUALS"); also add a regression test asserting intervals
with identical bounds but differing `closed` values are not equal.
Comment on lines
337
to
+350
| def _Series_dtype(self): | ||
| # Fast-path to extract dtype from the current | ||
| # object without round-tripping through the slow<->fast | ||
| return _maybe_wrap_result(self._fsproxy_wrapped.dtype, None) | ||
| dtype = self._fsproxy_wrapped.dtype | ||
| if isinstance( | ||
| dtype, | ||
| ( | ||
| cudf.ListDtype, | ||
| cudf.StructDtype, | ||
| cudf.Decimal32Dtype, | ||
| cudf.Decimal64Dtype, | ||
| cudf.Decimal128Dtype, | ||
| ), | ||
| ): | ||
| dtype = self._fsproxy_slow.dtype | ||
| return _maybe_wrap_result(dtype, None) |
There was a problem hiding this comment.
Avoid mutating proxy state in Series.dtype slow fallback.
At Line 349, self._fsproxy_slow.dtype mutates _fsproxy_wrapped to slow, so a dtype read can pin the proxy to slow mode and introduce avoidable fast→slow transfers on subsequent paths.
Suggested fix
def _Series_dtype(self):
dtype = self._fsproxy_wrapped.dtype
if isinstance(
dtype,
(
cudf.ListDtype,
cudf.StructDtype,
cudf.Decimal32Dtype,
cudf.Decimal64Dtype,
cudf.Decimal128Dtype,
),
):
- dtype = self._fsproxy_slow.dtype
+ dtype = self._fsproxy_fast_to_slow().dtype
return _maybe_wrap_result(dtype, None)As per coding guidelines: "Detect unnecessary host-device data transfers and repeated GPU-to-host round-trips in hot paths".
🤖 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/pandas/_wrappers/pandas.py` around lines 337 - 350,
Series.dtype currently reads self._fsproxy_slow.dtype in a way that can mutate
the proxy and pin it to slow mode; update _Series_dtype to detect the complex
dtypes (cudf.ListDtype, cudf.StructDtype, Decimal*Dtypes) and then obtain the
dtype from a non-mutating accessor so you don't swap _fsproxy_wrapped to slow.
Concretely, inside _Series_dtype use a read-only/metadata-only call (or an
existing non-mutating helper) to peek the dtype from the slow-side
representation (referencing _fsproxy_slow) without assigning or replacing
_fsproxy_wrapped, and return that value via _maybe_wrap_result; do not perform
any assignment to _fsproxy_wrapped or other state changes.
Comment on lines
+857
to
+873
| def test_groupby_apply_series_preserves_multiindex_names(): | ||
| pdf = pd.DataFrame( | ||
| {"value": [1.0, 2.0, 3.0, 4.0]}, | ||
| index=pd.MultiIndex.from_product([range(2), range(2)]), | ||
| ) | ||
| gdf = cudf.from_pandas(pdf) | ||
| by = np.array([0, 0, 1, 1]) | ||
|
|
||
| expect = pdf.groupby(by=by, group_keys=False)["value"].apply( | ||
| lambda x: x.cumprod() | ||
| ) | ||
| got = gdf.groupby(by=by, group_keys=False)["value"].apply( | ||
| lambda x: x.cumprod() | ||
| ) | ||
|
|
||
| assert_eq(expect, got) | ||
|
|
There was a problem hiding this comment.
Strengthen this regression by using explicitly named MultiIndex levels.
Right now the test can pass without proving that public MultiIndex names are preserved. Set concrete index level names to assert the intended contract directly.
Proposed test tightening
def test_groupby_apply_series_preserves_multiindex_names():
pdf = pd.DataFrame(
{"value": [1.0, 2.0, 3.0, 4.0]},
- index=pd.MultiIndex.from_product([range(2), range(2)]),
+ index=pd.MultiIndex.from_product(
+ [range(2), range(2)],
+ names=["outer", "inner"],
+ ),
)As per coding guidelines: "python/**/test_*.py: Ensure test files provide comprehensive edge case coverage."
📝 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.
Suggested change
| def test_groupby_apply_series_preserves_multiindex_names(): | |
| pdf = pd.DataFrame( | |
| {"value": [1.0, 2.0, 3.0, 4.0]}, | |
| index=pd.MultiIndex.from_product([range(2), range(2)]), | |
| ) | |
| gdf = cudf.from_pandas(pdf) | |
| by = np.array([0, 0, 1, 1]) | |
| expect = pdf.groupby(by=by, group_keys=False)["value"].apply( | |
| lambda x: x.cumprod() | |
| ) | |
| got = gdf.groupby(by=by, group_keys=False)["value"].apply( | |
| lambda x: x.cumprod() | |
| ) | |
| assert_eq(expect, got) | |
| def test_groupby_apply_series_preserves_multiindex_names(): | |
| pdf = pd.DataFrame( | |
| {"value": [1.0, 2.0, 3.0, 4.0]}, | |
| index=pd.MultiIndex.from_product( | |
| [range(2), range(2)], | |
| names=["outer", "inner"], | |
| ), | |
| ) | |
| gdf = cudf.from_pandas(pdf) | |
| by = np.array([0, 0, 1, 1]) | |
| expect = pdf.groupby(by=by, group_keys=False)["value"].apply( | |
| lambda x: x.cumprod() | |
| ) | |
| got = gdf.groupby(by=by, group_keys=False)["value"].apply( | |
| lambda x: x.cumprod() | |
| ) | |
| assert_eq(expect, got) |
🤖 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_apply.py` around lines 857 - 873, The
test test_groupby_apply_series_preserves_multiindex_names should explicitly set
MultiIndex level names on pdf (e.g., use MultiIndex.from_product(...,
names=[...]) rather than anonymous levels) so the test verifies public level
names are preserved; update the pdf creation (and gdf from_pandas) to use named
levels and add an assertion that the resulting indexes' .names on expect and got
are equal (or ensure assert_eq compares index names) after the groupby apply,
referencing pdf, gdf, expect, and got to locate the changes.
mroeschke
approved these changes
May 29, 2026
…lf_equal Re-add two pandas test xfails that were removed before the underlying fixes landed: - test_ser_div_ser[numexpr-float32-int64]: cudf promotes float32/int64 division to float64 - test_numpy_ufuncs_basic[complex64-log1p]: GPU complex64 precision differs from CPU Skip narwhals test_self_equal[pandas] in cudf.pandas CI: under the proxy, narwhals identifies DataFrames as Implementation.PANDAS and relies on str(series.dtype) starting with 'list'/'struct'/'decimal' to detect cudf nested types. Since _Series_dtype correctly returns pandas-compatible 'object', narwhals falls through to a generic left != right comparison that is undefined for nested lists with NaN.
The pandas test suite uses numexpr (via pandas[performance]) which changes dtype promotion behavior for float32/int64 division. Conda environments don't use pip extras, so numexpr must be listed explicitly. Create test_cudf_pandas_pandas_tests group (output_types: [conda]) and add it to the 'all' includes so devcontainer environments match CI.
jameslamb
approved these changes
Jun 1, 2026
galipremsagar
approved these changes
Jun 1, 2026
Contributor
Author
|
/merge |
This was referenced Jun 2, 2026
This was referenced Jun 10, 2026
3 tasks
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
5 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
These were fixes created using #22625.
Checklist