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)