fix: validate the BinnedGroup navigation array - #5802
Open
andiwand wants to merge 4 commits into
Open
Conversation
Contributor
The navigation array passed to `BinnedGroup` lists, per axis, the local bin indices to visit and in which order. The indices are 1-based -- the default that `BinnedGroup` fills in for an undefined axis is a `std::iota` from 1, and `GridBinFinder` looks its per-bin neighbour vectors up as `val[locBin - 1]`. Nothing enforced that convention: the array was moved into the member without any check. The consequences of getting it wrong were quiet rather than loud: - Index 0 is the underflow bin. It is never filled, since the space point grid rejects out-of-range positions, so listing it silently drops a slice of the detector from the seeding loop with no diagnostic. The only guard was an `assert` in `GridBinFinder::getSizePerAxis`, which is compiled out in the optimized builds where this actually runs. - An index past the last bin, or a repeated index, was equally unreported. Validate the array in both constructors instead, throwing `std::invalid_argument` with a message naming the axis, the offending bin and the valid range. The empty-vector default is unchanged. All in-tree configurations already respect the convention, including the ITk pixel and strip setups in `Python/Examples/python/itk.py`, so this only rejects inputs that were already misconfigured. Also adds the first unit test coverage for `BinnedGroup`, pinning down the visiting order for both the default and a custom navigation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lf7fWANWDMYH8HaasmEb9c
andiwand
force-pushed
the
fix/binned-group-navigation-validation
branch
from
August 4, 2026 07:01
19f2b90 to
93b2227
Compare
Contributor
Public API surface diffNo change to the public API surface. ✅ |
benjaminhuth
previously approved these changes
Aug 4, 2026
benjaminhuth
left a comment
benjaminhuth
left a comment
Member
There was a problem hiding this comment.
LGTM, small naming suggestion
Addresses review feedback: the name now says what is being validated and completed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JtMZozesV2rxUdyjyy9mqJ
|
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.
The
navigationarray handed toBinnedGrouplists, per axis, which local bins to visit and in which order. Those indices are 1-based — the defaultBinnedGroupfills in is astd::iotafrom1ul, andGridBinFinderlooks upval[locPosition[i] - 1ul]— but nothing enforced it.Getting it wrong failed quietly. Index
0is the underflow bin, which is never filled, so listing it just drops a slice of the detector from the seeding loop with no error or warning. The only guard was anassertinGridBinFinder::getSizePerAxis, compiled out in exactly the optimized builds where seeding runs. An index past the last bin, or a repeated one, was equally unreported.This validates the array in both constructors, throwing
std::invalid_argumentnaming the axis, the bin and the valid range. The empty-vector default is unchanged, and the fill-in logic is factored into one helper.--- END COMMIT MESSAGE ---
Compatibility
All in-tree configs (
itk.pypixel, pixel high-occupancy, strip) are already valid 1-based, so nothing that works today starts throwing. Same for the six seeding configurations that feed this array from Athena'sGridTripletSeedingTool— though Athena's own guard isi >= zBinEdges.size(), which accepts0, so a matching fix is going in there separately.Note for reviewers
Rejecting duplicates is the one judgement call here rather than an obvious bug — say the word if you'd rather that stayed permissive and I'll drop it to out-of-range checking only.
Tests
BinnedGrouphad no unit test coverage, so this addsTests/UnitTests/Core/Seeding/BinnedGroupTests.cpp: the three rejection cases, the mask-constructor path, and two visiting-order tests.