Fix scalar-broadcast fill after a non-inclusive axis (scikit-hep/boost-histogram#960)#431
Fix scalar-broadcast fill after a non-inclusive axis (scikit-hep/boost-histogram#960)#431NJManganelli wants to merge 2 commits into
Conversation
In the vectorized fill (fill_n), a scalar argument is broadcast across an axis by computing that axis' contribution to the linear index once and adding it to every entry. The contribution was derived from the change of the first index (*begin_). When a previous axis had already invalidated some entries, including the first one, the broadcast axis then invalidated *all* entries, silently discarding valid data. This is scikit-hep/boost-histogram#960: a non-growing axis combined with out-of-bounds entries and a broadcast scalar fill produced a sum of zero. Compute the axis contribution on a fresh index instead and add it to all entries, leaving already-invalid entries untouched. The growing and non-growing scalar paths share the same code via linearize/linearize_growth. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Compare the vectorized fill against the one-at-a-time reference for scalar broadcast after a non-inclusive axis, covering the growing-axis, zero-point-shift, and underflow variants. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I asked Claude to match the comment style of the repo, I don't know that it really did (and it left a comment I asked it to remove regarding a now-gone commit that templated a true/false growable axis to no real gain). I'll be ready to have Claude update based on any adversarial review, human feedback, etc. |
| linearize(idx, stride_, axis_, try_cast<value_type, std::invalid_argument>(value)); | ||
| } | ||
| if (is_valid(idx)) { | ||
| const auto delta = static_cast<std::intptr_t>(static_cast<std::size_t>(idx)) - |
There was a problem hiding this comment.
Is this relying on two's complement of the signed -> unsigned -> signed cast or just a superfluous cast? I need another comment here that explains that this double cast is not superfluous but intentional.
| // underflow bin is -stride_ and the index type is unsigned. The bias is | ||
| // subtracted again when the delta is computed. | ||
| index_type idx{stride_}; | ||
| if (IsGrowing::value) { |
There was a problem hiding this comment.
looks like this replaces call_2 completely, in that case, those call_2 functions should be removed.
There was a problem hiding this comment.
Likewise, call_1 then doesn't need to be called call_1 anymore, just call_impl.
|
@NJManganelli Looks good, please resolve the comments, though. The AI tends to not clean up after itself, always afraid to remove things. However, all API inside the detail namespace is fair game, even if public. |
2 participants
Problem
In the vectorized fill (
detail/fill_n.hpp), a scalar argument is broadcast across anaxis by computing that axis' contribution to the linear index once and adding it to
every entry. The contribution was derived from the change of the first index
(
*begin_). When a previous axis had already invalidated some entries — including thefirst one — the broadcast axis then invalidated all entries and silently dropped
valid data.
This surfaced downstream as scikit-hep/boost-histogram#960
("Histogram with IntCategory and StrCategory axes can be zeroed by out-of-bounds fill
values"): a non-growing category axis combined with out-of-bounds entries and a
broadcast scalar on a second axis produced a sum of zero. One-at-a-time filling was
always correct; only the array/
fill_npath was affected.Fix
Compute the axis contribution on a fresh index and add it to all entries, leaving
already-invalid entries untouched. The growing and non-growing scalar paths now share
one code path through
linearize/linearize_growth, so the "delta from*begin_"heuristic is gone entirely.
Regression tests compare the vectorized fill against the one-at-a-time reference for
scalar-broadcast-after-non-inclusive-axis, including the growing-axis, zero-point-shift,
and underflow variants (static and dynamic histograms).
Note
The reported bug is in the Python bindings, but the defect is here in core
fill_n;consuming it there is just a submodule bump. Happy to open the companion PR against
scikit-hep/boost-histogram once this lands.
🤖 Root-caused and drafted with Claude Code; reviewed by @NJManganelli.