[RF] RooFFTConvPdf: cache norm. val to avoid bookkeeping during scan

roofit/histfactory/CMakeLists.txt

@@ -53,7 +53,6 @@ ROOT_STANDARD_LIBRARY_PACKAGE(HistFactory
   DICTIONARY_OPTIONS
     "-writeEmptyRootPCM"
   LIBRARIES
-    RooBatchCompute
     ${HISTFACTORY_XML_LIBRARIES}
   DEPENDENCIES
     RooFit

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)

roofit/roofit/CMakeLists.txt

@@ -150,8 +150,6 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooFit
     "-writeEmptyRootPCM"
   LINKDEF
     LinkDef1.h
-  LIBRARIES
-    RooBatchCompute
   DEPENDENCIES
     Core
     RooFitCore

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

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 ;

roofit/roofitcore/inc/RooFFTConvPdf.h

@@ -79,7 +79,7 @@ class RooFFTConvPdf : public RooAbsCachedPdf {
 
   void calcParams() ;
 
-  std::vector<double>  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<double>  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<RooAbsPdf> pdf1Clone;
     std::unique_ptr<RooAbsPdf> pdf2Clone;
 
+    double normVal1 = 0.0;
+    double normVal2 = 0.0;
+
     std::unique_ptr<RooAbsBinning> histBinning;
     std::unique_ptr<RooAbsBinning> scanBinning;
   };

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<RooAbsPdf> _pdf;

roofit/roofitcore/res/RooFitImplHelpers.h

@@ -11,9 +11,14 @@
 #ifndef RooFit_RooFitImplHelpers_h
 #define RooFit_RooFitImplHelpers_h
 
-#include <RooMsgService.h>
 #include <RooAbsArg.h>
+#include <RooAbsPdf.h>
 #include <RooAbsReal.h>
+#include <RooMsgService.h>
+
+#include <RooNaNPacker.h>
+
+#include <TMath.h>
 
 #include <sstream>
 #include <string>
@@ -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);

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();
   }

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<RooRealVar*>(cacheHist.get()->find(_x.arg().GetName())) ;
   if (_bufStrat==Extend) histX->setBinning(*aux.scanBinning) ;
-  std::vector<double> input1 = scanPdf(const_cast<RooRealVar &>(static_cast<RooRealVar const&>(_x.arg())),*aux.pdf1Clone,cacheHist,slicePos,N,N2,binShift1,_shift1) ;
-  std::vector<double> input2 = scanPdf(const_cast<RooRealVar &>(static_cast<RooRealVar const&>(_x.arg())),*aux.pdf2Clone,cacheHist,slicePos,N,N2,binShift2,_shift2) ;
+  RooRealVar &xVar = const_cast<RooRealVar &>(static_cast<RooRealVar const&>(_x.arg()));
+  std::vector<double> input1 = scanPdf(xVar,*aux.pdf1Clone,aux.normVal1,cacheHist,slicePos,N,N2,binShift1,_shift1) ;
+  std::vector<double> 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<double>  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<double> 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<RooRealVar*>(hist.get()->find(obs.GetName())) ;
@@ -698,6 +709,12 @@ std::vector<double>  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<double> tmp(N2);
   Int_t k(0) ;
@@ -707,7 +724,7 @@ std::vector<double>  RooFFTConvPdf::scanPdf(RooRealVar& obs, RooAbsPdf& pdf, con
     // Sample entire extended range (N2 samples)
     for (k=0 ; k<N2 ; k++) {
       histX->setBin(k) ;
-      tmp[k] = pdf.getVal(hist.get()) ;
+      tmp[k] = getPdfVal();
     }
     break ;
 
@@ -716,16 +733,16 @@ std::vector<double>  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 ; k<Nbuf ; k++) {
    tmp[k] = val ;
       }
       for (k=0 ; k<N ; k++) {
    histX->setBin(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<Nbuf ; k++) {
    tmp[N+Nbuf+k] = val ;
       }
@@ -737,13 +754,13 @@ std::vector<double>  RooFFTConvPdf::scanPdf(RooRealVar& obs, RooAbsPdf& pdf, con
     // bins with mirror image of sampled range
     for (k=0 ; k<N ; k++) {
       histX->setBin(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 ;
   }

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 {
 

roofit/roofitcore/src/RooNormalizedPdf.cxx

@@ -13,6 +13,7 @@
 #include "RooFit/Detail/RooNormalizedPdf.h"
 
 #include "RooBatchCompute.h"
+#include "RooFitImplHelpers.h"
 
 #include <array>
 
@@ -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

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)

roofit/roofitmore/CMakeLists.txt

@@ -39,8 +39,6 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooFitMore
     "-writeEmptyRootPCM"
   LINKDEF
     LinkDef.h
-  LIBRARIES
-    RooBatchCompute
   DEPENDENCIES
     ${ROOT_MATHMORE_LIBRARY}
     Core