[v634][RF] Backport recent RooFit fixes to the release branch

bindings/pyroot/pythonizations/test/CMakeLists.txt

@@ -138,24 +138,21 @@ endif()
 ROOT_ADD_PYUNITTEST(pyroot_pyz_tf_pycallables tf_pycallables.py)
 
 if(roofit)
-  # RooAbsCollection and subclasses pythonizations
-  if(NOT MSVC OR CMAKE_SIZEOF_VOID_P EQUAL 4 OR win_broken_tests)
-    ROOT_ADD_PYUNITTEST(pyroot_roofit_rooabscollection roofit/rooabscollection.py)
-  endif()
-  ROOT_ADD_PYUNITTEST(pyroot_roofit_rooarglist roofit/rooarglist.py)
-
-  # RooDataHist pythonisations
-  ROOT_ADD_PYUNITTEST(pyroot_roofit_roodatahist_ploton roofit/roodatahist_ploton.py)
 
-  # RooDataSet pythonisations
-  ROOT_ADD_PYUNITTEST(pyroot_roofit_roodataset roofit/roodataset.py)
-
-  # RooWorkspace pythonizations
   ROOT_ADD_PYUNITTEST(pyroot_roofit_rooabspdf_fitto roofit/rooabspdf_fitto.py)
   ROOT_ADD_PYUNITTEST(pyroot_roofit_rooabsreal_ploton roofit/rooabsreal_ploton.py)
-
+  ROOT_ADD_PYUNITTEST(pyroot_roofit_rooarglist roofit/rooarglist.py)
+  ROOT_ADD_PYUNITTEST(pyroot_roofit_roocmdarg roofit/roocmdarg.py)
+  ROOT_ADD_PYUNITTEST(pyroot_roofit_roodatahist_numpy roofit/roodatahist_numpy.py PYTHON_DEPS numpy)
+  ROOT_ADD_PYUNITTEST(pyroot_roofit_roodatahist_ploton roofit/roodatahist_ploton.py)
+  ROOT_ADD_PYUNITTEST(pyroot_roofit_roodataset roofit/roodataset.py)
+  ROOT_ADD_PYUNITTEST(pyroot_roofit_roodataset_numpy roofit/roodataset_numpy.py PYTHON_DEPS numpy)
   ROOT_ADD_PYUNITTEST(pyroot_roofit_roolinkedlist roofit/roolinkedlist.py)
 
+  if(NOT MSVC OR CMAKE_SIZEOF_VOID_P EQUAL 4 OR win_broken_tests)
+    ROOT_ADD_PYUNITTEST(pyroot_roofit_rooabscollection roofit/rooabscollection.py)
+  endif()
+
   if(NOT MSVC OR win_broken_tests)
     # Test pythonizations for the RooFitHS3 package, which is not built on Windows.
     ROOT_ADD_PYUNITTEST(pyroot_roofit_roojsonfactorywstool roofit/roojsonfactorywstool.py)
@@ -168,10 +165,6 @@ if(roofit)
     ROOT_ADD_PYUNITTEST(pyroot_roofit_rooworkspace roofit/rooworkspace.py)
   endif()
 
-  # NumPy compatibility
-  ROOT_ADD_PYUNITTEST(pyroot_roofit_roodataset_numpy roofit/roodataset_numpy.py PYTHON_DEPS numpy)
-  ROOT_ADD_PYUNITTEST(pyroot_roofit_roodatahist_numpy roofit/roodatahist_numpy.py PYTHON_DEPS numpy)
-
 endif()
 
 if (dataframe)

bindings/pyroot/pythonizations/test/roofit/roocmdarg.py

