diff --git a/README/ReleaseNotes/v644/index.md b/README/ReleaseNotes/v644/index.md index 749fb68223131..ef32b862bcf5d 100644 --- a/README/ReleaseNotes/v644/index.md +++ b/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 diff --git a/roofit/roofitcore/inc/RooSimultaneous.h b/roofit/roofitcore/inc/RooSimultaneous.h index 9dcc04692f4c2..a794157257944 100644 --- a/roofit/roofitcore/inc/RooSimultaneous.h +++ b/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 finalPdfs; - std::vector finalCatLabels; - RooAbsCategoryLValue *indexCat = nullptr; - std::unique_ptr 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 - initialize(std::string const& name, RooAbsCategoryLValue &inIndexCat, - std::map const &pdfMap); + static RooAbsPdf *resolveSwitchMode(RooAbsPdf *pdf, RooArgSet const &vars); mutable std::unique_ptr _indexCatSet ; /// + 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(outer)) { + continue; + } + RooArgList outerBranches; + outer->branchNodeServerList(&outerBranches); + for (RooAbsArg *arg : outerBranches) { + auto *nested = dynamic_cast(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 minimize(RooAbsReal &pdf, RooAbsReal &nll, RooAbsD std::unique_ptr createNLL(RooAbsPdf &pdf, RooAbsData &data, const RooLinkedList &cmdList) { + checkNestedSimultaneous(pdf, *data.get()); + auto timingScope = std::make_unique( [&pdf](std::string const &msg) { oocoutI(&pdf, Fitting) << msg << std::endl; }, "Creation of NLL object took"); @@ -1061,6 +1094,8 @@ std::unique_ptr createNLL(RooAbsPdf &pdf, RooAbsData &data, const Ro std::unique_ptr 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(RooFit::EvalBackend::defaultValue())); diff --git a/roofit/roofitcore/src/RooSimGenContext.cxx b/roofit/roofitcore/src/RooSimGenContext.cxx index 1215272a6d522..4c6a6f9e77df4 100644 --- a/roofit/roofitcore/src/RooSimGenContext.cxx +++ b/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(model._pdfProxyList)) { auto* pdf = static_cast(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)) ; diff --git a/roofit/roofitcore/src/RooSimSplitGenContext.cxx b/roofit/roofitcore/src/RooSimSplitGenContext.cxx index 69d6c221bdadd..82a0974b835bb 100644 --- a/roofit/roofitcore/src/RooSimSplitGenContext.cxx +++ b/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(model._pdfProxyList)) { - auto pdf = static_cast(proxy->absArg()); + auto pdf = RooSimultaneous::resolveSwitchMode(static_cast(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 compVars{pdf->getObservables(pdfVars)}; diff --git a/roofit/roofitcore/src/RooSimultaneous.cxx b/roofit/roofitcore/src/RooSimultaneous.cxx index 8b7dfbc1d1c68..77c2cb52255ed 100644 --- a/roofit/roofitcore/src/RooSimultaneous.cxx +++ b/roofit/roofitcore/src/RooSimultaneous.cxx @@ -54,11 +54,12 @@ state, like a RooMultiPdf. Its expected number of events is then the one of the selected component. ###Nesting simultaneous PDFs -RooSimultaneous components are currently flattened into a RooSimultaneous over a -RooSuperCategory of all index categories. This is deprecated: from ROOT 6.44, -they are kept as they are and 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. +RooSimultaneous components are only supported if their index category is a +parameter (switch mode); otherwise, creating the likelihood fails. 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 flattened by ROOT 6.42 or earlier and read +from files still work. **/ #include "RooSimultaneous.h" @@ -118,14 +119,6 @@ void replaceOrAdd(RooLinkedList &lst, TObject &obj) } // namespace -RooSimultaneous::InitializationOutput::~InitializationOutput() = default; - -void RooSimultaneous::InitializationOutput::addPdf(const RooAbsPdf &pdf, std::string const &catLabel) -{ - finalPdfs.push_back(&pdf); - finalCatLabels.emplace_back(catLabel); -} - using std::string; @@ -175,8 +168,14 @@ RooSimultaneous::RooSimultaneous(const char *name, const char *title, RooSimultaneous::RooSimultaneous(const char *name, const char *title, std::map pdfMap, RooAbsCategoryLValue &inIndexCat) - : RooSimultaneous(name, title, std::move(*initialize(name ? name : "", inIndexCat, pdfMap))) + : RooAbsPdf(name, title), + _plotCoefNormSet("!plotCoefNormSet", "plotCoefNormSet", this, false, false), + _partIntMgr(this, 10), + _indexCat("indexCat", "Index category", this, inIndexCat) { + for (auto const &item : pdfMap) { + addPdf(*item.second, item.first.c_str()); + } } /// For internal use in RooFit. @@ -187,206 +186,6 @@ RooSimultaneous::RooSimultaneous(const char *name, const char *title, { } -RooSimultaneous::RooSimultaneous(const char *name, const char *title, RooSimultaneous::InitializationOutput &&initInfo) - : RooAbsPdf(name, title), - _plotCoefNormSet("!plotCoefNormSet", "plotCoefNormSet", this, false, false), - _partIntMgr(this, 10), - _indexCat("indexCat", "Index category", this, *initInfo.indexCat) -{ - for (std::size_t i = 0; i < initInfo.finalPdfs.size(); ++i) { - addPdf(*initInfo.finalPdfs[i], initInfo.finalCatLabels[i].c_str()); - } - - // Take ownership of eventual super category - if (initInfo.superIndex) { - addOwnedComponents(std::move(initInfo.superIndex)); - } -} - -/// \cond ROOFIT_INTERNAL - -// This class cannot be locally defined in initialize as it cannot be -// used as a template argument in that case -namespace RooSimultaneousAux { - struct CompInfo { - RooAbsPdf* pdf ; - RooSimultaneous* simPdf ; - const RooAbsCategoryLValue* subIndex ; - std::unique_ptr subIndexComps; - } ; -} - -/// \endcond - -std::unique_ptr -RooSimultaneous::initialize(std::string const& name, RooAbsCategoryLValue &inIndexCat, - std::map const& pdfMap) - -{ - auto out = std::make_unique(); - out->indexCat = &inIndexCat; - - // First see if there are any RooSimultaneous input components - bool simComps(false) ; - for (auto const& item : pdfMap) { - if (dynamic_cast(item.second)) { - simComps = true ; - break ; - } - } - - // If there are no simultaneous component p.d.f. do simple processing through addPdf() - if (!simComps) { - for (auto const& item : pdfMap) { - out->addPdf(*item.second,item.first); - } - return out; - } - - std::string msgPrefix = "RooSimultaneous::initialize(" + name + ") "; - - std::string nestedNames; - std::size_t nNested = 0; - for (auto const &item : pdfMap) { - if (dynamic_cast(item.second)) { - nestedNames += (nNested++ ? ", \"" : "\"") + std::string(item.second->GetName()) + "\""; - } - } - oocoutW(nullptr, InputArguments) - << msgPrefix << (nNested > 1 ? "the components " : "the component ") << nestedNames - << (nNested > 1 ? " are RooSimultaneous themselves and get" : " is a RooSimultaneous itself and gets") - << " flattened into a\nRooSimultaneous over a RooSuperCategory. This is deprecated: from ROOT 6.44, nested " - "RooSimultaneous are only\nsupported if their index category is a parameter (switch mode), and until then the " - "flattening breaks such\nswitches. To combine channels, use one RooSimultaneous over a RooCategory with one " - "state per channel." - << std::endl; - - // Replicating a component to several states of the flattened index category - // silently changes the model (GitHub issue #23342). - auto warnReplication = [&](RooAbsPdf const &pdf, std::string const &label, RooArgSet const &repliCats, - std::size_t nReplicas) { - oocoutW(nullptr, InputArguments) - << msgPrefix << "\"" << pdf.GetName() << "\" (state \"" << label << "\") doesn't depend on " << repliCats - << " and is copied into " << nReplicas << " states; if it is extended, its expected events are counted " - << nReplicas << " times (GitHub issue #23342)." << std::endl; - }; - - RooArgSet allAuxCats ; - std::map compMap ; - for (auto const& item : pdfMap) { - RooSimultaneousAux::CompInfo ci ; - ci.pdf = item.second ; - RooSimultaneous* simComp = dynamic_cast(item.second) ; - if (simComp) { - ci.simPdf = simComp ; - ci.subIndex = &simComp->indexCat() ; - ci.subIndexComps = simComp->indexCat().isFundamental() - ? std::make_unique(simComp->indexCat()) - : std::unique_ptr(simComp->indexCat().getVariables()); - allAuxCats.add(*ci.subIndexComps,true) ; - } else { - ci.simPdf = nullptr; - ci.subIndex = nullptr; - } - compMap[item.first] = std::move(ci); - } - - // Construct the 'superIndex' from the nominal index category and all auxiliary components - RooArgSet allCats(inIndexCat) ; - allCats.add(allAuxCats) ; - std::string siname = name + "_index"; - out->superIndex = std::make_unique(siname.c_str(),siname.c_str(),allCats) ; - auto *superIndex = out->superIndex.get(); - out->indexCat = superIndex; - - // Now process each of original pdf/state map entries - for (auto const& citem : compMap) { - - RooArgSet repliCats(allAuxCats) ; - if (citem.second.subIndexComps) { - repliCats.remove(*citem.second.subIndexComps) ; - } - inIndexCat.setLabel(citem.first.c_str()) ; - - if (!citem.second.simPdf) { - - // Entry is a plain p.d.f. assign it to every state permutation of the repliCats set - RooSuperCategory repliSuperCat("tmp","tmp",repliCats) ; - if (repliSuperCat.size() > 1) { - warnReplication(*citem.second.pdf, citem.first, repliCats, repliSuperCat.size()); - } - - // Iterator over all states of repliSuperCat - for (const auto& nameIdx : repliSuperCat) { - // Set value - repliSuperCat.setLabel(nameIdx.first) ; - // Retrieve corresponding label of superIndex - string superLabel = superIndex->getCurrentLabel() ; - out->addPdf(*citem.second.pdf,superLabel); - oocxcoutD(static_cast(nullptr), InputArguments) << msgPrefix - << "assigning pdf " << citem.second.pdf->GetName() << " to super label " << superLabel << std::endl ; - } - } else { - - // Entry is a simultaneous p.d.f - - if (repliCats.empty()) { - - // Case 1 -- No replication of components of RooSim component are required - - for (const auto& type : *citem.second.subIndex) { - const_cast(citem.second.subIndex)->setLabel(type.first.c_str()); - string superLabel = superIndex->getCurrentLabel() ; - RooAbsPdf* compPdf = citem.second.simPdf->getPdf(type.first); - if (compPdf) { - out->addPdf(*compPdf,superLabel); - oocxcoutD(static_cast(nullptr), InputArguments) << msgPrefix - << "assigning pdf " << compPdf->GetName() << "(member of " << citem.second.pdf->GetName() - << ") to super label " << superLabel << std::endl ; - } else { - oocoutW(nullptr, InputArguments) << msgPrefix << "WARNING: No p.d.f. associated with label " - << type.second << " for component RooSimultaneous p.d.f " << citem.second.pdf->GetName() - << "which is associated with master index label " << citem.first << std::endl ; - } - } - - } else { - - // Case 2 -- Replication of components of RooSim component are required - - // Make replication supercat - RooSuperCategory repliSuperCat("tmp","tmp",repliCats) ; - if (repliSuperCat.size() > 1) { - warnReplication(*citem.second.pdf, citem.first, repliCats, repliSuperCat.size()); - } - - for (const auto& stype : *citem.second.subIndex) { - const_cast(citem.second.subIndex)->setLabel(stype.first.c_str()); - - for (const auto& nameIdx : repliSuperCat) { - repliSuperCat.setLabel(nameIdx.first) ; - const string superLabel = superIndex->getCurrentLabel() ; - RooAbsPdf* compPdf = citem.second.simPdf->getPdf(stype.first); - if (compPdf) { - out->addPdf(*compPdf,superLabel); - oocxcoutD(static_cast(nullptr), InputArguments) << msgPrefix - << "assigning pdf " << compPdf->GetName() << "(member of " << citem.second.pdf->GetName() - << ") to super label " << superLabel << std::endl ; - } else { - oocoutW(nullptr, InputArguments) << msgPrefix << "WARNING: No p.d.f. associated with label " - << stype.second << " for component RooSimultaneous p.d.f " << citem.second.pdf->GetName() - << "which is associated with master index label " << citem.first << std::endl ; - } - } - } - } - } - } - - return out; -} - - //////////////////////////////////////////////////////////////////////////////// /// Copy constructor @@ -457,21 +256,10 @@ bool RooSimultaneous::addPdf(const RooAbsPdf& pdf, const char* catLabel) return true ; } - const RooSimultaneous* simPdf = dynamic_cast(&pdf) ; - if (simPdf) { - - coutE(InputArguments) << "RooSimultaneous::addPdf(" << GetName() - << ") ERROR: you cannot add a RooSimultaneous component to a RooSimultaneous using addPdf()." - << " Use the constructor with RooArgList if input p.d.f.s or the map instead." << std::endl ; - return true ; - - } else { - - // Create a proxy named after the associated index state - TObject* proxy = new RooRealProxy(catLabel,catLabel,this,const_cast(pdf)); - _pdfProxyList.Add(proxy) ; - _numPdf += 1 ; - } + // Create a proxy named after the associated index state + TObject* proxy = new RooRealProxy(catLabel,catLabel,this,const_cast(pdf)); + _pdfProxyList.Add(proxy) ; + _numPdf += 1 ; return false ; } @@ -752,6 +540,19 @@ RooPlot* RooSimultaneous::plotOn(RooPlot *frame, RooLinkedList& cmdList) const } } + // In switch mode, i.e., if the index category is neither in the projection + // data nor an observable of the frame, plot the selected component. This is + // the case for a nested RooSimultaneous when plotting a slice of the outer one. + if (auto *projWData = static_cast(cmdList.FindObject("ProjData"))) { + auto *projData = static_cast(projWData->getObject(1)); + const RooArgSet *normVars = frame->getNormVars(); + if (projData && !indexCatIsObservable(*projData->get()) && !(normVars && indexCatIsObservable(*normVars))) { + if (RooAbsPdf *pdf = getPdf(_indexCat.label())) { + return pdf->plotOn(frame, cmdList); + } + } + } + // Extract projection configuration from command list RooCmdConfig pc("RooSimultaneous::plotOn(" + std::string(GetName()) + ")"); pc.defineString("sliceCatState","SliceCat",0,"",true) ; @@ -1368,6 +1169,22 @@ RooArgSet const& RooSimultaneous::flattenedCatList() const return *_indexCatSet; } +//////////////////////////////////////////////////////////////////////////////// +/// For the generator contexts: a component that is a RooSimultaneous whose +/// index category is not among the generated variables `vars` is in switch +/// mode, so its selected component is generated instead. Returns nullptr if +/// the selected state has no pdf. +RooAbsPdf *RooSimultaneous::resolveSwitchMode(RooAbsPdf *pdf, RooArgSet const &vars) +{ + while (auto *sim = dynamic_cast(pdf)) { + if (sim->indexCatIsObservable(vars)) { + break; + } + pdf = sim->getPdf(sim->indexCat().getCurrentLabel()); + } + return pdf; +} + //////////////////////////////////////////////////////////////////////////////// /// Check if the index category is in the normalization set `nset`, matching by /// name. For a RooSuperCategory index, it's enough if all its input categories diff --git a/roofit/roofitcore/test/testRooSimultaneous.cxx b/roofit/roofitcore/test/testRooSimultaneous.cxx index e72cca8ac150a..dd58613c7460b 100644 --- a/roofit/roofitcore/test/testRooSimultaneous.cxx +++ b/roofit/roofitcore/test/testRooSimultaneous.cxx @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -1096,42 +1097,97 @@ TEST(RooSimultaneous, ExpectedEventsSuperCategory) EXPECT_DOUBLE_EQ(simPdf.expectedEvents(&nsetPartial), 50.); } -/// The flattening of nested RooSimultaneous is deprecated. Replicating a -/// component to several states of the flattened index category, which double -/// counts extended components, gets an extra warning. Covers GitHub issue -/// #23342. -TEST(RooSimultaneous, NestedSimultaneousFlatteningIsDeprecated) +/// A RooSimultaneous whose index category is a parameter ("switch mode") can +/// be nested in another RooSimultaneous over the channels, for example to +/// select the shape in one channel. +TEST(RooSimultaneous, NestedSwitchMode) { + using namespace RooFit; + + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::WARNING); + + RooRealVar x("x", "x", 0, 10); + RooRealVar mean("mean", "mean", 5, 0, 10); + RooRealVar sigma("sigma", "sigma", 1, 0.1, 5); + RooGaussian gauss("gauss", "gauss", x, mean, sigma); + RooRealVar c0("c0", "c0", 0.3, -1.0, 1.0); + RooChebychev cheb("cheb", "cheb", x, {c0}); + + RooRealVar nA("nA", "nA", 300, 0, 1e5); + RooRealVar nB("nB", "nB", 700, 0, 1e5); + RooExtendPdf gaussA("gaussA", "gaussA", gauss, nA); + RooExtendPdf chebA("chebA", "chebA", cheb, nA); + RooExtendPdf gaussB("gaussB", "gaussB", gauss, nB); + + RooCategory channel("channel", "channel", {{"A", 0}, {"B", 1}}); + RooCategory shape("shape", "shape", {{"gauss", 0}, {"cheb", 1}}); + + RooSimultaneous shapeSwitch("shapeSwitch", "shapeSwitch", {{"gauss", &gaussA}, {"cheb", &chebA}}, shape); + RooSimultaneous simPdf("simPdf", "simPdf", {{"A", &shapeSwitch}, {"B", &gaussB}}, channel); + + // Reference models with the switch resolved by hand + RooSimultaneous refPdf0("refPdf0", "refPdf0", {{"A", &gaussA}, {"B", &gaussB}}, channel); + RooSimultaneous refPdf1("refPdf1", "refPdf1", {{"A", &chebA}, {"B", &gaussB}}, channel); + + shape.setIndex(0); + std::unique_ptr data{simPdf.generate({x, channel}, Extended())}; + ASSERT_NE(data, nullptr); + + std::vector backends; +#ifdef ROOFIT_LEGACY_EVAL_BACKEND + backends.push_back(RooFit::EvalBackend(RooFit::EvalBackend::Value::Legacy)); +#endif + backends.push_back(RooFit::EvalBackend::Cpu()); + backends.push_back(RooFit::EvalBackend::CodegenNoGrad()); + + for (auto &backend : backends) { + shape.setIndex(0); + std::unique_ptr nll{simPdf.createNLL(*data, backend)}; + std::unique_ptr refNll0{refPdf0.createNLL(*data, backend)}; + std::unique_ptr refNll1{refPdf1.createNLL(*data, backend)}; + + shape.setIndex(0); + EXPECT_THAT(nll->getVal(), RelativeNear(refNll0->getVal(), 1e-10)) << backend.name() << ", index 0"; + shape.setIndex(1); + EXPECT_THAT(nll->getVal(), RelativeNear(refNll1->getVal(), 1e-10)) << backend.name() << ", index 1"; + } + + // Plotting a slice of the outer RooSimultaneous plots the selected component + std::unique_ptr frame{x.frame()}; + std::unique_ptr refFrame{x.frame()}; + simPdf.plotOn(frame.get(), Slice(channel, "A"), ProjWData(channel, *data)); + refPdf1.plotOn(refFrame.get(), Slice(channel, "A"), ProjWData(channel, *data)); + ASSERT_EQ(frame->numItems(), 1); + EXPECT_TRUE(frame->getCurve()->isIdentical(*refFrame->getCurve())); +} + +/// Nested RooSimultaneous whose index category is an observable are not +/// supported, also below a RooSimultaneous in switch mode. Creating the +/// likelihood should fail with a clear error instead of silently giving wrong +/// results. Covers GitHub issue #23342. +TEST(RooSimultaneous, NestedObservableIndexThrows) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLvl(RooFit::FATAL); + AnalysisCombination m; RooSimultaneous anaA{"anaA", "anaA", {{"SR", &m.pdf_A_SR}, {"CR", &m.pdf_A_CR}}, m.region}; + RooSimultaneous comb{"comb", "comb", {{"A", &anaA}, {"B", &m.pdf_B}}, m.analysis}; - { - // A plain pdf alongside a nested RooSimultaneous gets replicated - RooHelpers::HijackMessageStream hijack(RooFit::WARNING, RooFit::InputArguments); - RooSimultaneous comb{"comb", "comb", {{"A", &anaA}, {"B", &m.pdf_B}}, m.analysis}; - EXPECT_NE(hijack.str().find("deprecated"), std::string::npos) << hijack.str(); - EXPECT_NE(hijack.str().find("\"pdf_B\" (state \"B\")"), std::string::npos) << hijack.str(); - } + RooCategory model{"model", "model", {{"nominal", 0}, {"alternative", 1}}}; + RooSimultaneous anaAlt{"anaAlt", "anaAlt", {{"SR", &m.pdf_A_CR}, {"CR", &m.pdf_A_SR}}, m.region}; + RooSimultaneous modelSwitch{"modelSwitch", "modelSwitch", {{"nominal", &anaA}, {"alternative", &anaAlt}}, model}; - { - // Nested RooSimultaneous components over different categories are - // replicated over each other's categories - RooCategory period{"period", "period", {{"P1", 0}, {"P2", 1}}}; - RooSimultaneous anaB{"anaB", "anaB", {{"P1", &m.pdf_B}, {"P2", &m.pdf_B}}, period}; - RooHelpers::HijackMessageStream hijack(RooFit::WARNING, RooFit::InputArguments); - RooSimultaneous comb{"comb", "comb", {{"A", &anaA}, {"B", &anaB}}, m.analysis}; - EXPECT_NE(hijack.str().find("\"anaA\" (state \"A\")"), std::string::npos) << hijack.str(); - EXPECT_NE(hijack.str().find("\"anaB\" (state \"B\")"), std::string::npos) << hijack.str(); - } + RooDataSet data{"data", "data", {m.x, m.analysis, m.region}}; + data.add({m.x, m.analysis, m.region}); - { - // No replication if all nested RooSimultaneous have the same index - // category, but the flattening is deprecated nonetheless - RooSimultaneous anaB{"anaB", "anaB", {{"SR", &m.pdf_B}}, m.region}; - RooHelpers::HijackMessageStream hijack(RooFit::WARNING, RooFit::InputArguments); - RooSimultaneous comb{"comb", "comb", {{"A", &anaA}, {"B", &anaB}}, m.analysis}; - EXPECT_NE(hijack.str().find("deprecated"), std::string::npos) << hijack.str(); - EXPECT_EQ(hijack.str().find("copied"), std::string::npos) << hijack.str(); + for (RooAbsPdf *pdf : std::initializer_list{&comb, &modelSwitch}) { + EXPECT_THROW(std::unique_ptr{pdf->createNLL(data)}, std::runtime_error) << pdf->GetName(); +#ifdef ROOFIT_LEGACY_EVAL_BACKEND + EXPECT_THROW( + std::unique_ptr{pdf->createNLL(data, RooFit::EvalBackend(RooFit::EvalBackend::Value::Legacy))}, + std::runtime_error) + << pdf->GetName(); +#endif } }