[RF] Remove flattening of nested RooSimultaneous in the constructor

README/ReleaseNotes/v644/index.md

@@ -20,6 +20,7 @@ The following people have contributed to this new version:
 * A `RooArgSet` cannot be constructed with an implicit conversion from Python `set` anymore. The problem is that Python sets are unordered while RooArgSets are ordered, and this mismatch can lead to subtle problems later on. Prefer conversion from Python lists or tuples, which are ordered too.
 * The `RooFit::BatchMode` command argument has been removed, use `RooFit::EvalBackend` instead.
 * The `RooTrace` class was untested and is now removed. For memory debugging, please use general-purpose tools such as AddressSanitizer or Valgrind instead.
+* Flattening nested **RooSimultaneous** in the constructor, deprecated in ROOT 6.42, is removed. RooSimultaneous components are now kept as they are, which also allows adding them with `RooSimultaneous::addPdf()`. They are only supported if their index category is a parameter ("switch mode"), which now also works for toy generation and for plotting slices of the outer RooSimultaneous. If the index category of a nested RooSimultaneous is in the dataset, creating the likelihood fails with a clear error for all evaluation backends. To combine channels, use one RooSimultaneous over a **RooCategory** with one state per channel, or over a **RooSuperCategory** if the channels span the full product of several categories. RooSimultaneous objects that were flattened by previous ROOT versions and are read from files are not affected. The internal `RooSimultaneous::InitializationOutput` struct and `RooSimultaneous::initialize()` function are removed as well.
 * The PyMVA library and the associated `tmva-pymva` build option that were deprecated in ROOT 6.42 have been removed
 
 ## Python Interface

roofit/roofitcore/inc/RooSimultaneous.h

@@ -35,24 +35,10 @@ class RooFitResult ;
 class RooPlot ;
 class RooAbsData ;
 class RooLinkedList ;
