Skip to content

fix: groupby-sum fast path reduces multi-dim DataArray grouper over all its dims#824

Merged
FabianHofmann merged 2 commits into
masterfrom
fix-823-multidim-grouper
Jul 14, 2026
Merged

fix: groupby-sum fast path reduces multi-dim DataArray grouper over all its dims#824
FabianHofmann merged 2 commits into
masterfrom
fix-823-multidim-grouper

Conversation

@FabianHofmann

@FabianHofmann FabianHofmann commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Closes #823.

This aligns the grouping mechanism across fallback and non-fallback groupby operations.

Note

The following content was generated by AI.

Changes proposed in this Pull Request

LinearExpression.groupby(...).sum() returned wrong dimensions when grouping by a multi-dimensional DataArray grouper on its default (fast) path: the reduction happened over only one of the grouper's dimensions and the others survived in the result. Only use_fallback=True gave the correct result.

Root cause: the fast path did group.to_pandas(), which turns a 2-D grouper into a DataFrame that is then routed through the multikey encoding path — reducing over only the row axis. A genuinely N-D grouper (whose values define the groups jointly over all its dims) was never handled. This is a long-standing bug present since v0.8.0, not a regression from #802.

The fix:

  • Detect a genuine N-D grouper — a DataArray with ndim > 1 whose dims are all data dims — and stack the data over those dims into a single key, so the fast path reduces over every grouper dim at once. This preserves the fast path's memory benefit.
  • Grouper DataArrays whose extra axis is not a data dim (i.e. multikey frames built from a DataFrame) keep flowing through the existing DataFrame/multikey handling.
  • _grouped_sum now accepts the (stacked) dataset to operate on.

Tests

  • test_linear_expression_groupby_ndim now exercises the fast path too (use_fallback=[True, False]) and asserts it equals the fallback; the stale TODO was removed.
  • New test_linear_expression_groupby_multidim_preserves_extra_dim covers the issue's reproducer (2-D grouper over (i, j), preserved k).

Checklist

  • AI-generated content is marked (see AGENTS.md).
  • Code changes are sufficiently documented; i.e. new functions contain docstrings and further explanations may be given in doc.
  • Unit tests for new features were added (if applicable).
  • A note for the release notes doc/release_notes.rst of the upcoming release is included.
  • I consent to the release of this PR's code under the MIT license.

…sum fast path

Detect a genuine N-D grouper (all dims are data dims) and stack the data
over those dims into a single key, so the fast path reduces over every
grouper dim at once instead of routing a 2-D grouper through to_pandas()
into the multikey path (which leaked one dim into the result).
@codspeed-hq

codspeed-hq Bot commented Jul 14, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 173 untouched benchmarks
⏩ 173 skipped benchmarks1


Comparing fix-823-multidim-grouper (562c518) with master (a6ca0be)

Open in CodSpeed

Footnotes

  1. 173 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@FBumann

FBumann commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

@FabianHofmann Great that you found that bug right away. Im really sorry about the regression!

EDIT: Nevermind, seems like it wasnt my PR...

@FBumann

FBumann commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

@FabianHofmann I dived a bit deeper in and found some other issues (positional alignment when using pd.Series or xr.DataArray). This is arguably a bug, but could also be part of v1. Ill create issues for both. We should decide if we treat this as a bugfix or a semantics thing!

@FBumann

FBumann commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

See #827

@FBumann

FBumann commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

@FabianHofmann This should be merged no matter what happens in #827

@FabianHofmann FabianHofmann merged commit c02f5db into master Jul 14, 2026
23 of 24 checks passed
@FabianHofmann FabianHofmann deleted the fix-823-multidim-grouper branch July 14, 2026 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

groupby().sum() returns wrong dims for a multi-dimensional DataArray grouper (fast path)

2 participants