From 4c9384d94ac3b6de28f9b31a428f37d954caad16 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 16 Jun 2026 09:10:01 +0200 Subject: [PATCH] [RF] Don't keep the resolution model as a server of RooAbsAnaConvPdf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resolution model passed to a RooAbsAnaConvPdf (RooDecay, RooBDecay, ...) is only a *configuration* object: it specifies which model to convolve the basis functions with, and the pdf evaluates its own internal convolutions, not the model itself. Yet it was kept as a (non-value, non-shape) server, which polluted the computation graph and dragged the model into RooWorkspace imports and HS3/JSON exports for no reason. It also broke the HS3 round-trip, since the internal convolution objects leaked into the JSON with non-conforming names. The model is now an owned, non-server member instead. This deliberately mirrors how RooResolutionModel already manages its basis function: a raw owned pointer (_basis / _ownBasis) that is not a server and whose redirection is forwarded in redirectServersHook(). See RooResolutionModel.{h,cxx}. The HS3 exporter now lists its real dependents explicitly (t, tau, model) instead of auto-exporting servers. This follows up on 478ad2b8079, which noted that the model server could not simply be removed without a server/proxy desync; removing the proxy entirely avoids that desync by construction. Schema evolution from class version 3 to 4 is handled with the standard RooFit mechanism: a versioned `#pragma read` rule in LinkDef.h recovers the model pointer from the old RooRealProxy, and a RooAbsAnaConvPdf:: ioStreamerPass2() override severs the stale server link once the graph is live. This is the same two-stage pattern already used for the v5->v6 proxy-list migration (RooAbsArg::ioStreamerPass2) and for the read rules of RooProduct / RooSuperCategory in roofitcore/inc/LinkDef.h. A unit test reads back a v3 workspace fixture and checks the server structure, getModel() and the evaluated values. Closes #20245. 🤖 Done with the help of [Claude Code](https://claude.com/claude-code) (Claude Opus 4.8) --- README/ReleaseNotes/v642/index.md | 15 ++ roofit/hs3/src/JSONFactories_RooFitCore.cxx | 24 ++- roofit/roofitcore/inc/LinkDef.h | 9 + roofit/roofitcore/inc/RooAbsAnaConvPdf.h | 24 ++- roofit/roofitcore/src/RooAbsAnaConvPdf.cxx | 157 +++++++++++++----- roofit/roofitcore/test/CMakeLists.txt | 3 +- .../test/rooAbsAnaConvPdf_classV3.root | Bin 0 -> 10832 bytes roofit/roofitcore/test/testRooTruthModel.cxx | 94 +++++++++++ 8 files changed, 279 insertions(+), 47 deletions(-) create mode 100644 roofit/roofitcore/test/rooAbsAnaConvPdf_classV3.root 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 0000000000000000000000000000000000000000..e8e064749100a012c2035ce72ef94b6b3050a921 GIT binary patch literal 10832 zcmcJ#bx<8m^euXDcXxujyIXK~hr{9Ee$e19!3h=|5+Jxka0nh;g1fsz@XME1_1>TF z-m3fSn%&(~UA?AvcURAv*~`t@*&P6Q1Oos7O8|gL9ROgQe6I(+8`Qgv!oFwkjmVq; z0C^DrYD^s{Oy?vO-n;{s&KRGv_1^lwlb{d*{{bcY=UYbtJpUoTKMeqY)0VY$0Bf_` zI6H#bK@LC<%lE?hJ#%{>*1yaQ;s*M!VrBd9{=EP>yMfu=!H|FVk`@q2C!n;mlc&0+ zmDxK21ft8$_Fr$n{|qVs?mr*iE$V&v&+j(+ZvT7%02pTfe#y_QJU}`8 z*B@D1-OU*UhCmc#kpJHr2aP2M(2|!^z><|ukO#!d$!W#OD#*`m$;!dS#{(1qaae%C zRuTY!6aeZy7kP*KZ^{0p|5FqI0Fmjxp^@?b8yf3>NYDuHP?o5g&dz$yZuSrtAP5ZY z1$n;=l@V_c0Q_VCZ>UuR0~=B>>Dz=dcFHP`+zhD-hGyl~oLwcpQEcW{!=9qxS(v2O zoHM8Cxf7>hRfo?}P;giY>0= zUR=%m7-#rA{b05C^g4gv)5o@XBGmOz*i0o-HQn$(aRx(bzG-2(TCa6Oy7qN zuYhj5^b9DP^QMYU71U823msd)`0N~OGPFAJ6KfYpIqkRbmBBv8r0$DMkWOTO9r!Pv zZU1wa*#0+|gK`NL+#jNH-(rm%<|2b0M|`L%R?0;TFWieJw*M>&dcCpk8VA6~>Y6IZ zGV{mS{+{%*8{5|PsdqWP{o)S zjo>?JtAp8*#4cK=%_^3&?e|uKDo`<1m%gyrXy-keZK%Gzw9e2`p|p&fgD2VvHzuov z&hs#5(mGKmX8BcPJAu1C5>z9?>#SCFyVL%e&jm09PBrXZqigOObo*Au)@%^H`~}gX zBv1loy9oFG9)ubJMSVc5M6)zjc`UWgslb@{2fgLf-I=H)QnZ(v5p?U$29~X0XT(<6KjcAV`y0M{#75eRCQPz+F-`R6@~+O-7k~DZrQUxz z&IOCAF;*aG@9BO@i}D!7-;OPz-R5bubSSwQ&HmNVY2q!drbO-wth6dm)NZ{+Qud(ZVJ zXJ)+Ok^25)3y!=}jBMwr2DS6-tO zlQwp*uDo4%Ujtdo7*W(W&@*#X|FBMwk1LZ?O@m0>8#_@s=XNv=Of{A$M~nI$+QF*74$rGq5gIVv~&N5nJW zs{NPmk8%(jei9Ad9}_4HE=aPHl+RWZ*n)ig!K!j;B32ppSHzGX#QdY~W#%=CE+^j7 z1LqZ5yexL7u%V9^vRSNdwxH14U(4qV6Kh6-8sra*FhP)|8=X=LUem$I2$|d1YjgY0)3+xSxF3Y*OPg`8 zRTWBtU{9UNie75n8ZArfz353*_tX6eI(2QCAP$32viN*N8P(` z&%z`#9Q|Rwax)wzT*|)|{s8&o!MT_Rx3$Vt(Y4AQfO-uUdu5t_uG`^^ifHf_?SR|T zV0rW|6FCMthET7c#k>$+Jw}fGp^}uA^%3kcxTV#_WBaJ~!8OQ^<299doZ5LN)R)n% zF)xvk0ClQKwF~zzf47}oV?IZjF86R_%qi_H7DX04wC9u1>#VFR21OyR4JIq%JahP% zkTYhrx&}r5Mh^&mndJ+1!qQp$v9mNGa}!B{rsR+d+S@PKg3GE^L`42FR8jKrEsA6E zT%SH>{h-OI?yO!OFgsJiMM@d7?)7{118@7_6 zaTp4x(%^4q`dy^{d;Qg3Z&Ygl^p)){R%VRAJZ)V{U}NDgl)gSi6`~}#itw{yRU$K% zuiI5x$Ve(c_Jy2q6TfP2SDeGG!#vYWn9`u@%GATr*12`I6uW`eV17t|UvH{3U{F2|JAMLoFA zA*!+Ev)4qMr>C#gRoF~%mqHIEoC2~Np!+&^$j#rKd;gAm@E2&I@x%G_*CvHjcTAOz zfp(u#s&AR|eVx~iU^eSHoNpCO)-)$jJH^b6E>$PozG4K=d)#{=J)f;II&rXcYjQ}~Y^0xaRQm2DyJs9NrB zV4x$|O~J{^8C%BL!@|Lb(!tir9&GtuK-}NSgCRM9+WMV5>>48_lIr=qmUuM^IUYB# z{+2U>Z%_7E9;&&kvTkHH_)J^b-j)^z=03|gTqUw+^SVE=13N@~tN8%C<+7N~r$B2i zrQyn!A5#`qsi?E1$P^r+-X{@5r5=on$-n_uAl3O6SlwneF+Mu3C`FapJjcB(x_U0Y z7TEFjQa!`tPsUn=OXO(uRWt7aZ>i6F#B?LoD4_k&HR9deQHUlR9{Av!>WAmE)Xkfpb%3P+w z54xju(VXmz#qzXvY6_{1O7CJDiZuFeHmz&X*7QJ(Lm&W?Acz_csXM=3J=bayBQbme z2l^3@?f|T%ye|Yd#qucGAC_w3D+ZG6IoFHtDuN!mh02ZU(VI6H}3W2k#A}ZHDtMo2;E=%z^BoJSFV!P>tp2+Uy-mp-&;TvAN(2s~9!QPjP(abHi$$?zH>s$=XIoiot7VyY*yELqmKbJYm#R4Vtp^%4iuX zs7IwXQ|6g-yaljGFGguU=8%7)*;!_Ok8$_4B&39QS$KB@jB~%j&6Q23NK63tbyG|Z zUrd~G#k*rT4lmPT>e7Fnf*4Yc*M+IwF4)EeZu_plpAI9vV=Zhn{Il&OTBIcv0zJrf zd7hiYIQL8mQfo~_oWO`Gr5McHvRP!~musqaMkM3jX^1txx4sZn!WW~!GJaAqW*-yi z-#TECxg*TIsutJ0fEj4)O+T@78fnhq;;8ykmdaHTrG}g*)M?a|+HF0VMeb#Q@GOLc zTr$*~$;@!VqoJP(ydt{TRXFfl^rBAGv72Fo8ln!rV&ot62Tc0%E_fw|*VDtDlp=YW z(2*tp(E`*U7oLe;(94%A-T)LZ5RK841&wn7b~Dlz$B75)cz6_d#UM~RN(BiGAE32PopHA(`wD^@zoM6y{oh#a%vn;n#KM&00UjFxAy zq#&UgbE`GNiwTl1ub68X>j0?luAZutW@Vx*R$q}JHn3l#WCXcX+4`G&H`Of;)lz5? zcv;)6todoH-AhKTt^hfTbMv)e5|F%=$aR9#RG2gE*;Eag@GyS|KsZt0im&u?I-@;qQYD)E#@-RiyfU91o7 zjhYE%*>Q3!=3Jw4RO=raJ1CcFQAy$o_HDsQs}~TpqbMqq3Qru#-lacOuX1T@&{bHR zqJbHSh7?nnK>PxS3*F~44{Gp2BqJ;i72Vv$Ct>{JGPO6>$7sVLDe_&ppU4~cB&fElHJ;u`IUyEOYlv{)*N?u=XW+W&Gk+-sH;#lUEe{C+?Jax zn5zOd*j-dGB?8)v2yGa6Ikv*3VLhAKrwg%}Zn!PO%~-QN)*<2-8EsKEqUczZPv1Jr2?CL-aQN%<#|_9||)IgxZ}9P&e$1|RPfkSLLcs2fHm2|o1er+cj6v;-PhLI z*x?=5E!Qm|mJSo8M&sO4USAPS#CWwUEY;%L5)d^(Rp+0$ozCEU?8LWho*6EJQ;AWQ zXYPZt=LD9RR%x+5*ZB;>sC|ta@x&?M#gf8|ZcK!J;)TbzhU2c5+x<`%)bql{WTcXj z@tYdD-J#25fWRSMRFFVk+l_3amh!sCJw|<7*iPY*@?5u=Z3(o>yae%qg-@ssG9dM= zmk;*aW;UXbqEM%_=RdPPcLxknmXHNI0LF4WFnp50sl_#j@S!*Y`9RvX0*` zu8O2U7ojIvkQVvTeKXV_xSOe1i*G*S>h~8j*Ko2Evi?HB%N`LQTF@#~=99>hy~-?5 z!RuPX1yyhi}|MNd$)#H2st7#CrBD~8s6;{DK>N6EjX%Rf`z zm_x09Lo@Mn&s+~nosy(KD{T9WiofASJTgs$PmOoV;unTy! zzJ7y!;lc%Ul^2a>Vh*P09_~aE z3AR9AG6MmvZJd%v#2=l?P@6s;=x?6ntYda#)rqNeOhcvg4%TY2Ruk#*pTZBNh%}0_ zq5A}x8N6F5>yusZhM;@Oa$MTu3Oqn}V*;kZq5Idty%6GX>mfy)4y96x=vg#OT;vR$ zwb;-S!H$dgjB~1-N#9RdBeP^>srAbOlQ=M=40QCX69su`r-u4z-c)#5%FbWuXJIgp zs;rNhOtp}P7dpF%`!wYY-6=1Gl2JR?<8MlQE+Nbp9O z?m_6A^U^1`RAS2yX!k#StCGX(Bfsx2t9;e9Hl8T=mQFPkY}wRT;R+{I|9L=tz$h5b z<#s2+oD@yyVN(ND3A1ogy(+ulc1a2pFE|XyW*Lw<2*^>mA5mV+!5`uu)ZWI9#lptc zspdLV#*Ycl-^it=bDqoCK*`9)Qa`^!qBtQdkR0vm=HNK2Wfnbd{+4h{LOLdP~l(8G`*xNGb6t2WH%?k}-9+SY-MDq`Hx ztBZ+y!&AprZFMZAq#T+FRVDFcm{W)4WQX~%IqgPWGXCVQNGF*^zXqp)1y^+{1!KBL zQhd|QZ#8nGuJBm=qg_S!w;J*`g*unJZmEFEp$iF8)-U{<3}5VM`6B#=(H|ONO-^yY zLO#uC?BD#9%9?J-#JLO0Ghl2WWBBrdRvUrnn9?Gfvm7iT%|$Sb?THs{%jY;h_@l>R z>{=gbN9bDn0UclhE&hRt>J;6;cBmlMl!`XkBirpm3+b_dK5b}#EyDMFXx+iO_%qI5 zIOcop!knAu!?-yL&EB`JY3L94*+42`#@c68gV_$G=+)Qm_iYH#g%_`7=t0L=#7~g^`J1*22~ICv3Xi5S{u9MJud?UJ)qf*|l=Q_x zoL%A5tmnumlUec2t+1m?mG<$H2zcaj=}~N63S4%+{Xn;`ts$Y}%JnL9>N3j;W2)QL z>iWxsaPqU1_Chdc^V-p~Y6A|U2*Ik;CPO!4BE@!req3p3rujz8V?t^R9uJI@r}j%6 zP9fT$`6HnR?k<^uh-{^Y7Y^!S?gfKHe3Z6r7ImffC|xwCQX*XD=SHMHhG5CT^I@q~ zj=Mbib97HH?mWW$o%a}NqArnFg!(qRf8P)mNueu^?rs%|(1?IwPcRzGVplqi#4EiD z9GRun58tsPf%98!$!5X2`vh85yB%O~QH|2@?(!7xQo|o#W1FR~c(&V}k2H{vv~&yw z6IaO&OD=jqNR59HfbHeuE_p!M*1QypLvD@5t6dWidp54EXSu=LA22&I;p60jKF=qK zzc!+``O4|BnfIBwQ_GGoV;}7#)knN64Y7|}Xh^#g};!Z{v5>nIVqFUsTkNU)l{A@nX(Wn5YjgeHOYWy8q zPHBYmAI3duTLl<@@Fvz)hq%wud__aw$=a%0hiAuvWS-XX1wXVoglDzd{W30@|oEy3yB9r+-CFDl{q2cGi@aGR+XTPomEr>RRB)$A(PHTsu_Qr z%v^5NNUcSm7G3Ih36a9`>%^NGtf9h)=$S`}%^Z)`GYvm+Nqn&$5N1t$!`5`^D zM{hi^IR<@&If`_@Edd5Y>Q{={aWuQ*KI02@CdjftEA0>jp-u1QnXS2gp{(I`u@uId z@p`c2KK!L!xEneS)1{g5+TO!M$d@ICKBxx=M~ixGQEQ4hz;u(^ORDbS>{YAgp&2i7 zXMb1;yOkRbpGItSU9=q;7vEiX{IMKR1Xm-6)=_hpKM+bymX(Ksk?!`aH3P#=g90;{ zY854`&;A0nE$PoK6inHC)r-X!eElV1tfo?2fz?avyL1H>$9cs$N4+vv{od^Bj|~m^ zcjOECBAp4z;S<6i@3&aUY9FbYF=HuumC3BhMmSr)PLpK~f0sD|g9k466?Jaf){>^@ zq~+9379huG{>>+AkZVm!6?^-oPlpX*8CgkYvzBa4il5;;I9D)|Qqc)!Qjv@xR9MZ4Ec%-vkK;ZKMidLe4a3}a=Lh5Jal#qY5$pR@f;Z2 ziZuOb>_)$0jlf=cICSOM-~DBj2nM3;0EC^yPq_qjRnf9Y0;cdYR(sht-uu2))CHB1 zIuvvz2$J~ZoJ+*7;xBKd5*WQ~RztXVeHH#b1C@_8%g3(rf(ZCFiHJK=R5dZHzD5KT z?2#Vuwxg_7gHQ=g)cy=Z) z3;WHquqYMmsh8OLuqwVV6s)gpUZfX}=eUc?ITG)yMyLb0<4jP3RypaO@cS*-;d*x# z3NuKB`IXnon(a-w5|9sXVR=`~4$CHhId>W?SNciH zUl8p`x?N_u&lXFiUM5jX69btmTA5aUeM5^DV*^3u2;TExhimAdgvvKidj;$`?uFZ+ zC(=DujQ?uy_CAjF9gwkvL1wDSKRxbeQU=J$BHSR&3&AcFZGehmOc83x*8ixT)X*no zvVgEKkR&k5`e!zhR*Tv;<^rMpmOd4Z$x7*zcyi>!+;w}3{DhGc#~5=lK^r^e;&ENY ziYR7;QK*N;(z1ig-R0aGr4-x3kDp?3%22y@!m^4hi&%HAi=gi#Omc~>ijk$)yQn8` zI%^jKA;BjzXmmpM#7URF)NpydtQGj=gTxJ#SLaT4IYcwz9z~$11ge_Ho=|{u&jCO(n)I(>_PD zRfifwXq=FuQ)D`RtGc>NHPEn#o?{Vabsg@oHsEAHpuk3#PB5NHDSS@JlWk*~S#PYqG8N=;N8T`@*y_NpQ5s2nc59==yEv44_ z=wa)kOEO!8VMXp-qIg=yyw{<6}5Rr(Lr*P)|6U1M#DNlzdt9Te%qER_Z z-^nOvSU4bnD&g7nwd}H&h~PEvt=fEBq|NIU@0#OiC5lQrnekKs>c`q9rahC8VcI2> zHOQv&Aq+;sG{qE}@XM0YT#|uZp0dJl72C~%4=0Xq3$+K4+pJb~uYs{zToUTNpQJ_T z*ui`++Gi`ZN~7cHf<_~ucD7^<_ZIX^c*UCTee9UAmW3~!9v3ADhyY5tw@?JZT%VsB z@x6FC&6^PqJWcs^3x{qgzck*y{prE{b?@fjbxdAN|zwPD5Q*O2T&x`1-^I|ZYg~ zn*!=|y8tq&lx&Bm6v~sIDG_Z!UkR*OvxD?T#E@A;9`Bve17p7A_!B2^qy$taDX7B% zr%~Te7_$pjRON9msIS8h#fdoSozRWwe+qw```UL(Vk+KTuza#$nh4Mnqvue+fCjmI zI>Sp!llq`mq>tWY8ht_4c6Hxz#b#;}XZS58ziGJ&rEyh{kD?t@;80aUH$Cm53J!a` zDN)e7cAhIJL{IN3lwW7_9OztHr68+_B!yy?0;n&kcbaPK@Qs+;fp_1yaj8g*c zIF6V0bLmhBy?a+b%>1ss4ufZPpH^KTa*gg4X9}O zv<_?qm5BO4O4v+n1LwvEgp|LjEm%0CBaH}=D&-iUu~*HPn{8-v9={&(+|nMp_c)xW zYmZ2=pLRSLV5fY2BqR?}P!U}|5NLIQDf*N|ia<$ylbm|?{%3dboCnQ$r=Rp9JfJ#2 zPVNT(1C61dWg=aF6@(o6m~0_&IHc$JWn$B25r5G#M)$z1QvAkwlO3kA(_a6-p{d>wkojo z+XW`!Z(%CE1^ZmY?_=7J@M4j)Gh~93{9=Lk2!#3z$|-@aq+=aRQ?bdZbh8M&nKw~p zz*yH`n$N>8OT}?X;q(de^U*ASz?s>?wBcSO4TVqR$F@SpWH?_YUA)$WaUR24xwM(o za#=>z3vk)hlgQPh=6#~3@Y~xD{JI3AJHP+nnzPlb_L_diH>OIm)RJwMip3EtDqXh)y8!%R~vgd~uZld_jSJ+et3^MH-+go1aR0S^T zk(8arKt|s^nF5zP&KOhStl~KbGr5w1o8`?v#nEDnULfn$7B{+D;ES`w0 zYSb|NBJRDRD&f_j`lp3@Hxg&*m;o*--cFD?Mhe-moEgQ4Ha!z{YA81*ulNCLTfO;w zJl#r6rqtW_$X9XY;&ykjj5@FVfKXP6nH6|j5)u}i6^-kGZ-j*ROE1bU$D*nJ{u#lN z&mmE+u8}M#763N#sY~(^*s051{kyoAG$neWGY%@Dxk|am^ga2v3+Pz)*x0@_R)+NT zugUCP*(ey_Lf08&WQW-Idl3&Qt@@f9C7)XvvAU#Ur`B#F!Tc(xfzL&o zu&Bb_e~j;nm#(nc#e2e9Y(R3-r@wTyxhy~PdLB={Nr!JBu7xJ&EXUzUWJ?4Uu$_#^ zW#jBa*>^8P+e?Cl=M{)LL*dq~VaohipNx1qXU5COdk$Af*FK`iF|+C3;z?Vo2|sWw z470aK^iL&hN);*<;qHFx1n&bzMF>i6oP06W3(GeH`Ul99Jt!mS6LMo8m81fdiP~}Z zh_El)X9cF&o95m=nB>n~%4cCER5@`xcoYnZ>^Rf|#j}74vE|qq=wc)?K1rLxB!s}I zUq&iE*|l>#;o_^E{sDvD=cx%`4J1%Hgmr9ICjI^tQ~F&KeUFZnwN8z~9_Am2DxGw0e|dN3 zJMD*#SydzVW7y)Zg9`sz+T(5?Ub~uJB>UtCz^zKrEU&G62$7HY9PKkAbJ1)>y%58x zv;mA #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; + } +}