@@ -0,0 +1,83 @@
+import unittest
+
+import ROOT
+
+# Necessary inside the "eval" call
+RooArgSet = ROOT.RooArgSet
+RooCmdArg = ROOT.RooCmdArg
+
+x = ROOT.RooRealVar("x", "x", 1.0)
+y = ROOT.RooRealVar("y", "y", 2.0)
+z = ROOT.RooRealVar("z", "z", 3.0)
+
+
+def args_equal(arg_1, arg_2):
+    same = True
+
+    same &= str(arg_1.GetName()) == str(arg_2.GetName())
+    same &= str(arg_1.GetTitle()) == str(arg_2.GetTitle())
+
+    for i in range(2):
+        same &= arg_1.getInt(i) == arg_2.getInt(i)
+
+    for i in range(2):
+        same &= arg_1.getDouble(i) == arg_2.getDouble(i)
+
+    for i in range(3):
+        same &= str(arg_1.getString(i)) == str(arg_2.getString(i))
+
+    same &= arg_1.procSubArgs() == arg_2.procSubArgs()
+    same &= arg_1.prefixSubArgs() == arg_2.prefixSubArgs()
+
+    for i in range(2):
+        same &= arg_1.getObject(i) == arg_2.getObject(i)
+
+    def set_equal(set_1, set_2):
+        if set_1 == ROOT.nullptr and set_2 == ROOT.nullptr:
+            return True
+        if set_1 == ROOT.nullptr and set_2 != ROOT.nullptr:
+            return False
+        if set_1 != ROOT.nullptr and set_2 == ROOT.nullptr:
+            return False
+
+        if set_1.size() != set_2.size():
+            return False
+
+        return set_2.hasSameLayout(set_1)
+
+    for i in range(2):
+        same &= set_equal(arg_1.getSet(i), arg_2.getSet(i))
+
+    return same
+
+
+class TestRooArgList(unittest.TestCase):
+    """
+    Test for RooCmdArg pythonizations.
+    """
+
+    def test_constructor_eval(self):
+
+        set_1 = ROOT.RooArgSet(x, y)
+        set_2 = ROOT.RooArgSet(y, z)
+
+        def do_test(*args):
+            arg_1 = ROOT.RooCmdArg(*args)
+
+            # The arg should be able to recreate itself by emitting the right
+            # constructor code:
+            arg_2 = eval(arg_1.constructorCode())
+
+            self.assertTrue(args_equal(arg_1, arg_2))
+
+        nullp = ROOT.nullptr
+
+        # only fill the non-object fields:
+        do_test("Test", -1, 3, 4.2, 4.7, "hello", "world", nullp, nullp, nullp, "s3", nullp, nullp)
+
+        # RooArgSet tests:
+        do_test("Test", -1, 3, 4.2, 4.7, "hello", "world", nullp, nullp, nullp, "s3", set_1, set_2)
+
+
+if __name__ == "__main__":
+    unittest.main()

roofit/histfactory/inc/RooStats/HistFactory/FlexibleInterpVar.h

@@ -71,6 +71,10 @@ namespace HistFactory{
 
     double evaluate() const override;
 
+  private:
+
+    void setInterpCodeForParam(int iParam, int code);
+
     ClassDefOverride(RooStats::HistFactory::FlexibleInterpVar,2); // flexible interpolation
   };
 }

roofit/histfactory/inc/RooStats/HistFactory/PiecewiseInterpolation.h

