diff --git a/roofit/roofitcore/src/RooBinIntegrator.cxx b/roofit/roofitcore/src/RooBinIntegrator.cxx index 8bf0e35d61333..1a86b2899389c 100644 --- a/roofit/roofitcore/src/RooBinIntegrator.cxx +++ b/roofit/roofitcore/src/RooBinIntegrator.cxx @@ -94,7 +94,19 @@ RooBinIntegrator::RooBinIntegrator(const RooAbsFunc &function, int numBins) tmp->push_back(_xmin[i]+j*(_xmax[i]-_xmin[i])/_numBins) ; } } - _binb.emplace_back(tmp->begin(), tmp->end()); + std::vector binb{tmp->begin(), tmp->end()}; + + // The bin boundaries provided by the integrand only include the boundaries + // that lie strictly inside the integration range. To integrate correctly + // over partial bins at the edges of the range, the integration limits need + // to be added as the outermost boundaries (if not already present). + if (binb.empty() || binb.front() > _xmin[i]) { + binb.insert(binb.begin(), _xmin[i]); + } + if (binb.back() < _xmax[i]) { + binb.push_back(_xmax[i]); + } + _binb.emplace_back(std::move(binb)); } checkLimits(); diff --git a/roofit/roofitcore/test/testRooRealIntegral.cxx b/roofit/roofitcore/test/testRooRealIntegral.cxx index b4119a31415b5..abcd675c23ce8 100644 --- a/roofit/roofitcore/test/testRooRealIntegral.cxx +++ b/roofit/roofitcore/test/testRooRealIntegral.cxx @@ -389,6 +389,99 @@ TEST(RooRealIntegral, RooLinearVarModelIntegratedOverVariableClones) EXPECT_TRUE(static_cast(*integral2).numIntRealVars().empty()); } +// Make sure the RooBinIntegrator gives correct results when the integration +// range boundaries lie inside the bins of the integrand, i.e. the range cuts +// through bins. Previously, the RooBinIntegrator only summed the bins that were +// fully contained in the range and dropped the partial bins at the edges, +// because the integration limits were not added as the outermost bin +// boundaries. Covers GitHub issue #22858. +TEST(RooRealIntegral, RooBinIntegratorPartialRange) +{ + using namespace RooFit; + + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooRealVar x{"x", "x", 0., 10.}; + x.setBins(5); // five bins of width 2: edges at 0, 2, 4, 6, 8, 10 + + const double heights[5] = {1.0, 3.0, 2.0, 4.0, 1.5}; + + RooDataHist dh{"dh", "", x}; + for (int i = 0; i < 5; ++i) { + dh.set(i, heights[i], 0.0); + } + + // We compare the analytic RooHistPdf integral (which is known to be correct) + // against the numeric one that uses the RooBinIntegrator, for various ranges + // that cut through the bins in different ways. + auto checkRange = [&](const char *name, double lo, double hi) { + x.setRange(name, lo, hi); + + RooHistPdf analytic{"analytic", "", x, dh, 0}; + RooHistPdf numeric{"numeric", "", x, dh, 0}; + numeric.forceNumInt(true); + + std::unique_ptr integAna{analytic.createIntegral(x, Range(name))}; + std::unique_ptr integNum{numeric.createIntegral(x, Range(name))}; + + EXPECT_NEAR(integNum->getVal(), integAna->getVal(), 1e-12) + << "RooBinIntegrator disagrees with the analytic integral for range [" << lo << ", " << hi << "]"; + }; + + checkRange("full", 0.0, 10.0); // full range + checkRange("partial", 3.0, 7.0); // both boundaries cut through bins (the original report) + checkRange("aligned", 2.0, 8.0); // boundaries coincide with bin edges + checkRange("singlebin", 3.0, 3.5); // range entirely inside a single bin + checkRange("leftpart", 3.0, 10.0); // only the lower boundary cuts through a bin + checkRange("rightpart", 0.0, 7.0); // only the upper boundary cuts through a bin + + // Explicitly check the value from the original bug report as well. + x.setRange("report", 3.0, 7.0); + RooHistPdf numeric{"numeric", "", x, dh, 0}; + numeric.forceNumInt(true); + std::unique_ptr integNum{numeric.createIntegral(x, Range("report"))}; + const double binWidth = 2.0; + const double expected = (heights[1] * 1.0 + heights[2] * 2.0 + heights[3] * 1.0) / binWidth; // = 5.5 + EXPECT_NEAR(integNum->getVal(), expected, 1e-12); +} + +// Like RooBinIntegratorPartialRange, but for a two-dimensional integrand to +// make sure the fix for GitHub issue #22858 also holds for the N-dimensional +// recursive integration path, where the range cuts through bins in every +// dimension. This is the case most likely to regress silently. +TEST(RooRealIntegral, RooBinIntegratorPartialRange2D) +{ + using namespace RooFit; + + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooRealVar x{"x", "x", 0., 10.}; + RooRealVar y{"y", "y", 0., 6.}; + x.setBins(5); // edges at 0, 2, 4, 6, 8, 10 + y.setBins(3); // edges at 0, 2, 4, 6 + + RooDataHist dh{"dh", "", {x, y}}; + // Fill all 5x3 bins with distinct, non-trivial heights. + for (int i = 0; i < dh.numEntries(); ++i) { + dh.get(i); + dh.set(i, 1.0 + 0.5 * i, 0.0); + } + + // The sub-ranges cut through the bins in both dimensions. + x.setRange("sub", 3.0, 7.0); // cuts the x bins [2,4] and [6,8] + y.setRange("sub", 1.0, 5.0); // cuts the y bins [0,2] and [4,6] + + RooHistPdf analytic{"analytic", "", {x, y}, dh, 0}; + RooHistPdf numeric{"numeric", "", {x, y}, dh, 0}; + numeric.forceNumInt(true); + + std::unique_ptr integAna{analytic.createIntegral({x, y}, Range("sub"))}; + std::unique_ptr integNum{numeric.createIntegral({x, y}, Range("sub"))}; + + EXPECT_NEAR(integNum->getVal(), integAna->getVal(), 1e-12) + << "RooBinIntegrator disagrees with the analytic integral for the 2D sub-range"; +} + // Make sure that RooFit realizes that Gaussian(x, mu, sigma(x)) needs to be // integrated analytically. // Covers GitHub issue #14320.