From 8232088c159d8c38c1218a9000f160327cf866fc Mon Sep 17 00:00:00 2001 From: Nick Manganelli Date: Tue, 14 Jul 2026 07:43:25 +0200 Subject: [PATCH 1/2] Fix scalar-broadcast fill after a non-inclusive axis (#960) 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 --- include/boost/histogram/detail/fill_n.hpp | 42 ++++++++++++++++------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/include/boost/histogram/detail/fill_n.hpp b/include/boost/histogram/detail/fill_n.hpp index e5a540bc..ec73ff59 100644 --- a/include/boost/histogram/detail/fill_n.hpp +++ b/include/boost/histogram/detail/fill_n.hpp @@ -103,19 +103,37 @@ struct index_visitor { void call_1(std::true_type, const T& value) const { // T is compatible value; fill single value N times - // Optimization: We call call_2 only once and then add the index shift onto the - // whole array of indices, because it is always the same. This also works if the - // axis grows during this operation. There are no shifts to apply if the zero-point - // changes. - const auto before = *begin_; - call_2(IsGrowing{}, begin_, value); - if (is_valid(*begin_)) { - // since index can be std::size_t or optional_index, must do conversion here - const auto delta = - static_cast(*begin_) - static_cast(before); - for (auto it = begin_ + 1; it != begin_ + size_; ++it) *it += delta; - } else + // Compute the contribution of this axis to the linear index once on a + // fresh index and add it to the whole array of indices. The contribution + // must not be derived from the change of *begin_, because a previous axis + // may have already invalidated some entries, including the first one + // (scikit-hep/boost-histogram#960). Entries that are already invalid stay + // invalid, all other entries receive the same contribution. + // + // The fresh index is biased by stride_, because the contribution of an + // 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) { + // IsGrowing::value is a compile-time constant, the dead branch is + // eliminated; linearize_growth also handles non-growing axes. + axis::index_type shift; + linearize_growth(idx, shift, stride_, axis_, + try_cast(value)); + // No index shifts to apply if the zero-point changes, since all entries + // receive the same contribution which is computed after the growth, but + // the shift must be recorded so that the storage is resized correctly. + if (shift > 0) *shift_ += shift; + } else { + linearize(idx, stride_, axis_, try_cast(value)); + } + if (is_valid(idx)) { + const auto delta = static_cast(static_cast(idx)) - + static_cast(stride_); + for (auto it = begin_; it != begin_ + size_; ++it) *it += delta; + } else { std::fill(begin_, begin_ + size_, invalid_index); + } } template From 9695104442e4f1c9d883b1a5684594a8833ddc42 Mon Sep 17 00:00:00 2001 From: Nick Manganelli Date: Tue, 14 Jul 2026 07:43:25 +0200 Subject: [PATCH 2/2] Add regression tests for scikit-hep/boost-histogram#960 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 --- test/histogram_fill_test.cpp | 191 +++++++++++++++++++++++++++++++++++ 1 file changed, 191 insertions(+) diff --git a/test/histogram_fill_test.cpp b/test/histogram_fill_test.cpp index d647412f..9d97f7a0 100644 --- a/test/histogram_fill_test.cpp +++ b/test/histogram_fill_test.cpp @@ -44,7 +44,10 @@ using ing = axis::integer; using cs = axis::category; +using cs0 = axis::category; using csg = axis::category; +using ci = axis::category; +using ci0 = axis::category; struct axis2d { auto size() const { return axis::index_type{2}; } @@ -345,6 +348,191 @@ void run_tests(const std::vector& x, const std::vector& y, } } +// Test for GitHub issue scikit-hep/boost-histogram#960: +// Filling with scalar value for one axis and array for another, where array contains +// out-of-bounds values and axis has no overflow, should correctly accumulate valid +// entries instead of producing sum of zero. +template +void run_tests_issue_960() { + // 2D histogram with IntCategory (no overflow) and StrCategory + // Bug: when filling with array for first axis (containing out-of-bounds values) + // and scalar for second axis, valid entries were incorrectly discarded. + { + auto h = make(Tag(), ci0{1, 2, 3}, cs{"A", "B"}); + auto h2 = h; + + // Data with out-of-bounds value (4) and valid values (1, 2, 3) + std::vector data = {4, 1, 2, 3}; + + // Fill one-at-a-time (reference) + for (auto&& xi : data) h(xi, "A"); + + // Fill using array for first axis, scalar for second axis + using V = variant, std::string, std::vector>; + V xy[2]; + xy[0] = data; + xy[1] = "A"; + h2.fill(xy); + + BOOST_TEST_EQ(h, h2); + BOOST_TEST_EQ(sum(h), 3); // only 1, 2, 3 should be counted + } + + // Same test with weight storage + { + auto h = make_s(Tag(), weight_storage(), ci0{1, 2, 3}, cs{"A", "B"}); + auto h2 = h; + + std::vector data = {4, 1, 2, 3}; + std::vector w = {1.0, 1.0, 1.0, 1.0}; + + for (unsigned i = 0; i < data.size(); ++i) h(data[i], "A", weight(w[i])); + + using V = variant, std::string, std::vector>; + V xy[2]; + xy[0] = data; + xy[1] = "A"; + h2.fill(xy, weight(w)); + + BOOST_TEST_EQ(h, h2); + BOOST_TEST_EQ(sum(h).value(), 3); // only 1, 2, 3 should be counted + } + + // Test with both axes having no overflow + { + auto h = make(Tag(), ci0{1, 2, 3}, cs0{"A", "B"}); + auto h2 = h; + + std::vector data = {4, 1, 2, 3}; + + for (auto&& xi : data) h(xi, "A"); + + using V = variant, std::string, std::vector>; + V xy[2]; + xy[0] = data; + xy[1] = "A"; + h2.fill(xy); + + BOOST_TEST_EQ(h, h2); + BOOST_TEST_EQ(sum(h), 3); + } + + // Test with out-of-bounds value at different positions in array + { + auto h = make(Tag(), ci0{1, 2, 3}, cs{"A"}); + auto h2 = h; + + // Out-of-bounds at start + std::vector data1 = {99, 1, 2}; + for (auto&& xi : data1) h(xi, "A"); + using V = variant, std::string>; + V xy[2]; + xy[0] = data1; + xy[1] = "A"; + h2.fill(xy); + BOOST_TEST_EQ(h, h2); + + // Out-of-bounds in middle + std::vector data2 = {1, 99, 2}; + for (auto&& xi : data2) h(xi, "A"); + xy[0] = data2; + h2.fill(xy); + BOOST_TEST_EQ(h, h2); + + // Out-of-bounds at end + std::vector data3 = {1, 2, 99}; + for (auto&& xi : data3) h(xi, "A"); + xy[0] = data3; + h2.fill(xy); + BOOST_TEST_EQ(h, h2); + + BOOST_TEST_EQ(sum(h), 6); // 2 valid values * 3 fills + } + + // Test scalar for first axis, array for second axis + { + auto h = make(Tag(), ci0{1, 2, 3}, cs0{"A", "B"}); + auto h2 = h; + + std::vector data = {"C", "A", "B"}; // "C" is out-of-bounds + + for (auto&& si : data) h(1, si); + + using V = variant, std::string, std::vector>; + V xy[2]; + xy[0] = 1; + xy[1] = data; + h2.fill(xy); + + BOOST_TEST_EQ(h, h2); + BOOST_TEST_EQ(sum(h), 2); // only "A" and "B" should be counted + } + + // Scalar broadcast on a growing axis, while the previous non-inclusive axis + // has already invalidated the first entry (growing variant of the bug) + { + auto h = make(Tag(), in0{1, 3}, csg{"X", "A"}); + auto h2 = h; + + std::vector data = {0, 1, 2}; // 0 is out-of-bounds for in0{1, 3} + + for (auto&& xi : data) h(xi, "A"); + + using V = variant, std::string, std::vector>; + V xy[2]; + xy[0] = data; + xy[1] = "A"; + h2.fill(xy); + + BOOST_TEST_EQ(h, h2); + BOOST_TEST_EQ(sum(h), 2); + } + + // Scalar broadcast that triggers growth at the lower end (zero-point shift), + // while the previous non-inclusive axis has already invalidated the first entry + { + using ig = axis::integer; + auto h = make(Tag(), in0{1, 3}, ig{0, 2}); + auto h2 = h; + + std::vector data = {0, 1, 2}; // 0 is out-of-bounds for in0{1, 3} + + for (auto&& xi : data) h(xi, -1); // -1 grows ig downward + for (auto&& xi : data) h(xi, 5); // 5 grows ig upward + + using V = variant>; + V xy[2]; + xy[0] = data; + xy[1] = -1; + h2.fill(xy); + xy[1] = 5; + h2.fill(xy); + + BOOST_TEST_EQ(h, h2); + BOOST_TEST_EQ(sum(h), 4); + } + + // Scalar broadcast that lands in the underflow bin (negative index + // contribution), while the first entry is already invalid + { + auto h = make(Tag(), in0{1, 3}, in{1, 3}); + auto h2 = h; + + std::vector data = {0, 1, 2}; // 0 is out-of-bounds for in0{1, 3} + + for (auto&& xi : data) h(xi, 0); // 0 lands in underflow of in{1, 3} + + using V = variant>; + V xy[2]; + xy[0] = data; + xy[1] = 0; + h2.fill(xy); + + BOOST_TEST_EQ(h, h2); + BOOST_TEST_EQ(sum(h), 2); + } +} + int main() { std::mt19937 gen(1); std::normal_distribution<> id(0, 2); @@ -359,5 +547,8 @@ int main() { run_tests(x, y, w); run_tests(x, y, w); + run_tests_issue_960(); + run_tests_issue_960(); + return boost::report_errors(); }