From b5cd4fc5a80d4c24aedc07ba0709fd41e6d9a990 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Wed, 16 Jul 2025 11:13:21 +0200 Subject: [PATCH 1/2] [hist] Test ComputeLinearizedIndex exactly on the edges --- hist/histv7/test/hist_regular.cxx | 20 ++++++++++++++++++++ hist/histv7/test/hist_variable.cxx | 20 ++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/hist/histv7/test/hist_regular.cxx b/hist/histv7/test/hist_regular.cxx index 7c28c1816653f..fd511de869d3e 100644 --- a/hist/histv7/test/hist_regular.cxx +++ b/hist/histv7/test/hist_regular.cxx @@ -64,6 +64,16 @@ TEST(RRegularAxis, ComputeLinearizedIndex) EXPECT_FALSE(linIndex.fValid); } + // Exactly the lower end of the axis interval + { + auto linIndex = axis.ComputeLinearizedIndex(0); + EXPECT_EQ(linIndex.fIndex, 0); + EXPECT_TRUE(linIndex.fValid); + linIndex = axisNoFlowBins.ComputeLinearizedIndex(0); + EXPECT_EQ(linIndex.fIndex, 0); + EXPECT_TRUE(linIndex.fValid); + } + for (std::size_t i = 0; i < Bins; i++) { auto linIndex = axis.ComputeLinearizedIndex(i + 0.5); EXPECT_EQ(linIndex.fIndex, i); @@ -73,6 +83,16 @@ TEST(RRegularAxis, ComputeLinearizedIndex) EXPECT_TRUE(linIndex.fValid); } + // Exactly the upper end of the axis interval + { + auto linIndex = axis.ComputeLinearizedIndex(Bins); + EXPECT_EQ(linIndex.fIndex, Bins + 1); + EXPECT_TRUE(linIndex.fValid); + linIndex = axisNoFlowBins.ComputeLinearizedIndex(Bins); + EXPECT_EQ(linIndex.fIndex, Bins + 1); + EXPECT_FALSE(linIndex.fValid); + } + // Overflow static constexpr double PositiveInfinity = std::numeric_limits::infinity(); static constexpr double NaN = std::numeric_limits::quiet_NaN(); diff --git a/hist/histv7/test/hist_variable.cxx b/hist/histv7/test/hist_variable.cxx index 1b4782c2249f8..6cda6af224ecd 100644 --- a/hist/histv7/test/hist_variable.cxx +++ b/hist/histv7/test/hist_variable.cxx @@ -95,6 +95,26 @@ TEST(RVariableBinAxis, ComputeLinearizedIndex) EXPECT_TRUE(linIndex.fValid); } + // Exactly on the bin edges + for (std::size_t i = 0; i < Bins; i++) { + auto linIndex = axis.ComputeLinearizedIndex(i); + EXPECT_EQ(linIndex.fIndex, i); + EXPECT_TRUE(linIndex.fValid); + linIndex = axisNoFlowBins.ComputeLinearizedIndex(i); + EXPECT_EQ(linIndex.fIndex, i); + EXPECT_TRUE(linIndex.fValid); + } + + // Exactly the upper end of the axis interval + { + auto linIndex = axis.ComputeLinearizedIndex(Bins); + EXPECT_EQ(linIndex.fIndex, Bins + 1); + EXPECT_TRUE(linIndex.fValid); + linIndex = axisNoFlowBins.ComputeLinearizedIndex(Bins); + EXPECT_EQ(linIndex.fIndex, Bins + 1); + EXPECT_FALSE(linIndex.fValid); + } + // Overflow static constexpr double PositiveInfinity = std::numeric_limits::infinity(); static constexpr double NaN = std::numeric_limits::quiet_NaN(); From e90f5737f8915dc1c28e3674fd811c3237c86803 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Thu, 31 Jul 2025 10:49:58 +0200 Subject: [PATCH 2/2] [hist] Check arguments in axis constructors --- hist/histv7/inc/ROOT/RRegularAxis.hxx | 13 ++++++++++--- hist/histv7/inc/ROOT/RVariableBinAxis.hxx | 14 ++++++++++++-- hist/histv7/test/hist_regular.cxx | 3 +++ hist/histv7/test/hist_variable.cxx | 6 ++++++ 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/hist/histv7/inc/ROOT/RRegularAxis.hxx b/hist/histv7/inc/ROOT/RRegularAxis.hxx index aa68d641c32aa..a31984f67871d 100644 --- a/hist/histv7/inc/ROOT/RRegularAxis.hxx +++ b/hist/histv7/inc/ROOT/RRegularAxis.hxx @@ -9,6 +9,7 @@ #include #include +#include class TBuffer; @@ -44,14 +45,20 @@ class RRegularAxis final { public: /// Construct a regular axis object. /// - /// \param[in] numNormalBins the number of normal bins + /// \param[in] numNormalBins the number of normal bins, must be > 0 /// \param[in] low the lower end of the axis interval (inclusive) - /// \param[in] high the upper end of the axis interval (exclusive) + /// \param[in] high the upper end of the axis interval (exclusive), must be > low /// \param[in] enableFlowBins whether to enable underflow and overflow bins RRegularAxis(std::size_t numNormalBins, double low, double high, bool enableFlowBins = true) : fNumNormalBins(numNormalBins), fLow(low), fHigh(high), fEnableFlowBins(enableFlowBins) { - // FIXME: should validate numNormalBins > 0 and low < high + if (numNormalBins == 0) { + throw std::invalid_argument("numNormalBins must be > 0"); + } + if (low >= high) { + std::string msg = "high must be > low, but " + std::to_string(low) + " >= " + std::to_string(high); + throw std::invalid_argument(msg); + } fInvBinWidth = numNormalBins / (high - low); } diff --git a/hist/histv7/inc/ROOT/RVariableBinAxis.hxx b/hist/histv7/inc/ROOT/RVariableBinAxis.hxx index fbe5f3c94583a..9a4eabc981a95 100644 --- a/hist/histv7/inc/ROOT/RVariableBinAxis.hxx +++ b/hist/histv7/inc/ROOT/RVariableBinAxis.hxx @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -41,12 +42,21 @@ class RVariableBinAxis final { public: /// Construct an axis object with variable bins. /// - /// \param[in] binEdges the (ordered) edges of the normal bins + /// \param[in] binEdges the (ordered) edges of the normal bins, must define at least one bin (i.e. size >= 2) /// \param[in] enableFlowBins whether to enable underflow and overflow bins RVariableBinAxis(std::vector binEdges, bool enableFlowBins = true) : fBinEdges(std::move(binEdges)), fEnableFlowBins(enableFlowBins) { - // FIXME: should validate that fBinEdges is sorted + if (fBinEdges.size() < 2) { + throw std::invalid_argument("must have >= 2 bin edges"); + } + for (std::size_t i = 1; i < fBinEdges.size(); i++) { + if (fBinEdges[i - 1] >= fBinEdges[i]) { + std::string msg = "binEdges must be sorted, but for bin " + std::to_string(i - 1) + ": "; + msg += std::to_string(fBinEdges[i - 1]) + " >= " + std::to_string(fBinEdges[i]); + throw std::invalid_argument(msg); + } + } } std::size_t GetNumNormalBins() const { return fBinEdges.size() - 1; } diff --git a/hist/histv7/test/hist_regular.cxx b/hist/histv7/test/hist_regular.cxx index fd511de869d3e..e3ce3f5fea2f4 100644 --- a/hist/histv7/test/hist_regular.cxx +++ b/hist/histv7/test/hist_regular.cxx @@ -16,6 +16,9 @@ TEST(RRegularAxis, Constructor) EXPECT_EQ(axis.GetNumNormalBins(), Bins); EXPECT_EQ(axis.GetTotalNumBins(), Bins); EXPECT_FALSE(axis.HasFlowBins()); + + EXPECT_THROW(RRegularAxis(0, 0, Bins), std::invalid_argument); + EXPECT_THROW(RRegularAxis(Bins, 1, 1), std::invalid_argument); } TEST(RRegularAxis, Equality) diff --git a/hist/histv7/test/hist_variable.cxx b/hist/histv7/test/hist_variable.cxx index 6cda6af224ecd..1763710591a4a 100644 --- a/hist/histv7/test/hist_variable.cxx +++ b/hist/histv7/test/hist_variable.cxx @@ -21,6 +21,12 @@ TEST(RVariableBinAxis, Constructor) EXPECT_EQ(axis.GetNumNormalBins(), Bins); EXPECT_EQ(axis.GetTotalNumBins(), Bins); EXPECT_FALSE(axis.HasFlowBins()); + + EXPECT_THROW(RVariableBinAxis({}), std::invalid_argument); + EXPECT_THROW(RVariableBinAxis({0}), std::invalid_argument); + EXPECT_THROW(RVariableBinAxis({0, 0}), std::invalid_argument); + EXPECT_THROW(RVariableBinAxis({0, 1, 0}), std::invalid_argument); + EXPECT_THROW(RVariableBinAxis({0, 1, 1}), std::invalid_argument); } TEST(RVariableBinAxis, Equality)