[RF] Don't keep the resolution model as a server of RooAbsAnaConvPdf

README/ReleaseNotes/v642/index.md

@@ -71,6 +71,21 @@ Users are strongly encouraged to switch to the vectorized CPU backend if they ar
 
 If the vectorized backend does not work for a given use case, **please report it by opening an issue on the ROOT GitHub repository**.
 
+### 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
 
 ## Geometry
@@ -83,4 +98,4 @@ If the vectorized backend does not work for a given use case, **please report it
 
 The version of the following packages has been updated:
 
- - xrootd: 5.9.5
+ - xrootd: 5.9.5

roofit/hs3/src/JSONFactories_RooFitCore.cxx

@@ -921,7 +921,12 @@ class RooPoissonStreamer : public RooFit::JSONIO::Exporter {
 class RooDecayStreamer : public RooFit::JSONIO::Exporter {
 public:
    std::string const &key() const override;
-   bool exportObject(RooJSONFactoryWSTool *, const RooAbsArg *func, JSONNode &elem) const override
+   // 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 autoExportDependants() const override { return false; }
+   bool exportObject(RooJSONFactoryWSTool *tool, const RooAbsArg *func, JSONNode &elem) const override
    {
       auto *pdf = static_cast<const RooDecay *>(func);
       elem["type"] << key();
@@ -930,6 +935,10 @@ class RooDecayStreamer : public RooFit::JSONIO::Exporter {
       elem["resolutionModel"] << pdf->getModel().GetName();
       elem["decayType"] << pdf->getDecayType();
 
+      tool->queueExport(pdf->getT());
+      tool->queueExport(pdf->getTau());
+      tool->queueExport(pdf->getModel());
+
       return true;
    }
 };

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<RooAbsReal>) 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<RooAbsReal> _model" target="_model,_ownModel" \
+  code="{ _model = static_cast<RooResolutionModel*>(onfile._model.absArg()); _ownModel = false; }"
 #pragma link C++ class RooAddPdf+ ;
 #pragma link C++ class RooEfficiency+ ;
 #pragma link C++ class RooEffProd+ ;

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<RooAbsArg> 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 ;         ///<! Registry of analytical integration codes
 
-  ClassDefOverride(RooAbsAnaConvPdf,3) // Abstract Composite Convoluted PDF
+  ClassDefOverride(RooAbsAnaConvPdf, 4) // Abstract Composite Convoluted PDF
 };
 
 #endif

roofit/roofitcore/src/RooAbsAnaConvPdf.cxx

@@ -33,7 +33,8 @@
 ///
 ///  Classes derived from RooResolutionModel implement
 ///  \f[
-///     R_k(x,\bar{b},\bar{c}) = \int \mathrm{basis}_k(x', \bar{b}) \cdot \mathrm{resModel}(x-x',\bar{c}) \; \mathrm{d}x',
+///     R_k(x,\bar{b},\bar{c}) = \int \mathrm{basis}_k(x', \bar{b}) \cdot \mathrm{resModel}(x-x',\bar{c}) \;
+///     \mathrm{d}x',
 ///  \f]
 ///
 ///  which RooAbsAnaConvPdf uses to construct the pdf for [ Phys (x) R ] :
@@ -59,6 +60,40 @@
 ///  advertise the coefficient integration capabilities and implement them respectively.
 ///  Please see RooAbsPdf for additional details. Advertised analytical integrals must be
 ///  valid for all coefficients.
