Repository navigation
Reject bin-edge outer coords in combine_bins fast path - #3956
Merged
Merged
Conversation
jokasimr
approved these changes
Aug 18, 2026
SimonHeybrock
force-pushed
the
3955-reject-bin-edge-outer-coords
branch
from
August 25, 2026 09:17
c84fbc5 to
60947b7
Compare
Since combine_bins was introduced, bin and group have accepted a bin-edge outer coord, which the C++ implementation rejects with BinEdgeError. Which semantics applied depended on whether the fast path happened to be taken, and the fast path itself was inconsistent: flattening a single dim silently used the coord's first values, one per input bin, while flattening more than one dim dropped the coord and failed with a confusing KeyError. Reject such coords so that binning falls through to the C++ implementation, which reports the problem and how to fix it. The filtering docs relied on the silent behavior. Pass the leading values explicitly, which labels each time interval by the strain at its start, as before, and matches the interpolation used earlier in that notebook. Fixes #3955 Co-Authored-By: Claude Opus 5 <[email protected]>
Labelling each time interval by the strain at its start is not an arbitrary choice: it matches the mode='previous' interpolation that assigns a strain to each event, so that numerator and denominator of the normalization agree. Co-Authored-By: Claude Opus 5 <[email protected]>
SimonHeybrock
force-pushed
the
3955-reject-bin-edge-outer-coords
branch
from
August 25, 2026 09:50
60947b7 to
e2827d8
Compare
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
2 participants
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.
binandgrouphave accepted a bin-edge outer coord ever since thecombine_binsfast path was introduced, while the C++ implementation rejects such input withBinEdgeError. Which semantics applied therefore depended on whether the fast path happened to be taken, and the fast path itself was inconsistent: flattening a single dim silently used the coord's first values, one per input bin, whereas flattening more than one dim dropped the coord and failed later with a confusingKeyError: "Expected 'y' in <scipp.Dict.keys {}>.".Reject such coords in
_can_operate_on_binsso that binning falls through to the C++ implementation, which explains the problem and how to fix it.Breaking change. Code that binned or grouped by a bin-edge outer coord and relied on the fast path now raises
BinEdgeError. The fix is to pass a non-edge coord, choosing explicitly which value represents each bin. Our own filtering docs relied on this, and are updated accordingly; the numbers in that notebook are unchanged.Stacked on #3949, which marks these cases as
xfail. Review/merge that one first.Fixes #3955
Test plan
xfailmarkers added in Fix OOM in multi-dim hist depending on argument order #3949 are removed; the tests now pass.docs/user-guide/binned-data/filtering.ipynbwas executed against this branch and produces results identical tomain.