From 6d079f25549a2815befc019d198a7295f9b099b5 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 15 Oct 2026 09:09:58 +0000 Subject: [PATCH] [RF] Keep RooRealSumPdf integral code out of the codegen event loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The integral of a RooRealSumPdf reuses the translation of the sum itself, which placed its generated code via addToCodeBody(&arg, ...). Since the sum depends on the observables, the integral code ended up inside the event loop, even though the integral itself is data-independent. For an extended RooRealSumPdf, the same integral is the pdf normalization inside the loop and the expected number of events that the extended term reads after the loop. Because codegen caches results by node name, the post-loop term referenced a loop-local variable and cling refused to compile the generated function: error: use of undeclared identifier 't14' RooNLLVarNewResult += t14 - RooNLLVarNewWeightSum * std::log(t14); Decide the scope of the generated code from the inputs of the sum instead of from the pdf node: the code is scope-independent if all functions (or component integrals) and coefficients are. For the pdf itself this is equivalent to the previous check, because the sum depends on the data exactly when one of its inputs does. For the integral it keeps the code outside the loop unless a coefficient depends on the data. This needs CodegenContext::isScopeIndependent() to be public. Add an extended RooRealSumPdf case to testRooFuncWrapper that fails without the fix. 🤖 Done with the help of AI --- roofit/codegen/src/CodegenImpl.cxx | 31 ++++++++++++++----- roofit/roofitcore/inc/RooFit/CodegenContext.h | 4 +-- roofit/roofitcore/test/testRooFuncWrapper.cxx | 9 ++++++ 3 files changed, 34 insertions(+), 10 deletions(-) diff --git a/roofit/codegen/src/CodegenImpl.cxx b/roofit/codegen/src/CodegenImpl.cxx index c0186c5e9bc0c..3199a5311159a 100644 --- a/roofit/codegen/src/CodegenImpl.cxx +++ b/roofit/codegen/src/CodegenImpl.cxx @@ -111,18 +111,33 @@ void rooHistTranslateImpl(RooAbsArg const &arg, CodegenContext &ctx, int intOrde ctx.addResult(&arg, "(" + weightArr + ")[" + offset + "]"); } -std::string realSumPdfTranslateImpl(CodegenContext &ctx, RooAbsArg const &arg, RooArgList const &funcList, - RooArgList const &coefList, bool normalize) +std::string +realSumPdfTranslateImpl(CodegenContext &ctx, RooArgList const &funcList, RooArgList const &coefList, bool normalize) { bool noLastCoeff = funcList.size() != coefList.size(); + // Decide the scope from the inputs rather than from the RooRealSumPdf + // itself: this function is also used for the integral of the sum, whose + // inputs are the integrals of the components. Those usually don't depend + // on the data even though the sum does, and then the code must not end up + // inside the event loop, because the integral is also the expected number + // of events that the extended likelihood uses after the loop. + bool scopeIndependent = true; + for (RooAbsArg const *func : funcList) { + scopeIndependent = scopeIndependent && ctx.isScopeIndependent(func); + } + for (RooAbsArg const *coef : coefList) { + scopeIndependent = scopeIndependent && ctx.isScopeIndependent(coef); + } + auto addCode = [&](std::string const &code) { ctx.addToCodeBody(code, scopeIndependent); }; + std::string const &funcName = ctx.buildArg(funcList); std::string const &coeffName = ctx.buildArg(coefList); std::string const &coeffSize = std::to_string(coefList.size()); std::string sum = ctx.getTmpVarName(); std::string coeffSum = ctx.getTmpVarName(); - ctx.addToCodeBody(&arg, "double " + sum + " = 0;\ndouble " + coeffSum + "= 0;\n"); + addCode("double " + sum + " = 0;\ndouble " + coeffSum + "= 0;\n"); std::string iterator = "i_" + ctx.getTmpVarName(); std::string subscriptExpr = "[" + iterator + "]"; @@ -137,7 +152,7 @@ std::string realSumPdfTranslateImpl(CodegenContext &ctx, RooAbsArg const &arg, R } else if (normalize) { code += sum + " /= " + coeffSum + ";\n"; } - ctx.addToCodeBody(&arg, code); + addCode(code); return sum; } @@ -249,7 +264,7 @@ void codegenImpl(RooAbsArg &arg, CodegenContext &ctx) void codegenImpl(RooAddPdf &arg, CodegenContext &ctx) { - ctx.addResult(&arg, realSumPdfTranslateImpl(ctx, arg, arg.pdfList(), arg.coefList(), true)); + ctx.addResult(&arg, realSumPdfTranslateImpl(ctx, arg.pdfList(), arg.coefList(), true)); } void codegenImpl(RooMultiVarGaussian &arg, CodegenContext &ctx) @@ -832,12 +847,12 @@ void codegenImpl(RooRealIntegral &arg, CodegenContext &ctx) void codegenImpl(RooRealSumFunc &arg, CodegenContext &ctx) { - ctx.addResult(&arg, realSumPdfTranslateImpl(ctx, arg, arg.funcList(), arg.coefList(), false)); + ctx.addResult(&arg, realSumPdfTranslateImpl(ctx, arg.funcList(), arg.coefList(), false)); } void codegenImpl(RooRealSumPdf &arg, CodegenContext &ctx) { - ctx.addResult(&arg, realSumPdfTranslateImpl(ctx, arg, arg.funcList(), arg.coefList(), false)); + ctx.addResult(&arg, realSumPdfTranslateImpl(ctx, arg.funcList(), arg.coefList(), false)); } void codegenImpl(RooRealVar &arg, CodegenContext &ctx) @@ -1100,7 +1115,7 @@ std::string codegenIntegralImpl(RooPolynomial &arg, int, const char *rangeName, std::string codegenIntegralImpl(RooRealSumPdf &arg, int code, const char *rangeName, CodegenContext &ctx) { // Re-use translate, since integration is linear. - return realSumPdfTranslateImpl(ctx, arg, arg.funcIntListFromCache(code, rangeName), arg.coefList(), false); + return realSumPdfTranslateImpl(ctx, arg.funcIntListFromCache(code, rangeName), arg.coefList(), false); } std::string codegenIntegralImpl(RooUniform &arg, int code, const char *rangeName, CodegenContext &) diff --git a/roofit/roofitcore/inc/RooFit/CodegenContext.h b/roofit/roofitcore/inc/RooFit/CodegenContext.h index 18e70482c5453..1ef74da53de5c 100644 --- a/roofit/roofitcore/inc/RooFit/CodegenContext.h +++ b/roofit/roofitcore/inc/RooFit/CodegenContext.h @@ -65,6 +65,8 @@ class CodegenContext { void addToCodeBody(std::string const &in, bool isScopeIndep = false); + bool isScopeIndependent(RooAbsArg const *in) const; + /// @brief Build the code to call the function with name `funcname`, passing some arguments. /// The arguments can either be doubles or some RooFit arguments whose /// results will be looked up in the context. @@ -130,8 +132,6 @@ class CodegenContext { template std::string buildArgSpanImpl(std::span arr); - bool isScopeIndependent(RooAbsArg const *in) const; - void endLoop(LoopScope const &scope); void addResult(TNamed const *key, std::string const &value); diff --git a/roofit/roofitcore/test/testRooFuncWrapper.cxx b/roofit/roofitcore/test/testRooFuncWrapper.cxx index 97d1b8d416199..801b6c7e68aff 100644 --- a/roofit/roofitcore/test/testRooFuncWrapper.cxx +++ b/roofit/roofitcore/test/testRooFuncWrapper.cxx @@ -650,6 +650,15 @@ auto testValues = testing::Values( "RooRealSumPdf1", {"Gaussian::gx(x[-10,10],m[0],1.0)", "Chebychev::ch(x,{0.1,0.2,-0.3})", "RealSumPdf::model({gx, ch}, {f[0,1]})"}, 6e-3, false), + // Extended RooRealSumPdf (one coefficient per function, like in HistFactory + // models). The expected number of events is the integral of the sum, which + // the NLL reads after the event loop. Here, the same integral is also the + // normalization of the pdf inside the loop, so this checks that the + // generated integral code is placed outside the loop. + makeTestParams("RooRealSumPdfExtended", + {"Gaussian::gx(x[-10,10],m[0,-3,3],1.5)", "Exponential::ex(x,-0.1)", + "ASUM::model(n1[100,0,1000]*gx, n2[100,0,1000]*ex)"}, + 6e-3, true), // Express a Gaussian as a RooFormulaVar/expr and put in a RooWrapper pdf to // get test coverage for RooWrapperPdf. makeTestParams("RooWrapperPdf",