+///
+///  ### The resolution model is a configuration object, not a graph node
+///
+///  The resolution model passed to the constructor is **not** a node of the
+///  computation graph of the RooAbsAnaConvPdf. It is never evaluated directly;
+///  it only serves as a *configuration* object that specifies which resolution
+///  model should be convolved with the basis functions. From it, the
+///  RooAbsAnaConvPdf builds its own internal \f$ \mathrm{basis}_k \otimes
+///  \mathrm{resModel} \f$ convolution objects (one per declared basis function),
+///  and it is *those* convolutions that are the actual value servers of the pdf
+///  and that get evaluated.
+///
+///  Consequently, the resolution model itself is not a server of the
+///  RooAbsAnaConvPdf. It remains accessible via getModel() (for example for
+///  serialization), but it does not appear in the pdf's `servers()` list, in
+///  `getParameters()` / `getVariables()`, or in the printed computation graph.
+///
+///  \note **Behavior change in ROOT 6.42:** in earlier releases the resolution
+///  model was kept as a (non-value, non-shape) server of the RooAbsAnaConvPdf.
+///  As a side effect, importing a RooAbsAnaConvPdf into a RooWorkspace also
+///  dragged the original resolution model into the workspace (and into HS3/JSON
+///  exports), even though it played no role in the computation. As of ROOT 6.42
+///  this is no longer the case: a resolution model that is only used as the
+///  configuration of a RooAbsAnaConvPdf is not imported into the workspace on
+///  its own anymore.
+///
+///  Objects written with older ROOT versions are read back correctly via schema
+///  evolution: the resolution model is dropped as a server, so the *pdf's*
+///  computation graph is the same as for a freshly constructed one. Note,
+///  however, that if such an old file already stored the resolution model as a
+///  standalone RooWorkspace member (which used to happen on import), that member
+///  is *not* retroactively removed from the workspace on read-back -- it simply
+///  is no longer wired into the RooAbsAnaConvPdf. Only newly created workspaces
+///  are guaranteed to be free of the standalone resolution model.
 
 #include "RooAbsAnaConvPdf.h"
 
@@ -93,33 +128,37 @@ RooAbsAnaConvPdf::RooAbsAnaConvPdf() :
 /// Constructor. The supplied resolution model must be constructed with the same
 /// convoluted variable as this physics model ('convVar')
 
-RooAbsAnaConvPdf::RooAbsAnaConvPdf(const char *name, const char *title,
-               const RooResolutionModel& model, RooRealVar& cVar) :
-  RooAbsPdf(name,title), _isCopy(false),
-  _model("!model","Original resolution model",this,(RooResolutionModel&)model,false,false),
-  _convVar("!convVar","Convolution variable",this,cVar,false,false),
-  _convSet("!convSet","Set of resModel X basisFunc convolutions",this),
-  _coefNormMgr(this,10),
-  _codeReg(10)
+RooAbsAnaConvPdf::RooAbsAnaConvPdf(const char *name, const char *title, const RooResolutionModel &model,
+                                   RooRealVar &cVar)
+   : RooAbsPdf(name, title),
+     _isCopy(false),
+     _model{static_cast<RooResolutionModel *>(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<RooResolutionModel *>(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<RooResolutionModel*>(_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<RooResolutionModel*>(_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 '"
@@ -229,14 +312,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<RooResolutionModel&>(newModel).setAttribute(attrib.c_str());
-
-  redirectServers(RooArgSet{newModel}, false, true);
-
-  // reset temporary attribute for server redirection
-  const_cast<RooResolutionModel&>(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<RooResolutionModel *>(newModel.clone(newModel.GetName()));
+  _ownModel = true;
+  _model->setAttribute("NOCacheAndTrack");
 
   return false ;
 }
@@ -255,7 +339,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<RooResolutionModel*>(_model.absArg());
+  RooResolutionModel *conv = _model;
   assert(conv);
 
   std::unique_ptr<RooArgSet> modelDep {_model->getObservables(&vars)};
@@ -297,9 +381,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<RooTruthModel*>(_model.absArg())) {
-    return true ;
+  if (!TString(_convVar.absArg()->GetName()).CompareTo(arg.GetName()) && dynamic_cast<RooTruthModel *>(_model)) {
+     return true;
   }
 
   return RooAbsPdf::isDirectGenSafe(arg) ;

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