From 4ed9b487fa31cb550390671c75d729c5dddf3ded Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 3 Aug 2026 17:03:12 +0000 Subject: [PATCH] [RF] Throw when dataset has out-of-range values for the CPU eval backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If an observable's range is shrunk after a dataset was already filled, the dataset's internal clone of the observable keeps the old, wider range metadata, decoupled from the live variable used by the pdf. The new vectorizing evaluation backends (EvalBackend::Cpu and friends) would then silently evaluate/normalize the likelihood over the shrunken range while still including the now out-of-range data points, biasing the fit without any warning. Add a check in RooEvaluatorWrapper::setData() that scans the data spans for each observable of the pdf (handling RooSimultaneous by looking at the "__obs__"-tagged, per-channel-prefixed clones) and throws a descriptive std::runtime_error if any value falls outside of the observable's current range. The check is skipped when a named range is explicitly passed to the fit, since out-of-range events are then intentionally and consistently dropped by RooFit::BatchModeDataHelpers::getDataSpans(). The error message points users at the correct fix: define a named range with setRange() and pass it via RooFit::Range() instead of shrinking the observable itself. Fixes #22740 🤖 Done with the help of AI. --- roofit/roofitcore/src/RooEvaluatorWrapper.cxx | 88 +++++++++++++++++++ roofit/roofitcore/test/testRooAbsPdf.cxx | 59 +++++++++++++ 2 files changed, 147 insertions(+) diff --git a/roofit/roofitcore/src/RooEvaluatorWrapper.cxx b/roofit/roofitcore/src/RooEvaluatorWrapper.cxx index 5773de05da7ee..c7446f9985270 100644 --- a/roofit/roofitcore/src/RooEvaluatorWrapper.cxx +++ b/roofit/roofitcore/src/RooEvaluatorWrapper.cxx @@ -36,6 +36,91 @@ Wraps a RooFit::Evaluator that evaluates a RooAbsReal back into a RooAbsReal. #include +namespace { + +// Throws an exception if any value in `span` is outside of the (default, +// unnamed) range of `var`. The `obsName` is used only for the error message, +// because it might differ from `var.GetName()` (e.g. the per-channel prefix +// that RooSimultaneous adds is stripped for readability). +void checkObservableSpanInRange(RooRealVar const &var, std::string const &obsName, std::string const &datasetName, + std::span span) +{ + for (double val : span) { + if (!var.inRange(val, nullptr)) { + const double lo = var.getMin(); + const double hi = var.getMax(); + std::stringstream errMsg; + errMsg << "RooAbsPdf::fitTo/createNLL: cannot evaluate the likelihood because dataset \"" << datasetName + << "\" has an entry for observable \"" << obsName << "\" with value " << val + << ", which is outside of its range [" << lo << ", " << hi << "]. The probability density is " + << "normalized over exactly that range, so events outside of it would silently bias the fit. If " + << "you want to fit only a subset of the data, define a named range and use it in the fit, for example:\n" + << " " << obsName << ".setRange(\"fitRange\", " << lo << ", " << hi << ");\n" + << " pdf.fitTo(data, RooFit::Range(\"fitRange\"));\n" + << "This way, only the events inside \"fitRange\" enter the likelihood, consistent with how the " + << "pdf is normalized."; + oocoutE(nullptr, InputArguments) << errMsg.str() << std::endl; + throw std::runtime_error(errMsg.str()); + } + } +} + +// Validates that no dataset entry lies outside of the range of the +// corresponding observable, for every real-valued observable of `pdf`. This +// check is skipped when a range name was explicitly given to the fit, +// because in that case out-of-range events are intentionally and +// consistently dropped by RooFit::BatchModeDataHelpers::getDataSpans(). +void validateObservableRanges(RooAbsPdf const *pdf, RooAbsData const &data, + std::map> const &dataSpans) +{ + if (!pdf) + return; + + if (auto const *simPdf = dynamic_cast(pdf)) { + // The per-channel pdfs coming out of RooSimultaneous::compileForNormSet() + // have their observables cloned and renamed with a "__" prefix + // (and tagged with the "__obs__" attribute), so that the shared data map + // can hold independent columns for each channel. We look those up the + // same way, and strip the prefix again for a readable error message. + for (auto const &nameIdx : simPdf->indexCat()) { + RooAbsPdf *channelPdf = simPdf->getPdf(nameIdx.first); + if (!channelPdf) + continue; + const std::string prefix = "_" + nameIdx.first + "_"; + std::unique_ptr vars{channelPdf->getVariables()}; + std::unique_ptr obs{vars->selectByAttrib("__obs__", true)}; + for (RooAbsArg *arg : *obs) { + auto *realVar = dynamic_cast(arg); + if (!realVar) + continue; + auto it = dataSpans.find(RooFit::Detail::DataKey(realVar)); + if (it == dataSpans.end()) + continue; + std::string obsName = realVar->GetName(); + if (obsName.rfind(prefix, 0) == 0) { + obsName = obsName.substr(prefix.size()); + } + checkObservableSpanInRange(*realVar, obsName, data.GetName(), it->second); + } + } + return; + } + + RooArgSet obs; + pdf->getObservables(data.get(), obs); + for (RooAbsArg *arg : obs) { + auto *realVar = dynamic_cast(arg); + if (!realVar) + continue; + auto it = dataSpans.find(RooFit::Detail::DataKey(realVar)); + if (it == dataSpans.end()) + continue; + checkObservableSpanInRange(*realVar, realVar->GetName(), data.GetName(), it->second); + } +} + +} // namespace + namespace RooFit::Experimental { RooEvaluatorWrapper::RooEvaluatorWrapper(RooAbsReal &topNode, RooAbsData *data, bool useGPU, @@ -669,6 +754,9 @@ bool RooEvaluatorWrapper::setData(RooAbsData &data, bool /*cloneData*/) auto simPdf = dynamic_cast(_pdf); _dataSpans = RooFit::BatchModeDataHelpers::getDataSpans(*_data, _rangeName, simPdf, skipZeroWeights, _takeGlobalObservablesFromData, _vectorBuffers); + if (_rangeName.empty()) { + validateObservableRanges(_pdf, *_data, _dataSpans); + } if (!isInitializing && _dataSpans.size() != oldSize) { coutE(DataHandling) << errMsg << std::endl; throw std::runtime_error(errMsg); diff --git a/roofit/roofitcore/test/testRooAbsPdf.cxx b/roofit/roofitcore/test/testRooAbsPdf.cxx index d217dbb23bf9d..f13d2afa9e316 100644 --- a/roofit/roofitcore/test/testRooAbsPdf.cxx +++ b/roofit/roofitcore/test/testRooAbsPdf.cxx @@ -437,6 +437,65 @@ TEST_P(FitTest, PdfAsFunctionInFormulaVar) EXPECT_FLOAT_EQ(nll->getVal(), ref); } +// If an observable's range is shrunk after a dataset was already filled, the +// dataset still contains entries that are now outside of the range over which +// the pdf is normalized. Evaluating the likelihood anyway would silently bias +// the fit. The vectorizing evaluation backends must detect this and throw a +// descriptive error, while the correct workaround (a named range) keeps +// working for all backends. Covers GitHub issue #22740. +TEST_P(FitTest, OutOfRangeDataThrows) +{ + using namespace RooFit; + + RooWorkspace ws; + ws.factory("Gaussian::gauss(x[0, 5], mean[0.5, -10, 10], sigma[1.0, 0.1, 10.0])"); + + auto &x = *ws.var("x"); + auto &mean = *ws.var("mean"); + RooAbsPdf &gauss = *ws.pdf("gauss"); + + // Fill a dataset with entries at 1, 2 and 3 while the range is still [0, 5]. + RooDataSet data{"data", "data", x}; + for (double val : {1.0, 2.0, 3.0}) { + x.setVal(val); + for (int i = 0; i < 50; ++i) { + data.add(x); + } + } + + // Shrink the range so that the entries at 3 are now out of range, while the + // dataset's internal clone of the observable still remembers [0, 5]. + x.setMax(2.5); + + const bool isLegacy = _evalBackend == EvalBackend::Legacy(); + + { + // Normalizing over [0, 2.5] while still evaluating the entries at 3 would + // bias the fit, so the vectorizing backends throw. The legacy backend is + // not affected by this check and keeps its historical behavior. + RooHelpers::HijackMessageStream hijack(RooFit::ERROR, RooFit::InputArguments); + auto doFit = [&]() { + std::unique_ptr{gauss.fitTo(data, _evalBackend, Save(), PrintLevel(-1))}; + }; + if (isLegacy) { + EXPECT_NO_THROW(doFit()); + } else { + EXPECT_THROW(doFit(), std::runtime_error); + } + } + + // Restricting the fit with a named range is the correct approach: the + // out-of-range entries at 3 are dropped consistently for both normalization + // and evaluation. This must work for every backend and recover a mean close + // to the mean of the clipped data (50 entries at 1 and 50 at 2). + mean.setVal(0.5); + x.setRange("fitRange", 0, 2.5); + std::unique_ptr result{gauss.fitTo(data, _evalBackend, Range("fitRange"), Save(), PrintLevel(-1))}; + ASSERT_NE(result, nullptr); + EXPECT_EQ(result->status(), 0); + EXPECT_NEAR(mean.getVal(), 1.5, 0.2); +} + // Verifies that a server pdf gets correctly reevaluated when the normalization // set is changed. TEST(RooAbsPdf, NormSetChange)