From 53100b7bb3431fb314689adce6cb1dcdde7f4a98 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 20 Jul 2026 14:28:23 +0200 Subject: [PATCH] [RF] Fix implementation of RooGenericPdf/RooFormulaVar::binBoundaries() Follows up on 9ea4ec66745, which got the implementation of `RooFormulaVar::binBoundaries()` and `RooGenericPdf::binBoundaries()` wrong. The limis passed to the `binBoundaries()` call should not be included in the returned `std::list`. I was led to believe that including the query boundaries was necessary, because otherwise the numeric integrals didn't work, but I see now that this is actually a bug on the RooBinIntegrator side, which is now reported as https://github.com/root-project/root/issues/22858. Also, implement a unit test that ensures the fixed implementation of `binBoundaries()` is consistent with the one of RooHistPdf and matches the expected values. Finally, move a remaining helper that was added in 9ea4ec66745 from the public to the private interface (from RooHelpers.h to RooFitImplHelpers.h, which never gets installed). (cherry picked from commit 28b3942d15730145dc3f3bd1571f8d4a85299a37) --- roofit/roofitcore/inc/RooHelpers.h | 20 ------- roofit/roofitcore/res/RooFitImplHelpers.h | 9 ++++ roofit/roofitcore/src/RooFitImplHelpers.cxx | 36 +++++++++++++ roofit/roofitcore/src/RooFormulaVar.cxx | 12 +++-- roofit/roofitcore/src/RooGenericPdf.cxx | 12 +++-- roofit/roofitcore/src/RooHelpers.cxx | 58 --------------------- roofit/roofitcore/test/testGenericPdf.cxx | 54 +++++++++++++++++-- 7 files changed, 113 insertions(+), 88 deletions(-) diff --git a/roofit/roofitcore/inc/RooHelpers.h b/roofit/roofitcore/inc/RooHelpers.h index 75362c204beb6..cb1ebd71c5a11 100644 --- a/roofit/roofitcore/inc/RooHelpers.h +++ b/roofit/roofitcore/inc/RooHelpers.h @@ -17,17 +17,13 @@ #include #include -#include - #include -#include #include #include #include class RooAbsPdf; class RooAbsData; -class RooAbsRealLValue; namespace RooHelpers { @@ -95,22 +91,6 @@ void checkRangeOfParameters(const RooAbsReal *callingClass, std::initializer_lis /// set all RooRealVars to constants. return true if at least one changed status bool setAllConstant(const RooAbsCollection &coll, bool constant = true); -/// Check that `function` is constant (flat) inside each bin defined by the -/// sorted `boundaries` when scanning the observable `obs`. Several interior -/// points are sampled per bin and compared to the bin's first sample; if any -/// of them deviates by more than `relTol` (relative to the value scale), the -/// function is not flat and false is returned. The value of `obs` is restored -/// on return. -bool isFunctionFlatInBins(const RooAbsReal &function, RooAbsRealLValue &obs, std::span boundaries, - double relTol = 1e-9); - -/// Return a newly allocated list with the subset of `boundaries` that lies -/// strictly inside [`xlo`, `xhi`], with `xlo` and `xhi` added as the first and -/// last entries. This is the form expected by RooFit's binBoundaries() -/// interface, so the bin integrator covers exactly the integration range. -/// The caller takes ownership of the returned list. -std::list *binBoundariesInRange(std::span boundaries, double xlo, double xhi); - } // namespace RooHelpers #endif diff --git a/roofit/roofitcore/res/RooFitImplHelpers.h b/roofit/roofitcore/res/RooFitImplHelpers.h index d14bc905cd21e..371327bd0d789 100644 --- a/roofit/roofitcore/res/RooFitImplHelpers.h +++ b/roofit/roofitcore/res/RooFitImplHelpers.h @@ -127,6 +127,15 @@ BinnedLOutput getBinnedL(RooAbsPdf const &pdf); void getSortedComputationGraph(RooAbsArg const &func, RooArgSet &out); +/// Check that `function` is constant (flat) inside each bin defined by the +/// sorted `boundaries` when scanning the observable `obs`. Several interior +/// points are sampled per bin and compared to the bin's first sample; if any +/// of them deviates by more than `relTol` (relative to the value scale), the +/// function is not flat and false is returned. The value of `obs` is restored +/// on return. +bool isFunctionFlatInBins(const RooAbsReal &function, RooAbsRealLValue &obs, std::span boundaries, + double relTol = 1e-9); + } // namespace RooHelpers namespace RooFit::Detail { diff --git a/roofit/roofitcore/src/RooFitImplHelpers.cxx b/roofit/roofitcore/src/RooFitImplHelpers.cxx index 9fb2e20bffcf8..c85f6296604b3 100644 --- a/roofit/roofitcore/src/RooFitImplHelpers.cxx +++ b/roofit/roofitcore/src/RooFitImplHelpers.cxx @@ -286,6 +286,42 @@ RooAbsArg *cloneTreeWithSameParametersImpl(RooAbsArg const &arg, RooArgSet const } // namespace Detail +bool isFunctionFlatInBins(const RooAbsReal &function, RooAbsRealLValue &obs, std::span boundaries, + double relTol) +{ + // Fractions of the bin width at which the function is sampled. They are kept + // strictly inside the bin (away from the boundaries) so that the evaluation + // is not affected by which side of a boundary a step function jumps. + const double fractions[] = {0.04, 0.27, 0.5, 0.73, 0.96}; + + const double savedVal = obs.getVal(); + + bool isFlat = true; + for (std::size_t i = 0; i + 1 < boundaries.size() && isFlat; ++i) { + const double lo = boundaries[i]; + const double hi = boundaries[i + 1]; + double reference = 0.0; + bool first = true; + for (double frac : fractions) { + obs.setVal(lo + frac * (hi - lo)); + const double val = function.getVal(); + if (first) { + reference = val; + first = false; + continue; + } + const double scale = std::max(std::abs(reference), 1e-12); + if (std::abs(val - reference) > relTol * scale) { + isFlat = false; + break; + } + } + } + + obs.setVal(savedVal); + return isFlat; +} + } // namespace RooHelpers namespace RooFit::Detail { diff --git a/roofit/roofitcore/src/RooFormulaVar.cxx b/roofit/roofitcore/src/RooFormulaVar.cxx index b1e19db40f145..394ef9b112dd6 100644 --- a/roofit/roofitcore/src/RooFormulaVar.cxx +++ b/roofit/roofitcore/src/RooFormulaVar.cxx @@ -55,7 +55,7 @@ #include "RooAbsRealLValue.h" #include "RooAbsBinning.h" #include "RooCurve.h" -#include "RooHelpers.h" +#include "RooFitImplHelpers.h" #ifdef ROOFIT_LEGACY_EVAL_BACKEND #include "RooNLLVar.h" @@ -319,8 +319,14 @@ std::list* RooFormulaVar::binBoundaries(RooAbsRealLValue& obs, double xl auto found = _binnings.find(_actualVars.index(obs.GetName())); if (found != _binnings.end()) { const RooAbsBinning &binning = *found->second; - return RooHelpers::binBoundariesInRange({binning.array(), static_cast(binning.numBoundaries())}, xlo, - xhi); + auto hint = new std::list; + for (int i = 0; i < binning.numBoundaries(); ++i) { + const double boundary = binning.array()[i]; + if (boundary >= xlo && boundary <= xhi) { + hint->push_back(boundary); + } + } + return hint; } for (const auto par : _actualVars) { diff --git a/roofit/roofitcore/src/RooGenericPdf.cxx b/roofit/roofitcore/src/RooGenericPdf.cxx index 874054865dfab..dea4df85a4664 100644 --- a/roofit/roofitcore/src/RooGenericPdf.cxx +++ b/roofit/roofitcore/src/RooGenericPdf.cxx @@ -51,7 +51,7 @@ the names of the arguments are not hard coded. #include "RooAbsRealLValue.h" #include "RooAbsBinning.h" #include "RooCurve.h" -#include "RooHelpers.h" +#include "RooFitImplHelpers.h" using std::istream, std::ostream, std::endl; @@ -223,8 +223,14 @@ std::list *RooGenericPdf::binBoundaries(RooAbsRealLValue &obs, double xl return nullptr; } const RooAbsBinning &binning = *found->second; - return RooHelpers::binBoundariesInRange({binning.array(), static_cast(binning.numBoundaries())}, xlo, - xhi); + auto hint = new std::list; + for (int i = 0; i < binning.numBoundaries(); ++i) { + const double boundary = binning.array()[i]; + if (boundary >= xlo && boundary <= xhi) { + hint->push_back(boundary); + } + } + return hint; } //////////////////////////////////////////////////////////////////////////////// diff --git a/roofit/roofitcore/src/RooHelpers.cxx b/roofit/roofitcore/src/RooHelpers.cxx index 766170309ca4b..322a451edc137 100644 --- a/roofit/roofitcore/src/RooHelpers.cxx +++ b/roofit/roofitcore/src/RooHelpers.cxx @@ -29,8 +29,6 @@ #include #include -#include -#include #include namespace RooHelpers { @@ -170,60 +168,4 @@ bool setAllConstant(const RooAbsCollection &coll, bool constant) return changed; } -bool isFunctionFlatInBins(const RooAbsReal &function, RooAbsRealLValue &obs, std::span boundaries, - double relTol) -{ - // Fractions of the bin width at which the function is sampled. They are kept - // strictly inside the bin (away from the boundaries) so that the evaluation - // is not affected by which side of a boundary a step function jumps. - const double fractions[] = {0.04, 0.27, 0.5, 0.73, 0.96}; - - const double savedVal = obs.getVal(); - - bool isFlat = true; - for (std::size_t i = 0; i + 1 < boundaries.size() && isFlat; ++i) { - const double lo = boundaries[i]; - const double hi = boundaries[i + 1]; - double reference = 0.0; - bool first = true; - for (double frac : fractions) { - obs.setVal(lo + frac * (hi - lo)); - const double val = function.getVal(); - if (first) { - reference = val; - first = false; - continue; - } - const double scale = std::max(std::abs(reference), 1e-12); - if (std::abs(val - reference) > relTol * scale) { - isFlat = false; - break; - } - } - } - - obs.setVal(savedVal); - return isFlat; -} - -std::list *binBoundariesInRange(std::span boundaries, double xlo, double xhi) -{ - auto out = new std::list; - - // Small tolerance so that boundaries numerically coinciding with the range - // limits are not duplicated by the explicit xlo/xhi endpoints below. - const double delta = (xhi - xlo) * 1e-8; - - for (double boundary : boundaries) { - if (boundary > xlo + delta && boundary < xhi - delta) { - out->push_back(boundary); - } - } - - out->push_front(xlo); - out->push_back(xhi); - - return out; -} - } // namespace RooHelpers diff --git a/roofit/roofitcore/test/testGenericPdf.cxx b/roofit/roofitcore/test/testGenericPdf.cxx index 1a3ce3275932e..592e81bb73905 100644 --- a/roofit/roofitcore/test/testGenericPdf.cxx +++ b/roofit/roofitcore/test/testGenericPdf.cxx @@ -2,13 +2,15 @@ // Authors: Stephan Hageboeck, CERN 05/2019 // Jonas Rembser, CERN 06/2022 -#include -#include -#include #include #include -#include +#include +#include #include +#include +#include +#include +#include #include #include @@ -340,3 +342,47 @@ TEST(GenericPdf, BinnedBoundariesSurviveRename) EXPECT_NE(integratorName.find("RooBinIntegrator"), std::string::npos) << integratorName; EXPECT_DOUBLE_EQ(value, piecewiseFlatIntegral); } + +// Ensure the implementation of RooGenericPdf::binBoundaries() (and +// RooFormulaVar::binBoundaries()) is consistent with equivalent RooHistPdf. +TEST(GenericPdf, BinnedBoundariesConsistentWithHistPdf) +{ + int nBins = 5; + + RooRealVar x{"x", "x", 0, 0, 5}; + x.setBins(nBins); + + RooDataHist dh{"dh", "", x}; + + RooHistPdf hpdf{"hpdf", "", x, dh}; + RooGenericPdf gpdf{"gpdf", "x[0] - x[0] + 1", {x}}; // uniform dummy + gpdf.setBinning(x, x.getBinning()); + + RooFormulaVar fvar{"fvar", "x[0] - x[0] + 1", {x}}; // uniform dummy + fvar.setBinning(x, x.getBinning()); + + // intentionally beyond the bin boundaries + double lo = -10; + double hi = 10; + + std::unique_ptr> boundsHistPdf{hpdf.binBoundaries(x, lo, hi)}; + std::unique_ptr> boundsGenericPdf{gpdf.binBoundaries(x, lo, hi)}; + std::unique_ptr> boundsFormulaVar{fvar.binBoundaries(x, lo, hi)}; + + EXPECT_EQ(boundsHistPdf->size(), nBins + 1); + EXPECT_EQ(boundsGenericPdf->size(), nBins + 1); + EXPECT_EQ(boundsFormulaVar->size(), nBins + 1); + + auto iterHistPdf = boundsHistPdf->begin(); + auto iterGenericPdf = boundsGenericPdf->begin(); + auto iterFormulaVar = boundsFormulaVar->begin(); + + for (int i = 0; i < nBins + 1; ++i) { + EXPECT_EQ(*iterHistPdf, static_cast(i)); + EXPECT_EQ(*iterGenericPdf, static_cast(i)); + EXPECT_EQ(*iterFormulaVar, static_cast(i)); + iterHistPdf++; + iterGenericPdf++; + iterFormulaVar++; + } +}