From af107899af4c8dd9ecbee2738c3f9800965c07f5 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Sat, 21 Feb 2026 15:35:27 +0100 Subject: [PATCH] [RF] RooFFTConvPdf: cache norm. val to avoid bookkeeping during scan `RooFFTConvPdf::scanPdf()` repeatedly evaluates the component pdfs with `pdf.getVal(normSet)` while filling the FFT input buffers. For large scans this incurs unnecessary overhead from `RooAbsPdf::getValV()`'s normalization-set tracking and cache management, even though the normalization set is fixed for a given FFTCacheElem. This change caches the normalization integrals for both component pdf clones (normVal1, normVal2) when the FFTCacheElem is constructed. During scanning, the pdfs are then evaluated without passing a normalization set and are normalized manually using the cached integral values. To exactly reproduce `RooAbsPdf::getValV()` behaviour (including NaN-packing semantics), the `RooAbsPdf::normalizeWithNaNPacking(rawVal, normVal)` helper function was moved to an internal helper namespace. This reduces evaluation overhead in FFT convolutions while preserving bitwise-equivalent normalization behaviour. --- roofit/histfactory/CMakeLists.txt | 1 - roofit/multiprocess/test/CMakeLists.txt | 2 +- roofit/roofit/CMakeLists.txt | 2 - roofit/roofitcore/CMakeLists.txt | 2 +- roofit/roofitcore/inc/RooAbsPdf.h | 2 - roofit/roofitcore/inc/RooFFTConvPdf.h | 5 ++- .../inc/RooFit/Detail/RooNormalizedPdf.h | 5 +-- roofit/roofitcore/res/RooFitImplHelpers.h | 34 +++++++++++++++- roofit/roofitcore/src/RooAbsPdf.cxx | 30 +------------- roofit/roofitcore/src/RooFFTConvPdf.cxx | 39 +++++++++++++------ roofit/roofitcore/src/RooFitImplHelpers.cxx | 6 +-- roofit/roofitcore/src/RooNormalizedPdf.cxx | 6 +++ roofit/roofitcore/test/CMakeLists.txt | 2 +- roofit/roofitmore/CMakeLists.txt | 2 - 14 files changed, 78 insertions(+), 60 deletions(-) diff --git a/roofit/histfactory/CMakeLists.txt b/roofit/histfactory/CMakeLists.txt index 5241d32d035d2..242f206971f4a 100644 --- a/roofit/histfactory/CMakeLists.txt +++ b/roofit/histfactory/CMakeLists.txt @@ -53,7 +53,6 @@ ROOT_STANDARD_LIBRARY_PACKAGE(HistFactory DICTIONARY_OPTIONS "-writeEmptyRootPCM" LIBRARIES - RooBatchCompute ${HISTFACTORY_XML_LIBRARIES} DEPENDENCIES RooFit diff --git a/roofit/multiprocess/test/CMakeLists.txt b/roofit/multiprocess/test/CMakeLists.txt index e9bb53b5fd447..c191d56346d22 100644 --- a/roofit/multiprocess/test/CMakeLists.txt +++ b/roofit/multiprocess/test/CMakeLists.txt @@ -1,7 +1,7 @@ # @author Patrick Bos, NL eScience Center, 2019-2022 add_library(RooFit_multiprocess_testing_utils INTERFACE) -target_link_libraries(RooFit_multiprocess_testing_utils INTERFACE RooFitCore RooBatchCompute) +target_link_libraries(RooFit_multiprocess_testing_utils INTERFACE RooFitCore) target_include_directories(RooFit_multiprocess_testing_utils INTERFACE ${RooFitMultiProcess_INCLUDE_DIR}) ROOT_ADD_GTEST(test_RooFit_MultiProcess_Job test_Job.cxx LIBRARIES RooFitMultiProcess Core) diff --git a/roofit/roofit/CMakeLists.txt b/roofit/roofit/CMakeLists.txt index b6d89bbf7de18..9acc215897036 100644 --- a/roofit/roofit/CMakeLists.txt +++ b/roofit/roofit/CMakeLists.txt @@ -150,8 +150,6 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooFit "-writeEmptyRootPCM" LINKDEF LinkDef1.h - LIBRARIES - RooBatchCompute DEPENDENCIES Core RooFitCore diff --git a/roofit/roofitcore/CMakeLists.txt b/roofit/roofitcore/CMakeLists.txt index 7031e842ecb20..65e4b2e3d5e87 100644 --- a/roofit/roofitcore/CMakeLists.txt +++ b/roofit/roofitcore/CMakeLists.txt @@ -453,7 +453,6 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooFitCore DICTIONARY_OPTIONS "-writeEmptyRootPCM" LIBRARIES - RooBatchCompute ${EXTRA_LIBRARIES} DEPENDENCIES Core @@ -464,6 +463,7 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooFitCore RIO MathCore Foam + RooBatchCompute ${EXTRA_DEPENDENCIES} LINKDEF inc/LinkDef.h diff --git a/roofit/roofitcore/inc/RooAbsPdf.h b/roofit/roofitcore/inc/RooAbsPdf.h index 56cfe34e98c02..18d154764ef2d 100644 --- a/roofit/roofitcore/inc/RooAbsPdf.h +++ b/roofit/roofitcore/inc/RooAbsPdf.h @@ -296,8 +296,6 @@ class RooAbsPdf : public RooAbsReal { return RooFit::getUniqueId(normSet).value() == _normSetId; } - double normalizeWithNaNPacking(double rawVal, double normVal) const; - RooPlot *plotOn(RooPlot *frame, PlotOpt o) const override; friend class RooMCStudy ; diff --git a/roofit/roofitcore/inc/RooFFTConvPdf.h b/roofit/roofitcore/inc/RooFFTConvPdf.h index ef2b0e520dedc..e9da4453f6138 100644 --- a/roofit/roofitcore/inc/RooFFTConvPdf.h +++ b/roofit/roofitcore/inc/RooFFTConvPdf.h @@ -79,7 +79,7 @@ class RooFFTConvPdf : public RooAbsCachedPdf { void calcParams() ; - std::vector scanPdf(RooRealVar& obs, RooAbsPdf& pdf, const RooDataHist& hist, const RooArgSet& slicePos, Int_t& N, Int_t& N2, Int_t& zeroBin, double shift) const ; + std::vector scanPdf(RooRealVar& obs, RooAbsPdf& pdf, double normVal, const RooDataHist& hist, const RooArgSet& slicePos, Int_t& N, Int_t& N2, Int_t& zeroBin, double shift) const ; class FFTCacheElem : public PdfCacheElem { public: @@ -94,6 +94,9 @@ class RooFFTConvPdf : public RooAbsCachedPdf { std::unique_ptr pdf1Clone; std::unique_ptr pdf2Clone; + double normVal1 = 0.0; + double normVal2 = 0.0; + std::unique_ptr histBinning; std::unique_ptr scanBinning; }; diff --git a/roofit/roofitcore/inc/RooFit/Detail/RooNormalizedPdf.h b/roofit/roofitcore/inc/RooFit/Detail/RooNormalizedPdf.h index 5e9f3c7fa0dd9..ff70d3a96edf6 100644 --- a/roofit/roofitcore/inc/RooFit/Detail/RooNormalizedPdf.h +++ b/roofit/roofitcore/inc/RooFit/Detail/RooNormalizedPdf.h @@ -82,10 +82,7 @@ class RooNormalizedPdf : public RooAbsPdf { // still need it to support printing of the object. return getValV(nullptr); } - double getValV(const RooArgSet * /*normSet*/) const override - { - return normalizeWithNaNPacking(_pdf->getVal(), _normIntegral->getVal()); - }; + double getValV(const RooArgSet * normSet) const override; private: RooTemplateProxy _pdf; diff --git a/roofit/roofitcore/res/RooFitImplHelpers.h b/roofit/roofitcore/res/RooFitImplHelpers.h index 3f790d247783c..619a5f5572cb2 100644 --- a/roofit/roofitcore/res/RooFitImplHelpers.h +++ b/roofit/roofitcore/res/RooFitImplHelpers.h @@ -11,9 +11,14 @@ #ifndef RooFit_RooFitImplHelpers_h #define RooFit_RooFitImplHelpers_h -#include #include +#include #include +#include + +#include + +#include #include #include @@ -104,6 +109,33 @@ void replaceAll(std::string &inOut, std::string_view what, std::string_view with std::string makeSliceCutString(RooArgSet const &sliceDataSet); +// Inlined because this is called inside RooAbsPdf::getValV(), and therefore +// performance critical. +inline double normalizeWithNaNPacking(RooAbsPdf const &pdf, double rawVal, double normVal) +{ + + if (normVal < 0. || (normVal == 0. && rawVal != 0)) { + // Unreasonable normalisations. A zero integral can be tolerated if the function vanishes, though. + const std::string msg = "p.d.f normalization integral is zero or negative: " + std::to_string(normVal); + pdf.logEvalError(msg.c_str()); + return RooNaNPacker::packFloatIntoNaN(-normVal + (rawVal < 0. ? -rawVal : 0.)); + } + + if (rawVal < 0.) { + std::stringstream ss; + ss << "p.d.f value is less than zero (" << rawVal << "), trying to recover"; + pdf.logEvalError(ss.str().c_str()); + return RooNaNPacker::packFloatIntoNaN(-rawVal); + } + + if (TMath::IsNaN(rawVal)) { + pdf.logEvalError("p.d.f value is Not-a-Number"); + return rawVal; + } + + return (rawVal == 0. && normVal == 0.) ? 0. : rawVal / normVal; +} + } // namespace RooFit::Detail double toDouble(const char *s); diff --git a/roofit/roofitcore/src/RooAbsPdf.cxx b/roofit/roofitcore/src/RooAbsPdf.cxx index a8c87452d248a..f81be846bc1b2 100644 --- a/roofit/roofitcore/src/RooAbsPdf.cxx +++ b/roofit/roofitcore/src/RooAbsPdf.cxx @@ -286,34 +286,6 @@ RooAbsPdf::~RooAbsPdf() } -double RooAbsPdf::normalizeWithNaNPacking(double rawVal, double normVal) const { - - if (normVal < 0. || (normVal == 0. && rawVal != 0)) { - //Unreasonable normalisations. A zero integral can be tolerated if the function vanishes, though. - const std::string msg = "p.d.f normalization integral is zero or negative: " + std::to_string(normVal); - logEvalError(msg.c_str()); - clearValueAndShapeDirty(); - return RooNaNPacker::packFloatIntoNaN(-normVal + (rawVal < 0. ? -rawVal : 0.)); - } - - if (rawVal < 0.) { - std::stringstream ss; - ss << "p.d.f value is less than zero (" << rawVal << "), trying to recover"; - logEvalError(ss.str().c_str()); - clearValueAndShapeDirty(); - return RooNaNPacker::packFloatIntoNaN(-rawVal); - } - - if (TMath::IsNaN(rawVal)) { - logEvalError("p.d.f value is Not-a-Number"); - clearValueAndShapeDirty(); - return rawVal; - } - - return (rawVal == 0. && normVal == 0.) ? 0. : rawVal / normVal; -} - - //////////////////////////////////////////////////////////////////////////////// /// Return current value, normalized by integrating over /// the observables in `nset`. If `nset` is 0, the unnormalized value @@ -354,7 +326,7 @@ double RooAbsPdf::getValV(const RooArgSet* nset) const // Evaluate denominator const double normVal = _norm->getVal(); - _value = normalizeWithNaNPacking(rawVal, normVal); + _value = RooFit::Detail::normalizeWithNaNPacking(*this, rawVal, normVal); clearValueAndShapeDirty(); } diff --git a/roofit/roofitcore/src/RooFFTConvPdf.cxx b/roofit/roofitcore/src/RooFFTConvPdf.cxx index bf7d882a9de28..15bfde3f3883a 100644 --- a/roofit/roofitcore/src/RooFFTConvPdf.cxx +++ b/roofit/roofitcore/src/RooFFTConvPdf.cxx @@ -123,6 +123,7 @@ #include "RooGlobalFunc.h" #include "RooConstVar.h" #include "RooUniformBinning.h" +#include "RooFitImplHelpers.h" #include "TClass.h" #include "TComplex.h" @@ -399,6 +400,14 @@ RooFFTConvPdf::FFTCacheElem::FFTCacheElem(const RooFFTConvPdf& self, const RooAr pdf2Clone.reset(clonePdf2) ; } + // Cache normalization integral values, since we know they don't change for + // the given normalization set in this cache object. When using this cache, + // we evaluate the pdfs without normalization set and then do the + // normalization manually using these cached values. This has less overhead + // compared to letting RooAbsPdf::getVal(normSet) figure out if the normSet + // has changed and get the caching right. + normVal1 = pdf1Clone->getNorm(hist()->get()); + normVal2 = pdf2Clone->getNorm(hist()->get()); // Attach cloned pdf to all original parameters of self RooArgSet convObsSet{*convObs}; @@ -579,8 +588,9 @@ void RooFFTConvPdf::fillCacheSlice(FFTCacheElem& aux, const RooArgSet& slicePos) RooRealVar* histX = static_cast(cacheHist.get()->find(_x.arg().GetName())) ; if (_bufStrat==Extend) histX->setBinning(*aux.scanBinning) ; - std::vector input1 = scanPdf(const_cast(static_cast(_x.arg())),*aux.pdf1Clone,cacheHist,slicePos,N,N2,binShift1,_shift1) ; - std::vector input2 = scanPdf(const_cast(static_cast(_x.arg())),*aux.pdf2Clone,cacheHist,slicePos,N,N2,binShift2,_shift2) ; + RooRealVar &xVar = const_cast(static_cast(_x.arg())); + std::vector input1 = scanPdf(xVar,*aux.pdf1Clone,aux.normVal1,cacheHist,slicePos,N,N2,binShift1,_shift1) ; + std::vector input2 = scanPdf(xVar,*aux.pdf2Clone,aux.normVal2,cacheHist,slicePos,N,N2,binShift2,_shift2) ; if (_bufStrat==Extend) histX->setBinning(*aux.histBinning) ; #ifndef ROOFIT_MATH_FFTW3 @@ -662,8 +672,9 @@ void RooFFTConvPdf::fillCacheSlice(FFTCacheElem& aux, const RooArgSet& slicePos) /// The return value is an array of doubles of length N2 with the sampled values. The caller takes ownership /// of the array -std::vector RooFFTConvPdf::scanPdf(RooRealVar& obs, RooAbsPdf& pdf, const RooDataHist& hist, const RooArgSet& slicePos, - Int_t& N, Int_t& N2, Int_t& zeroBin, double shift) const +std::vector RooFFTConvPdf::scanPdf(RooRealVar &obs, RooAbsPdf &pdf, double normVal, const RooDataHist &hist, + const RooArgSet &slicePos, Int_t &N, Int_t &N2, Int_t &zeroBin, + double shift) const { RooRealVar* histX = static_cast(hist.get()->find(obs.GetName())) ; @@ -698,6 +709,12 @@ std::vector RooFFTConvPdf::scanPdf(RooRealVar& obs, RooAbsPdf& pdf, con while(zeroBin>=N2) zeroBin-= N2 ; while(zeroBin<0) zeroBin+= N2 ; + // To mimic exactly the normalization code in RooAbsPdf::getValV() + auto getPdfVal = [&]() { + double rawVal = pdf.getVal(); + return RooFit::Detail::normalizeWithNaNPacking(pdf, rawVal, normVal); + }; + // First scan hist into temp array std::vector tmp(N2); Int_t k(0) ; @@ -707,7 +724,7 @@ std::vector RooFFTConvPdf::scanPdf(RooRealVar& obs, RooAbsPdf& pdf, con // Sample entire extended range (N2 samples) for (k=0 ; ksetBin(k) ; - tmp[k] = pdf.getVal(hist.get()) ; + tmp[k] = getPdfVal(); } break ; @@ -716,16 +733,16 @@ std::vector RooFFTConvPdf::scanPdf(RooRealVar& obs, RooAbsPdf& pdf, con // bins with p.d.f. value at respective boundary { histX->setBin(0) ; - double val = pdf.getVal(hist.get()) ; + double val = getPdfVal(); for (k=0 ; ksetBin(k) ; - tmp[k+Nbuf] = pdf.getVal(hist.get()) ; + tmp[k+Nbuf] = getPdfVal(); } histX->setBin(N-1) ; - val = pdf.getVal(hist.get()) ; + val = getPdfVal(); for (k=0 ; k RooFFTConvPdf::scanPdf(RooRealVar& obs, RooAbsPdf& pdf, con // bins with mirror image of sampled range for (k=0 ; ksetBin(k) ; - tmp[k+Nbuf] = pdf.getVal(hist.get()) ; + tmp[k+Nbuf] = getPdfVal(); } for (k=1 ; k<=Nbuf ; k++) { histX->setBin(k) ; - tmp[Nbuf-k] = pdf.getVal(hist.get()) ; + tmp[Nbuf-k] = getPdfVal(); histX->setBin(N-k) ; - tmp[Nbuf+N+k-1] = pdf.getVal(hist.get()) ; + tmp[Nbuf+N+k-1] = getPdfVal(); } break ; } diff --git a/roofit/roofitcore/src/RooFitImplHelpers.cxx b/roofit/roofitcore/src/RooFitImplHelpers.cxx index f5943656c1b41..9fb2e20bffcf8 100644 --- a/roofit/roofitcore/src/RooFitImplHelpers.cxx +++ b/roofit/roofitcore/src/RooFitImplHelpers.cxx @@ -288,8 +288,7 @@ RooAbsArg *cloneTreeWithSameParametersImpl(RooAbsArg const &arg, RooArgSet const } // namespace RooHelpers -namespace RooFit { -namespace Detail { +namespace RooFit::Detail { /// Transform a string into a valid C++ variable name by replacing forbidden /// characters with underscores. @@ -335,8 +334,7 @@ std::string makeSliceCutString(RooArgSet const &sliceDataSet) return cutString.str(); } -} // namespace Detail -} // namespace RooFit +} // namespace RooFit::Detail namespace { diff --git a/roofit/roofitcore/src/RooNormalizedPdf.cxx b/roofit/roofitcore/src/RooNormalizedPdf.cxx index 1d2da63612eaa..b6eabfc20662b 100644 --- a/roofit/roofitcore/src/RooNormalizedPdf.cxx +++ b/roofit/roofitcore/src/RooNormalizedPdf.cxx @@ -13,6 +13,7 @@ #include "RooFit/Detail/RooNormalizedPdf.h" #include "RooBatchCompute.h" +#include "RooFitImplHelpers.h" #include @@ -51,4 +52,9 @@ void RooNormalizedPdf::doEval(RooFit::EvalContext &ctx) const } } +double RooNormalizedPdf::getValV(const RooArgSet * /*normSet*/) const +{ + return normalizeWithNaNPacking(*_pdf, _pdf->getVal(), _normIntegral->getVal()); +} + } // namespace RooFit::Detail diff --git a/roofit/roofitcore/test/CMakeLists.txt b/roofit/roofitcore/test/CMakeLists.txt index 5733a0a67cb8f..def1fa324c837 100644 --- a/roofit/roofitcore/test/CMakeLists.txt +++ b/roofit/roofitcore/test/CMakeLists.txt @@ -80,7 +80,7 @@ ROOT_ADD_GTEST(testRooRealVar testRooRealVar.cxx LIBRARIES RooFitCore if(clad) ROOT_ADD_GTEST(testRooFuncWrapper testRooFuncWrapper.cxx LIBRARIES RooFitCore RooFit HistFactory) endif() -ROOT_ADD_GTEST(testNaNPacker testNaNPacker.cxx LIBRARIES RooFitCore RooBatchCompute) +ROOT_ADD_GTEST(testNaNPacker testNaNPacker.cxx LIBRARIES RooFitCore) ROOT_ADD_GTEST(testRooExtendedBinding testRooExtendedBinding.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooMinimizer testRooMinimizer.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooMulti testRooMulti.cxx LIBRARIES RooFitCore RooFit) diff --git a/roofit/roofitmore/CMakeLists.txt b/roofit/roofitmore/CMakeLists.txt index 39cdeacd780db..561d55382309f 100644 --- a/roofit/roofitmore/CMakeLists.txt +++ b/roofit/roofitmore/CMakeLists.txt @@ -39,8 +39,6 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooFitMore "-writeEmptyRootPCM" LINKDEF LinkDef.h - LIBRARIES - RooBatchCompute DEPENDENCIES ${ROOT_MATHMORE_LIBRARY} Core