-class RooSuperCategory ;
 
 class RooSimultaneous : public RooAbsPdf {
 public:
 
-  /// Internal struct used for initialization.
-  struct InitializationOutput {
-
-     ~InitializationOutput();
-
-     void addPdf(const RooAbsPdf &pdf, std::string const &catLabel);
-
-     std::vector<RooAbsPdf const *> finalPdfs;
-     std::vector<std::string> finalCatLabels;
-     RooAbsCategoryLValue *indexCat = nullptr;
-     std::unique_ptr<RooSuperCategory> superIndex;
-  };
-
   // Constructors, assignment etc
   inline RooSimultaneous() : _partIntMgr(this,10) {}
   RooSimultaneous(const char *name, const char *title, RooAbsCategoryLValue& indexCat) ;
@@ -141,12 +127,7 @@ class RooSimultaneous : public RooAbsPdf {
 
 private:
 
-  /// Private internal constructor.
-  RooSimultaneous(const char *name, const char *title, InitializationOutput && initInfo);
-
-  static std::unique_ptr<RooSimultaneous::InitializationOutput>
-  initialize(std::string const& name, RooAbsCategoryLValue &inIndexCat,
-             std::map<std::string, RooAbsPdf *> const &pdfMap);
+  static RooAbsPdf *resolveSwitchMode(RooAbsPdf *pdf, RooArgSet const &vars);
 
   mutable std::unique_ptr<RooArgSet> _indexCatSet ; ///<! Index category wrapped in a RooArgSet if needed internally
 

roofit/roofitcore/src/FitHelpers.cxx

@@ -54,12 +54,43 @@
 #endif
 #endif
 
+#include <sstream>
+
 using RooFit::Detail::RooNLLVarNew;
 
 namespace {
 
 constexpr int extendedFitDefault = 2;
 
+/// Nested RooSimultaneous are only supported in "switch mode", i.e., if their
+/// index category is not an observable. Throw an error otherwise.
+void checkNestedSimultaneous(RooAbsArg const &top, RooArgSet const &observables)
+{
+   RooArgList branches;
+   top.branchNodeServerList(&branches);
+   for (RooAbsArg *outer : branches) {
+      if (!dynamic_cast<RooSimultaneous const *>(outer)) {
+         continue;
+      }
+      RooArgList outerBranches;
+      outer->branchNodeServerList(&outerBranches);
+      for (RooAbsArg *arg : outerBranches) {
+         auto *nested = dynamic_cast<RooSimultaneous const *>(arg);
+         if (arg == outer || !nested || !nested->indexCatIsObservable(observables)) {
+            continue;
+         }
+         std::stringstream errMsg;
+         errMsg << "RooSimultaneous \"" << outer->GetName() << "\" contains the RooSimultaneous \"" << nested->GetName()
+                << "\", whose index category \"" << nested->indexCat().GetName()
+                << "\" is in the dataset. Nested RooSimultaneous are only supported if their index category is a "
+                   "parameter (switch mode). To combine channels, use one RooSimultaneous over a RooCategory with one "
+                   "state per channel.";
+         oocoutE(&top, InputArguments) << errMsg.str() << std::endl;
+         throw std::runtime_error(errMsg.str());
+      }
+   }
+}
+
 #ifdef ROOFIT_LEGACY_EVAL_BACKEND
 /// Print a deprecation warning when the legacy evaluation backend is selected for a fit.
 void printLegacyEvalBackendWarning(RooAbsReal const &topLevelArg)
@@ -756,6 +787,8 @@ std::unique_ptr<RooFitResult> minimize(RooAbsReal &pdf, RooAbsReal &nll, RooAbsD
 
 std::unique_ptr<RooAbsReal> createNLL(RooAbsPdf &pdf, RooAbsData &data, const RooLinkedList &cmdList)
 {
+   checkNestedSimultaneous(pdf, *data.get());
+
    auto timingScope = std::make_unique<ROOT::Math::Util::TimingScope>(
       [&pdf](std::string const &msg) { oocoutI(&pdf, Fitting) << msg << std::endl; }, "Creation of NLL object took");
 
@@ -1061,6 +1094,8 @@ std::unique_ptr<RooAbsReal> createNLL(RooAbsPdf &pdf, RooAbsData &data, const Ro
 
 std::unique_ptr<RooAbsReal> createChi2(RooAbsReal &real, RooDataHist &data, const RooLinkedList &cmdList)
 {
+   checkNestedSimultaneous(real, *data.get());
+
    RooCmdConfig pc("createChi2(" + std::string(real.GetName()) + ")");
 
    pc.defineInt("EvalBackend", "EvalBackend", 0, static_cast<int>(RooFit::EvalBackend::defaultValue()));

roofit/roofitcore/src/RooSimGenContext.cxx

@@ -94,6 +94,18 @@ RooSimGenContext::RooSimGenContext(const RooSimultaneous &model, const RooArgSet
   Int_t i(1) ;
   for(auto * proxy : static_range_cast<RooRealProxy*>(model._pdfProxyList)) {
     auto* pdf = static_cast<RooAbsPdf*>(proxy->absArg());
+    RooArgSet genVars{pdfVars};
+    if (prototype) {
+      genVars.add(*prototype->get(), true);
+    }
+    pdf = RooSimultaneous::resolveSwitchMode(pdf, genVars);
+    if (!pdf) {
+      oocoutE(_pdf,Generation) << "RooSimGenContext::ctor(" << GetName() << ") ERROR: the nested RooSimultaneous for the state "
+                << proxy->name() << " has no pdf for its selected state" << std::endl ;
+      _isValid = false ;
+      _numPdf = 0 ;
+      return ;
+    }
 
     // Name the context after the associated state and add to list
     _gcList.emplace_back(pdf->genContext(pdfVars,prototype,auxProto,verbose)) ;

roofit/roofitcore/src/RooSimSplitGenContext.cxx

@@ -82,7 +82,14 @@ RooSimSplitGenContext::RooSimSplitGenContext(const RooSimultaneous &model, const
   // Generate index category and all registered PDFS
   _allVarsPdf.add(allPdfVars) ;
   for(auto * proxy : static_range_cast<RooRealProxy*>(model._pdfProxyList)) {
-    auto pdf = static_cast<RooAbsPdf*>(proxy->absArg());
+    auto pdf = RooSimultaneous::resolveSwitchMode(static_cast<RooAbsPdf*>(proxy->absArg()), pdfVars);
+    if (!pdf) {
+      oocoutE(_pdf,Generation) << "RooSimSplitGenContext::ctor(" << GetName() << ") ERROR: the nested RooSimultaneous for the state "
+                << proxy->name() << " has no pdf for its selected state" << std::endl ;
+      _isValid = false ;
+      _numPdf = 0 ;
+      return ;
+    }
 
     // Create generator context for this PDF
     std::unique_ptr<RooArgSet> compVars{pdf->getObservables(pdfVars)};