[RF] Don't normalize over non-dependents when compiling RooFormulaVar

Don't normalize pdfs over non-dependents in
`RooFormulaVar::compileForNormSet()`.

This fixes use cases where pdfs that don't depend on any observables are
used for their unnormalized `RooAbsPdf::evaluate()` shape as functions.

Even thought technically not allowed because evaluating pdfs without a
normalization set is undefined, this pattern is used a lot in the wild,
so we need to support it also in the new vectorizing evaluation backend.

A unit test that covers the original user-reported problem is also
added.

Author: guitargeek

roofit/roofitcore/inc/RooFormulaVar.h

@@ -77,6 +77,9 @@ class RooFormulaVar : public RooAbsReal {
 
   std::string getUniqueFuncName() const;
 
+  std::unique_ptr<RooAbsArg>
+  compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileContext &ctx) const override;
+
   protected:
   // Post-processing of server redirection
   bool redirectServersHook(const RooAbsCollection& newServerList, bool mustReplaceAll, bool nameChange, bool isRecursive) override ;

roofit/roofitcore/src/RooFormulaVar.cxx

@@ -316,3 +316,22 @@ std::string RooFormulaVar::getUniqueFuncName() const
 {
    return getFormula().getTFormula()->GetUniqueFuncName().Data();
 }
+
+std::unique_ptr<RooAbsArg>
+RooFormulaVar::compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileContext &ctx) const
+{
+   // Some users exploit unnormalized RooAbsPdfs as inputs for RooFormulaVars,
+   // relying on what the pdf returns from RooAbsPdf::evaluate(). This is in
+   // principle not allowed because every pdf needs to be evaluated with a
+   // normalization set, but it's so common in user code that we need to
+   // support it. To make this work, we need to make sure that the no
+   // normalization over non-dependents is happening at this point, reducing
+   // the normalization set to the subset of actual dependents.
+   // See also the "PdfAsFunctionInFormulaVar" test in testRooAbsPdf.
+   RooArgSet depList;
+   getObservables(&normSet, depList);
+   auto newArg = std::unique_ptr<RooAbsArg>{static_cast<RooAbsArg *>(Clone())};
+   ctx.markAsCompiled(*newArg);
+   ctx.compileServers(*newArg, depList);
+   return newArg;
+}

roofit/roofitcore/test/CMakeLists.txt

@@ -18,7 +18,7 @@ ROOT_ADD_GTEST(testRooDataHist testRooDataHist.cxx LIBRARIES RooFitCore
 ROOT_ADD_GTEST(testRooBinSamplingPdf testRooBinSamplingPdf.cxx LIBRARIES RooFitCore)
 ROOT_ADD_GTEST(testRooWrapperPdf testRooWrapperPdf.cxx LIBRARIES Gpad RooFitCore)
 ROOT_ADD_GTEST(testGenericPdf testGenericPdf.cxx LIBRARIES RooFitCore)
-ROOT_ADD_GTEST(testRooAbsPdf testRooAbsPdf.cxx LIBRARIES RooFitCore)
+ROOT_ADD_GTEST(testRooAbsPdf testRooAbsPdf.cxx LIBRARIES RooFitCore RooFit)
 ROOT_ADD_GTEST(testRooAbsCollection testRooAbsCollection.cxx LIBRARIES RooFitCore)
 
 ROOT_ADD_GTEST(testRooDataSet testRooDataSet.cxx LIBRARIES Tree RooFitCore

roofit/roofitcore/test/testRooAbsPdf.cxx

@@ -2,23 +2,26 @@
 // Authors: Stephan Hageboeck, CERN 04/2020
 //          Jonas Rembser, CERN 04/2021
 
-#include <RooAddition.h>
 #include <RooAddPdf.h>
+#include <RooAddition.h>
 #include <RooCategory.h>
 #include <RooConstVar.h>
 #include <RooDataHist.h>
 #include <RooDataSet.h>
 #include <RooFitResult.h>
 #include <RooFormulaVar.h>
+#include <RooGaussian.h>
 #include <RooGenericPdf.h>
 #include <RooHelpers.h>
+#include <RooParametricStepFunction.h>
 #include <RooProdPdf.h>
 #include <RooProduct.h>
+#include <RooRandom.h>
 #include <RooRealVar.h>
 #include <RooSimultaneous.h>
 #include <RooWorkspace.h>
-#include <RooRandom.h>
 
+#include <TArrayD.h>
 #include <TClass.h>
 #include <TRandom.h>
 
@@ -375,6 +378,42 @@ TEST_P(FitTest, ProblemsWith2DSimultaneousFit)
    simPdf.fitTo(*data, PrintLevel(-1), _evalBackend);
 }
 
+// This test covers a usecase by an ATLAS collaborator. The unnormalized shape
+// of a RooFit pdf is used as a function inside a RooFormulaVar, which is used
+// for the bins of a RooParametricStep function. This case is potentially
+// fragile, because it requires that the top-level normalization set is ignored
+// for the inner pdfs that don't depend on the observable, as normalizing over
+// a non-dependent is a corner case where the variable should be dropped.
+TEST_P(FitTest, PdfAsFunctionInFormulaVar)
+{
+   using namespace RooFit;
+
+   RooRealVar x{"x", "x", 0., 1.};
+
+   const double arg = std::sqrt(-8 * std::log(0.5));
+   RooGaussian gauss1{"gauss1", "", 10 - arg, 10, 2};
+
+   RooFormulaVar func1{"func1", "x[0]", {gauss1}};
+
+   // The parametric step function doesn't make any attempt at self
+   // normalization, so the pdf value in the first bin is just the value of the
+   // unnormalized Gaussian. And the first bin is the almost the whole domain.
+   TArrayD limits(3);
+   limits[0] = 0.;
+   limits[1] = 1 - 1e-9;
+   limits[2] = 1.;
+   RooParametricStepFunction pdf("pdf", "pdf", x, {func1}, limits, limits.size() - 1);
+
+   int nEvents = 10000;
+   std::unique_ptr<RooAbsData> data{pdf.generateBinned(x, nEvents)};
+
+   std::unique_ptr<RooAbsReal> nll{pdf.createNLL(*data, _evalBackend)};
+
+   // The test is designed to have an analytical reference value
+   double ref = nEvents * -std::log(0.5);
+   EXPECT_FLOAT_EQ(nll->getVal(), ref);
+}
+
 // Verifies that a server pdf gets correctly reevaluated when the normalization
 // set is changed.
 TEST(RooAbsPdf, NormSetChange)