Preserve MultiIndex column fidelity through ColumnAccessor round trips - #23365
Merged
rapids-bot[bot] merged 6 commits intoJul 22, 2026
Merged
Conversation
Rebuilding a frame's pandas columns MultiIndex from tuples re-sorts the
levels and re-infers their dtypes, losing the exact source layout:
unsorted explicit level orders (which change the behavior of pandas
operations that work on codes, e.g. legacy stack(sort=True) after a
fast-to-slow conversion), categorical/object/int64 level dtypes (int64
levels with missing entries upcast to float64), and NaN column labels
(fresh float('nan') objects hash unequal, so lookups miss).
- prime the cached to_pandas_index with the exact source pd.MultiIndex
at DataFrame construction and propagate it through accessor copies
- restore recorded per-level dtypes in to_pandas_index when the cast
round-trips losslessly
- match NaN-containing labels under pandas' all-NaNs-equal semantics
- read level dtypes off MultiIndex.levels (get_level_values materializes
missing entries as NaN and upcasts), also for cudf.MultiIndex columns
- keep hierarchical columns through DataFrame binops when only level
dtypes differ (restored Int8 vs int64 fails Index.equals)
Fixes 13 pandas-tests; their xfail entries are removed. Attribution
verified against an isolated build containing only this change.
github-actions
Bot
added
Python
galipremsagar
added
bug
Contributor
Author
|
/okay to test 485f7fd |
|
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:
📝 WalkthroughWalkthroughColumnAccessor now supports NaN-aware label lookup and restores preserved MultiIndex level dtypes. DataFrame construction, column assignment, and binary operations retain pandas MultiIndex metadata. Regression tests and pandas-testing expected-failure mappings were updated. ChangesNaN-aware MultiIndex handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 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: 2
🤖 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_accessor.py`:
- Around line 38-45: Update _is_nan_scalar in
python/cudf/cudf/core/column_accessor.py:38-45 to recognize NaN values across
NumPy floating scalar types, while preserving existing Python-float behavior;
adjust the comparison flow at python/cudf/cudf/core/column_accessor.py:204-207
to safely handle nullable results such as pd.NA without raising before checking
later NaN labels; add or update coverage at
python/cudf/cudf/tests/private_objects/test_column_accessor.py:416-425 for NumPy
NaN labels and nullable key comparisons.
In `@python/cudf/cudf/tests/private_objects/test_column_accessor.py`:
- Around line 410-413: Update the test around gdf._data.to_pandas_index to
invoke the accessor on an uncached copy of the column/index data, ensuring the
from_tuples/set_levels dtype-restoration path executes. Keep the existing dtype
and index equality assertions unchanged.
🪄 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: c12d4a64-b856-43d5-941d-adee2551ffdb
📒 Files selected for processing (4)
python/cudf/cudf/core/column_accessor.pypython/cudf/cudf/core/dataframe.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/private_objects/test_column_accessor.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
This was referenced Jul 21, 2026
vyasr
requested changes
Jul 21, 2026
- Move the to_pandas_index cache priming into ColumnAccessor: a pandas_index constructor argument primes via _prime_to_pandas_index, which owns the multiindex/length conditions; the four DataFrame call sites pass the source index instead of reaching into the cache. - Recognize NumPy floating NaNs in _is_nan_scalar (only np.float64 subclasses python float; np.float16/np.float32 NaN labels previously bypassed the canonicalized lookup). - Treat ambiguous label comparisons as non-matches in the NaN lookup retry: a pd.NA label makes == return pd.NA, whose truthiness raises TypeError, which previously escaped before later NaN labels matched. - Reword the canonicalization docstring around the identity shortcut: no NaN compares equal to any NaN; dict/tuple comparisons short-circuit on identity, and mapping every NaN to the np.nan singleton is what makes the lookup work. - Exercise the dtype-restoration path on an unprimed accessor copy in the test, and cover NumPy floating NaN labels and pd.NA labels.
Contributor
Author
|
All review points addressed in 358a983:
Re-verified: classic dataframe/reshape/groupby-agg suites (16k tests, no regressions) and the PR's 13 pandas-tests pass strict under an isolated build of this branch. |
There was a problem hiding this comment.
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/column_accessor.py (1)
163-167: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve exact MultiIndex metadata in method copies.
ColumnAccessor.copy()has unchanged labels, so dropping its cached source index causes an avoidable tuple rebuild and loses unsorted level order.
python/cudf/cudf/core/column_accessor.py#L163-L167: forward an already-cachedto_pandas_indexthroughColumnAccessor.copy().python/cudf/cudf/tests/private_objects/test_column_accessor.py#L398-L400: assert thatgdf._data.copy()preserves the original unsorted MultiIndex.python/cudf/cudf/tests/private_objects/test_column_accessor.py#L411-L413: explicitly clear the copied accessor’s cache before asserting dtype restoration.Proposed fix
def copy(self, deep: bool = False) -> Self: data = {k: v.copy(deep=deep) for k, v in self._data.items()} return self.__class__( data=data, multiindex=self.multiindex, level_names=self.level_names, rangeindex=self.rangeindex, label_dtype=self.label_dtype, verify=False, level_dtypes=self._level_dtypes, + pandas_index=self.__dict__.get("to_pandas_index"), )- copied = ColumnAccessor(gdf._data) + copied = gdf._data.copy() pd.testing.assert_index_equal(copied.to_pandas_index, pmi, exact=True) - result = gdf._data.copy().to_pandas_index + unprimed = gdf._data.copy() + unprimed.__dict__.pop("to_pandas_index") + result = unprimed.to_pandas_indexBased on PR objectives, exact source MultiIndex fidelity must survive reconstruction.
🤖 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_accessor.py` around lines 163 - 167, Update ColumnAccessor.copy() to propagate an already-cached to_pandas_index so copied accessors preserve exact unsorted MultiIndex metadata; add the requested assertion in python/cudf/cudf/tests/private_objects/test_column_accessor.py lines 398-400, and explicitly clear the copied accessor cache before the dtype-restoration assertion at lines 411-413.
🤖 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.
Outside diff comments:
In `@python/cudf/cudf/core/column_accessor.py`:
- Around line 163-167: Update ColumnAccessor.copy() to propagate an
already-cached to_pandas_index so copied accessors preserve exact unsorted
MultiIndex metadata; add the requested assertion in
python/cudf/cudf/tests/private_objects/test_column_accessor.py lines 398-400,
and explicitly clear the copied accessor cache before the dtype-restoration
assertion at lines 411-413.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 757ff81d-9aca-4bf8-843e-771ed234a534
📒 Files selected for processing (3)
python/cudf/cudf/core/column_accessor.pypython/cudf/cudf/core/dataframe.pypython/cudf/cudf/tests/private_objects/test_column_accessor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cudf/cudf/core/dataframe.py
…-column-fidelity # Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
…-column-fidelity # Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Contributor
Author
|
/okay to test b422758 |
Contributor
Author
|
/okay to test 29bf8ea |
vyasr
approved these changes
Jul 22, 2026
Contributor
Author
|
/merge |
galipremsagar
added
5 - Ready to Merge
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jul 23, 2026
#23370) Split out of #23255 (3/6), superseding it. **Depends on #23365 (MultiIndex column fidelity)** — 14 of the 28 un-xfailed pandas-tests need both fixes, so this PR's pandas-tests job goes green once #23365 merges. - Resolve `level` positionally: integer column-level *names* no longer collide with level *positions* (pandas' `Index.get_level_values` resolves integers by name first, so frames with integer level names returned data from the wrong level). - Validate out-of-bounds integer levels (`IndexError`) and duplicated level names (`ValueError`) with pandas' messages; negative out-of-bounds levels previously wrapped around silently. - Build the stacked level keys from the column MultiIndex's own levels/codes so per-level dtypes survive: int64 levels with missing entries no longer upcast to float64, and categorical levels stay categorical through the pylibcudf `tile` step (which only sees codes). - Emit stacked keys in appearance order, matching pandas. This replaces the argsort-based reordering, which misaligned column data for non-involution column permutations (e.g. a 3-cycle) and NaN keys; pandas legacy stack sorts multi-level keys by level *codes*, not values. - Attach pandas-faithful levels/codes to the result index eagerly (the original index contributes its own levels/codes; flat indexes and the tiled level get appearance-order factorization) so a later `unstack` restores the original row/column order; the legacy `dropna` path preserves them by masking codes instead of gathering the index. Fixes 28 pandas-tests; their xfail entries are removed. Three classic categorical unstack params are un-xfailed (fixed by this change together with #23365). Attribution verified per node id against isolated builds: 14 pass with only this change, 14 need this plus #23365. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Matthew Murray (https://github.com/Matt711) URL: #23370
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jul 23, 2026
Split out of #23255 (4/6). **Depends on #23365 (fidelity), the stack PR (#23370), and #23366 (groupby agg)** — 8 of the 29 un-xfailed pandas-tests need those fixes too, so this PR's pandas-tests job goes green once they merge. - Order result rows/columns by the removed level's codes (level order preserved, missing keys first) instead of sorted values with nulls last, by encoding the integer code columns instead of the level values. - Propagate the source frame's column-axis level names into the result instead of hardcoding `None`; also fixes the `ValueError: Length of names must match number of levels` crash when unstacking MultiIndex-column frames. - Promote integer source columns to float64 when the reshape introduces missing cells (pandas' block semantics), gated on `mode.pandas_compatible`; `pivot_table`/`crosstab` opt out via a module-private `_unstack` parameter when `fill_value` fills the cells afterwards. - Preserve unused categories of the removed level in the result's column levels (pandas GH 17845); also fixes a libcudf `Column sizes don't match` crash for indexes with unused categorical categories. - Validate the level on flat-index frames (`KeyError`) and duplicated index names (`ValueError`) like pandas; `pivot` with `values=` drops the original columns-axis names. Fixes 29 pandas-tests; their xfail entries are removed, three remaining `test_stack_unstack.py` entries get real failure reasons, and two classic categorical unstack params are un-xfailed. Attribution verified per node id against isolated builds: 21 pass with only this change, 4 need the stack PR, 2 need stack+fidelity, 2 need the groupby-agg PR. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Matthew Roeschke (https://github.com/mroeschke) URL: #23368
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
4 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
Split out of #23255 (2/6).
Rebuilding a frame's pandas columns
MultiIndexfrom tuples re-sorts the levels and re-infers their dtypes, losing the exact source layout: unsorted explicit level orders (which change the behavior of pandas operations that work on level codes, e.g. legacystack(sort=True)after a fast-to-slow conversion undercudf.pandas), categorical/object/int64 level dtypes (int64 levels with missing entries upcast to float64), and NaN column labels (freshfloat('nan')objects hash unequal, so lookups miss).to_pandas_indexwith the exact sourcepd.MultiIndexatDataFrameconstruction and propagate it through accessor copies.to_pandas_indexwhen the cast round-trips losslessly.MultiIndex.levels(get_level_valuesmaterializes missing entries as NaN and upcasts), also forcudf.MultiIndexcolumns.Int8vsint64failsIndex.equals).Fixes 13 pandas-tests (constructor dict-NaN-key, concat keys with specific levels, groupby ordered multi-func aggregate, MultiIndex loc, and several
test_stack_unstack.pycases); their xfail entries are removed. Attribution verified by running each node id against an isolated build containing only this change (pass) and a clean build (fail).Independent of the other #23255 split PRs, but the stack (3/6) and unstack (4/6) PRs depend on this one.
Checklist