From 62e575fddc484ebe2641abb33933fe14edfe9573 Mon Sep 17 00:00:00 2001 From: Florian Harz Date: Mon, 26 May 2025 13:11:36 +0200 Subject: [PATCH 01/19] Adding multi_weight storage --- include/bh_python/multi_weight.hpp | 251 +++++++++++++++++++++++++++++ 1 file changed, 251 insertions(+) create mode 100644 include/bh_python/multi_weight.hpp diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp new file mode 100644 index 00000000..816a5e6b --- /dev/null +++ b/include/bh_python/multi_weight.hpp @@ -0,0 +1,251 @@ +#ifndef BOOST_HISTOGRAM_MULTI_WEIGHT_HPP +#define BOOST_HISTOGRAM_MULTI_WEIGHT_HPP + +#include +#include +#include +#include +#include +#include +#include + +namespace boost { +namespace histogram { + +template +struct multi_weight_value : public boost::span { + using boost::span::span; + + void operator()(const boost::span values) { + if (values.size() != this->size()) + throw std::runtime_error("size does not match"); + auto it = this->begin(); + for (const T& x : values) + *it++ += x; + } + + bool operator==(const boost::span values) const { + if (values.size() != this->size()) + return false; + + return std::equal(this->begin(), this->end(), values.begin()); + } + bool operator!=(const boost::span values) const {return !operator==(values);} + void operator+=(const std::vector values) { + if (values.size() != this->size()) + throw std::runtime_error("size does not match"); + auto it = this->begin(); + for (const T& x : values) + *it++ += x; + } + void operator+=(const boost::span values) { + if (values.size() != this->size()) + throw std::runtime_error("size does not match"); + auto it = this->begin(); + for (const T& x : values) + *it++ += x; + } + void operator=(const std::vector values) { + if (values.size() != this->size()) + throw std::runtime_error("size does not match"); + auto it = this->begin(); + for (const T& x : values) + *it++ = x; + } +}; + + +template +class multi_weight { +public: + using element_type = ElementType; + using value_type = multi_weight_value; + using reference = value_type; + using const_reference = const value_type&; + + template + struct iterator_base : public detail::iterator_adaptor, std::size_t, Reference> { + using base_type = detail::iterator_adaptor, std::size_t, Reference>; + + iterator_base() = default; + iterator_base(const iterator_base& other) : iterator_base(other.par_, other.base()) {} + iterator_base(MWPtr par, std::size_t idx) : base_type{idx}, par_{par} {} + + Reference operator*() const { + return par_->span_buffer_[this->base()]; + } + + MWPtr par_ = nullptr; + }; + + using iterator = iterator_base; + using const_iterator = iterator_base; + + static constexpr bool has_threading_support() { return false; } + + multi_weight(const std::size_t k = 0) : nelem_{k} {} + + multi_weight(const multi_weight& other) : nelem_{other.nelem_} { + reset(other.size_); + std::copy(other.buffer_.get(), other.buffer_.get() + buffer_length_, buffer_.get()); + } + + multi_weight& operator=(const multi_weight& other) { + nelem_ = other.nelem_; + reset(other.size_); + std::copy(other.buffer_.get(), other.buffer_.get() + buffer_length_, buffer_.get()); + return *this; + } + + + std::size_t size() const { return size_; } + + void reset(std::size_t n) { + size_ = n; + buffer_length_ = n * nelem_; + buffer_.reset(new element_type[buffer_length_]); + default_fill(); + span_buffer_.reset(new value_type[size_]); + std::size_t i = 0; + std::generate_n(span_buffer_.get(), size_, [&] () { + auto tmp_span = value_type{buffer_.get() + i * nelem_, nelem_}; + i++; + return tmp_span; + }); + } + + template ::value, bool> = true> + void default_fill() {} + + template ::value, bool> = true> + void default_fill() { + std::fill_n(buffer_.get(), buffer_length_, 0); + } + + iterator begin() { return {this, 0}; } + iterator end() { return {this, size_}; } + + const_iterator begin() const { return {this, 0}; } + const_iterator end() const { return {this, size_}; } + + reference operator[](std::size_t i) { return span_buffer_[i]; } + const_reference operator[](std::size_t i) const { return span_buffer_[i]; } + + template + bool operator==(const multi_weight& other) const { + if (buffer_length_ != other.buffer_length_) + return false; + return std::equal(buffer_.get(), buffer_.get() + buffer_length_, other.buffer_.get()); + } + + template + bool operator!=(const multi_weight& other) const { return !operator==(other); } + + template + void operator+=(const multi_weight& other) { + if (buffer_length_ != other.buffer_length_) { + throw std::runtime_error("size does not match"); + } + for (std::size_t i = 0; i < buffer_length_; i++) { + buffer_[i] += other.buffer_[i]; + } + } + + + template + void serialize(Archive& ar, unsigned /* version */) { + ar& make_nvp("size", size_); + ar& make_nvp("nelem", nelem_); + std::vector w; + if (Archive::is_loading::value) + { + ar& make_nvp("buffer", w); + reset(size_); + std::swap_ranges(buffer_.get(), buffer_.get() + buffer_length_, w.data()); + } else { + w.assign(buffer_.get(), buffer_.get() + buffer_length_); + ar& make_nvp("buffer", w); + } + } + +public: + std::size_t size_ = 0; + std::size_t nelem_ = 0; + std::size_t buffer_length_ = 0; + std::unique_ptr buffer_; + std::unique_ptr span_buffer_; +}; + +template +std::ostream& operator<<(std::ostream& os, const multi_weight_value& v) { + os << "multi_weight_value("; + bool first = true; + for (const T& x : v) + if (first) { first = false; os << x; } + else os << ", " << x; + os << ")"; + return os; +} + +template +std::ostream& operator<<(std::ostream& os, const multi_weight& v) { + os << "multi_weight(\n"; + int index = 0; + for (const multi_weight_value& x : v) { + os << "Index " << index << ": " << x << "\n"; + index++; + } + os << ")"; + return os; +} + +namespace algorithm { + +/** Compute the sum over all histogram cells (underflow/overflow included by default). + + The implementation favors accuracy and protection against overflow over speed. If the + value type of the histogram is an integral or floating point type, + accumulators::sum is used to compute the sum, else the original value type is + used. Compilation fails, if the value type does not support operator+=. The return type + is double if the value type of the histogram is integral or floating point, and the + original value type otherwise. + + If you need a different trade-off, you can write your own loop or use `std::accumulate`: + ``` + // iterate over all bins + auto sum_all = std::accumulate(hist.begin(), hist.end(), 0.0); + + // skip underflow/overflow bins + double sum = 0; + for (auto&& x : indexed(hist)) + sum += *x; // dereference accessor + + // or: + // auto ind = boost::histogram::indexed(hist); + // auto sum = std::accumulate(ind.begin(), ind.end(), 0.0); + ``` + + @returns accumulator type or double + + @param hist Const reference to the histogram. + @param cov Iterate over all or only inner bins (optional, default: all). +*/ +template +std::vector sum(const histogram>& hist, const coverage cov = coverage::all) { + using sum_type = typename histogram>::value_type; + // T is arithmetic, compute sum accurately with high dynamic range + std::vector v(unsafe_access::storage(hist).nelem_, 0.); + sum_type sum(v); + if (cov == coverage::all) + for (auto&& x : hist) sum += x; + else + // sum += x also works if sum_type::operator+=(const sum_type&) exists + for (auto&& x : indexed(hist)) sum += *x; + return v; +} + +} // namespace algorithm +} // namespace histogram +} // namespace boost + +#endif From f85c9c6b38bc91a5f0bee5b798b827b08ee4a538 Mon Sep 17 00:00:00 2001 From: Florian Harz Date: Mon, 26 May 2025 13:26:36 +0200 Subject: [PATCH 02/19] Add implementation in the python bindings --- include/bh_python/fill.hpp | 30 ++++ include/bh_python/histogram.hpp | 9 ++ include/bh_python/register_histogram.hpp | 188 +++++++++++++++++++++++ include/bh_python/register_storage.hpp | 32 ++++ include/bh_python/storage.hpp | 7 + src/boost_histogram/_core/hist.pyi | 10 ++ src/boost_histogram/_core/storage.pyi | 1 + src/boost_histogram/histogram.py | 1 + src/boost_histogram/storage.py | 4 + src/register_histograms.cpp | 5 + src/register_storage.cpp | 5 + 11 files changed, 292 insertions(+) diff --git a/include/bh_python/fill.hpp b/include/bh_python/fill.hpp index 1f727f15..c63a04a7 100644 --- a/include/bh_python/fill.hpp +++ b/include/bh_python/fill.hpp @@ -213,6 +213,36 @@ void fill_impl(bh::detail::accumulator_traits_holder, weight); } +// for multi_weight +template +void fill_impl(bh::detail::accumulator_traits_holder>, + Histogram& h, + const VArgs& vargs, + const weight_t& weight, + py::kwargs& kwargs) { + // weight is not used, "use" it once to suppress "unused variable" complaints by compiler + (void)weight; + auto s = required_arg(kwargs, "sample"); + finalize_args(kwargs); + auto sarray = py::cast>(s); + if(sarray.ndim() != 2) + throw std::invalid_argument("Sample array for MultiWeight must be 2D"); + + + auto buf = sarray.request(); + std::size_t buf_shape0 = static_cast(buf.shape[0]); + std::size_t buf_shape1 = static_cast(buf.shape[1]); + double* src = static_cast(buf.ptr); + std::vector> vec_s; + vec_s.reserve(buf_shape0); + for (std::size_t i = 0; i < buf_shape0; i++) { + vec_s.emplace_back(boost::span{src + i * buf_shape1, buf_shape1}); + } + // releasing gil here is safe, we don't manipulate refcounts + py::gil_scoped_release lock; + h.fill(vargs, bh::sample(vec_s)); +} + } // namespace detail template diff --git a/include/bh_python/histogram.hpp b/include/bh_python/histogram.hpp index 0025dd7d..aa5f827e 100644 --- a/include/bh_python/histogram.hpp +++ b/include/bh_python/histogram.hpp @@ -7,6 +7,7 @@ #include +#include #include #include #include @@ -96,6 +97,14 @@ py::buffer_info make_buffer(bh::histogram>& return detail::make_buffer_impl(axes, flow, static_cast(buffer.ptr)); } +/// Specialization for multi_weight buffer +template +py::buffer_info make_buffer(bh::histogram>& h, bool flow) { + const auto& axes = bh::unsafe_access::axes(h); + auto& storage = bh::unsafe_access::storage(h); + return detail::make_buffer_impl(axes, flow, static_cast(storage.buffer_.get())); +} + /// Compute the bin of an array from a runtime list /// For example, [1,3,2] will return that bin of an array template diff --git a/include/bh_python/register_histogram.hpp b/include/bh_python/register_histogram.hpp index 5f784eb1..6335e5d6 100644 --- a/include/bh_python/register_histogram.hpp +++ b/include/bh_python/register_histogram.hpp @@ -209,3 +209,191 @@ auto register_histogram(py::module& m, const char* name, const char* desc) { return hist; } + +template <> +auto register_histogram>(py::module& m, const char* name, const char* desc) { + using S = bh::multi_weight; + using histogram_t = bh::histogram; + using value_type = std::vector; + + py::class_ hist(m, name, desc, py::buffer_protocol()); + + hist.def(py::init(), "axes"_a, "storage"_a = S()) + + .def_buffer( + [](histogram_t& h) -> py::buffer_info { return make_buffer(h, false); }) + + .def("rank", &histogram_t::rank) + .def("size", &histogram_t::size) + .def("reset", &histogram_t::reset) + + .def("__copy__", [](const histogram_t& self) { return histogram_t(self); }) + .def("__deepcopy__", + [](const histogram_t& self, py::object memo) { + auto* a = new histogram_t(self); + py::module copy = py::module::import("copy"); + for(unsigned i = 0; i < a->rank(); i++) { + bh::unsafe_access::axis(*a, i).metadata() + = copy.attr("deepcopy")(a->axis(i).metadata(), memo); + } + return a; + }) + + .def(py::self += py::self) + + .def("__eq__", + [](const histogram_t& self, const py::object& other) { + try { + return self == py::cast(other); + } catch(const py::cast_error&) { + return false; + } + }) + .def("__ne__", + [](const histogram_t& self, const py::object& other) { + try { + return self != py::cast(other); + } catch(const py::cast_error&) { + return true; + } + }) + + .def_property_readonly_static( + "_storage_type", + [](py::object) { + return py::type::of(); + }) + + ; + +// Protection against an overzealous warning system +// https://bugs.llvm.org/show_bug.cgi?id=43124 +#ifdef __clang__ +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wself-assign-overloaded" +#endif + def_optionally(hist, + bh::detail::has_operator_rdiv{}, + py::self /= py::self); + def_optionally(hist, + bh::detail::has_operator_rmul{}, + py::self *= py::self); + def_optionally(hist, + bh::detail::has_operator_rsub{}, + py::self -= py::self); +#ifdef __clang__ +#pragma GCC diagnostic pop +#endif + + hist.def( + "to_numpy", + [](histogram_t& h, bool flow) { + py::tuple tup(1 + h.rank()); + + // Add the histogram buffer as the first argument + unchecked_set(tup, 0, py::array(make_buffer(h, flow))); + + // Add the axis edges + h.for_each_axis([&tup, flow, i = 0u](const auto& ax) mutable { + unchecked_set(tup, ++i, axis::edges(ax, flow, true)); + }); + + return tup; + }, + "flow"_a = false) + + .def( + "view", + [](py::object self, bool flow) { + auto& h = py::cast(self); + return py::array(make_buffer(h, flow), self); + }, + "flow"_a = false) + + .def( + "axis", + [](const histogram_t& self, int i) -> py::object { + unsigned ii = i < 0 ? self.rank() - static_cast(std::abs(i)) + : static_cast(i); + + if(ii < self.rank()) { + const axis_variant& var = self.axis(ii); + return bh::axis::visit( + [](auto&& item) -> py::object { + // Here we return a new, no-copy py::object that + // is not yet tied to the histogram. py::keep_alive + // is needed to make sure the histogram is alive as long + // as the axes references are. + return py::cast(item, py::return_value_policy::reference); + }, + var); + + } + + else + throw std::out_of_range( + "The axis value must be less than the rank"); + }, + "i"_a = 0, + py::keep_alive<0, 1>()) + + .def("at", + [](const histogram_t& self, py::args& args) -> value_type { + auto int_args = py::cast>(args); + auto at_value = self.at(int_args); + //value_type return_obj; + //return_obj.insert(return_obj.end(), at_value.begin(), at_value.end()); + //return_obj.assign(at_value.begin(), at_value.end()); + //return return_obj; + return value_type(at_value.begin(), at_value.end()); + }) + + .def("_at_set", + [](histogram_t& self, const value_type& input, py::args& args) { + auto int_args = py::cast>(args); + self.at(int_args) = input; + }) + + .def("__repr__", &shift_to_string) + + .def( + "sum", + [](const histogram_t& self, bool flow) -> value_type{ + py::gil_scoped_release release; + return bh::algorithm::sum( + self, flow ? bh::coverage::all : bh::coverage::inner); + }, + "flow"_a = false) + + .def( + "empty", + [](const histogram_t& self, bool flow) { + py::gil_scoped_release release; + return bh::algorithm::empty( + self, flow ? bh::coverage::all : bh::coverage::inner); + }, + "flow"_a = false) + + .def("reduce", + [](const histogram_t& self, py::args args) { + auto commands + = py::cast>(args); + py::gil_scoped_release release; + return bh::algorithm::reduce(self, commands); + }) + + .def("project", + [](const histogram_t& self, py::args values) { + auto cpp_values = py::cast>(values); + py::gil_scoped_release release; + return bh::algorithm::project(self, cpp_values); + }) + + .def("fill", &fill) + + .def(make_pickle()) + + ; + + return hist; +} diff --git a/include/bh_python/register_storage.hpp b/include/bh_python/register_storage.hpp index a701d3dd..662b71ac 100644 --- a/include/bh_python/register_storage.hpp +++ b/include/bh_python/register_storage.hpp @@ -71,3 +71,35 @@ py::class_ inline register_storage(py::module& m, return storage; } + +/// Add helpers to the multi_weight storage type +template <> +py::class_ +register_storage(py::module& m, const char* name, const char* desc) { + using A = storage::multi_weight; // match code above + + py::class_ storage(m, name, desc); + + storage.def(py::init(), py::arg("k") = 0) + .def("__eq__", + [](const A& self, const py::object& other) { + try { + return self == py::cast(other); + } catch(const py::cast_error&) { + return false; + } + }) + .def("__ne__", + [](const A& self, const py::object& other) { + try { + return !(self == py::cast(other)); + } catch(const py::cast_error&) { + return true; + } + }) + .def(make_pickle()) + .def("__copy__", [](const A& self) { return A(self); }) + .def("__deepcopy__", [](const A& self, py::object) { return A(self); }); + + return storage; +} diff --git a/include/bh_python/storage.hpp b/include/bh_python/storage.hpp index 33f99446..45fb80a9 100644 --- a/include/bh_python/storage.hpp +++ b/include/bh_python/storage.hpp @@ -7,6 +7,7 @@ #include +#include #include #include #include @@ -27,6 +28,7 @@ using atomic_int64 = bh::dense_storage>; using double_ = bh::dense_storage; using unlimited = bh::unlimited_storage<>; using weight = bh::dense_storage>; +using multi_weight = bh::multi_weight; using mean = bh::dense_storage>; using weighted_mean = bh::dense_storage>; @@ -61,6 +63,11 @@ inline const char* name() { return "weight"; } +template <> +inline const char* name() { + return "multi_weight"; +} + template <> inline const char* name() { return "mean"; diff --git a/src/boost_histogram/_core/hist.pyi b/src/boost_histogram/_core/hist.pyi index 7333e964..322dd707 100644 --- a/src/boost_histogram/_core/hist.pyi +++ b/src/boost_histogram/_core/hist.pyi @@ -94,3 +94,13 @@ class any_weighted_mean(_BaseHistogram): weight: ArrayLike | None = ..., sample: ArrayLike | None = ..., ) -> None: ... + +class any_multi_weight(_BaseHistogram): + def at(self, *args: int) -> float: ... + def _at_set(self, value: float, *args: int) -> ArrayLike: ... + def sum(self, flow: bool = ...) -> ArrayLike: ... + def fill( + self, + *args: ArrayLike, + sample: ArrayLike | None = ..., + ) -> None: ... diff --git a/src/boost_histogram/_core/storage.pyi b/src/boost_histogram/_core/storage.pyi index 7153e45f..175ec223 100644 --- a/src/boost_histogram/_core/storage.pyi +++ b/src/boost_histogram/_core/storage.pyi @@ -16,3 +16,4 @@ class unlimited(_BaseStorage): ... class weight(_BaseStorage): ... class mean(_BaseStorage): ... class weighted_mean(_BaseStorage): ... +class multi_weight(_BaseStorage): ... diff --git a/src/boost_histogram/histogram.py b/src/boost_histogram/histogram.py index 7d5a5e36..87f50faf 100644 --- a/src/boost_histogram/histogram.py +++ b/src/boost_histogram/histogram.py @@ -95,6 +95,7 @@ def __dir__() -> list[str]: _core.hist.any_weight, _core.hist.any_mean, _core.hist.any_weighted_mean, + _core.hist.any_multi_weight, } logger = logging.getLogger(__name__) diff --git a/src/boost_histogram/storage.py b/src/boost_histogram/storage.py index 387bd7a1..0a0eef46 100644 --- a/src/boost_histogram/storage.py +++ b/src/boost_histogram/storage.py @@ -16,6 +16,7 @@ "Unlimited", "Weight", "WeightedMean", + "MultiWeight", ] @@ -73,3 +74,6 @@ class Mean(store.mean, Storage, family=boost_histogram): class WeightedMean(store.weighted_mean, Storage, family=boost_histogram): accumulator = accumulators.WeightedMean + +class MultiWeight(store.multi_weight, Storage, family=boost_histogram): + accumulator = float diff --git a/src/register_histograms.cpp b/src/register_histograms.cpp index db43cc9e..d16f3cf9 100644 --- a/src/register_histograms.cpp +++ b/src/register_histograms.cpp @@ -50,4 +50,9 @@ void register_histograms(py::module& hist) { hist, "any_weighted_mean", "N-dimensional histogram for weighted and sampled data with any axis types."); + + register_histogram( + hist, + "any_multi_weight", + "N-dimensional histogram for storing multiple weights at once with any axis types."); } diff --git a/src/register_storage.cpp b/src/register_storage.cpp index 47c71fce..39cd3ccd 100644 --- a/src/register_storage.cpp +++ b/src/register_storage.cpp @@ -35,4 +35,9 @@ void register_storages(py::module& storage) { storage, "weighted_mean", "Dense storage which tracks means of weighted samples in each cell"); + + register_storage( + storage, + "multi_weight", + "Dense storage which tracks sums of weights for multiple weights per entry"); } From bd12642c37ef65aef5efbd88e3bb66ceaa80f117 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 26 May 2025 11:33:27 +0000 Subject: [PATCH 03/19] style: pre-commit fixes --- include/bh_python/fill.hpp | 14 +-- include/bh_python/histogram.hpp | 5 +- include/bh_python/multi_weight.hpp | 150 +++++++++++++---------- include/bh_python/register_histogram.hpp | 16 +-- include/bh_python/storage.hpp | 2 +- src/boost_histogram/storage.py | 3 +- src/register_histograms.cpp | 3 +- 7 files changed, 109 insertions(+), 84 deletions(-) diff --git a/include/bh_python/fill.hpp b/include/bh_python/fill.hpp index c63a04a7..fa8a1870 100644 --- a/include/bh_python/fill.hpp +++ b/include/bh_python/fill.hpp @@ -220,7 +220,8 @@ void fill_impl(bh::detail::accumulator_traits_holder> const VArgs& vargs, const weight_t& weight, py::kwargs& kwargs) { - // weight is not used, "use" it once to suppress "unused variable" complaints by compiler + // weight is not used, "use" it once to suppress "unused variable" complaints by + // compiler (void)weight; auto s = required_arg(kwargs, "sample"); finalize_args(kwargs); @@ -228,14 +229,13 @@ void fill_impl(bh::detail::accumulator_traits_holder> if(sarray.ndim() != 2) throw std::invalid_argument("Sample array for MultiWeight must be 2D"); - - auto buf = sarray.request(); - std::size_t buf_shape0 = static_cast(buf.shape[0]); - std::size_t buf_shape1 = static_cast(buf.shape[1]); - double* src = static_cast(buf.ptr); + auto buf = sarray.request(); + std::size_t buf_shape0 = static_cast(buf.shape[0]); + std::size_t buf_shape1 = static_cast(buf.shape[1]); + double* src = static_cast(buf.ptr); std::vector> vec_s; vec_s.reserve(buf_shape0); - for (std::size_t i = 0; i < buf_shape0; i++) { + for(std::size_t i = 0; i < buf_shape0; i++) { vec_s.emplace_back(boost::span{src + i * buf_shape1, buf_shape1}); } // releasing gil here is safe, we don't manipulate refcounts diff --git a/include/bh_python/histogram.hpp b/include/bh_python/histogram.hpp index aa5f827e..3e441ae5 100644 --- a/include/bh_python/histogram.hpp +++ b/include/bh_python/histogram.hpp @@ -7,10 +7,10 @@ #include -#include #include #include #include +#include #include #include @@ -102,7 +102,8 @@ template py::buffer_info make_buffer(bh::histogram>& h, bool flow) { const auto& axes = bh::unsafe_access::axes(h); auto& storage = bh::unsafe_access::storage(h); - return detail::make_buffer_impl(axes, flow, static_cast(storage.buffer_.get())); + return detail::make_buffer_impl( + axes, flow, static_cast(storage.buffer_.get())); } /// Compute the bin of an array from a runtime list diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index 816a5e6b..18d21c49 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -1,13 +1,13 @@ #ifndef BOOST_HISTOGRAM_MULTI_WEIGHT_HPP #define BOOST_HISTOGRAM_MULTI_WEIGHT_HPP +#include #include +#include #include #include -#include -#include -#include #include +#include namespace boost { namespace histogram { @@ -17,107 +17,119 @@ struct multi_weight_value : public boost::span { using boost::span::span; void operator()(const boost::span values) { - if (values.size() != this->size()) + if(values.size() != this->size()) throw std::runtime_error("size does not match"); auto it = this->begin(); - for (const T& x : values) + for(const T& x : values) *it++ += x; } bool operator==(const boost::span values) const { - if (values.size() != this->size()) + if(values.size() != this->size()) return false; return std::equal(this->begin(), this->end(), values.begin()); } - bool operator!=(const boost::span values) const {return !operator==(values);} + bool operator!=(const boost::span values) const { return !operator==(values); } void operator+=(const std::vector values) { - if (values.size() != this->size()) + if(values.size() != this->size()) throw std::runtime_error("size does not match"); auto it = this->begin(); - for (const T& x : values) + for(const T& x : values) *it++ += x; } void operator+=(const boost::span values) { - if (values.size() != this->size()) + if(values.size() != this->size()) throw std::runtime_error("size does not match"); auto it = this->begin(); - for (const T& x : values) + for(const T& x : values) *it++ += x; } void operator=(const std::vector values) { - if (values.size() != this->size()) + if(values.size() != this->size()) throw std::runtime_error("size does not match"); auto it = this->begin(); - for (const T& x : values) + for(const T& x : values) *it++ = x; } }; - template class multi_weight { -public: - using element_type = ElementType; - using value_type = multi_weight_value; - using reference = value_type; + public: + using element_type = ElementType; + using value_type = multi_weight_value; + using reference = value_type; using const_reference = const value_type&; template - struct iterator_base : public detail::iterator_adaptor, std::size_t, Reference> { - using base_type = detail::iterator_adaptor, std::size_t, Reference>; + struct iterator_base + : public detail::iterator_adaptor, + std::size_t, + Reference> { + using base_type + = detail::iterator_adaptor, + std::size_t, + Reference>; iterator_base() = default; - iterator_base(const iterator_base& other) : iterator_base(other.par_, other.base()) {} - iterator_base(MWPtr par, std::size_t idx) : base_type{idx}, par_{par} {} + iterator_base(const iterator_base& other) + : iterator_base(other.par_, other.base()) {} + iterator_base(MWPtr par, std::size_t idx) + : base_type{idx} + , par_{par} {} - Reference operator*() const { - return par_->span_buffer_[this->base()]; - } + Reference operator*() const { return par_->span_buffer_[this->base()]; } MWPtr par_ = nullptr; }; using iterator = iterator_base; - using const_iterator = iterator_base; + using const_iterator + = iterator_base; static constexpr bool has_threading_support() { return false; } - multi_weight(const std::size_t k = 0) : nelem_{k} {} + multi_weight(const std::size_t k = 0) + : nelem_{k} {} - multi_weight(const multi_weight& other) : nelem_{other.nelem_} { + multi_weight(const multi_weight& other) + : nelem_{other.nelem_} { reset(other.size_); - std::copy(other.buffer_.get(), other.buffer_.get() + buffer_length_, buffer_.get()); + std::copy( + other.buffer_.get(), other.buffer_.get() + buffer_length_, buffer_.get()); } multi_weight& operator=(const multi_weight& other) { nelem_ = other.nelem_; reset(other.size_); - std::copy(other.buffer_.get(), other.buffer_.get() + buffer_length_, buffer_.get()); + std::copy( + other.buffer_.get(), other.buffer_.get() + buffer_length_, buffer_.get()); return *this; } - std::size_t size() const { return size_; } void reset(std::size_t n) { - size_ = n; + size_ = n; buffer_length_ = n * nelem_; buffer_.reset(new element_type[buffer_length_]); default_fill(); span_buffer_.reset(new value_type[size_]); std::size_t i = 0; - std::generate_n(span_buffer_.get(), size_, [&] () { + std::generate_n(span_buffer_.get(), size_, [&]() { auto tmp_span = value_type{buffer_.get() + i * nelem_, nelem_}; i++; return tmp_span; }); } - template ::value, bool> = true> + template ::value, bool> = true> void default_fill() {} - template ::value, bool> = true> + template ::value, bool> = true> void default_fill() { std::fill_n(buffer_.get(), buffer_length_, 0); } @@ -133,32 +145,33 @@ class multi_weight { template bool operator==(const multi_weight& other) const { - if (buffer_length_ != other.buffer_length_) + if(buffer_length_ != other.buffer_length_) return false; - return std::equal(buffer_.get(), buffer_.get() + buffer_length_, other.buffer_.get()); + return std::equal( + buffer_.get(), buffer_.get() + buffer_length_, other.buffer_.get()); } template - bool operator!=(const multi_weight& other) const { return !operator==(other); } + bool operator!=(const multi_weight& other) const { + return !operator==(other); + } template void operator+=(const multi_weight& other) { - if (buffer_length_ != other.buffer_length_) { + if(buffer_length_ != other.buffer_length_) { throw std::runtime_error("size does not match"); } - for (std::size_t i = 0; i < buffer_length_; i++) { + for(std::size_t i = 0; i < buffer_length_; i++) { buffer_[i] += other.buffer_[i]; } } - template void serialize(Archive& ar, unsigned /* version */) { ar& make_nvp("size", size_); ar& make_nvp("nelem", nelem_); std::vector w; - if (Archive::is_loading::value) - { + if(Archive::is_loading::value) { ar& make_nvp("buffer", w); reset(size_); std::swap_ranges(buffer_.get(), buffer_.get() + buffer_length_, w.data()); @@ -168,9 +181,9 @@ class multi_weight { } } -public: - std::size_t size_ = 0; - std::size_t nelem_ = 0; + public: + std::size_t size_ = 0; + std::size_t nelem_ = 0; std::size_t buffer_length_ = 0; std::unique_ptr buffer_; std::unique_ptr span_buffer_; @@ -180,9 +193,12 @@ template std::ostream& operator<<(std::ostream& os, const multi_weight_value& v) { os << "multi_weight_value("; bool first = true; - for (const T& x : v) - if (first) { first = false; os << x; } - else os << ", " << x; + for(const T& x : v) + if(first) { + first = false; + os << x; + } else + os << ", " << x; os << ")"; return os; } @@ -191,7 +207,7 @@ template std::ostream& operator<<(std::ostream& os, const multi_weight& v) { os << "multi_weight(\n"; int index = 0; - for (const multi_weight_value& x : v) { + for(const multi_weight_value& x : v) { os << "Index " << index << ": " << x << "\n"; index++; } @@ -206,11 +222,12 @@ namespace algorithm { The implementation favors accuracy and protection against overflow over speed. If the value type of the histogram is an integral or floating point type, accumulators::sum is used to compute the sum, else the original value type is - used. Compilation fails, if the value type does not support operator+=. The return type - is double if the value type of the histogram is integral or floating point, and the - original value type otherwise. + used. Compilation fails, if the value type does not support operator+=. The return + type is double if the value type of the histogram is integral or floating point, and + the original value type otherwise. - If you need a different trade-off, you can write your own loop or use `std::accumulate`: + If you need a different trade-off, you can write your own loop or use + `std::accumulate`: ``` // iterate over all bins auto sum_all = std::accumulate(hist.begin(), hist.end(), 0.0); @@ -231,17 +248,20 @@ namespace algorithm { @param cov Iterate over all or only inner bins (optional, default: all). */ template -std::vector sum(const histogram>& hist, const coverage cov = coverage::all) { - using sum_type = typename histogram>::value_type; - // T is arithmetic, compute sum accurately with high dynamic range - std::vector v(unsafe_access::storage(hist).nelem_, 0.); - sum_type sum(v); - if (cov == coverage::all) - for (auto&& x : hist) sum += x; - else - // sum += x also works if sum_type::operator+=(const sum_type&) exists - for (auto&& x : indexed(hist)) sum += *x; - return v; +std::vector sum(const histogram>& hist, + const coverage cov = coverage::all) { + using sum_type = typename histogram>::value_type; + // T is arithmetic, compute sum accurately with high dynamic range + std::vector v(unsafe_access::storage(hist).nelem_, 0.); + sum_type sum(v); + if(cov == coverage::all) + for(auto&& x : hist) + sum += x; + else + // sum += x also works if sum_type::operator+=(const sum_type&) exists + for(auto&& x : indexed(hist)) + sum += *x; + return v; } } // namespace algorithm diff --git a/include/bh_python/register_histogram.hpp b/include/bh_python/register_histogram.hpp index 6335e5d6..7f0c10ff 100644 --- a/include/bh_python/register_histogram.hpp +++ b/include/bh_python/register_histogram.hpp @@ -211,8 +211,10 @@ auto register_histogram(py::module& m, const char* name, const char* desc) { } template <> -auto register_histogram>(py::module& m, const char* name, const char* desc) { - using S = bh::multi_weight; +auto register_histogram>(py::module& m, + const char* name, + const char* desc) { + using S = bh::multi_weight; using histogram_t = bh::histogram; using value_type = std::vector; @@ -341,10 +343,10 @@ auto register_histogram>(py::module& m, const char* nam [](const histogram_t& self, py::args& args) -> value_type { auto int_args = py::cast>(args); auto at_value = self.at(int_args); - //value_type return_obj; - //return_obj.insert(return_obj.end(), at_value.begin(), at_value.end()); - //return_obj.assign(at_value.begin(), at_value.end()); - //return return_obj; + // value_type return_obj; + // return_obj.insert(return_obj.end(), at_value.begin(), + // at_value.end()); return_obj.assign(at_value.begin(), + // at_value.end()); return return_obj; return value_type(at_value.begin(), at_value.end()); }) @@ -358,7 +360,7 @@ auto register_histogram>(py::module& m, const char* nam .def( "sum", - [](const histogram_t& self, bool flow) -> value_type{ + [](const histogram_t& self, bool flow) -> value_type { py::gil_scoped_release release; return bh::algorithm::sum( self, flow ? bh::coverage::all : bh::coverage::inner); diff --git a/include/bh_python/storage.hpp b/include/bh_python/storage.hpp index 45fb80a9..7377aa63 100644 --- a/include/bh_python/storage.hpp +++ b/include/bh_python/storage.hpp @@ -7,10 +7,10 @@ #include -#include #include #include #include +#include #include #include diff --git a/src/boost_histogram/storage.py b/src/boost_histogram/storage.py index 0a0eef46..d1e6d2ec 100644 --- a/src/boost_histogram/storage.py +++ b/src/boost_histogram/storage.py @@ -12,11 +12,11 @@ "Double", "Int64", "Mean", + "MultiWeight", "Storage", "Unlimited", "Weight", "WeightedMean", - "MultiWeight", ] @@ -75,5 +75,6 @@ class Mean(store.mean, Storage, family=boost_histogram): class WeightedMean(store.weighted_mean, Storage, family=boost_histogram): accumulator = accumulators.WeightedMean + class MultiWeight(store.multi_weight, Storage, family=boost_histogram): accumulator = float diff --git a/src/register_histograms.cpp b/src/register_histograms.cpp index d16f3cf9..6a94bb7c 100644 --- a/src/register_histograms.cpp +++ b/src/register_histograms.cpp @@ -54,5 +54,6 @@ void register_histograms(py::module& hist) { register_histogram( hist, "any_multi_weight", - "N-dimensional histogram for storing multiple weights at once with any axis types."); + "N-dimensional histogram for storing multiple weights at once with any axis " + "types."); } From b2a7de24cf75ed2eb9b74cd6ff207d43ccea45a6 Mon Sep 17 00:00:00 2001 From: Florian Harz Date: Tue, 27 May 2025 10:24:47 +0200 Subject: [PATCH 04/19] Implement requested changes in fill.hpp --- include/bh_python/fill.hpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/include/bh_python/fill.hpp b/include/bh_python/fill.hpp index fa8a1870..59d82b95 100644 --- a/include/bh_python/fill.hpp +++ b/include/bh_python/fill.hpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -220,9 +221,7 @@ void fill_impl(bh::detail::accumulator_traits_holder> const VArgs& vargs, const weight_t& weight, py::kwargs& kwargs) { - // weight is not used, "use" it once to suppress "unused variable" complaints by - // compiler - (void)weight; + boost::ignore_unused(weight); auto s = required_arg(kwargs, "sample"); finalize_args(kwargs); auto sarray = py::cast>(s); @@ -230,6 +229,8 @@ void fill_impl(bh::detail::accumulator_traits_holder> throw std::invalid_argument("Sample array for MultiWeight must be 2D"); auto buf = sarray.request(); + // releasing gil here is safe, we don't manipulate refcounts + py::gil_scoped_release lock; std::size_t buf_shape0 = static_cast(buf.shape[0]); std::size_t buf_shape1 = static_cast(buf.shape[1]); double* src = static_cast(buf.ptr); @@ -238,8 +239,6 @@ void fill_impl(bh::detail::accumulator_traits_holder> for(std::size_t i = 0; i < buf_shape0; i++) { vec_s.emplace_back(boost::span{src + i * buf_shape1, buf_shape1}); } - // releasing gil here is safe, we don't manipulate refcounts - py::gil_scoped_release lock; h.fill(vargs, bh::sample(vec_s)); } From 1d1771dbc28675e70d56277464a2a2b699984802 Mon Sep 17 00:00:00 2001 From: Florian Harz Date: Tue, 27 May 2025 16:21:07 +0200 Subject: [PATCH 05/19] Implement first batch of requested changes in multi_weight.hpp --- include/bh_python/multi_weight.hpp | 85 ++++++++++++++---------------- 1 file changed, 39 insertions(+), 46 deletions(-) diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index 18d21c49..aca90753 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -17,35 +17,41 @@ struct multi_weight_value : public boost::span { using boost::span::span; void operator()(const boost::span values) { - if(values.size() != this->size()) - throw std::runtime_error("size does not match"); - auto it = this->begin(); - for(const T& x : values) - *it++ += x; + operator+=(values); } - bool operator==(const boost::span values) const { + template + bool operator==(const S values) const { if(values.size() != this->size()) return false; return std::equal(this->begin(), this->end(), values.begin()); } - bool operator!=(const boost::span values) const { return !operator==(values); } - void operator+=(const std::vector values) { - if(values.size() != this->size()) - throw std::runtime_error("size does not match"); - auto it = this->begin(); - for(const T& x : values) - *it++ += x; - } - void operator+=(const boost::span values) { + + template + bool operator!=(const S values) const { return !operator==(values); } + + //template + //void operator+=(const S values) { + //void operator+=(const std::vector values) { + // if(values.size() != this->size()) + // throw std::runtime_error("size does not match"); + // auto it = this->begin(); + // for(const T& x : values) + // *it++ += x; + //} + //void operator+=(const boost::span values) { + template + void operator+=(const S values) { if(values.size() != this->size()) throw std::runtime_error("size does not match"); auto it = this->begin(); for(const T& x : values) *it++ += x; } - void operator=(const std::vector values) { + + template + void operator=(const S values) { if(values.size() != this->size()) throw std::runtime_error("size does not match"); auto it = this->begin(); @@ -60,7 +66,7 @@ class multi_weight { using element_type = ElementType; using value_type = multi_weight_value; using reference = value_type; - using const_reference = const value_type&; + using const_reference = const value_type; template struct iterator_base @@ -79,7 +85,7 @@ class multi_weight { : base_type{idx} , par_{par} {} - Reference operator*() const { return par_->span_buffer_[this->base()]; } + decltype(auto) operator*() const { return Reference{par_->buffer_.get() + this->base() * par_->nelem_, par_->nelem_}; } MWPtr par_ = nullptr; }; @@ -93,18 +99,15 @@ class multi_weight { multi_weight(const std::size_t k = 0) : nelem_{k} {} - multi_weight(const multi_weight& other) - : nelem_{other.nelem_} { - reset(other.size_); - std::copy( - other.buffer_.get(), other.buffer_.get() + buffer_length_, buffer_.get()); + multi_weight(const multi_weight& other) { + *this = other; } multi_weight& operator=(const multi_weight& other) { nelem_ = other.nelem_; reset(other.size_); std::copy( - other.buffer_.get(), other.buffer_.get() + buffer_length_, buffer_.get()); + other.buffer_.get(), other.buffer_.get() + size_ * nelem_, buffer_.get()); return *this; } @@ -112,16 +115,8 @@ class multi_weight { void reset(std::size_t n) { size_ = n; - buffer_length_ = n * nelem_; - buffer_.reset(new element_type[buffer_length_]); + buffer_.reset(new element_type[size_ * nelem_]); default_fill(); - span_buffer_.reset(new value_type[size_]); - std::size_t i = 0; - std::generate_n(span_buffer_.get(), size_, [&]() { - auto tmp_span = value_type{buffer_.get() + i * nelem_, nelem_}; - i++; - return tmp_span; - }); } template ::value, bool> = true> void default_fill() { - std::fill_n(buffer_.get(), buffer_length_, 0); + std::fill_n(buffer_.get(), size_ * nelem_, 0); } iterator begin() { return {this, 0}; } @@ -140,15 +135,15 @@ class multi_weight { const_iterator begin() const { return {this, 0}; } const_iterator end() const { return {this, size_}; } - reference operator[](std::size_t i) { return span_buffer_[i]; } - const_reference operator[](std::size_t i) const { return span_buffer_[i]; } + reference operator[](std::size_t i) { return reference{buffer_.get() + i * nelem_, nelem_}; } + const_reference operator[](std::size_t i) const { return const_reference{buffer_.get() + i * nelem_, nelem_}; } template bool operator==(const multi_weight& other) const { - if(buffer_length_ != other.buffer_length_) + if(size_ * nelem_ != other.size_ * other.nelem_) return false; return std::equal( - buffer_.get(), buffer_.get() + buffer_length_, other.buffer_.get()); + buffer_.get(), buffer_.get() + size_ * nelem_, other.buffer_.get()); } template @@ -158,10 +153,10 @@ class multi_weight { template void operator+=(const multi_weight& other) { - if(buffer_length_ != other.buffer_length_) { + if(size_ * nelem_ != other.size_ * other.nelem_) { throw std::runtime_error("size does not match"); } - for(std::size_t i = 0; i < buffer_length_; i++) { + for(std::size_t i = 0; i < size_ * nelem_; i++) { buffer_[i] += other.buffer_[i]; } } @@ -174,19 +169,17 @@ class multi_weight { if(Archive::is_loading::value) { ar& make_nvp("buffer", w); reset(size_); - std::swap_ranges(buffer_.get(), buffer_.get() + buffer_length_, w.data()); + std::swap_ranges(buffer_.get(), buffer_.get() + size_ * nelem_, w.data()); } else { - w.assign(buffer_.get(), buffer_.get() + buffer_length_); + w.assign(buffer_.get(), buffer_.get() + size_ * nelem_); ar& make_nvp("buffer", w); } } public: - std::size_t size_ = 0; - std::size_t nelem_ = 0; - std::size_t buffer_length_ = 0; + std::size_t size_ = 0; // Number of bins + std::size_t nelem_ = 0; // Number of weights per bin std::unique_ptr buffer_; - std::unique_ptr span_buffer_; }; template From a6b5e810735df16fec1176857b92c8a6cb43f29e Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 27 May 2025 14:22:54 +0000 Subject: [PATCH 06/19] style: pre-commit fixes --- include/bh_python/fill.hpp | 4 +-- include/bh_python/multi_weight.hpp | 53 ++++++++++++++++-------------- 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/include/bh_python/fill.hpp b/include/bh_python/fill.hpp index 59d82b95..d5849f61 100644 --- a/include/bh_python/fill.hpp +++ b/include/bh_python/fill.hpp @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -19,7 +20,6 @@ #include #include #include -#include #include #include @@ -228,7 +228,7 @@ void fill_impl(bh::detail::accumulator_traits_holder> if(sarray.ndim() != 2) throw std::invalid_argument("Sample array for MultiWeight must be 2D"); - auto buf = sarray.request(); + auto buf = sarray.request(); // releasing gil here is safe, we don't manipulate refcounts py::gil_scoped_release lock; std::size_t buf_shape0 = static_cast(buf.shape[0]); diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index aca90753..95cdd794 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -16,9 +16,7 @@ template struct multi_weight_value : public boost::span { using boost::span::span; - void operator()(const boost::span values) { - operator+=(values); - } + void operator()(const boost::span values) { operator+=(values); } template bool operator==(const S values) const { @@ -29,18 +27,20 @@ struct multi_weight_value : public boost::span { } template - bool operator!=(const S values) const { return !operator==(values); } - - //template - //void operator+=(const S values) { - //void operator+=(const std::vector values) { - // if(values.size() != this->size()) - // throw std::runtime_error("size does not match"); - // auto it = this->begin(); - // for(const T& x : values) - // *it++ += x; - //} - //void operator+=(const boost::span values) { + bool operator!=(const S values) const { + return !operator==(values); + } + + // template + // void operator+=(const S values) { + // void operator+=(const std::vector values) { + // if(values.size() != this->size()) + // throw std::runtime_error("size does not match"); + // auto it = this->begin(); + // for(const T& x : values) + // *it++ += x; + // } + // void operator+=(const boost::span values) { template void operator+=(const S values) { if(values.size() != this->size()) @@ -85,7 +85,10 @@ class multi_weight { : base_type{idx} , par_{par} {} - decltype(auto) operator*() const { return Reference{par_->buffer_.get() + this->base() * par_->nelem_, par_->nelem_}; } + decltype(auto) operator*() const { + return Reference{par_->buffer_.get() + this->base() * par_->nelem_, + par_->nelem_}; + } MWPtr par_ = nullptr; }; @@ -99,9 +102,7 @@ class multi_weight { multi_weight(const std::size_t k = 0) : nelem_{k} {} - multi_weight(const multi_weight& other) { - *this = other; - } + multi_weight(const multi_weight& other) { *this = other; } multi_weight& operator=(const multi_weight& other) { nelem_ = other.nelem_; @@ -114,7 +115,7 @@ class multi_weight { std::size_t size() const { return size_; } void reset(std::size_t n) { - size_ = n; + size_ = n; buffer_.reset(new element_type[size_ * nelem_]); default_fill(); } @@ -135,8 +136,12 @@ class multi_weight { const_iterator begin() const { return {this, 0}; } const_iterator end() const { return {this, size_}; } - reference operator[](std::size_t i) { return reference{buffer_.get() + i * nelem_, nelem_}; } - const_reference operator[](std::size_t i) const { return const_reference{buffer_.get() + i * nelem_, nelem_}; } + reference operator[](std::size_t i) { + return reference{buffer_.get() + i * nelem_, nelem_}; + } + const_reference operator[](std::size_t i) const { + return const_reference{buffer_.get() + i * nelem_, nelem_}; + } template bool operator==(const multi_weight& other) const { @@ -177,8 +182,8 @@ class multi_weight { } public: - std::size_t size_ = 0; // Number of bins - std::size_t nelem_ = 0; // Number of weights per bin + std::size_t size_ = 0; // Number of bins + std::size_t nelem_ = 0; // Number of weights per bin std::unique_ptr buffer_; }; From e8b3fbbdba39732e893455d1cb25daefca0fdf21 Mon Sep 17 00:00:00 2001 From: Florian Harz Date: Tue, 27 May 2025 17:06:07 +0200 Subject: [PATCH 07/19] Small fix to multi_weight.hpp --- include/bh_python/multi_weight.hpp | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index 95cdd794..6b473a96 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -31,18 +31,13 @@ struct multi_weight_value : public boost::span { return !operator==(values); } - // template - // void operator+=(const S values) { - // void operator+=(const std::vector values) { - // if(values.size() != this->size()) - // throw std::runtime_error("size does not match"); - // auto it = this->begin(); - // for(const T& x : values) - // *it++ += x; - // } - // void operator+=(const boost::span values) { - template - void operator+=(const S values) { + void operator+=(const std::vector values) { + operator+=(boost::span(values)); + } + + void operator+=(const boost::span values) { + //template + //void operator+=(const S values) { if(values.size() != this->size()) throw std::runtime_error("size does not match"); auto it = this->begin(); From 8800399f376cdeda8e683d0c2427762d5db19f72 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 27 May 2025 15:06:51 +0000 Subject: [PATCH 08/19] style: pre-commit fixes --- include/bh_python/multi_weight.hpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index 6b473a96..45d5be3e 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -31,13 +31,11 @@ struct multi_weight_value : public boost::span { return !operator==(values); } - void operator+=(const std::vector values) { - operator+=(boost::span(values)); - } + void operator+=(const std::vector values) { operator+=(boost::span(values)); } void operator+=(const boost::span values) { - //template - //void operator+=(const S values) { + // template + // void operator+=(const S values) { if(values.size() != this->size()) throw std::runtime_error("size does not match"); auto it = this->begin(); From e60a9d9be9eefe57cbb5069b12ce8b4df1c70148 Mon Sep 17 00:00:00 2001 From: Florian Harz Date: Thu, 12 Jun 2025 13:19:40 +0200 Subject: [PATCH 09/19] Split multi_weight_value into multi_weight_value and multi_weight_reference --- include/bh_python/multi_weight.hpp | 149 ++++++++++++++--------- include/bh_python/register_histogram.hpp | 4 - 2 files changed, 91 insertions(+), 62 deletions(-) diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index 45d5be3e..3ee54251 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -12,11 +12,10 @@ namespace boost { namespace histogram { -template -struct multi_weight_value : public boost::span { - using boost::span::span; - void operator()(const boost::span values) { operator+=(values); } +template +struct multi_weight_base : public BASE { + using BASE::BASE; template bool operator==(const S values) const { @@ -31,7 +30,30 @@ struct multi_weight_value : public boost::span { return !operator==(values); } - void operator+=(const std::vector values) { operator+=(boost::span(values)); } + +}; + +template +struct multi_weight_reference : public multi_weight_base> { + //using boost::span::span; + using multi_weight_base::multi_weight_base; + + void operator()(const boost::span values) { operator+=(values); } + + //template + //bool operator==(const S values) const { + // if(values.size() != this->size()) + // return false; +// + // return std::equal(this->begin(), this->end(), values.begin()); + //} +// + //template + //bool operator!=(const S values) const { + // return !operator==(values); + //} + + //void operator+=(const std::vector values) { operator+=(boost::span(values)); } void operator+=(const boost::span values) { // template @@ -53,13 +75,60 @@ struct multi_weight_value : public boost::span { } }; +template +struct multi_weight_value : public multi_weight_base> { + using multi_weight_base::multi_weight_base; + + multi_weight_value(const boost::span values) { + this->assign(values.begin(), values.end()); + } + multi_weight_value() = default; + + void operator()(const boost::span values) { operator+=(values); } + + //template + //bool operator==(const S values) const { + // if(values.size() != this->size()) + // return false; +// + // return std::equal(this->begin(), this->end(), values.begin()); + //} +// + //template + //bool operator!=(const S values) const { + // return !operator==(values); + //} +// + //void operator+=(const std::vector values) { operator+=(boost::span(values)); } + + //template + //void operator+=(const S values) { + void operator+=(const boost::span values) { + if(values.size() != this->size()) { + if (this->size() > 0) { + throw std::runtime_error("size does not match"); + } + this->assign(values.begin(), values.end()); + return; + } + auto it = this->begin(); + for(const T& x : values) + *it++ += x; + } + + template + void operator=(const S values) { + this->assign(values.begin(), values.end()); + } +}; + template class multi_weight { public: using element_type = ElementType; using value_type = multi_weight_value; - using reference = value_type; - using const_reference = const value_type; + using reference = multi_weight_reference; + using const_reference = const reference; template struct iterator_base @@ -194,11 +263,25 @@ std::ostream& operator<<(std::ostream& os, const multi_weight_value& v) { return os; } +template +std::ostream& operator<<(std::ostream& os, const multi_weight_reference& v) { + os << "multi_weight_reference("; + bool first = true; + for(const T& x : v) + if(first) { + first = false; + os << x; + } else + os << ", " << x; + os << ")"; + return os; +} + template std::ostream& operator<<(std::ostream& os, const multi_weight& v) { os << "multi_weight(\n"; int index = 0; - for(const multi_weight_value& x : v) { + for(const multi_weight_reference& x : v) { os << "Index " << index << ": " << x << "\n"; index++; } @@ -206,56 +289,6 @@ std::ostream& operator<<(std::ostream& os, const multi_weight& v) { return os; } -namespace algorithm { - -/** Compute the sum over all histogram cells (underflow/overflow included by default). - - The implementation favors accuracy and protection against overflow over speed. If the - value type of the histogram is an integral or floating point type, - accumulators::sum is used to compute the sum, else the original value type is - used. Compilation fails, if the value type does not support operator+=. The return - type is double if the value type of the histogram is integral or floating point, and - the original value type otherwise. - - If you need a different trade-off, you can write your own loop or use - `std::accumulate`: - ``` - // iterate over all bins - auto sum_all = std::accumulate(hist.begin(), hist.end(), 0.0); - - // skip underflow/overflow bins - double sum = 0; - for (auto&& x : indexed(hist)) - sum += *x; // dereference accessor - - // or: - // auto ind = boost::histogram::indexed(hist); - // auto sum = std::accumulate(ind.begin(), ind.end(), 0.0); - ``` - - @returns accumulator type or double - - @param hist Const reference to the histogram. - @param cov Iterate over all or only inner bins (optional, default: all). -*/ -template -std::vector sum(const histogram>& hist, - const coverage cov = coverage::all) { - using sum_type = typename histogram>::value_type; - // T is arithmetic, compute sum accurately with high dynamic range - std::vector v(unsafe_access::storage(hist).nelem_, 0.); - sum_type sum(v); - if(cov == coverage::all) - for(auto&& x : hist) - sum += x; - else - // sum += x also works if sum_type::operator+=(const sum_type&) exists - for(auto&& x : indexed(hist)) - sum += *x; - return v; -} - -} // namespace algorithm } // namespace histogram } // namespace boost diff --git a/include/bh_python/register_histogram.hpp b/include/bh_python/register_histogram.hpp index 7f0c10ff..d2104586 100644 --- a/include/bh_python/register_histogram.hpp +++ b/include/bh_python/register_histogram.hpp @@ -343,10 +343,6 @@ auto register_histogram>(py::module& m, [](const histogram_t& self, py::args& args) -> value_type { auto int_args = py::cast>(args); auto at_value = self.at(int_args); - // value_type return_obj; - // return_obj.insert(return_obj.end(), at_value.begin(), - // at_value.end()); return_obj.assign(at_value.begin(), - // at_value.end()); return return_obj; return value_type(at_value.begin(), at_value.end()); }) From 161c8dbf6ca1ad7c059e104838911e3609beb187 Mon Sep 17 00:00:00 2001 From: Florian Harz Date: Thu, 12 Jun 2025 16:28:45 +0200 Subject: [PATCH 10/19] Fix compiling on linux --- include/bh_python/multi_weight.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index 3ee54251..cff2d211 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -36,7 +36,7 @@ struct multi_weight_base : public BASE { template struct multi_weight_reference : public multi_weight_base> { //using boost::span::span; - using multi_weight_base::multi_weight_base; + using multi_weight_base>::multi_weight_base; void operator()(const boost::span values) { operator+=(values); } @@ -77,7 +77,7 @@ struct multi_weight_reference : public multi_weight_base> { template struct multi_weight_value : public multi_weight_base> { - using multi_weight_base::multi_weight_base; + using multi_weight_base>::multi_weight_base; multi_weight_value(const boost::span values) { this->assign(values.begin(), values.end()); From 7b986401f15a4444b1b56c292103a8d8a1b5135d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 12 Jun 2025 11:20:19 +0000 Subject: [PATCH 11/19] style: pre-commit fixes --- include/bh_python/multi_weight.hpp | 55 +++++++++++++++--------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index cff2d211..c5e34647 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -12,7 +12,6 @@ namespace boost { namespace histogram { - template struct multi_weight_base : public BASE { using BASE::BASE; @@ -29,31 +28,30 @@ struct multi_weight_base : public BASE { bool operator!=(const S values) const { return !operator==(values); } - - }; template struct multi_weight_reference : public multi_weight_base> { - //using boost::span::span; + // using boost::span::span; using multi_weight_base>::multi_weight_base; void operator()(const boost::span values) { operator+=(values); } - //template - //bool operator==(const S values) const { - // if(values.size() != this->size()) - // return false; -// + // template + // bool operator==(const S values) const { + // if(values.size() != this->size()) + // return false; + // // return std::equal(this->begin(), this->end(), values.begin()); //} -// - //template - //bool operator!=(const S values) const { + // + // template + // bool operator!=(const S values) const { // return !operator==(values); //} - //void operator+=(const std::vector values) { operator+=(boost::span(values)); } + // void operator+=(const std::vector values) { + // operator+=(boost::span(values)); } void operator+=(const boost::span values) { // template @@ -81,31 +79,32 @@ struct multi_weight_value : public multi_weight_base> { multi_weight_value(const boost::span values) { this->assign(values.begin(), values.end()); - } + } multi_weight_value() = default; void operator()(const boost::span values) { operator+=(values); } - //template - //bool operator==(const S values) const { - // if(values.size() != this->size()) - // return false; -// + // template + // bool operator==(const S values) const { + // if(values.size() != this->size()) + // return false; + // // return std::equal(this->begin(), this->end(), values.begin()); //} -// - //template - //bool operator!=(const S values) const { + // + // template + // bool operator!=(const S values) const { // return !operator==(values); //} -// - //void operator+=(const std::vector values) { operator+=(boost::span(values)); } - - //template - //void operator+=(const S values) { + // + // void operator+=(const std::vector values) { + // operator+=(boost::span(values)); } + + // template + // void operator+=(const S values) { void operator+=(const boost::span values) { if(values.size() != this->size()) { - if (this->size() > 0) { + if(this->size() > 0) { throw std::runtime_error("size does not match"); } this->assign(values.begin(), values.end()); From 0eef303b9bd9de1af191579e8fa14dad87c3b6d1 Mon Sep 17 00:00:00 2001 From: Hans Dembinski Date: Sun, 22 Jun 2025 14:55:42 +0200 Subject: [PATCH 12/19] add a test --- tests/test_storage.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/test_storage.py b/tests/test_storage.py index 59308803..3add63e1 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -392,3 +392,12 @@ def test_non_uniform_rebin_with_weights(): [1.0, 1.05, 1.15, 3.0], ) ) + + +def test_multi_weight(): + x = np.array([1, 2]) + weights = np.array([[1, 2, 3], [4, 5, 6]]) + h = bh.Histogram(bh.axis.Regular(5, 0, 5), storage=bh.storage.MultiWeight(3)) + h.fill(x, sample=weights) + assert_array_equal(h[1], [1, 2, 3]) + assert_array_equal(h[2], [4, 5, 6]) From b5ba67eb09c255e175e8c99454b05dddf3f3ffaa Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Tue, 29 Jul 2025 15:15:29 -0600 Subject: [PATCH 13/19] chore: fix typing issue Signed-off-by: Henry Schreiner --- src/boost_histogram/_core/hist.pyi | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/boost_histogram/_core/hist.pyi b/src/boost_histogram/_core/hist.pyi index 322dd707..2272fa74 100644 --- a/src/boost_histogram/_core/hist.pyi +++ b/src/boost_histogram/_core/hist.pyi @@ -77,30 +77,13 @@ class any_mean(_BaseHistogram): def at(self, *args: int) -> accumulators.Mean: ... def _at_set(self, value: accumulators.Mean, *args: int) -> None: ... def sum(self, flow: bool = ...) -> accumulators.Mean: ... - def fill( - self, - *args: ArrayLike, - weight: ArrayLike | None = ..., - sample: ArrayLike | None = ..., - ) -> None: ... class any_weighted_mean(_BaseHistogram): def at(self, *args: int) -> accumulators.WeightedMean: ... def _at_set(self, value: accumulators.WeightedMean, *args: int) -> None: ... def sum(self, flow: bool = ...) -> accumulators.WeightedMean: ... - def fill( - self, - *args: ArrayLike, - weight: ArrayLike | None = ..., - sample: ArrayLike | None = ..., - ) -> None: ... class any_multi_weight(_BaseHistogram): def at(self, *args: int) -> float: ... def _at_set(self, value: float, *args: int) -> ArrayLike: ... def sum(self, flow: bool = ...) -> ArrayLike: ... - def fill( - self, - *args: ArrayLike, - sample: ArrayLike | None = ..., - ) -> None: ... From 532edd4e9d447d91125d6cc614ce5b4bbd158019 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Thu, 31 Jul 2025 11:57:21 -0400 Subject: [PATCH 14/19] chore: address some lints Signed-off-by: Henry Schreiner --- include/bh_python/fill.hpp | 14 +++++++------- include/bh_python/multi_weight.hpp | 12 ++++++------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/include/bh_python/fill.hpp b/include/bh_python/fill.hpp index d5849f61..ab0f6251 100644 --- a/include/bh_python/fill.hpp +++ b/include/bh_python/fill.hpp @@ -182,7 +182,7 @@ void fill_impl(bh::detail::accumulator_traits_holder, finalize_args(kwargs); // releasing gil here is safe, we don't manipulate refcounts - py::gil_scoped_release const lock; + const py::gil_scoped_release lock; variant::visit( overload([&h, &vargs](const variant::monostate&) { h.fill(vargs); }, [&h, &vargs](const auto& w) { h.fill(vargs, bh::weight(w)); }), @@ -204,7 +204,7 @@ void fill_impl(bh::detail::accumulator_traits_holder, throw std::invalid_argument("Sample array must be 1D"); // releasing gil here is safe, we don't manipulate refcounts - py::gil_scoped_release const lock; + const py::gil_scoped_release lock; variant::visit( overload([&h, &vargs, &sarray]( const variant::monostate&) { h.fill(vargs, bh::sample(sarray)); }, @@ -216,7 +216,7 @@ void fill_impl(bh::detail::accumulator_traits_holder, // for multi_weight template -void fill_impl(bh::detail::accumulator_traits_holder>, +void fill_impl(bh::detail::accumulator_traits_holder&>, Histogram& h, const VArgs& vargs, const weight_t& weight, @@ -230,10 +230,10 @@ void fill_impl(bh::detail::accumulator_traits_holder> auto buf = sarray.request(); // releasing gil here is safe, we don't manipulate refcounts - py::gil_scoped_release lock; - std::size_t buf_shape0 = static_cast(buf.shape[0]); - std::size_t buf_shape1 = static_cast(buf.shape[1]); - double* src = static_cast(buf.ptr); + const py::gil_scoped_release lock; + const auto buf_shape0 = static_cast(buf.shape[0]); + const auto buf_shape1 = static_cast(buf.shape[1]); + double* src = static_cast(buf.ptr); std::vector> vec_s; vec_s.reserve(buf_shape0); for(std::size_t i = 0; i < buf_shape0; i++) { diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index c5e34647..71813cde 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -57,7 +57,7 @@ struct multi_weight_reference : public multi_weight_base> { // template // void operator+=(const S values) { if(values.size() != this->size()) - throw std::runtime_error("size does not match"); + throw std::range_error("size does not match for += ref"); auto it = this->begin(); for(const T& x : values) *it++ += x; @@ -66,7 +66,7 @@ struct multi_weight_reference : public multi_weight_base> { template void operator=(const S values) { if(values.size() != this->size()) - throw std::runtime_error("size does not match"); + throw std::range_error("size does not match for = ref"); auto it = this->begin(); for(const T& x : values) *it++ = x; @@ -82,7 +82,7 @@ struct multi_weight_value : public multi_weight_base> { } multi_weight_value() = default; - void operator()(const boost::span values) { operator+=(values); } + void operator()(const boost::span& values) { operator+=(values); } // template // bool operator==(const S values) const { @@ -102,10 +102,10 @@ struct multi_weight_value : public multi_weight_base> { // template // void operator+=(const S values) { - void operator+=(const boost::span values) { + void operator+=(const boost::span& values) { if(values.size() != this->size()) { if(this->size() > 0) { - throw std::runtime_error("size does not match"); + throw std::range_error("size does not match for += val"); } this->assign(values.begin(), values.end()); return; @@ -220,7 +220,7 @@ class multi_weight { template void operator+=(const multi_weight& other) { if(size_ * nelem_ != other.size_ * other.nelem_) { - throw std::runtime_error("size does not match"); + throw std::range_error("size does not match"); } for(std::size_t i = 0; i < size_ * nelem_; i++) { buffer_[i] += other.buffer_[i]; From b70523b3a4ba57f414edca506c3856a317a2d01e Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Thu, 31 Jul 2025 12:09:42 -0400 Subject: [PATCH 15/19] feat: add nelem Signed-off-by: Henry Schreiner --- CMakeLists.txt | 11 +++++++++-- include/bh_python/histogram.hpp | 2 +- include/bh_python/multi_weight.hpp | 6 +++++- include/bh_python/register_storage.hpp | 5 ++++- src/boost_histogram/_core/storage.pyi | 5 ++++- src/boost_histogram/storage.py | 3 +++ 6 files changed, 26 insertions(+), 6 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e766348f..d229821c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -120,8 +120,15 @@ option(BOOST_HISTOGRAM_ERRORS "Make warnings errors (for CI mostly)") # Adding warnings # Boost.Histogram doesn't pass sign -Wsign-conversion if("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang" OR "${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU") - target_compile_options(_core PRIVATE -Wall -Wextra -pedantic-errors -Wconversion -Wsign-compare - -Wno-unused-value) + target_compile_options( + _core + PRIVATE -Wall + -Wextra + -pedantic-errors + -Wconversion + -Wsign-compare + -Wno-unused-value + -Wno-sign-conversion) if(BOOST_HISTOGRAM_ERRORS) target_compile_options(_core PRIVATE -Werror) endif() diff --git a/include/bh_python/histogram.hpp b/include/bh_python/histogram.hpp index 3e441ae5..23903aaf 100644 --- a/include/bh_python/histogram.hpp +++ b/include/bh_python/histogram.hpp @@ -103,7 +103,7 @@ py::buffer_info make_buffer(bh::histogram>& h, bool flow) const auto& axes = bh::unsafe_access::axes(h); auto& storage = bh::unsafe_access::storage(h); return detail::make_buffer_impl( - axes, flow, static_cast(storage.buffer_.get())); + axes, flow, static_cast(storage.get_buffer())); } /// Compute the bin of an array from a runtime list diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index 71813cde..2a3466cd 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -175,6 +175,8 @@ class multi_weight { std::size_t size() const { return size_; } + std::size_t nelem() const { return nelem_; } + void reset(std::size_t n) { size_ = n; buffer_.reset(new element_type[size_ * nelem_]); @@ -242,7 +244,9 @@ class multi_weight { } } - public: + element_type* get_buffer() { return buffer_.get(); } + + private: std::size_t size_ = 0; // Number of bins std::size_t nelem_ = 0; // Number of weights per bin std::unique_ptr buffer_; diff --git a/include/bh_python/register_storage.hpp b/include/bh_python/register_storage.hpp index 662b71ac..be422abb 100644 --- a/include/bh_python/register_storage.hpp +++ b/include/bh_python/register_storage.hpp @@ -99,7 +99,10 @@ register_storage(py::module& m, const char* name, const char* desc) { }) .def(make_pickle()) .def("__copy__", [](const A& self) { return A(self); }) - .def("__deepcopy__", [](const A& self, py::object) { return A(self); }); + .def("__deepcopy__", [](const A& self, py::object) { return A(self); }) + .def_property_readonly("nelem", [](const A& self) { return self.nelem(); }) + + ; return storage; } diff --git a/src/boost_histogram/_core/storage.pyi b/src/boost_histogram/_core/storage.pyi index 175ec223..63700d0d 100644 --- a/src/boost_histogram/_core/storage.pyi +++ b/src/boost_histogram/_core/storage.pyi @@ -16,4 +16,7 @@ class unlimited(_BaseStorage): ... class weight(_BaseStorage): ... class mean(_BaseStorage): ... class weighted_mean(_BaseStorage): ... -class multi_weight(_BaseStorage): ... + +class multi_weight(_BaseStorage): + @property + def nelem(self) -> int: ... diff --git a/src/boost_histogram/storage.py b/src/boost_histogram/storage.py index d1e6d2ec..3e27f663 100644 --- a/src/boost_histogram/storage.py +++ b/src/boost_histogram/storage.py @@ -78,3 +78,6 @@ class WeightedMean(store.weighted_mean, Storage, family=boost_histogram): class MultiWeight(store.multi_weight, Storage, family=boost_histogram): accumulator = float + + def __repr__(self) -> str: + return f"{self.__class__.__name__}({self.nelem})" From 599d0da86ee2ec8fddfe5a640f1ff59bbdc2afae Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Thu, 31 Jul 2025 12:15:51 -0400 Subject: [PATCH 16/19] chore: minor lint fixes Signed-off-by: Henry Schreiner --- include/bh_python/fill.hpp | 2 +- include/bh_python/register_storage.hpp | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/include/bh_python/fill.hpp b/include/bh_python/fill.hpp index ab0f6251..67b8a1bd 100644 --- a/include/bh_python/fill.hpp +++ b/include/bh_python/fill.hpp @@ -237,7 +237,7 @@ void fill_impl(bh::detail::accumulator_traits_holder> vec_s; vec_s.reserve(buf_shape0); for(std::size_t i = 0; i < buf_shape0; i++) { - vec_s.emplace_back(boost::span{src + i * buf_shape1, buf_shape1}); + vec_s.emplace_back(src + (i * buf_shape1), buf_shape1); } h.fill(vargs, bh::sample(vec_s)); } diff --git a/include/bh_python/register_storage.hpp b/include/bh_python/register_storage.hpp index be422abb..cb82127a 100644 --- a/include/bh_python/register_storage.hpp +++ b/include/bh_python/register_storage.hpp @@ -74,8 +74,9 @@ py::class_ inline register_storage(py::module& m, /// Add helpers to the multi_weight storage type template <> -py::class_ -register_storage(py::module& m, const char* name, const char* desc) { +py::class_ inline register_storage(py::module& m, + const char* name, + const char* desc) { using A = storage::multi_weight; // match code above py::class_ storage(m, name, desc); From cadcd71b3d7a303d41643d2a1bed7e7d0f5460bd Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Thu, 31 Jul 2025 12:23:29 -0400 Subject: [PATCH 17/19] chore: more explicits Signed-off-by: Henry Schreiner --- include/bh_python/multi_weight.hpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index 2a3466cd..4d689bc0 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -64,12 +64,13 @@ struct multi_weight_reference : public multi_weight_base> { } template - void operator=(const S values) { + multi_weight_reference& operator=(const S values) { if(values.size() != this->size()) throw std::range_error("size does not match for = ref"); auto it = this->begin(); for(const T& x : values) *it++ = x; + return *this; } }; @@ -77,7 +78,7 @@ template struct multi_weight_value : public multi_weight_base> { using multi_weight_base>::multi_weight_base; - multi_weight_value(const boost::span values) { + explicit multi_weight_value(const boost::span values) { this->assign(values.begin(), values.end()); } multi_weight_value() = default; @@ -118,6 +119,7 @@ struct multi_weight_value : public multi_weight_base> { template void operator=(const S values) { this->assign(values.begin(), values.end()); + return *this; } }; @@ -160,7 +162,7 @@ class multi_weight { static constexpr bool has_threading_support() { return false; } - multi_weight(const std::size_t k = 0) + explicit multi_weight(const std::size_t k = 0) : nelem_{k} {} multi_weight(const multi_weight& other) { *this = other; } From 045583d732df292077258ee4792dfa7dfa4256d5 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Thu, 31 Jul 2025 12:33:52 -0400 Subject: [PATCH 18/19] fix: protect against self assignment Signed-off-by: Henry Schreiner --- include/bh_python/multi_weight.hpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index 4d689bc0..81b6ae4c 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -168,6 +168,10 @@ class multi_weight { multi_weight(const multi_weight& other) { *this = other; } multi_weight& operator=(const multi_weight& other) { + // Protect against self assignment + if(this == &other) { + return *this; + } nelem_ = other.nelem_; reset(other.size_); std::copy( From eceee1087c67fa459bac021c6548dd8657a565e7 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Thu, 31 Jul 2025 14:36:29 -0400 Subject: [PATCH 19/19] fix: rest of clang-tidy checks Signed-off-by: Henry Schreiner --- .github/CONTRIBUTING.md | 7 +-- CMakePresets.json | 31 +++++++++++++ include/bh_python/fill.hpp | 2 +- include/bh_python/multi_weight.hpp | 10 ++-- include/bh_python/register_histogram.hpp | 58 ++++++++++++------------ include/bh_python/register_storage.hpp | 2 +- 6 files changed, 71 insertions(+), 39 deletions(-) diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 814958a9..50eb2b7b 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -205,11 +205,12 @@ cmake --build --preset tidy To autofix, use: ```bash -cmake --preset --preset tidy -DCMAKE_CXX_CLANG_TIDY="clang-tidy;--fix" -cmake --build --preset tidy -j1 +cmake --preset --preset tidy-fix +cmake --build --preset tidy-fix ``` -Remember to build single-threaded if applying fixes! +We also provide matching `--workflow`'s, but you'll need a newer CMake for that +(you can use pip to get it, though). ## Include what you use diff --git a/CMakePresets.json b/CMakePresets.json index 621e7f0a..5f6cc352 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -24,6 +24,14 @@ "cacheVariables": { "CMAKE_CXX_CLANG_TIDY": "clang-tidy;--warnings-as-errors=*" } + }, + { + "name": "tidy-fix", + "displayName": "Clang-tidy autofix", + "inherits": "tidy", + "cacheVariables": { + "CMAKE_CXX_CLANG_TIDY": "clang-tidy;--warnings-as-errors=*;--fix" + } } ], "buildPresets": [ @@ -38,6 +46,13 @@ "displayName": "Clang-tidy build", "configurePreset": "tidy", "nativeToolOptions": ["-k0"] + }, + { + "name": "tidy-fix", + "displayName": "Clang-tidy autofix build", + "configurePreset": "tidy-fix", + "jobs": 1, + "nativeToolOptions": ["-k0"] } ], "testPresets": [ @@ -59,6 +74,22 @@ { "type": "build", "name": "default" }, { "type": "test", "name": "default" } ] + }, + { + "name": "tidy", + "displayName": "Clang-tidy workflow", + "steps": [ + { "type": "configure", "name": "tidy" }, + { "type": "build", "name": "tidy" } + ] + }, + { + "name": "tidy-fix", + "displayName": "Clang-tidy autofix workflow", + "steps": [ + { "type": "configure", "name": "tidy-fix" }, + { "type": "build", "name": "tidy-fix" } + ] } ] } diff --git a/include/bh_python/fill.hpp b/include/bh_python/fill.hpp index 67b8a1bd..14256e94 100644 --- a/include/bh_python/fill.hpp +++ b/include/bh_python/fill.hpp @@ -233,7 +233,7 @@ void fill_impl(bh::detail::accumulator_traits_holder(buf.shape[0]); const auto buf_shape1 = static_cast(buf.shape[1]); - double* src = static_cast(buf.ptr); + auto* src = static_cast(buf.ptr); std::vector> vec_s; vec_s.reserve(buf_shape0); for(std::size_t i = 0; i < buf_shape0; i++) { diff --git a/include/bh_python/multi_weight.hpp b/include/bh_python/multi_weight.hpp index 81b6ae4c..d22716aa 100644 --- a/include/bh_python/multi_weight.hpp +++ b/include/bh_python/multi_weight.hpp @@ -17,7 +17,7 @@ struct multi_weight_base : public BASE { using BASE::BASE; template - bool operator==(const S values) const { + bool operator==(const S& values) const { if(values.size() != this->size()) return false; @@ -25,7 +25,7 @@ struct multi_weight_base : public BASE { } template - bool operator!=(const S values) const { + bool operator!=(const S& values) const { return !operator==(values); } }; @@ -64,7 +64,7 @@ struct multi_weight_reference : public multi_weight_base> { } template - multi_weight_reference& operator=(const S values) { + multi_weight_reference& operator=(const S& values) { if(values.size() != this->size()) throw std::range_error("size does not match for = ref"); auto it = this->begin(); @@ -117,7 +117,7 @@ struct multi_weight_value : public multi_weight_base> { } template - void operator=(const S values) { + multi_weight_value& operator=(const S values) { this->assign(values.begin(), values.end()); return *this; } @@ -148,6 +148,8 @@ class multi_weight { : base_type{idx} , par_{par} {} + iterator_base& operator=(const iterator_base& other) = default; + decltype(auto) operator*() const { return Reference{par_->buffer_.get() + this->base() * par_->nelem_, par_->nelem_}; diff --git a/include/bh_python/register_histogram.hpp b/include/bh_python/register_histogram.hpp index d2104586..b369e8b6 100644 --- a/include/bh_python/register_histogram.hpp +++ b/include/bh_python/register_histogram.hpp @@ -155,13 +155,13 @@ auto register_histogram(py::module& m, const char* name, const char* desc) { py::keep_alive<0, 1>()) .def("at", - [](const histogram_t& self, py::args& args) -> value_type { + [](const histogram_t& self, const py::args& args) -> value_type { auto int_args = py::cast>(args); return self.at(int_args); }) .def("_at_set", - [](histogram_t& self, const value_type& input, py::args& args) { + [](histogram_t& self, const value_type& input, const py::args& args) { auto int_args = py::cast>(args); self.at(int_args) = input; }) @@ -171,7 +171,7 @@ auto register_histogram(py::module& m, const char* name, const char* desc) { .def( "sum", [](const histogram_t& self, bool flow) { - py::gil_scoped_release const release; + const py::gil_scoped_release release; return bh::algorithm::sum( self, flow ? bh::coverage::all : bh::coverage::inner); }, @@ -180,7 +180,7 @@ auto register_histogram(py::module& m, const char* name, const char* desc) { .def( "empty", [](const histogram_t& self, bool flow) { - py::gil_scoped_release const release; + const py::gil_scoped_release release; return bh::algorithm::empty( self, flow ? bh::coverage::all : bh::coverage::inner); }, @@ -190,14 +190,14 @@ auto register_histogram(py::module& m, const char* name, const char* desc) { [](const histogram_t& self, const py::args& args) { auto commands = py::cast>(args); - py::gil_scoped_release const release; + const py::gil_scoped_release release; return bh::algorithm::reduce(self, commands); }) .def("project", [](const histogram_t& self, const py::args& values) { auto cpp_values = py::cast>(values); - py::gil_scoped_release const release; + const py::gil_scoped_release release; return bh::algorithm::project(self, cpp_values); }) @@ -211,9 +211,9 @@ auto register_histogram(py::module& m, const char* name, const char* desc) { } template <> -auto register_histogram>(py::module& m, - const char* name, - const char* desc) { +auto inline register_histogram>(py::module& m, + const char* name, + const char* desc) { using S = bh::multi_weight; using histogram_t = bh::histogram; using value_type = std::vector; @@ -231,9 +231,9 @@ auto register_histogram>(py::module& m, .def("__copy__", [](const histogram_t& self) { return histogram_t(self); }) .def("__deepcopy__", - [](const histogram_t& self, py::object memo) { - auto* a = new histogram_t(self); - py::module copy = py::module::import("copy"); + [](const histogram_t& self, const py::object& memo) { + auto* a = new histogram_t(self); + const py::module copy = py::module::import("copy"); for(unsigned i = 0; i < a->rank(); i++) { bh::unsafe_access::axis(*a, i).metadata() = copy.attr("deepcopy")(a->axis(i).metadata(), memo); @@ -262,7 +262,7 @@ auto register_histogram>(py::module& m, .def_property_readonly_static( "_storage_type", - [](py::object) { + [](const py::object&) { return py::type::of(); }) @@ -296,7 +296,7 @@ auto register_histogram>(py::module& m, unchecked_set(tup, 0, py::array(make_buffer(h, flow))); // Add the axis edges - h.for_each_axis([&tup, flow, i = 0u](const auto& ax) mutable { + h.for_each_axis([&tup, flow, i = 0U](const auto& ax) mutable { unchecked_set(tup, ++i, axis::edges(ax, flow, true)); }); @@ -306,7 +306,7 @@ auto register_histogram>(py::module& m, .def( "view", - [](py::object self, bool flow) { + [](const py::object& self, bool flow) { auto& h = py::cast(self); return py::array(make_buffer(h, flow), self); }, @@ -315,8 +315,9 @@ auto register_histogram>(py::module& m, .def( "axis", [](const histogram_t& self, int i) -> py::object { - unsigned ii = i < 0 ? self.rank() - static_cast(std::abs(i)) - : static_cast(i); + unsigned const ii + = i < 0 ? self.rank() - static_cast(std::abs(i)) + : static_cast(i); if(ii < self.rank()) { const axis_variant& var = self.axis(ii); @@ -329,25 +330,22 @@ auto register_histogram>(py::module& m, return py::cast(item, py::return_value_policy::reference); }, var); - } - else - throw std::out_of_range( - "The axis value must be less than the rank"); + throw std::out_of_range("The axis value must be less than the rank"); }, "i"_a = 0, py::keep_alive<0, 1>()) .def("at", - [](const histogram_t& self, py::args& args) -> value_type { + [](const histogram_t& self, const py::args& args) -> value_type { auto int_args = py::cast>(args); auto at_value = self.at(int_args); - return value_type(at_value.begin(), at_value.end()); + return {at_value.begin(), at_value.end()}; }) .def("_at_set", - [](histogram_t& self, const value_type& input, py::args& args) { + [](histogram_t& self, const value_type& input, const py::args& args) { auto int_args = py::cast>(args); self.at(int_args) = input; }) @@ -357,7 +355,7 @@ auto register_histogram>(py::module& m, .def( "sum", [](const histogram_t& self, bool flow) -> value_type { - py::gil_scoped_release release; + const py::gil_scoped_release release; return bh::algorithm::sum( self, flow ? bh::coverage::all : bh::coverage::inner); }, @@ -366,24 +364,24 @@ auto register_histogram>(py::module& m, .def( "empty", [](const histogram_t& self, bool flow) { - py::gil_scoped_release release; + const py::gil_scoped_release release; return bh::algorithm::empty( self, flow ? bh::coverage::all : bh::coverage::inner); }, "flow"_a = false) .def("reduce", - [](const histogram_t& self, py::args args) { + [](const histogram_t& self, const py::args& args) { auto commands = py::cast>(args); - py::gil_scoped_release release; + const py::gil_scoped_release release; return bh::algorithm::reduce(self, commands); }) .def("project", - [](const histogram_t& self, py::args values) { + [](const histogram_t& self, const py::args& values) { auto cpp_values = py::cast>(values); - py::gil_scoped_release release; + const py::gil_scoped_release release; return bh::algorithm::project(self, cpp_values); }) diff --git a/include/bh_python/register_storage.hpp b/include/bh_python/register_storage.hpp index cb82127a..537193b3 100644 --- a/include/bh_python/register_storage.hpp +++ b/include/bh_python/register_storage.hpp @@ -100,7 +100,7 @@ py::class_ inline register_storage(py::module& m, }) .def(make_pickle()) .def("__copy__", [](const A& self) { return A(self); }) - .def("__deepcopy__", [](const A& self, py::object) { return A(self); }) + .def("__deepcopy__", [](const A& self, const py::object&) { return A(self); }) .def_property_readonly("nelem", [](const A& self) { return self.nelem(); }) ;