Skip to content

Honor the dim argument when histogramming by a dense coord - #3959

Merged
SimonHeybrock merged 1 commit into
3955-reject-bin-edge-outer-coordsfrom
3958-hist-dim-arg-with-dense-coord
Aug 25, 2026
Merged

SimonHeybrock merged 1 commit into
3955-reject-bin-edge-outer-coordsfrom
3958-hist-dim-arg-with-dense-coord

Conversation

@SimonHeybrock

@SimonHeybrock SimonHeybrock commented Aug 19, 2026 •

Copy link
Copy Markdown
Member

Fixes #3958.

ADR 0018 defines dim as the dimensions to be replaced, so dim=() must preserve all input dims and add a new one. bin, group and nanhist do this; hist did not, whenever the coord used for histogramming is a dense (outer) coord of binned data rather than an event coord. The tabulated ADR row (x,) | (x,) | (x,y) | .hist(y=y, dim=()) was violated, and the same call with a multi-dimensional coord raised DimensionError instead — including the exact example the hist docstring uses to advertise dim=().

In that situation make_histogrammed summed the bins and handed the result to the C++ dense histogram, which always replaces all dims of the coord by the new dim. erase only drove a flatten beforehand, so dims outside it could not survive. Bin instead when erase does not already cover the coord's dims, which is what nanhist (bin followed by nansum) has always done. The dense fast path is kept when erase does cover them, so the common dim=None case is unaffected — hist(t=10) over 5M events stays at 2 ms rather than moving event data.

This is not a regression from a recent optimization: the fallback dates from #3020 (2023) and predates the ADR. It went unnoticed because test_op_on_binned_with_explicit_dim_arg_yields_expected_output_dims leaves the event coord in place, so the fallback branch was never exercised. This PR adds the same table without the event coord.

Two consequences beyond the bug itself:

Not in scope: dense input

Dense (non-binned) data has the same gap on a separate code path, which this PR does not touch. There bin, group and nanhist are non-conforming too, so routing through bin is not an option and a different mechanism is needed. Tracked in #3960, reproduced here so the boundary of this PR is clear.

bin, group and nanhist agree in every cell, so they share a column. ! marks a result that violates ADR 0018. Kind is the dividing line: degenerate means the coord is constant across everything being summed (coord.dims ∩ dim = ∅), so each preserved position feeds exactly one z bin and the output is the input scattered one-hot. Meaningful means the coord varies over a summed dim, so the histogram does real work.

data coord z dim ADR expected bin/group/nanhist hist kind proposed
(x,) (x,) () (x,z) (z,) ! (z,) ! degenerate raise
(x,) (x,) 'x' (z,) (z,) (z,) meaningful unchanged
(x,y) (x,) () (x,y,z) (y,z) ! (y,z) ! degenerate raise
(x,y) (x,) 'x' (y,z) (y,z) (y,z) meaningful unchanged
(x,y) (x,) 'y' (x,z) (z,) ! DTypeError ! degenerate raise
(x,y) (x,) ('x','y') (z,) (z,) (z,) meaningful unchanged
(x,y) (y,) () (x,y,z) (x,z) ! (x,z) ! degenerate raise
(x,y) (y,) 'x' (y,z) (z,) ! DTypeError ! degenerate raise
(x,y) (y,) 'y' (x,z) (x,z) (x,z) meaningful unchanged
(x,y) (y,) ('x','y') (z,) (z,) (z,) meaningful unchanged
(x,y) (x,y) () (x,y,z) (x,z) ! DimensionError ! degenerate raise
(x,y) (x,y) 'x' (y,z) (z,) ! (y,z) meaningful fix bin/group/nanhist
(x,y) (x,y) 'y' (x,z) (x,z) (x,z) meaningful unchanged
(x,y) (x,y) ('x','y') (z,) (z,) (z,) meaningful unchanged

Binned input conforms in all twelve equivalent cells after this PR, degenerate ones included. The asymmetry is intended: binned data composes the degenerate case with useful edges, which is exactly what #3958 was, whereas dense data cannot.

Stacked on #3956, which is stacked on #3949. Review/merge those first.

Test plan

  • New parametrized test covering the full dim table for bin, group, hist and nanhist on binned data whose histogramming coord exists only as an outer coord, over 1-D and 2-D coords. All three parametrizations fail on the base branch.
  • The hist docstring gains a dim=() example, so the documented behaviour is now checked by the doctests.

@SimonHeybrock
SimonHeybrock marked this pull request as draft August 19, 2026 05:07
@SimonHeybrock
SimonHeybrock marked this pull request as ready for review August 19, 2026 05:47
@SimonHeybrock
SimonHeybrock force-pushed the 3958-hist-dim-arg-with-dense-coord branch from f432ed4 to 79e1843 Compare August 25, 2026 09:17
When the coord used for histogramming is not an event coord,
make_histogrammed summed the bins and let the C++ dense histogram pick the
dims to replace, which always consumes all dims of the coord. Dims outside
`erase` were therefore erased anyway, and a multi-dimensional coord raised
DimensionError. Bin instead when `erase` does not already cover the coord's
dims, mirroring what nanhist has always done.

The reordering guard added in #3949 is no longer needed: which edges come
last no longer decides which dims the result has.

Fixes #3958
@SimonHeybrock
SimonHeybrock force-pushed the 3958-hist-dim-arg-with-dense-coord branch from 79e1843 to 54051c7 Compare August 25, 2026 09:50
@SimonHeybrock
SimonHeybrock merged commit 08cb238 into main Aug 25, 2026
4 checks passed
@SimonHeybrock
SimonHeybrock deleted the 3958-hist-dim-arg-with-dense-coord branch August 25, 2026 10:22
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.

hist ignores the dim argument when histogramming by a dense coord (ADR 0018 violation)

2 participants