diff --git a/README/ReleaseNotes/v642/index.md b/README/ReleaseNotes/v642/index.md index 512aba2f3c274..ad566dfd2e70a 100644 --- a/README/ReleaseNotes/v642/index.md +++ b/README/ReleaseNotes/v642/index.md @@ -105,6 +105,21 @@ As a result, plotting, generating binned data and creating histograms from a def Code that reads `getBins()`/`numBins()` of a bare variable and expected the value `100` should either set the binning explicitly, or read the bin count from the relevant histogram or plot frame instead. +### Resolution models are no longer imported into the workspace as standalone objects + +When a `RooResolutionModel` (like `RooGaussModel` or `RooTruthModel`) is used wi th a +`RooAbsAnaConvPdf` (like `RooDecay`, `RooBDecay`, etc.), it acts as a *configuration* object that specifies which model to convolve the basis functions with, rather than as a nod +e of the pdf's computation graph. +The `RooAbsAnaConvPdf` builds its own internal basis-function convolutions from it and evaluates *those*. + +Until now, the resolution model was nevertheless kept as a (non-value, non-shape) server of the `RooAbsAnaConvPdf`. +As a side effect, importing such a pdf into a `RooWorkspace` also imported the original resolution model as a standalone workspace object, and it leaked into HS3/JSON exports, even though it played no role in the computation. + +Starting with ROOT 6.42, a resolution model that is only used as the configurati on of a `RooAbsAnaConvPdf` is no longer a server of that pdf, and is therefore not imported into the +workspace on its own anymore. The model remains accessible via `RooAbsAnaConvPdf::getModel()`. + +This is not expected to affect typical usage, since the resolution model was never part of the actual likelihood. Workspaces written with older ROOT versions are read back correctly via schema evolution. + ## Graphics and GUI ### Store canvas as HTML file diff --git a/roofit/hs3/src/JSONFactories_RooFitCore.cxx b/roofit/hs3/src/JSONFactories_RooFitCore.cxx index 80c9e7afdc63b..657ea1d0683b0 100644 --- a/roofit/hs3/src/JSONFactories_RooFitCore.cxx +++ b/roofit/hs3/src/JSONFactories_RooFitCore.cxx @@ -811,7 +811,11 @@ bool exportPoisson(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem return true; } -bool exportDecay(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem, std::string const &key) +// The RooDecay servers are the internal resModel-times-basis convolutions, +// which are implementation details that should not leak into the JSON. We +// only export the actual dependents (t, tau and the original resolution +// model) explicitly. +bool exportDecay(RooJSONFactoryWSTool *tool, const RooAbsArg *func, JSONNode &elem, std::string const &key) { auto *pdf = static_cast(func); elem["type"] << key; @@ -820,6 +824,10 @@ bool exportDecay(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem, elem["resolutionModel"] << pdf->getModel().GetName(); elem["decayType"] << pdf->getDecayType(); + tool->queueExport(pdf->getT()); + tool->queueExport(pdf->getTau()); + tool->queueExport(pdf->getModel()); + return true; } @@ -1060,8 +1068,12 @@ class FuncImporter : public RooFit::JSONIO::Importer { template class FuncExporter : public RooFit::JSONIO::Exporter { public: - FuncExporter(std::string key) : _key{std::move(key)} {} + FuncExporter(std::string key, bool autoExportDependants) + : _key{std::move(key)}, _autoExportDependants{autoExportDependants} + { + } std::string const &key() const override { return _key; } + bool autoExportDependants() const override { return _autoExportDependants; } bool exportObject(RooJSONFactoryWSTool *tool, const RooAbsArg *func, JSONNode &elem) const override { return Func(tool, func, elem, _key); @@ -1069,6 +1081,7 @@ class FuncExporter : public RooFit::JSONIO::Exporter { private: const std::string _key; + const bool _autoExportDependants; }; template @@ -1078,9 +1091,10 @@ void registerImporter(const std::string &key, bool topPriority = true) } template -void registerExporter(TClass const *cl, std::string key, bool topPriority = true) +void registerExporter(TClass const *cl, std::string key, bool topPriority = true, bool autoExportDependants = true) { - RooFit::JSONIO::registerExporter(cl, std::make_unique>(std::move(key)), topPriority); + RooFit::JSONIO::registerExporter(cl, std::make_unique>(std::move(key), autoExportDependants), + topPriority); } STATIC_EXECUTE([]() { @@ -1133,7 +1147,7 @@ STATIC_EXECUTE([]() { registerExporter(RooLognormal::Class(), "lognormal_dist", false); registerExporter(RooMultiVarGaussian::Class(), "multivariate_normal_dist", false); registerExporter(RooPoisson::Class(), "poisson_dist", false); - registerExporter(RooDecay::Class(), "decay_dist", false); + registerExporter(RooDecay::Class(), "decay_dist", false, /*autoExportDependants=*/false); registerExporter(RooTruthModel::Class(), "delta_resolution_model", false); registerExporter(RooGaussModel::Class(), "gauss_resolution_model", false); registerExporter>(RooPolynomial::Class(), "polynomial_dist", false); diff --git a/roofit/roofitcore/inc/LinkDef.h b/roofit/roofitcore/inc/LinkDef.h index dcad126c6033a..2dfbf74869e15 100644 --- a/roofit/roofitcore/inc/LinkDef.h +++ b/roofit/roofitcore/inc/LinkDef.h @@ -93,6 +93,15 @@ #pragma link C++ class RooDirItem+ ; #pragma link C++ class RooDLLSignificanceMCSModule+ ; #pragma link C++ class RooAbsAnaConvPdf+ ; +// Schema evolution: up to version 3, the original resolution model was stored +// in a RooRealProxy (evolved to RooTemplateProxy) and was a server +// of the pdf. As of version 4 it is an owned, non-server RooResolutionModel*. +// Recover the model pointer from the old proxy here; the stale server link is +// cleaned up afterwards in RooAbsAnaConvPdf::ioStreamerPass2(). +#pragma read sourceClass="RooAbsAnaConvPdf" targetClass="RooAbsAnaConvPdf" version="[-3]" \ + include="RooResolutionModel.h" \ + source="RooTemplateProxy _model" target="_model,_ownModel" \ + code="{ _model = static_cast(onfile._model.absArg()); _ownModel = false; }" #pragma link C++ class RooAddPdf+ ; #pragma link C++ class RooEfficiency+ ; #pragma link C++ class RooEffProd+ ; diff --git a/roofit/roofitcore/inc/RooAbsAnaConvPdf.h b/roofit/roofitcore/inc/RooAbsAnaConvPdf.h index 13fa4f5511ab6..e2667488c6d4c 100644 --- a/roofit/roofitcore/inc/RooAbsAnaConvPdf.h +++ b/roofit/roofitcore/inc/RooAbsAnaConvPdf.h @@ -24,8 +24,8 @@ #include "RooAICRegistry.h" #include "RooObjCacheManager.h" #include "RooAbsCacheElement.h" +#include "RooResolutionModel.h" -class RooResolutionModel ; class RooRealVar ; class RooConvGenContext ; @@ -81,7 +81,10 @@ class RooAbsAnaConvPdf : public RooAbsPdf { } /// Get the resolution model. - RooAbsReal const &getModel() const { return _model.arg(); } + /// Note that the resolution model is only a configuration object specifying + /// the model to convolve the basis functions with; it is not a server of this + /// pdf and is not part of its computation graph (see the class documentation). + RooAbsReal const &getModel() const { return *_model; } std::unique_ptr compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileContext & ctx) const override; @@ -92,11 +95,24 @@ class RooAbsAnaConvPdf : public RooAbsPdf { double evaluate() const override ; + bool redirectServersHook(const RooAbsCollection &newServerList, bool mustReplaceAll, bool nameChange, + bool isRecursive) override; + + void ioStreamerPass2() override; + void makeCoefVarList(RooArgList&) const ; friend class RooConvGenContext ; - RooRealProxy _model ; ///< Original model + // The original resolution model is intentionally *not* a server of the + // RooAbsAnaConvPdf: it is only used to build the convolutions (stored in + // _convSet) and for some operations like generation. Keeping it as a server + // would pollute the computation graph (and any RooWorkspace or JSON export) + // with an object that is never evaluated. It is owned and kept in sync with + // the rest of the graph via redirectServersHook(), analogous to how + // RooResolutionModel handles its basis function. + RooResolutionModel *_model = nullptr; ///< Original resolution model (not a server) + bool _ownModel = false; ///< Flag indicating ownership of _model RooRealProxy _convVar ; ///< Convolution variable RooArgSet* parseIntegrationRequest(const RooArgSet& intSet, Int_t& coefCode, RooArgSet* analVars=nullptr) const ; @@ -120,7 +136,7 @@ class RooAbsAnaConvPdf : public RooAbsPdf { mutable RooAICRegistry _codeReg ; ///(model.clone(model.GetName()))}, + _ownModel{true}, + _convVar("!convVar", "Convolution variable", this, cVar, false, false), + _convSet("!convSet", "Set of resModel X basisFunc convolutions", this), + _coefNormMgr(this, 10), + _codeReg(10) { - _model.absArg()->setAttribute("NOCacheAndTrack") ; + _model->setAttribute("NOCacheAndTrack"); } //////////////////////////////////////////////////////////////////////////////// -RooAbsAnaConvPdf::RooAbsAnaConvPdf(const RooAbsAnaConvPdf& other, const char* name) : - RooAbsPdf(other,name), _isCopy(true), - _model("!model",this,other._model), - _convVar("!convVar",this,other._convVar), - _convSet("!convSet",this,other._convSet), - _coefNormMgr(other._coefNormMgr,this), - _codeReg(other._codeReg) +RooAbsAnaConvPdf::RooAbsAnaConvPdf(const RooAbsAnaConvPdf &other, const char *name) + : RooAbsPdf(other, name), + _isCopy(true), + _model{other._model ? static_cast(other._model->clone(other._model->GetName())) : nullptr}, + _ownModel{true}, + _convVar("!convVar", this, other._convVar), + _convSet("!convSet", this, other._convSet), + _coefNormMgr(other._coefNormMgr, this), + _codeReg(other._codeReg) { // Copy constructor - if (_model.absArg()) { - _model.absArg()->setAttribute("NOCacheAndTrack") ; + if (_model) { + _model->setAttribute("NOCacheAndTrack"); } other._basisList.snapshot(_basisList); } @@ -140,8 +179,53 @@ RooAbsAnaConvPdf::~RooAbsAnaConvPdf() } } + if (_ownModel) { + delete _model; + } } +//////////////////////////////////////////////////////////////////////////////// +/// Forward server redirection to the original resolution model. The resolution +/// model is not a server of this pdf (it is only used to build the convolutions +/// and for generation), so it is not redirected by the standard machinery. We +/// keep its servers in sync here, analogous to RooResolutionModel forwarding +/// the redirection to its basis function. + +bool RooAbsAnaConvPdf::redirectServersHook(const RooAbsCollection &newServerList, bool mustReplaceAll, bool nameChange, + bool isRecursive) +{ + if (_model) { + // Pass mustReplaceAll=false: the model may legitimately reference servers + // that are not part of this particular redirection, and it is never + // evaluated as part of the computation graph anyway. + _model->redirectServers(newServerList, false, nameChange); + } + + return RooAbsPdf::redirectServersHook(newServerList, mustReplaceAll, nameChange, isRecursive); +} + +//////////////////////////////////////////////////////////////////////////////// +/// Second-pass schema evolution, called by the RooWorkspace after reading. +/// +/// In class version <= 3, the original resolution model was held in a +/// RooRealProxy and was therefore a (non-value, non-shape) server of this pdf. +/// The schema evolution read rule (see LinkDef.h) already recovered the model +/// pointer into _model, but the stale server link is also restored from the +/// file via the RooAbsArg server list. We remove it here, once the full graph +/// is live, so that an object read from an old file has the same clean server +/// structure as a freshly constructed one. This is safe because there is no +/// longer a proxy that could resurrect the server link on copy. + +void RooAbsAnaConvPdf::ioStreamerPass2() +{ + RooAbsPdf::ioStreamerPass2(); + + // Force removal (the link may have been added with a reference count > 1): + // the model must be completely severed from the server list. + if (_model && findServer(*_model)) { + removeServer(*_model, true); + } +} //////////////////////////////////////////////////////////////////////////////// /// Declare a basis function for use in this physics model. The string expression @@ -165,11 +249,10 @@ Int_t RooAbsAnaConvPdf::declareBasis(const char* expression, const RooArgList& p } // Resolution model must support declared basis - if (!(static_cast(_model.absArg()))->isBasisSupported(expression)) { - coutE(InputArguments) << "RooAbsAnaConvPdf::declareBasis(" << GetName() << "): resolution model " - << _model.absArg()->GetName() - << " doesn't support basis function " << expression << std::endl ; - return -1 ; + if (!_model->isBasisSupported(expression)) { + coutE(InputArguments) << "RooAbsAnaConvPdf::declareBasis(" << GetName() << "): resolution model " + << _model->GetName() << " doesn't support basis function " << expression << std::endl; + return -1; } // Instantiate basis function @@ -188,7 +271,7 @@ Int_t RooAbsAnaConvPdf::declareBasis(const char* expression, const RooArgList& p basisFunc->setOperMode(operMode()) ; // Instantiate resModel x basisFunc convolution - RooAbsReal* conv = static_cast(_model.absArg())->convolution(basisFunc.get(),this); + RooAbsReal *conv = _model->convolution(basisFunc.get(), this); _basisList.addOwned(std::move(basisFunc)); if (!conv) { coutE(InputArguments) << "RooAbsAnaConvPdf::declareBasis(" << GetName() << "): unable to construct convolution with basis function '" @@ -228,14 +311,15 @@ bool RooAbsAnaConvPdf::changeModel(const RooResolutionModel& newModel) _convSet.removeAll() ; _convSet.addOwned(std::move(newConvSet)); - const std::string attrib = std::string("ORIGNAME:") + _model->GetName(); - const bool oldAttrib = newModel.getAttribute(attrib.c_str()); - const_cast(newModel).setAttribute(attrib.c_str()); - - redirectServers(RooArgSet{newModel}, false, true); - - // reset temporary attribute for server redirection - const_cast(newModel).setAttribute(attrib.c_str(), oldAttrib); + // Replace the stored original resolution model. Since it is not a server of + // this pdf, it cannot (and need not) be redirected via redirectServers(): we + // simply own a fresh clone of the new model. + if (_ownModel) { + delete _model; + } + _model = static_cast(newModel.clone(newModel.GetName())); + _ownModel = true; + _model->setAttribute("NOCacheAndTrack"); return false ; } @@ -254,7 +338,7 @@ RooAbsGenContext* RooAbsAnaConvPdf::genContext(const RooArgSet &vars, const RooD const RooArgSet* auxProto, bool verbose) const { // Check if the resolution model specifies a special context to be used. - RooResolutionModel* conv = dynamic_cast(_model.absArg()); + RooResolutionModel *conv = _model; assert(conv); std::unique_ptr modelDep {_model->getObservables(&vars)}; @@ -296,9 +380,8 @@ bool RooAbsAnaConvPdf::isDirectGenSafe(const RooAbsArg& arg) const { // All direct generation of convolution arg if model is truth model - if (!TString(_convVar.absArg()->GetName()).CompareTo(arg.GetName()) && - dynamic_cast(_model.absArg())) { - return true ; + if (!TString(_convVar.absArg()->GetName()).CompareTo(arg.GetName()) && dynamic_cast(_model)) { + return true; } return RooAbsPdf::isDirectGenSafe(arg) ; diff --git a/roofit/roofitcore/test/CMakeLists.txt b/roofit/roofitcore/test/CMakeLists.txt index def1fa324c837..c55a921859a64 100644 --- a/roofit/roofitcore/test/CMakeLists.txt +++ b/roofit/roofitcore/test/CMakeLists.txt @@ -86,7 +86,8 @@ ROOT_ADD_GTEST(testRooMinimizer testRooMinimizer.cxx LIBRARIES RooFitCore RooFit ROOT_ADD_GTEST(testRooMulti testRooMulti.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooRombergIntegrator testRooRombergIntegrator.cxx LIBRARIES MathCore RooFitCore) ROOT_ADD_GTEST(testRooSimultaneous testRooSimultaneous.cxx LIBRARIES RooFitCore RooFit) -ROOT_ADD_GTEST(testRooTruthModel testRooTruthModel.cxx LIBRARIES RooFitCore RooFit) +ROOT_ADD_GTEST(testRooTruthModel testRooTruthModel.cxx LIBRARIES RooFitCore RooFit + COPY_TO_BUILDDIR ${CMAKE_CURRENT_SOURCE_DIR}/rooAbsAnaConvPdf_classV3.root) if (roofit_multiprocess) ROOT_ADD_GTEST(testTestStatisticsPlot TestStatistics/testPlot.cxx LIBRARIES RooFitMultiProcess RooFitCore RooFit diff --git a/roofit/roofitcore/test/rooAbsAnaConvPdf_classV3.root b/roofit/roofitcore/test/rooAbsAnaConvPdf_classV3.root new file mode 100644 index 0000000000000..e8e064749100a Binary files /dev/null and b/roofit/roofitcore/test/rooAbsAnaConvPdf_classV3.root differ diff --git a/roofit/roofitcore/test/testRooTruthModel.cxx b/roofit/roofitcore/test/testRooTruthModel.cxx index ff3464a880dab..86ac26d052f2e 100644 --- a/roofit/roofitcore/test/testRooTruthModel.cxx +++ b/roofit/roofitcore/test/testRooTruthModel.cxx @@ -1,14 +1,20 @@ // Tests for the RooTruthModel // Authors: Jonas Rembser, CERN 11/2023 +#include #include #include +#include #include #include +#include + +#include #include #include +#include /// Check that the integration over a subrange works when using an analytical /// convolution with the RooTruthModel. @@ -37,3 +43,91 @@ TEST(RooTruthModel, IntegrateSubrange) std::unique_ptr integ{bcpg.createIntegral({dt}, "integral")}; EXPECT_NEAR(integ->getVal(), 0.0, 1e-16); } + +/// The original resolution model passed to a RooAbsAnaConvPdf is only used to +/// build the internal resModel-times-basis convolutions; it must not become a +/// (non-value, non-shape) server of the pdf itself. Otherwise it pollutes the +/// computation graph and gets dragged into RooWorkspace and JSON exports. +TEST(RooAbsAnaConvPdf, ResolutionModelIsNotASpuriousServer) +{ + RooRealVar dt{"dt", "dt", -10, 10}; + RooRealVar tau{"tau", "tau", 1.548}; + RooTruthModel tm{"tm", "truth model", dt}; + RooDecay decay{"decay_tm", "decay", dt, tau, tm, RooDecay::SingleSided}; + + for (RooAbsArg *server : decay.servers()) { + // The original resolution model should not appear in the graph at all. + EXPECT_STRNE(server->GetName(), tm.GetName()) + << "The original resolution model should not be a server of the RooDecay!"; + // Every remaining server must be a genuine value or shape server. + EXPECT_TRUE(server->isValueServer(decay) || server->isShapeServer(decay)) + << "Server '" << server->GetName() << "' is neither a value nor a shape server of the RooDecay!"; + } + + // The resolution model must still be reachable (e.g. for JSON serialization). + EXPECT_STREQ(decay.getModel().GetName(), tm.GetName()); +} + +/// Schema evolution test for RooAbsAnaConvPdf from class version 3 to 4. +/// +/// In version 3, the original resolution model was stored in a RooRealProxy and +/// was therefore a (non-value, non-shape) server of the pdf. In version 4 it is +/// an owned, non-server object. Reading back a RooDecay from a version-3 +/// workspace must give the same clean server structure as a freshly constructed +/// one, while staying numerically identical. +/// +/// The fixture file was created with ROOT 6.40 (RooAbsAnaConvPdf v3) via: +/// \code +/// RooRealVar dt{"dt", "dt", -10, 10}; +/// RooRealVar tau{"tau", "tau", 1.548}; +/// RooTruthModel tm{"tm", "truth model", dt}; +/// RooDecay decay_tm{"decay_tm", "decay", dt, tau, tm, RooDecay::SingleSided}; +/// RooRealVar bias{"bias", "bias", 0.2}; +/// RooRealVar sigma{"sigma", "sigma", 0.9}; +/// RooGaussModel gm{"gm", "gauss model", dt, bias, sigma}; +/// RooDecay decay_gm{"decay_gm", "decay", dt, tau, gm, RooDecay::SingleSided}; +/// RooWorkspace ws{"ws"}; +/// ws.import(decay_tm); +/// ws.import(decay_gm); +/// ws.writeToFile("rooAbsAnaConvPdf_classV3.root"); +/// \endcode +TEST(RooAbsAnaConvPdf, SchemaEvolutionV3) +{ + std::unique_ptr file{TFile::Open("rooAbsAnaConvPdf_classV3.root", "READ")}; + ASSERT_TRUE(file && !file->IsZombie()); + + auto *ws = file->Get("ws"); + ASSERT_NE(ws, nullptr); + + RooRealVar &dt = *ws->var("dt"); + + struct Reference { + const char *pdfName; + const char *modelName; + double value; // unnormalized value at dt = 1.5 + }; + + // Reference values were recorded with the version-3 code that wrote the file. + for (auto const &ref : + {Reference{"decay_tm", "tm", 0.37946525232591943}, Reference{"decay_gm", "gm", 0.824172112571901}}) { + auto *decay = dynamic_cast(ws->pdf(ref.pdfName)); + ASSERT_NE(decay, nullptr) << ref.pdfName; + + // The original resolution model must not have survived as a spurious + // (non-value, non-shape) server after the schema evolution. + for (RooAbsArg *server : decay->servers()) { + EXPECT_TRUE(server->isValueServer(*decay) || server->isShapeServer(*decay)) + << "Server '" << server->GetName() << "' of '" << ref.pdfName + << "' is neither a value nor a shape server after schema evolution!"; + EXPECT_STRNE(server->GetName(), ref.modelName) + << "The original resolution model must not be a server of '" << ref.pdfName << "'!"; + } + + // The model must still be reachable and identical to what was stored. + EXPECT_STREQ(decay->getModel().GetName(), ref.modelName); + + // The pdf must evaluate to exactly the same value as before. + dt.setVal(1.5); + EXPECT_DOUBLE_EQ(decay->getVal(), ref.value) << ref.pdfName; + } +}