@@ -61,7 +61,7 @@ class PiecewiseInterpolation : public RooAbsReal {
   void setPositiveDefinite(bool flag=true){_positiveDefinite=flag;}
   bool positiveDefinite() const {return _positiveDefinite;}
 
-  void setInterpCode(RooAbsReal& param, int code, bool silent=false);
+  void setInterpCode(RooAbsReal& param, int code, bool silent=true);
   void setAllInterpCodes(int code);
   void printAllInterpCodes();
 
@@ -102,6 +102,10 @@ class PiecewiseInterpolation : public RooAbsReal {
   double evaluate() const override;
   void doEval(RooFit::EvalContext &) const override;
 
+private:
+
+  void setInterpCodeForParam(int iParam, int code);
+
   ClassDefOverride(PiecewiseInterpolation,4) // Sum of RooAbsReal objects
 };
 

roofit/histfactory/src/FlexibleInterpVar.cxx

@@ -54,10 +54,10 @@ FlexibleInterpVar::FlexibleInterpVar(const char* name, const char* title,
 FlexibleInterpVar::FlexibleInterpVar(const char* name, const char* title,
                  const RooArgList& paramList,
                  double argNominal, std::vector<double> const& lowVec, std::vector<double> const& highVec,
-                 std::vector<int> const& code) :
+                 std::vector<int> const& codes) :
   RooAbsReal(name, title),
   _paramList("paramList","List of paramficients",this),
-  _nominal(argNominal), _low(lowVec), _high(highVec), _interpCode(code)
+  _nominal(argNominal), _low(lowVec), _high(highVec)
 {
   for (auto param : paramList) {
     if (!dynamic_cast<RooAbsReal*>(param)) {
@@ -69,6 +69,11 @@ FlexibleInterpVar::FlexibleInterpVar(const char* name, const char* title,
     _paramList.add(*param) ;
   }
 
+  _interpCode.resize(_paramList.size());
+  for (std::size_t i = 0; i < codes.size(); ++i) {
+    setInterpCodeForParam(i, codes[i]);
+  }
+
   if (_low.size() != _paramList.size() || _low.size() != _high.size() || _low.size() != _interpCode.size()) {
      coutE(InputArguments) << "FlexibleInterpVar::ctor(" << GetName() << ") invalid input std::vectors " << std::endl;
      R__ASSERT(_low.size() == _paramList.size());
@@ -109,31 +114,43 @@ FlexibleInterpVar::~FlexibleInterpVar()
   TRACE_DESTROY;
 }
 
-
-////////////////////////////////////////////////////////////////////////////////
-
-void FlexibleInterpVar::setInterpCode(RooAbsReal& param, int code){
-  int index = _paramList.index(&param);
-  if(index<0){
-      coutE(InputArguments) << "FlexibleInterpVar::setInterpCode ERROR:  " << param.GetName()
-                            << " is not in list" << std::endl;
-  } else if(_interpCode.at(index) != code){
-      coutI(InputArguments) << "FlexibleInterpVar::setInterpCode :  " << param.GetName()
-                            << " is now " << code << std::endl;
-      _interpCode.at(index) = code;
-      // GHL: Adding suggestion by Swagato:
-      setValueDirty();
-  }
+void FlexibleInterpVar::setInterpCode(RooAbsReal &param, int code)
+{
+   int index = _paramList.index(&param);
+   if (index < 0) {
+      coutE(InputArguments) << "FlexibleInterpVar::setInterpCode ERROR:  " << param.GetName() << " is not in list"
+                            << std::endl;
+      return;
+   }
+   setInterpCodeForParam(index, code);
 }
 
-////////////////////////////////////////////////////////////////////////////////
+void FlexibleInterpVar::setAllInterpCodes(int code)
+{
+   for (std::size_t i = 0; i < _interpCode.size(); ++i) {
+      setInterpCodeForParam(i, code);
+   }
+}
 
-void FlexibleInterpVar::setAllInterpCodes(int code){
-  for(unsigned int i=0; i<_interpCode.size(); ++i){
-    _interpCode.at(i) = code;
-  }
-  // GHL: Adding suggestion by Swagato:
-  setValueDirty();
+void FlexibleInterpVar::setInterpCodeForParam(int iParam, int code)
+{
+   RooAbsArg const &param = _paramList[iParam];
+   if (code < 0 || code > 5) {
+      coutE(InputArguments) << "FlexibleInterpVar::setInterpCode ERROR: " << param.GetName()
+                            << " with unknown interpolation code " << code << ", keeping current code "
+                            << _interpCode[iParam] << std::endl;
+      return;
+   }
+   if (code == 3) {
+      // In the past, code 3 was equivalent to code 2, which confused users.
+      // Now, we just say that code 3 doesn't exist and default to code 2 in
+      // that case for backwards compatible behavior.
+      coutE(InputArguments) << "FlexibleInterpVar::setInterpCode ERROR: " << param.GetName()
+                            << " with unknown interpolation code " << code << ", defaulting to code 2" << std::endl;
+      code = 2;
+   }
+   _interpCode.at(iParam) = code;
+   setValueDirty();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -198,10 +215,6 @@ double FlexibleInterpVar::evaluate() const
    double total(_nominal);
    for (std::size_t i = 0; i < _paramList.size(); ++i) {
       int code = _interpCode[i];
-      if (code < 0 || code > 4) {
-         coutE(InputArguments) << "FlexibleInterpVar::evaluate ERROR:  param " << i
-                               << " with unknown interpolation code" << std::endl;
-      }
       // To get consistent codes with the PiecewiseInterpolation
       if (code == 4) {
          code = 5;
@@ -223,10 +236,6 @@ void FlexibleInterpVar::translate(RooFit::Detail::CodeSquashContext &ctx) const
    unsigned int n = _interpCode.size();
 
    int interpCode = _interpCode[0];
-   if (interpCode < 0 || interpCode > 4) {
-      coutE(InputArguments) << "FlexibleInterpVar::evaluate ERROR:  param " << 0
-                            << " with unknown interpolation code" << std::endl;
-   }
    // To get consistent codes with the PiecewiseInterpolation
    if (interpCode == 4) {
       interpCode = 5;
@@ -251,10 +260,6 @@ void FlexibleInterpVar::doEval(RooFit::EvalContext &ctx) const
 
    for (std::size_t i = 0; i < _paramList.size(); ++i) {
       int code = _interpCode[i];
-      if (code < 0 || code > 4) {
-         coutE(InputArguments) << "FlexibleInterpVar::evaluate ERROR:  param " << i
-                               << " with unknown interpolation code" << std::endl;
-      }
       // To get consistent codes with the PiecewiseInterpolation
       if (code == 4) {
          code = 5;

roofit/histfactory/src/HistoToWorkspaceFactoryFast.cxx

@@ -603,7 +603,7 @@ RooArgList HistoToWorkspaceFactoryFast::createObservables(const TH1 *hist, RooWo
        assert(lowVec.size() == params.size());
 
        FlexibleInterpVar interp( (interpName).c_str(), "", params, 1., lowVec, highVec);
-       interp.setAllInterpCodes(4); // LM: change to 4 (piece-wise linear to 6th order polynomial interpolation + linear extrapolation )
+       interp.setAllInterpCodes(4); // LM: change to 4 (piece-wise exponential to 6th order polynomial interpolation + exponential extrapolation )
        //interp.setAllInterpCodes(0); // simple linear interpolation
        proto.import(interp); // params have already been imported in first loop of this function
     } else{