[RF] Don't deep clone the RooAbsArg in RooAbsReal::getPropagatedError

The `RooAbsReal::getPropagatedError()` function was using some of the
most expensive operations in RooFit for larger computation graphs:
cloning the model, and figuring out parameters and observables.

This was done for no apparent reason, as the `RooAbsReal` is not mutated
by `getPropagaterError`. Parameter values are slightly changed for
reevaluation, but they are reset right after. A final call to `getVal()`
is enough to reset the original state, which is much more efficient than
cloning everything.

This commit also adds a check if the parameters in the RooAbsReal have
the same values as in the fit result (otherwise the logic of
getPropagatedError() is broken).

This change was motivated by the following forum post:
https://root-forum.cern.ch/t/getpropagatederror-method-taking-too-long-to-run/50392

Author: guitargeek

roofit/roofitcore/src/RooAbsReal.cxx

@@ -2658,68 +2658,65 @@ RooPlot* RooAbsReal::plotAsymOn(RooPlot *frame, const RooAbsCategoryLValue& asym
 /// \f$ \mathrm{Cov}(mathbf{a},mathbf{a}') \f$ = the covariance matrix from the fit result.
 ///
 
-double RooAbsReal::getPropagatedError(const RooFitResult &fr, const RooArgSet &nset_in) const
+double RooAbsReal::getPropagatedError(const RooFitResult &fr, const RooArgSet &nset) const
 {
+  // Calling getParameters() might be costly, but necessary to get the right
+  // parameters in the RooAbsReal. The RooFitResult only stores snapshots.
+  RooArgSet allParamsInAbsReal;
+  getParameters(&nset, allParamsInAbsReal);
+
+  // Strip out parameters with zero error
+  RooArgList paramList;
+  for(auto * rrvInFitResult : static_range_cast<RooRealVar*>(fr.floatParsFinal())) {
+     if (rrvInFitResult->getError() > 1e-20) {
+        auto * rrvInAbsReal = static_cast<RooRealVar*>(allParamsInAbsReal.find(*rrvInFitResult));
+
+        if(rrvInAbsReal->getVal() != rrvInFitResult->getVal()) {
+           throw std::runtime_error(
+                   std::string("RooAbsReal::getPropagatedError(): the parameters of the RooAbsReal don't have") +
+                   "the same values as in the fit result! The logic of getPropagatedError is broken in this case.");
+        }
 
-   // Strip out parameters with zero error
-   RooArgList fpf_stripped;
-   RooFIter fi = fr.floatParsFinal().fwdIterator();
-   RooRealVar *frv;
-   while ((frv = (RooRealVar *)fi.next())) {
-      if (frv->getError() > 1e-20) {
-         fpf_stripped.add(*frv);
-      }
-   }
-
-   // Clone self for internal use
-   std::unique_ptr<RooAbsReal> cloneFunc{static_cast<RooAbsReal*>(cloneTree())};
-   RooArgSet errorParams;
-   cloneFunc->getObservables(&fpf_stripped, errorParams);
-
-   RooArgSet nset;
-   if (nset_in.empty()) {
-     cloneFunc->getParameters(&errorParams, nset);
-   } else {
-     cloneFunc->getObservables(&nset_in, nset);
-   }
-
-   // Make list of parameter instances of cloneFunc in order of error matrix
-   RooArgList paramList;
-   const RooArgList &fpf = fpf_stripped;
-   vector<int> fpf_idx;
-   for (Int_t i = 0; i < fpf.getSize(); i++) {
-      RooAbsArg *par = errorParams.find(fpf[i].GetName());
-      if (par) {
-         paramList.add(*par);
-         fpf_idx.push_back(i);
-      }
+        paramList.add(*rrvInAbsReal);
+     }
   }
 
-  vector<double> plusVar, minusVar ;
+  std::vector<double> plusVar, minusVar ;
+  plusVar.reserve(paramList.size());
+  minusVar.reserve(paramList.size());
 
   // Create vector of plus,minus variations for each parameter
-  TMatrixDSym V(paramList.getSize()==fr.floatParsFinal().getSize()?
-      fr.covarianceMatrix():
+  TMatrixDSym V(paramList.size() == fr.floatParsFinal().size() ?
+      fr.covarianceMatrix() :
       fr.reducedCovarianceMatrix(paramList)) ;
 
   for (Int_t ivar=0 ; ivar<paramList.getSize() ; ivar++) {
 
-    RooRealVar& rrv = (RooRealVar&)fpf[fpf_idx[ivar]] ;
+    auto& rrv = static_cast<RooRealVar&>(paramList[ivar]);
 
     double cenVal = rrv.getVal() ;
     double errVal = sqrt(V(ivar,ivar)) ;
 
     // Make Plus variation
-    ((RooRealVar*)paramList.at(ivar))->setVal(cenVal+errVal) ;
-    plusVar.push_back(cloneFunc->getVal(nset)) ;
+    rrv.setVal(cenVal+errVal) ;
+    plusVar.push_back(getVal(nset)) ;
 
     // Make Minus variation
-    ((RooRealVar*)paramList.at(ivar))->setVal(cenVal-errVal) ;
-    minusVar.push_back(cloneFunc->getVal(nset)) ;
+    rrv.setVal(cenVal-errVal) ;
+    minusVar.push_back(getVal(nset)) ;
 
-    ((RooRealVar*)paramList.at(ivar))->setVal(cenVal) ;
+    rrv.setVal(cenVal) ;
   }
 
+  // Re-evaluate this RooAbsReal with the central parameters just to be
+  // extra-safe that a call to `getPropagatedError()` doesn't change any state.
+  // It should not be necessarry because thanks to the dirty flag propagation
+  // the RooAbsReal is re-evaluated anyway the next time getVal() is called.
+  // Still there are imaginable corner cases where it would not be triggered,
+  // for example if the user changes the RooFit operation more after the error
+  // propagation.
+  getVal(nset);
+
   TMatrixDSym C(paramList.getSize()) ;
   vector<double> errVec(paramList.getSize()) ;
   for (int i=0 ; i<paramList.getSize() ; i++) {