Fix MultiIndex indexing/.loc to match pandas - #22966
Merged
Merged
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
github-actions
Bot
added
Python
galipremsagar
marked this pull request as ready for review
June 24, 2026 14:42
|
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:
📝 WalkthroughWalkthroughcuDF updates MultiIndex ChangesMultiIndex .loc and metadata parity
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
🤖 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/dataframe.py`:
- Around line 4169-4172: The MultiIndex level rename path in DataFrame.rename is
missing the all-null replacement handling that the generic index path already
uses. Update the ca[ca_label] = level_values._column.find_and_replace call in
dataframe.py to pass the same is_all_na flag used elsewhere in this rename
logic, so rename(index={old: None}, level=...) behaves consistently for all-null
replacements.
- Around line 280-289: The row selection logic in DataFrame indexing is
collapsing a single match to a scalar too early for MultiIndex rows. Update the
branch around the existing row label handling in cudf.DataFrame so that the
`row_is_full_label` check only treats a scalar `row_arg` as a full label when
the index is not a MultiIndex or when all MultiIndex levels are explicitly
provided; for partial MultiIndex row keys, keep the result as a length-1 Series.
Preserve the current behavior in the `result = result[result._column_names[0]]`
path, but gate the final `element_indexing(0)` collapse behind the stricter
full-label condition.
In `@python/cudf/cudf/core/multiindex.py`:
- Around line 982-995: The all-scalar MultiIndex selection path in multiindex.py
is incorrectly treating duplicate matches as the empty-Series case when
len(result) > 1, which drops valid rows. Update the logic in the scalar-collapse
branch around the len(keep) == 0 handling so that duplicate-row results are
preserved instead of falling through to the empty Series return; use the
existing result object from the MultiIndex lookup and ensure full scalar keys
return all matched rows consistently.
- Around line 802-824: In the MultiIndex lookup path inside the row-tuple
handling logic, all-wildcard keys like a tuple of slice(None) currently produce
an empty positions/lookup_dict and fall through to a 0-column merge instead of
returning the full row selection. Update the branch that builds lookup_dict so
that when every element of row_tuple is a wildcard, it short-circuits to an
identity selection in the MultiIndex access path (for example, by returning the
original rows or equivalent full-row mask) before calling the merge logic. Use
the existing row_tuple, positions, and lookup_dict flow to locate the fix.
In `@python/cudf/cudf/core/series.py`:
- Around line 307-315: The full-scalar tuple path in Series.loc is collapsing
duplicate matches by unconditionally taking result.iloc[0], which silently drops
rows for non-unique MultiIndex keys. Update the lookup logic in the Series
indexing code around the tuple/scalar check so it only collapses to a scalar
when the match is truly a single row, and preserves the full Series when the
full key resolves to multiple rows. Use the existing Series.loc / result
handling and the tuple-is-scalar branch to locate the fix.
🪄 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: 63410b90-e8f5-401e-a76d-21d8967bf738
📒 Files selected for processing (11)
python/cudf/cudf/core/dataframe.pypython/cudf/cudf/core/frame.pypython/cudf/cudf/core/indexed_frame.pypython/cudf/cudf/core/multiindex.pypython/cudf/cudf/core/series.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/dataframe/indexing/test_loc.pypython/cudf/cudf/tests/dataframe/methods/test_rename.pypython/cudf/cudf/tests/indexes/multiindex/methods/test_argsort.pypython/cudf/cudf/tests/indexes/multiindex/methods/test_contains.pypython/cudf/cudf/tests/series/indexing/test_loc.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Contributor
Author
|
/okay to test a3c5e57 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cudf/cudf/core/dataframe.py (1)
268-276: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH: Treat full MultiIndex column tuples as single labels here.
This special full-row-tuple-list path only downcasts scalar column keys, so
df.loc[[row_tuple], full_column_tuple]with MultiIndex columns can return a one-columnDataFrameinstead of the pandas-compatibleSeries.Proposed fix
- if ( - isinstance(arg, tuple) - and len(arg) > 1 - and is_scalar(arg[1]) - ): + if isinstance(arg, tuple) and len(arg) > 1: + col_key = arg[1] + col_nlevels = self._frame._data.nlevels + col_is_single_label = ( + is_scalar(col_key) + if col_nlevels == 1 + else ( + isinstance(col_key, tuple) + and len(col_key) == col_nlevels + and all(is_scalar(x) for x in col_key) + ) + ) + else: + col_is_single_label = False + if col_is_single_label: # A scalar column key yields a single column, so # downcast the result to a Series. - return result[arg[1]] + return result[result._column_names[0]]🤖 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/dataframe.py` around lines 268 - 276, The tuple-key downcast in DataFrame.loc handling only checks is_scalar(arg[1]), so full MultiIndex column tuples are treated as multi-column keys instead of single labels. Update the special case in dataframe.py around the result[arg[1]] path to recognize a full column tuple as a single column label when the columns are a MultiIndex, and downcast to the Series result accordingly. Keep the change localized to the row-tuple-list branch that already handles scalar column keys.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/pandas/scripts/pandas-testing-plugin.py`:
- Line 5369: The entry for
tests/indexes/test_setops.py::TestSetOps::test_symmetric_difference[multi] is
incorrectly listed in NODEIDS_TO_SKIP even though it xpasses with cudf.pandas.
Remove this nodeid from the skip mapping in pandas-testing-plugin.py and place
it in the xpass/xfail tracking path used for pandas xfails that pass under
cudf.pandas, keeping the existing reason string consistent with the other
tracking entries.
---
Outside diff comments:
In `@python/cudf/cudf/core/dataframe.py`:
- Around line 268-276: The tuple-key downcast in DataFrame.loc handling only
checks is_scalar(arg[1]), so full MultiIndex column tuples are treated as
multi-column keys instead of single labels. Update the special case in
dataframe.py around the result[arg[1]] path to recognize a full column tuple as
a single column label when the columns are a MultiIndex, and downcast to the
Series result accordingly. Keep the change localized to the row-tuple-list
branch that already handles scalar column keys.
🪄 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: 82f279b4-50f1-4bca-af71-7c3eda87d3da
📒 Files selected for processing (3)
python/cudf/cudf/core/dataframe.pypython/cudf/cudf/core/multiindex.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cudf/cudf/core/multiindex.py
mroeschke
reviewed
Jun 30, 2026
mroeschke
left a comment
mroeschke
left a comment
Contributor
There was a problem hiding this comment.
I did a first pass. Still need to fully digest all the MultiIndex changes here. If any fixes are independent and could be split into another PR that would be helpful
3 tasks
…nt, merge loc tests; split out argsort
3 tasks
Contributor
Author
|
Thanks for the review! Split out two independent fixes into their own PRs: argsort dtype in #23061 and DataFrame.rename in #23062. The remaining MultiIndex .loc and contains changes are interdependent (contains delegates to get_loc, which uses the updated _compute_validity_mask), so they stay here. Inline comments addressed above. |
# Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Contributor
Author
|
/okay to test 667d2f5 |
Contributor
Author
|
/okay to test 94189ac |
Contributor
Author
|
/okay to test 18605de |
Contributor
Author
|
/okay to test 840f4f2 |
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jul 2, 2026
Split out of #22966 per review feedback. `Frame.argsort` (and `Series.argsort`/`Index.argsort`, which delegate to it) now returns an `np.intp` (int64) positional indexer, matching numpy's and pandas' `argsort`. Previously cuDF returned an `int32` gather-map dtype, which broke dtype-strict comparisons (e.g. `assert_numpy_array_equal`) against pandas. This is applied unconditionally (not gated on `mode.pandas_compatible`) per review. The `argsort`/`sort_values(return_indexer=True)` docstrings are updated to the new dtype, and the now-passing `cudf.pandas` xfail entries are removed. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Matthew Murray (https://github.com/Matt711) URL: #23061
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jul 2, 2026
Split out of #22966 per review feedback. `DataFrame.rename(index=..., level=...)` on a MultiIndex with an **unnamed** level previously inserted a spurious extra level instead of overwriting the target level: the level *name* (`None` for an unnamed level) was used as the `ColumnAccessor` key. It now resolves to the positional ColumnAccessor label via `MultiIndex._level_to_ca_label`, restores the original level names afterward, and forwards `all_nan` to `find_and_replace` so all-null replacements behave like the non-level path. Adds a cuDF unit test and removes the now-passing `cudf.pandas` xfail entry. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: #23062
Contributor
Author
|
/okay to test a2ff7bc |
# Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Contributor
Author
|
/okay to test ae14ee8 |
mroeschke
approved these changes
Jul 13, 2026
Contributor
Author
|
/okay to test 87bc09f |
Contributor
Author
|
/merge |
galipremsagar
added
the
5 - Ready to Merge
3 tasks
This was referenced Jul 21, 2026
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 a range of
MultiIndex.loc/__getitem__incompatibilities surfaced by thecudf.pandastests/indexing/multiindexsuite (72 → 14 failures, no cuDF regressions). Behavior changes that would affect the classic fast path are gated onmode.pandas_compatible.MultiIndex.__contains__: handle partial/full tuple keys (delegate toget_loc).locrow lookup (pandas-compatible): cartesian-product of per-level keys, de-duplicated labels, pandas-matching result order, andKeyErroron missing labels/combinationsSeries;Series.loc/single-tuple.locdrop scalar-selected index levelssetindexers withTypeError; raiseKeyErroron a missing scalar row label instead of falling back to positional indexingNotImplementedErrorfor strided/reversedMultiIndexlabel slicesMultiIndexcolumns keep their per-level dtypestackmaterializes levels in first-appearance order so a laterto_pandas/unstackno longer lexicographically reorders the pivoted axisAlso drops the now-passing entries from the
cudf.pandasxfail list and adds cuDF unit tests.The
argsort(np.intpreturn) andDataFrame.renameMultiIndex fixes from the original branch have been split into separate PRs.Checklist