Fix DataFrame.stack level resolution, ordering, and dtype preservation - #23370
Conversation
- Resolve level positionally: integer column-level names no longer collide with level positions (pandas' Index.get_level_values resolves integers by name first, returning 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 stacked level keys from the column MultiIndex's own levels and 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. - Emit stacked keys in appearance order like pandas, replacing the argsort-based reordering that misaligned column data for non-involution column permutations 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 so a later unstack restores the original row/column order; the legacy dropna path preserves them by masking the codes instead of gathering. Fixes 28 pandas-tests; their xfail entries are removed. Three classic categorical unstack params are un-xfailed (fixed by this change plus the MultiIndex-column-fidelity PR). 14 of the 28 also require the fidelity PR; merge that first.
|
/okay to test f8dcf59 |
@galipremsagar, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/okay to test 9ceceb7 |
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesStack and reshape compatibility
Estimated code review effort: 4 (Complex) | ~45 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 |
|
/okay to test d9f89b0 |
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/dataframe.py (1)
8314-8340: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
dropna=Falsein thisgroupby
unnamed_level_values.unique()keeps NaN-containing keys, butgroupby(..., sort=False)drops them by default. That can break partial stacks with unnamed column levels containing-1codes by leavingstackedandkeysout of sync atzip(..., strict=True). Add a regression test for a partial stack with missing unnamed-level entries.🤖 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 8314 - 8340, Update the groupby call in unnamed_group_generator to use dropna=False so NaN-containing unnamed level keys are retained and remain aligned with unnamed_level_values.unique() during the strict zip. Add a regression test covering a partial stack with missing unnamed-level entries, verifying the resulting stacked columns and keys stay synchronized.
🤖 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/dataframe.py`:
- Around line 8314-8340: Update the groupby call in unnamed_group_generator to
use dropna=False so NaN-containing unnamed level keys are retained and remain
aligned with unnamed_level_values.unique() during the strict zip. Add a
regression test covering a partial stack with missing unnamed-level entries,
verifying the resulting stacked columns and keys stay synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cb6e34ed-f212-48da-8b69-a4914228de8c
📒 Files selected for processing (2)
python/cudf/cudf/core/dataframe.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Matt711
left a comment
There was a problem hiding this comment.
Minor suggestions, otherwise looks good
- Use MultiIndex._simple_new with pre-built levels/codes instead of _from_data + post-assignment, per mroeschke's suggestion - Move levels/codes propagation into _apply_boolean_mask so any frame with a pre-set MultiIndex index preserves level fidelity through boolean-mask operations (simplifies the stack dropna path) - Add dropna=False to the groupby in unnamed_group_generator so NaN-containing unnamed-level keys are retained and stay aligned with unique() during the strict zip - Tighten keys type annotation from Any to list[tuple[Any, ...]] - Add match= arguments to pytest.raises calls in test_stack.py
|
/okay to test c119803 |
|
/merge |
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
Resolves a conflict in MultiIndex._level_index_from_level, which main rewrote in NVIDIA#23370 to fix the same negative-level wraparound. Keeps main's `norm` formulation and layers on the two things pandas' MultiIndex._get_level_number does that neither side had: distinct wording for underflow ("-3 is not a valid level number") versus overflow ("not 3"), and the ValueError for an ambiguous duplicate level name. Also restores _check_duplicate_level_names, which this branch had removed as unused. Main added two callers of it in the meantime (DataFrame.stack and reshape's unstack), and the textual merge took the deletion without conflicting.
Description
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.
levelpositionally: integer column-level names no longer collide with level positions (pandas'Index.get_level_valuesresolves integers by name first, so frames with integer level names returned data from the wrong level).IndexError) and duplicated level names (ValueError) with pandas' messages; negative out-of-bounds levels previously wrapped around silently.tilestep (which only sees codes).unstackrestores the original row/column order; the legacydropnapath 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.
Checklist