[Math] Make building old C++ Minuit implementation optional

.github/workflows/root-ci.yml

@@ -402,7 +402,7 @@ jobs:
           - image: alma9
             is_special: true
             property: modules_off
-            overrides: ["runtime_cxxmodules=Off"]
+            overrides: ["runtime_cxxmodules=OFF", "minuit1=OFF"]
           - image: alma9
             is_special: true
             property: march_native

cmake/modules/RootBuildOptions.cmake

@@ -146,6 +146,7 @@ ROOT_BUILD_OPTION(llvm13_broken_tests OFF "Enable broken tests with LLVM 13 on W
 ROOT_BUILD_OPTION(macos_native OFF "Disable looking for libraries, includes and binaries in locations other than a native installation (MacOS only)")
 ROOT_BUILD_OPTION(mathmore OFF "Build libMathMore extended math library (requires GSL) [GPL]")
 ROOT_BUILD_OPTION(memory_termination OFF "Free internal ROOT memory before process termination (experimental, used for leak checking)")
+ROOT_BUILD_OPTION(minuit1 ON "Build also the original C++ implementation of Minuit, alongside Minuit 2. This flag has no effect on Minuit 2, which is always built.")
 ROOT_BUILD_OPTION(minuit2_mpi OFF "Enable support for MPI in Minuit2")
 ROOT_BUILD_OPTION(minuit2_omp OFF "Enable support for OpenMP in Minuit2")
 ROOT_BUILD_OPTION(mpi OFF "Enable support for Message Passing Interface (MPI)")

math/CMakeLists.txt

@@ -9,7 +9,9 @@ if(mathmore)
   add_subdirectory(mathmore)
 endif()
 add_subdirectory(matrix)
-add_subdirectory(minuit)
+if(minuit1)
+  add_subdirectory(minuit)
+endif()
 add_subdirectory(minuit2)
 add_subdirectory(fumili)
 add_subdirectory(physics)

roofit/roofitcore/CMakeLists.txt

@@ -492,6 +492,10 @@ if(fftw3)
   target_compile_definitions(RooFitCore PUBLIC ROOFIT_MATH_FFTW3)
 endif()
 
+if(minuit1)
+  target_compile_definitions(RooFitCore PUBLIC ROOFIT_MINUIT_1)
+endif()
+
 target_include_directories(RooFitCore PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/res>)
 
 # The SMatrix package is header-only, but it's only exposed via the Smatrix

roofit/roofitcore/inc/RooProdPdf.h

@@ -102,6 +102,10 @@ class RooProdPdf : public RooAbsPdf {
 
   std::unique_ptr<RooAbsArg> compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileContext & ctx) const override;
 
+  std::unique_ptr<RooAbsArg> compileToFixedProdPdf(RooArgSet const &normSet, RooArgSet const *intSet,
+                                                   const char *rangeName, RooFit::Detail::CompileContext &ctx,
+                                                   bool compileServers) const;
+
   // The cache object. Internal, do not use.
   class CacheElem final : public RooAbsCacheElement {
   public:
@@ -211,7 +215,8 @@ namespace RooFit::Detail {
 /// normalization set.
 class RooFixedProdPdf : public RooAbsPdf {
 public:
-   RooFixedProdPdf(std::unique_ptr<RooProdPdf> &&prodPdf, RooArgSet const &normSet);
+   RooFixedProdPdf(std::unique_ptr<RooProdPdf> &&prodPdf, RooArgSet const &normSet, RooArgSet const *intSet,
+                   const char *rangeName);
    RooFixedProdPdf(const RooFixedProdPdf &other, const char *name = nullptr);
 
    inline TObject *clone(const char *newname) const override { return new RooFixedProdPdf(*this, newname); }
@@ -269,10 +274,12 @@ class RooFixedProdPdf : public RooAbsPdf {
 private:
    double evaluate() const override;
 
+   RooArgSet _intSet;
    RooArgSet _normSet;
    RooSetProxy _servers;
    std::unique_ptr<RooProdPdf> _prodPdf;
    bool _isRearranged = false;
+   std::string _rangeName;
 
    ClassDefOverride(RooFit::Detail::RooFixedProdPdf, 0);
 };

roofit/roofitcore/src/RooFit/Evaluator.cxx

@@ -112,6 +112,7 @@ struct NodeInfo {
    bool isCategory = false;
    bool hasLogged = false;
    bool computeInGPU = false;
+   bool isValueServer = false; // if this node is a value server to the top node
    std::size_t outputSize = 1;
    std::size_t lastSetValCount = std::numeric_limits<std::size_t>::max();
    int lastCatVal = std::numeric_limits<int>::max();
@@ -207,6 +208,16 @@ Evaluator::Evaluator(const RooAbsReal &absReal, bool useGPU)
       }
    }
 
+   // Figure out which nodes are value servers to the top node
+   _nodes.back().isValueServer = true; // the top node itself
+   for (auto iter = _nodes.rbegin(); iter != _nodes.rend(); ++iter) {
+      if (!iter->isValueServer)
+         continue;
+      for (auto &serverInfo : iter->serverInfos) {
+         serverInfo->isValueServer = true;
+      }
+   }
+
    syncDataTokens();
 
    if (_useGPU) {
@@ -687,7 +698,7 @@ RooArgSet Evaluator::getParameters() const
 {
    RooArgSet parameters;
    for (auto &nodeInfo : _nodes) {
-      if (nodeInfo.absArg->isFundamental()) {
+      if (nodeInfo.isValueServer && nodeInfo.absArg->isFundamental()) {
          parameters.add(*nodeInfo.absArg);
       }
    }

roofit/roofitcore/src/RooProdPdf.cxx

@@ -2147,34 +2147,53 @@ RooProdPdf::compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileC
       }
    }
 
+   return compileToFixedProdPdf(normSet, nullptr, nullptr, ctx, true);
+}
+
+std::unique_ptr<RooAbsArg> RooProdPdf::compileToFixedProdPdf(RooArgSet const &normSet, RooArgSet const *intSet,
+                                                             const char *rangeName, RooFit::Detail::CompileContext &ctx,
+                                                             bool compileServers) const
+{
    std::unique_ptr<RooProdPdf> prodPdfClone{static_cast<RooProdPdf *>(this->Clone())};
    ctx.markAsCompiled(*prodPdfClone);
 
-   for (const auto server : prodPdfClone->servers()) {
-      auto nsetForServer = fillNormSetForServer(normSet, *server);
-      RooArgSet const &nset = nsetForServer ? *nsetForServer : normSet;
+   if (compileServers)
+      for (const auto server : prodPdfClone->servers()) {
+         auto nsetForServer = fillNormSetForServer(normSet, *server);
+         RooArgSet const &nset = nsetForServer ? *nsetForServer : normSet;
 
-      RooArgSet depList;
-      server->getObservables(&nset, depList);
+         RooArgSet depList;
+         server->getObservables(&nset, depList);
 
-      ctx.compileServer(*server, *prodPdfClone, depList);
-   }
+         ctx.compileServer(*server, *prodPdfClone, depList);
+      }
 
-   auto fixedProdPdf = std::make_unique<RooFit::Detail::RooFixedProdPdf>(std::move(prodPdfClone), normSet);
-   ctx.markAsCompiled(*fixedProdPdf);
+   auto fixedProdPdf =
+      std::make_unique<RooFit::Detail::RooFixedProdPdf>(std::move(prodPdfClone), normSet, intSet, rangeName);
+   if (compileServers)
+      ctx.markAsCompiled(*fixedProdPdf);
+   else
+      ctx.compileServers(*fixedProdPdf, normSet);
 
    return fixedProdPdf;
 }
 
 namespace RooFit::Detail {
 
-RooFixedProdPdf::RooFixedProdPdf(std::unique_ptr<RooProdPdf> &&prodPdf, RooArgSet const &normSet)
+RooFixedProdPdf::RooFixedProdPdf(std::unique_ptr<RooProdPdf> &&prodPdf, RooArgSet const &normSet,
+                                 RooArgSet const *intSet, const char *rangeName)
    : RooAbsPdf(prodPdf->GetName(), prodPdf->GetTitle()),
      _normSet{normSet},
      _servers("!servers", "List of servers", this),
-     _prodPdf{std::move(prodPdf)}
+     _prodPdf{std::move(prodPdf)},
+     _rangeName{rangeName ? rangeName : ""}
 {
-   auto cache = _prodPdf->createCacheElem(&_normSet, nullptr);
+   if (intSet) {
+      _intSet.add(*intSet);
+   }
+
+   RooArgSet const *iset = _intSet.empty() ? nullptr : &_intSet;
+   auto cache = _prodPdf->createCacheElem(&_normSet, iset, _rangeName.empty() ? nullptr : _rangeName.c_str());
    _isRearranged = cache->_isRearranged;
 
    // The actual servers for a given normalization set depend on whether the
@@ -2190,25 +2209,27 @@ RooFixedProdPdf::RooFixedProdPdf(std::unique_ptr<RooProdPdf> &&prodPdf, RooArgSe
    // We don't want to carry the full cache object around, so we let it go out
    // of scope and transfer the ownership of the args that we actually need.
    cache->_ownedList.releaseOwnership();
-   std::vector<std::unique_ptr<RooAbsArg>> owned;
-   for (RooAbsArg *arg : cache->_ownedList) {
-      owned.emplace_back(arg);
-   }
+   // std::vector<std::unique_ptr<RooAbsArg>> owned;
+   // for (RooAbsArg *arg : cache->_ownedList) {
+   // owned.emplace_back(arg);
+   //}
    for (RooAbsArg *arg : cache->_partList) {
       _servers.add(*arg);
-      auto found = std::find_if(owned.begin(), owned.end(), [&](auto const &ptr) { return arg == ptr.get(); });
-      if (found != owned.end()) {
-         addOwnedComponents(std::move(owned[std::distance(owned.begin(), found)]));
-      }
+      // auto found = std::find_if(owned.begin(), owned.end(), [&](auto const &ptr) { return arg == ptr.get(); });
+      // if (found != owned.end()) {
+      // addOwnedComponents(std::move(owned[std::distance(owned.begin(), found)]));
+      //}
    }
 }
 
 RooFixedProdPdf::RooFixedProdPdf(const RooFixedProdPdf &other, const char *name)
    : RooAbsPdf(other, name),
+     _intSet{other._intSet},
      _normSet{other._normSet},
      _servers("!servers", this, other._servers),
      _prodPdf{static_cast<RooProdPdf *>(other._prodPdf->Clone())},
-     _isRearranged{other._isRearranged}
+     _isRearranged{other._isRearranged},
+     _rangeName{other._rangeName}
 {
 }
 

roofit/roofitcore/src/RooProjectedPdf.cxx

@@ -286,10 +286,9 @@ RooProjectedPdf::compileForNormSet(RooArgSet const &normSet, RooFit::Detail::Com
 
    auto newArgPdf = std::make_unique<RooWrapperPdf>(namePdf.c_str(), namePdf.c_str(), *newArg);
 
-   ctx.markAsCompiled(*newArg);
-   ctx.markAsCompiled(*newArgPdf);
+   ctx.compileServers(*newArgPdf, normSet);
 
-   newArgPdf->addOwnedComponents(std::move(newArg));
+   ctx.markAsCompiled(*newArgPdf);
 
    return newArgPdf;
 }

roofit/roofitcore/src/RooRealIntegral.cxx

@@ -40,6 +40,7 @@ integration is performed in the various implementations of the RooAbsIntegrator
 #include <RooNameReg.h>
 #include <RooNumIntConfig.h>
 #include <RooNumIntFactory.h>
+#include <RooProdPdf.h>
 #include <RooRealBinding.h>
 #include <RooSuperCategory.h>
 #include <RooTrace.h>
@@ -1114,6 +1115,13 @@ Int_t RooRealIntegral::getCacheAllNumeric()
 std::unique_ptr<RooAbsArg>
 RooRealIntegral::compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileContext &ctx) const
 {
+   if (auto *prodPdf = dynamic_cast<RooProdPdf *>(&*_function)) {
+      getVal();
+      auto out = prodPdf->compileToFixedProdPdf(_funcNormSet ? *_funcNormSet : normSet, &_anaList,
+                                                RooNameReg::str(_rangeName), ctx, false);
+      out->SetName(GetName());
+      return out;
+   }
    return RooAbsReal::compileForNormSet(_funcNormSet ? *_funcNormSet : normSet, ctx);
 }
 

roofit/roofitcore/test/CMakeLists.txt

@@ -90,6 +90,10 @@ if(mathmore)
 endif()
 
 configure_file(stressRooFit_ref.root stressRooFit_ref.root COPYONLY)
+configure_file(stressRooFit_ref_minuit1.root stressRooFit_ref_minuit1.root COPYONLY)
+if(minuit1)
+  ROOT_ADD_TEST(test-stressroofit-cpu-minuit1 COMMAND stressRooFit -minim Minuit -f stressRooFit_ref_minuit1.root -b cpu FAILREGEX "FAILED|Error in")
+endif()
 if(roofit_legacy_eval_backend)
   ROOT_ADD_TEST(test-stressroofit-legacy COMMAND stressRooFit -b legacy FAILREGEX "FAILED|Error in")
 endif()

roofit/roofitcore/test/stressRooFit.cxx

@@ -255,7 +255,7 @@ int main(int argc, const char *argv[])
 
    // string refFileName = "http://root.cern.ch/files/stressRooFit_v534_ref.root" ;
    string refFileName = "stressRooFit_ref.root";
-   string minimizerName = "Minuit";
+   string minimizerName = "Minuit2";
 
    auto verbosityOptionErrorMsg = "Multiple verbosity-related options have been passed to stressRooFit! The options "
                                   "-v, -vv, and -q are mutually exclusive.";

roofit/roofitcore/test/testSumW2Error.cxx

@@ -49,55 +49,38 @@ TEST(SumW2Error, BatchMode)
 
    using namespace RooFit;
 
-   auto fit = [&](RooAbsData &data, bool sumw2, EvalBackend evalBackend, std::string const &minimizer,
-                  int printLevel = -1) {
+   auto fit = [&](RooAbsData &data, bool sumw2, EvalBackend evalBackend, int printLevel = -1) {
       params.assign(initialParams);
 
-      return std::unique_ptr<RooFitResult>{model.fitTo(data, Save(), SumW2Error(sumw2), Strategy(1), evalBackend,
-                                                       Minimizer(minimizer.c_str()), PrintLevel(printLevel))};
+      return std::unique_ptr<RooFitResult>{
+         model.fitTo(data, Save(), SumW2Error(sumw2), Strategy(1), evalBackend, PrintLevel(printLevel))};
    };
 
    auto scalar = EvalBackend::Legacy();
    auto batchMode = EvalBackend::Cpu();
 
    // Compare batch mode vs. scalar mode for non-SumW2 fits on UNWEIGHTED datasets
-   EXPECT_TRUE(fit(*dataSet, 0, scalar, "Minuit")->isIdentical(*fit(*dataSet, 0, batchMode, "Minuit")))
-      << " different results for Minuit fit to RooDataSet without SumW2Error correction.";
-   EXPECT_TRUE(fit(*dataHist, 0, scalar, "Minuit")->isIdentical(*fit(*dataHist, 0, batchMode, "Minuit")))
-      << " different results for Minuit fit to RooDataHist without SumW2Error correction.";
-   EXPECT_TRUE(fit(*dataSet, 0, scalar, "Minuit2")->isIdentical(*fit(*dataSet, 0, batchMode, "Minuit2")))
-      << " different results for Minuit2 fit to RooDataSet without SumW2Error correction.";
-   EXPECT_TRUE(fit(*dataHist, 0, scalar, "Minuit2")->isIdentical(*fit(*dataHist, 0, batchMode, "Minuit2")))
-      << " different results for Minuit2 fit to RooDataHist without SumW2Error correction.";
+   EXPECT_TRUE(fit(*dataSet, 0, scalar)->isIdentical(*fit(*dataSet, 0, batchMode)))
+      << " different results for fit to RooDataSet without SumW2Error correction.";
+   EXPECT_TRUE(fit(*dataHist, 0, scalar)->isIdentical(*fit(*dataHist, 0, batchMode)))
+      << " different results for fit to RooDataHist without SumW2Error correction.";
 
    // We can't compare the covariance matrix in these next cases, because it is
    // externally provided. Still, it's okay because the parameter values and
    // errors are compared, where the errors are inferred from the external
    // covariance matrix.
 
    // Compare batch mode vs. scalar mode for SumW2 fits on UNWEIGHTED datasets
-   EXPECT_TRUE(fit(*dataSet, 1, scalar, "Minuit")->isIdenticalNoCov(*fit(*dataSet, 1, batchMode, "Minuit")))
-      << " different results for Minuit fit to RooDataSet with SumW2Error correction.";
-   EXPECT_TRUE(fit(*dataHist, 1, scalar, "Minuit")->isIdenticalNoCov(*fit(*dataHist, 1, batchMode, "Minuit")))
-      << " different results for Minuit fit to RooDataHist with SumW2Error correction.";
-   EXPECT_TRUE(fit(*dataSet, 1, scalar, "Minuit2")->isIdenticalNoCov(*fit(*dataSet, 1, batchMode, "Minuit2")))
-      << " different results for Minuit2 fit to RooDataSet with SumW2Error correction.";
-   EXPECT_TRUE(fit(*dataHist, 1, scalar, "Minuit2")->isIdenticalNoCov(*fit(*dataHist, 1, batchMode, "Minuit2")))
-      << " different results for Minuit2 fit to RooDataHist with SumW2Error correction.";
+   EXPECT_TRUE(fit(*dataSet, 1, scalar)->isIdenticalNoCov(*fit(*dataSet, 1, batchMode)))
+      << " different results for fit to RooDataSet with SumW2Error correction.";
+   EXPECT_TRUE(fit(*dataHist, 1, scalar)->isIdenticalNoCov(*fit(*dataHist, 1, batchMode)))
+      << " different results for fit to RooDataHist with SumW2Error correction.";
 
    // Compare batch mode vs. scalar mode for SumW2 fits on WEIGHTED datasets
-   EXPECT_TRUE(
-      fit(dataSetWeighted, 1, scalar, "Minuit")->isIdenticalNoCov(*fit(dataSetWeighted, 1, batchMode, "Minuit")))
-      << " different results for Minuit fit to weighted RooDataSet with SumW2Error correction.";
-   EXPECT_TRUE(
-      fit(*dataHistWeighted, 1, scalar, "Minuit")->isIdenticalNoCov(*fit(*dataHistWeighted, 1, batchMode, "Minuit")))
-      << " different results for Minuit fit to weighted RooDataHist with SumW2Error correction.";
-   EXPECT_TRUE(
-      fit(dataSetWeighted, 1, scalar, "Minuit2")->isIdenticalNoCov(*fit(dataSetWeighted, 1, batchMode, "Minuit2")))
-      << " different results for Minuit2 fit to weighted RooDataSet with SumW2Error correction.";
-   EXPECT_TRUE(
-      fit(*dataHistWeighted, 1, scalar, "Minuit2")->isIdenticalNoCov(*fit(*dataHistWeighted, 1, batchMode, "Minuit2")))
-      << " different results for Minuit2 fit to weighted RooDataHist with SumW2Error correction.";
+   EXPECT_TRUE(fit(dataSetWeighted, 1, scalar)->isIdenticalNoCov(*fit(dataSetWeighted, 1, batchMode)))
+      << " different results for fit to weighted RooDataSet with SumW2Error correction.";
+   EXPECT_TRUE(fit(*dataHistWeighted, 1, scalar)->isIdenticalNoCov(*fit(*dataHistWeighted, 1, batchMode)))
+      << " different results for fit to weighted RooDataHist with SumW2Error correction.";
 }
 
 TEST(SumW2Error, ExtendedFit)
@@ -128,11 +111,8 @@ TEST(SumW2Error, ExtendedFit)
    auto *wFunc = ws.factory("w[1.5]");
 
    auto *w = dataNoWeights->addColumn(*wFunc);
-   RooDataSet data{dataNoWeights->GetName(),
-                   dataNoWeights->GetTitle(),
-                   *dataNoWeights->get(),
-                   RooFit::Import(*dataNoWeights),
-                   RooFit::WeightVar(w->GetName())};
+   RooDataSet data{dataNoWeights->GetName(), dataNoWeights->GetTitle(), *dataNoWeights->get(),
+                   RooFit::Import(*dataNoWeights), RooFit::WeightVar(w->GetName())};
    RooDataHist datahist{"datahist", "datahist", *data.get(), data};
 
    RooArgSet params;
@@ -143,9 +123,8 @@ TEST(SumW2Error, ExtendedFit)
 
    auto doFit = [&](RooFit::EvalBackend evalBackend, bool sumW2Error, const char *range) {
       params.assign(initialParams);
-      return std::unique_ptr<RooFitResult>{shp->fitTo(datahist, Extended(), Range(range), Save(),
-                                                      SumW2Error(sumW2Error), Strategy(1), PrintLevel(-1), evalBackend,
-                                                      Minimizer("Minuit2", "migrad"))};
+      return std::unique_ptr<RooFitResult>{shp->fitTo(
+         datahist, Extended(), Range(range), Save(), SumW2Error(sumW2Error), Strategy(1), PrintLevel(-1), evalBackend)};
    };
 
    // compare batch mode and scalar mode fit results for full range

roofit/roostats/CMakeLists.txt

@@ -9,6 +9,10 @@
 # @author Pere Mato, CERN
 ############################################################################
 
+if(minuit1)
+  list(APPEND EXTRA_LIBRARIES Minuit)
+endif()
+
 set (EXTRA_DICT_OPTS)
 if (runtime_cxxmodules AND WIN32)
   set (EXTRA_DICT_OPTS NO_CXXMODULE)
@@ -121,6 +125,8 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooStats
     src/UpperLimitMCSModule.cxx
   DICTIONARY_OPTIONS
     "-writeEmptyRootPCM"
+  LIBRARIES
+    ${EXTRA_LIBRARIES}
   DEPENDENCIES
     Core
     RooFit
@@ -130,7 +136,6 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooStats
     Hist
     Matrix
     MathCore
-    Minuit
     Foam
     Graf
     Gpad