fix(groupby): align groupers by label, raise on mismatch (#827)#830
Merged
Conversation
groupby(...).sum() matched pd.Series/DataArray groupers to the expression by position, so a grouper whose labels were reordered silently regrouped. Validate the grouper's labels against the expression's coordinates and raise on a reorder or a different label set, unified across Series, DataFrame and DataArray groupers. The DataFrame path no longer reindexes; all grouper types now follow the same label-alignment rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merging this PR will not alter performance
Comparing Footnotes
|
Collaborator
|
@FBumann one thought that came to my mind. since here we don't have the ability to set the join convention - in contrast to |
FabianHofmann
approved these changes
Jul 16, 2026
FBumann
added a commit
that referenced
this pull request
Jul 16, 2026
Two follow-ups to the §8-exact work (#831/#834), from an open-items.md audit. Gap 1 — transition-surface hole: after #831 made a pure reorder raise under v1, reordered *constant* operands (`x + array`, `-`, `*`, `/`, rhs) were still silently reindexed by label under legacy with no warning, breaking the "no silent change" guarantee (coeff and merge already warned). Thread a `warn_reorder` flag broadcast_to_coords -> _reindex_reordered_dims, set at the arithmetic const/rhs sites; legacy now emits `_legacy_const_reorder_message` (accurately: reindexed by label, not positional). Legacy result unchanged; no double-warn; construction (bounds/mask, strict=True) stays silent. Groupers — add a strict-alignment paragraph to convention.md §13: a groupby grouper aligns to the grouped dimension by §8 (reorder or set-mismatch raises, never positional), multi-key -> flat group dim + aux coords. Implementation is #830 (on master, lands here on merge). open-items.md records the §8-exact decision and the pending grouper landing. Full suite green; ruff/mypy clean. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FBumann
added a commit
that referenced
this pull request
Jul 16, 2026
Add a Features entry announcing v1 (`options["semantics"] = "v1"`, legacy stays the default) with the strict-alignment / user-NaN / absence / aux-coord / MultiIndex summary and a link to the convention. Mark the changelog and grouper-landing (#830 merged) items done in open-items.md. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Closes #827.
@FabianHofmann We decided to NOT reindex. As all groupers should behave similarly, they now all raise if the grouper is misaligned.
But Dataframes were reindex before. We could also change this to reindex all groupers instead of raising if its a pure reorder. Users relying on the reindex will get an error now...
Warning
Potentially breaking. Groupers that were previously accepted (silently regrouping by position, or — for
DataFramegroupers — silently reindexing) now raiseValueError. Code that relied on a reordered grouper "just working" must reorder the grouper to match the expression's coordinates.Note
The following content was generated by AI.
What & why
On the default (fast) path,
groupby(...).sum()matched apd.Series/DataArraygrouper to the expression by position, ignoring labels — a reordered grouper silently regrouped and produced a wrong result (#827). The three grouper paths also disagreed:DataFramereindexed by label, the fallback raised.This makes all grouper types follow one rule, per the issue decision ("Don't reindex, just check size and index fits — else raise"): match by label, never reindex, raise on mismatch.
_check_grouper_alignment(group, data)validates each shared dimension's labels against the expression's coordinates and raises a clearValueError, distinguishing "same labels in a different order" from "a different set of labels". Runs on both the fast path anduse_fallback=True(replacing the fallback's misleading "length does not match" message).pd.Series,pd.DataFrameand 1-D/N-DDataArray. A grouper without an index along a dim has nothing to align by and keeps the positional match._encode_multikey_groupno longer reindexes (alignment is now guaranteed upstream).Breaking change
DataFramegroupers previously reindexed a reordered index silently; they now raise like the others.test_linear_expression_groupby_with_dataframe_non_alignedwas updated to assert the raise.Tests
New
TestGroupbyGrouperAlignment: reordered Series/DataFrame/DataArray raise, N-D reorder raises, mismatched label-set raises, aligned + unindexed groupers still work (parametrized overuse_fallback). Fulltest_linear_expression.pyand all-k grouptests pass;ruffandmypyclean.