[RF] Remove all code related to constant term optimization

This is one of the most complicated aspects of RooFit, which was
deprecated in 6.40 and scheduled for removal in 6.42.

It amounted to over 1 percent of the RooFitCore source code, and is now
superseeded by the vectorized evaluation backend, which does the caching
of vector buffers transparently in the RooFit evaluator.

Author: guitargeek

roofit/histfactory/test/testHistFactory.cxx

@@ -590,82 +590,67 @@ TEST_P(HFFixtureFit, Fit)
    auto mc = dynamic_cast<RooStats::ModelConfig *>(ws->obj("ModelConfig"));
    ASSERT_NE(mc, nullptr);
 
-   // This tests both correct pre-caching of constant terms and (if false) that all doEval() are correct.
-   for (bool constTermOptimization : {true, false}) {
+   std::unique_ptr<RooArgSet> pars(simPdf->getParameters(*data));
+   // Kick parameters:
+   for (auto par : *pars) {
+      auto real = dynamic_cast<RooAbsRealLValue *>(par);
+      if (real && !real->isConstant())
+         real->setVal(real->getVal() * 0.95);
+   }
+   if (makeModelMode == MakeModelMode::StatSyst) {
+      auto poi = dynamic_cast<RooRealVar *>(pars->find("SigXsecOverSM"));
+      ASSERT_NE(poi, nullptr);
+      poi->setVal(2.);
+      poi->setConstant();
+   }
 
-      // constTermOptimization makes only sense in the legacy backend
-      if (constTermOptimization && evalBackend != RooFit::EvalBackend::Legacy()) {
-         continue;
-      }
-      SCOPED_TRACE(constTermOptimization ? "const term optimisation" : "No const term optimisation");
-
-      // Stop if one of the previous runs had a failure to keep the terminal clean.
-      if (HasFailure())
-         break;
-
-      std::unique_ptr<RooArgSet> pars(simPdf->getParameters(*data));
-      // Kick parameters:
-      for (auto par : *pars) {
-         auto real = dynamic_cast<RooAbsRealLValue *>(par);
-         if (real && !real->isConstant())
-            real->setVal(real->getVal() * 0.95);
-      }
-      if (makeModelMode == MakeModelMode::StatSyst) {
-         auto poi = dynamic_cast<RooRealVar *>(pars->find("SigXsecOverSM"));
-         ASSERT_NE(poi, nullptr);
-         poi->setVal(2.);
-         poi->setConstant();
+   using namespace RooFit;
+   std::unique_ptr<RooFitResult> fitResult{simPdf->fitTo(
+      *data, evalBackend, GlobalObservables(*mc->GetGlobalObservables()), Save(), PrintLevel(verbose ? 1 : -1))};
+   ASSERT_NE(fitResult, nullptr);
+   if (verbose)
+      fitResult->Print("v");
+   EXPECT_EQ(fitResult->status(), 0);
+
+   auto checkParam = [&](const std::string &param, double target, double absPrecision = 1.e-2) {
+      auto par = dynamic_cast<RooRealVar *>(fitResult->floatParsFinal().find(param.c_str()));
+      if (!par) {
+         // Parameter was constant in this fit
+         par = dynamic_cast<RooRealVar *>(fitResult->constPars().find(param.c_str()));
+         ASSERT_NE(par, nullptr) << param;
+         EXPECT_DOUBLE_EQ(par->getVal(), target) << "Constant parameter " << param << " is off target.";
+      } else {
+         EXPECT_NEAR(par->getVal(), target, par->getError())
+            << "Parameter " << param << " close to target " << target << " within uncertainty";
+         EXPECT_NEAR(par->getVal(), target, absPrecision) << "Parameter " << param << " close to target " << target;
       }
+   };
 
-      using namespace RooFit;
-      std::unique_ptr<RooFitResult> fitResult{simPdf->fitTo(*data, evalBackend, Optimize(constTermOptimization),
-                                                            GlobalObservables(*mc->GetGlobalObservables()), Save(),
-                                                            PrintLevel(verbose ? 1 : -1))};
-      ASSERT_NE(fitResult, nullptr);
-      if (verbose)
-         fitResult->Print("v");
-      EXPECT_EQ(fitResult->status(), 0);
-
-      auto checkParam = [&](const std::string &param, double target, double absPrecision = 1.e-2) {
-         auto par = dynamic_cast<RooRealVar *>(fitResult->floatParsFinal().find(param.c_str()));
-         if (!par) {
-            // Parameter was constant in this fit
-            par = dynamic_cast<RooRealVar *>(fitResult->constPars().find(param.c_str()));
-            ASSERT_NE(par, nullptr) << param;
-            EXPECT_DOUBLE_EQ(par->getVal(), target) << "Constant parameter " << param << " is off target.";
-         } else {
-            EXPECT_NEAR(par->getVal(), target, par->getError())
-               << "Parameter " << param << " close to target " << target << " within uncertainty";
-            EXPECT_NEAR(par->getVal(), target, absPrecision) << "Parameter " << param << " close to target " << target;
-         }
-      };
-
-      if (makeModelMode == MakeModelMode::OverallSyst) {
-         // Model is set up such that background scale factors should be close to 1, and signal == 2
-         checkParam("SigXsecOverSM", 2.);
-         checkParam("alpha_syst2", 0.);
-         checkParam("alpha_syst3", 0.);
-         checkParam("alpha_syst4", 0.);
-         checkParam("gamma_stat_channel1_bin_0", 1.);
-         checkParam("gamma_stat_channel1_bin_1", 1.);
-      } else if (makeModelMode == MakeModelMode::HistoSyst) {
-         // Model is set up with a -1 sigma pull on the signal shape parameter.
-         checkParam("SigXsecOverSM", 2., 1.1E-1); // Higher tolerance: Expect a pull due to shape syst.
-         checkParam("gamma_stat_channel1_bin_0", 1.);
-         checkParam("gamma_stat_channel1_bin_1", 1.);
-         checkParam("alpha_SignalShape", -0.9, 5.E-2); // Pull slightly lower than 1 because of constraint term
-      } else if (makeModelMode == MakeModelMode::StatSyst) {
-         // Model is set up with a -1 sigma pull on the signal shape parameter.
-         checkParam("SigXsecOverSM", 2., 1.1E-1);       // Higher tolerance: Expect a pull due to shape syst.
-         checkParam("gamma_stat_channel1_bin_0", 1.09); // This should be pulled
-         checkParam("gamma_stat_channel1_bin_1", 1.);
-      } else if (makeModelMode == MakeModelMode::ShapeSyst) {
-         // This should be pulled down
-         checkParam("gamma_background1Shape_bin_0", 0.8866, 0.03);
-         // This should be pulled up, but not so much because the free signal
-         // strength will fit the excess in this bin.
-         checkParam("gamma_background2Shape_bin_1", 1.0250, 0.03);
-      }
+   if (makeModelMode == MakeModelMode::OverallSyst) {
+      // Model is set up such that background scale factors should be close to 1, and signal == 2
+      checkParam("SigXsecOverSM", 2.);
+      checkParam("alpha_syst2", 0.);
+      checkParam("alpha_syst3", 0.);
+      checkParam("alpha_syst4", 0.);
+      checkParam("gamma_stat_channel1_bin_0", 1.);
+      checkParam("gamma_stat_channel1_bin_1", 1.);
+   } else if (makeModelMode == MakeModelMode::HistoSyst) {
+      // Model is set up with a -1 sigma pull on the signal shape parameter.
+      checkParam("SigXsecOverSM", 2., 1.1E-1); // Higher tolerance: Expect a pull due to shape syst.
+      checkParam("gamma_stat_channel1_bin_0", 1.);
+      checkParam("gamma_stat_channel1_bin_1", 1.);
+      checkParam("alpha_SignalShape", -0.9, 5.E-2); // Pull slightly lower than 1 because of constraint term
+   } else if (makeModelMode == MakeModelMode::StatSyst) {
+      // Model is set up with a -1 sigma pull on the signal shape parameter.
+      checkParam("SigXsecOverSM", 2., 1.1E-1);       // Higher tolerance: Expect a pull due to shape syst.
+      checkParam("gamma_stat_channel1_bin_0", 1.09); // This should be pulled
+      checkParam("gamma_stat_channel1_bin_1", 1.);
+   } else if (makeModelMode == MakeModelMode::ShapeSyst) {
+      // This should be pulled down
+      checkParam("gamma_background1Shape_bin_0", 0.8866, 0.03);
+      // This should be pulled up, but not so much because the free signal
+      // strength will fit the excess in this bin.
+      checkParam("gamma_background2Shape_bin_1", 1.0250, 0.03);
    }
 
    if (false) {

roofit/roofit/test/vectorisedPDFs/VectorisedPDFTests.cxx

@@ -479,7 +479,7 @@ std::unique_ptr<RooFitResult> PDFTest::runBatchFit(RooAbsPdf *pdf)
 
    MyTimer batchTimer("Fitting batch mode " + _name);
    std::unique_ptr<RooFitResult> result{pdf->fitTo(*_dataFit, RooFit::EvalBackend::Cpu(), RooFit::SumW2Error(false),
-                                                   RooFit::Optimize(1), RooFit::PrintLevel(_printLevel), RooFit::Save(),
+                                                   RooFit::PrintLevel(_printLevel), RooFit::Save(),
                                                    _multiProcess > 0 ? RooFit::NumCPU(_multiProcess) : RooCmdArg())};
    std::cout << batchTimer;
    EXPECT_NE(result, nullptr);

roofit/roofit/test/vectorisedPDFs/testGaussBinned.cxx

@@ -87,7 +87,7 @@ TEST_P(GaussBinnedFit, BatchFit)
    m.setVal(-1.);
    s.setVal(3.);
    MyTimer timer(evalBackend == EvalBackend::Value::Cpu ? "BatchBinned" : "ScalarBinned");
-   gaus.fitTo(*dataHist, EvalBackend(evalBackend), PrintLevel(-1), Optimize(0));
+   gaus.fitTo(*dataHist, EvalBackend(evalBackend), PrintLevel(-1));
    timer.interval();
    std::cout << timer << std::endl;
    EXPECT_NEAR(m.getVal(), 1., m.getError());

roofit/roofitcore/CMakeLists.txt

@@ -436,7 +436,6 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooFitCore
     src/RooVectorDataStore.cxx
     src/RooWorkspace.cxx
     src/RooWrapperPdf.cxx
-    src/TestStatistics/ConstantTermsOptimizer.cxx
     src/TestStatistics/LikelihoodGradientWrapper.cxx
     src/TestStatistics/LikelihoodSerial.cxx
     src/TestStatistics/LikelihoodWrapper.cxx

roofit/roofitcore/inc/RooAbsArg.h

@@ -340,9 +340,6 @@ class RooAbsArg : public TNamed, public RooPrintable {
    bool findConstantNodes(const RooArgSet &observables, RooArgSet &cacheList);
    bool findConstantNodes(const RooArgSet &observables, RooArgSet &cacheList, RooLinkedList &processedNodes);
 
-   // constant term optimization
-   virtual void constOptimizeTestStatistic(ConstOpCode opcode, bool doAlsoTrackingOpt = true);
-
    virtual CacheMode canNodeBeCached() const { return Always; }
    virtual void setCacheAndTrackHints(RooArgSet & /*trackNodes*/) {};
 

roofit/roofitcore/inc/RooAbsData.h

@@ -304,9 +304,6 @@ class RooAbsData : public TNamed, public RooPrintable {
   double corrcov(const RooRealVar& x, const RooRealVar& y, const char* cutSpec, const char* cutRange, bool corr) const  ;
   RooFit::OwningPtr<TMatrixDSym> corrcovMatrix(const RooArgList& vars, const char* cutSpec, const char* cutRange, bool corr) const  ;
 
-  virtual void optimizeReadingWithCaching(RooAbsArg& arg, const RooArgSet& cacheList, const RooArgSet& keepObsList) ;
-  bool allClientsCached(RooAbsArg*, const RooArgSet&) ;
-
   struct PlotOpt {
    const char* cuts = "";
    Option_t* drawOptions = "P";
@@ -341,11 +338,6 @@ class RooAbsData : public TNamed, public RooPrintable {
   // for access into copied dataset:
   friend class RooFit::TestStatistics::RooAbsL;
 
-  virtual void cacheArgs(const RooAbsArg* owner, RooArgSet& varSet, const RooArgSet* nset=nullptr, bool skipZeroWeights=false) ;
-  virtual void resetCache() ;
-  virtual void setArgStatus(const RooArgSet& set, bool active) ;
-  virtual void attachCache(const RooAbsArg* newOwner, const RooArgSet& cachedVars) ;
-
   virtual std::unique_ptr<RooAbsData> reduceEng(const RooArgSet& varSubset, const RooFormulaVar* cutVar, const char* cutRange=nullptr,
                            std::size_t nStart = 0, std::size_t = std::numeric_limits<std::size_t>::max()) const = 0 ;
 

roofit/roofitcore/inc/RooAbsDataStore.h

@@ -121,15 +121,6 @@ class RooAbsDataStore : public TNamed, public RooPrintable {
   int defaultPrintContents(Option_t* /*opt*/) const override { return kName|kClassName|kArgs|kValue ; }
 
 
-  // Constant term  optimizer interface
-  virtual void cacheArgs(const RooAbsArg* cacheOwner, RooArgSet& varSet, const RooArgSet* nset=nullptr, bool skipZeroWeights=false) = 0 ;
-  virtual const RooAbsArg* cacheOwner() = 0 ;
-  virtual void attachCache(const RooAbsArg* newOwner, const RooArgSet& cachedVars) = 0 ;
-  virtual void setArgStatus(const RooArgSet& set, bool active) = 0 ;
-  const RooArgSet& cachedVars() const { return _cachedVars ; }
-  virtual void resetCache() = 0 ;
-  virtual void recalculateCache(const RooArgSet* /*proj*/, Int_t /*firstEvent*/, Int_t /*lastEvent*/, Int_t /*stepSize*/, bool /* skipZeroWeights*/) {} ;
-
   virtual void setDirtyProp(bool flag) { _doDirtyProp = flag ; }
   bool dirtyProp() const { return _doDirtyProp ; }
 
@@ -142,8 +133,6 @@ class RooAbsDataStore : public TNamed, public RooPrintable {
   virtual void loadValues(const RooAbsDataStore *tds, const RooFormulaVar* select=nullptr, const char* rangeName=nullptr,
       std::size_t nStart=0, std::size_t nStop = std::numeric_limits<std::size_t>::max()) = 0 ;
 
-  virtual void forceCacheUpdate() {} ;
-
  protected:
 
   RooArgSet _vars;

roofit/roofitcore/inc/RooCompositeDataStore.h

@@ -89,20 +89,9 @@ class RooCompositeDataStore : public RooAbsDataStore {
   void attachBuffers(const RooArgSet& extObs) override ;
   void resetBuffers() override ;
 
-  // Constant term  optimizer interface
-  void cacheArgs(const RooAbsArg* owner, RooArgSet& varSet, const RooArgSet* nset=nullptr, bool skipZeroWeights=false) override ;
-  const RooAbsArg* cacheOwner() override { return nullptr ; }
-  void setArgStatus(const RooArgSet& set, bool active) override ;
-  void resetCache() override ;
-
-  void recalculateCache(const RooArgSet* /*proj*/, Int_t /*firstEvent*/, Int_t /*lastEvent*/, Int_t /*stepSize*/, bool /*skipZeroWeights*/) override ;
-  bool hasFilledCache() const override ;
-
   void loadValues(const RooAbsDataStore *tds, const RooFormulaVar* select=nullptr, const char* rangeName=nullptr,
       std::size_t nStart=0, std::size_t nStop = std::numeric_limits<std::size_t>::max()) override;
 
-  void forceCacheUpdate() override ;
-
   RooAbsData::RealSpans getBatches(std::size_t first, std::size_t len) const override {
     //TODO
     std::cerr << "This functionality is not yet implemented for composite data stores." << std::endl;
@@ -115,8 +104,6 @@ class RooCompositeDataStore : public RooAbsDataStore {
 
  protected:
 
-  void attachCache(const RooAbsArg* newOwner, const RooArgSet& cachedVars) override ;
-
   std::map<Int_t,RooAbsDataStore*> _dataMap ;
   RooCategory* _indexCat = nullptr;
   mutable RooAbsDataStore* _curStore = nullptr; ///<! Datastore associated with current event

roofit/roofitcore/inc/RooEvaluatorWrapper.h

@@ -58,9 +58,6 @@ class RooEvaluatorWrapper final : public RooAbsReal {
       _evaluator->print(os);
    }
 
-   /// The RooFit::Evaluator is dealing with constant terms itself.
-   void constOptimizeTestStatistic(ConstOpCode /*opcode*/, bool /*doAlsoTrackingOpt*/) override {}
-
    bool hasGradient() const override;
    bool hasHessian() const override;
 

roofit/roofitcore/inc/RooFit/TestStatistics/LikelihoodWrapper.h

@@ -94,7 +94,6 @@ class LikelihoodWrapper {
    virtual void updateMinuitExternalParameterValues(const std::vector<double> &minuit_external_x);
 
    // The following functions are necessary from MinuitFcnGrad to reach likelihood properties:
-   void constOptimizeTestStatistic(RooAbsArg::ConstOpCode opcode, bool doAlsoTrackingOpt);
    double defaultErrorLevel() const;
    virtual std::string GetName() const;
    virtual std::string GetTitle() const;