Match pandas ordering, dtypes, and validation in unstack/_pivot - #23368
Conversation
- 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 level values. - Propagate the source frame's column-axis level names into the result instead of hardcoding None; also fixes the 'Length of names must match number of levels' crash for 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. - Preserve unused categories of the removed level (pandas GH 17845); also fixes a libcudf 'Column sizes don't match' crash for 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, and three remaining test_stack_unstack entries get real failure reasons. Two classic categorical unstack params are un-xfailed. Depends on the MultiIndex-column-fidelity, stack, and groupby-agg split PRs of NVIDIA#23255 for entangled tests.
|
/okay to test 2f3a872 |
|
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 (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughPivot and unstack now use encoded axes for pandas-compatible ordering, preserve MultiIndex metadata, and conditionally promote integer results when missing cells occur. Pivot-table routing, reshape tests, and expected-failure mappings were updated accordingly. ChangesReshape compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/reshape.py`:
- Around line 1374-1381: Update the flat-index branch around
_validate_index_level to validate every level value, including integers, by
removing the is_integer(level) guard. Preserve the existing level normalization
and let _validate_index_level handle positional integer bounds and name-based
validation before df.T.stack executes.
- Around line 1219-1228: Update the _pivot call in pivot() to pass
promote_ints_on_missing=True, ensuring integer columns introduced with missing
values are promoted to float64 under pandas-compatible behavior. Preserve the
existing selection, encoding, and reshape arguments.
🪄 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: 30df6cae-74e0-42c2-9fb4-52ec15d610db
📒 Files selected for processing (4)
python/cudf/cudf/core/reshape.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/reshape/test_pivot_table.pypython/cudf/cudf/tests/reshape/test_unstack.py
…ndas-ordering # Conflicts: # python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
- Pass promote_ints_on_missing=True from pivot(): pandas' pivot also promotes integer values to float64 when the reshape introduces missing cells (it reshapes via unstack internally). - Gate the unused-categories column rebuild on the cheap result._data.multiindex flag and only materialize the removed level / pandas column index inside that block.
swaplevel rebuilt the result from the level columns' accessor keys and only restored names when every name was None. An index whose levels are keyed positionally but carry real names - e.g. the index produced by an unstack/stack round trip - came back with the positional keys as its names, and name-based level lookups silently swapped nothing. Resolve the levels with _level_index_from_level (name first, then positional with bounds checking, like pandas) and always carry the swapped names over from self.names. Fixes the re-enabled pandas test tests/frame/test_stack_unstack.py::TestStackUnstackMultiLevel::test_stack[True] against the merged stack changes and makes invalid levels raise KeyError/IndexError like pandas.
|
/okay to test a73c75a |
|
/merge |
Description
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.
None; also fixes theValueError: Length of names must match number of levelscrash when unstacking MultiIndex-column frames.mode.pandas_compatible;pivot_table/crosstabopt out via a module-private_unstackparameter whenfill_valuefills the cells afterwards.Column sizes don't matchcrash for indexes with unused categorical categories.KeyError) and duplicated index names (ValueError) like pandas;pivotwithvalues=drops the original columns-axis names.Fixes 29 pandas-tests; their xfail entries are removed, three remaining
test_stack_unstack.pyentries 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.Checklist