Repository navigation
Honor the dim argument when histogramming by a dense coord - #3959
Merged
SimonHeybrock merged 1 commit intoAug 25, 2026
Merged
SimonHeybrock merged 1 commit into
SimonHeybrock merged 1 commit into
Conversation
SimonHeybrock
marked this pull request as draft
August 19, 2026 05:07
SimonHeybrock
marked this pull request as ready for review
August 19, 2026 05:47
SimonHeybrock
force-pushed
the
3958-hist-dim-arg-with-dense-coord
branch
from
August 25, 2026 09:17
f432ed4 to
79e1843
Compare
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
force-pushed
the
3958-hist-dim-arg-with-dense-coord
branch
from
August 25, 2026 09:50
79e1843 to
54051c7
Compare
jokasimr
approved these changes
Aug 25, 2026
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.
Fixes #3958.
ADR 0018 defines
dimas the dimensions to be replaced, sodim=()must preserve all input dims and add a new one.bin,groupandnanhistdo this;histdid 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 raisedDimensionErrorinstead — including the exact example thehistdocstring uses to advertisedim=().In that situation
make_histogrammedsummed the bins and handed the result to the C++ densehistogram, which always replaces all dims of the coord by the new dim.eraseonly drove aflattenbeforehand, so dims outside it could not survive. Bin instead whenerasedoes not already cover the coord's dims, which is whatnanhist(binfollowed bynansum) has always done. The dense fast path is kept whenerasedoes cover them, so the commondim=Nonecase 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_dimsleaves 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:
xfailcases from Fix OOM in multi-dim hist depending on argument order #3949 now passes and is strengthened to check the values against a reference that broadcasts the outer coords onto the events, as requested in review. The remainingxfailthere (re-binning a dim drops coords defined over it) is unaffected, so hist depends on argument order when an edge coord is defined on a re-binned dim #3952 stays open.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,groupandnanhistare non-conforming too, so routing throughbinis not an option and a different mechanism is needed. Tracked in #3960, reproduced here so the boundary of this PR is clear.bin,groupandnanhistagree 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 onezbin and the output is the input scattered one-hot. Meaningful means the coord varies over a summed dim, so the histogram does real work.zdim(x,)(x,)()(x,z)(z,)!(z,)!(x,)(x,)'x'(z,)(z,)(z,)(x,y)(x,)()(x,y,z)(y,z)!(y,z)!(x,y)(x,)'x'(y,z)(y,z)(y,z)(x,y)(x,)'y'(x,z)(z,)!DTypeError!(x,y)(x,)('x','y')(z,)(z,)(z,)(x,y)(y,)()(x,y,z)(x,z)!(x,z)!(x,y)(y,)'x'(y,z)(z,)!DTypeError!(x,y)(y,)'y'(x,z)(x,z)(x,z)(x,y)(y,)('x','y')(z,)(z,)(z,)(x,y)(x,y)()(x,y,z)(x,z)!DimensionError!(x,y)(x,y)'x'(y,z)(z,)!(y,z)(x,y)(x,y)'y'(x,z)(x,z)(x,z)(x,y)(x,y)('x','y')(z,)(z,)(z,)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
dimtable forbin,group,histandnanhiston 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.histdocstring gains adim=()example, so the documented behaviour is now checked by the doctests.