Sustainable fix for getParameters signature change problem

Follows up on `91b791a`, making sure that only the right overload is implemented. Also, apply the same fix to the `CachingAddNLL` class.
cms-analysis · Apr 18, 2024 · bcbec56 · bcbec56
1 parent fa36dd4
commit bcbec56
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 8 deletions.
diff --git a/interface/CachingNLL.h b/interface/CachingNLL.h
@@ -122,7 +122,11 @@ class CachingAddNLL : public RooAbsReal {
         Double_t defaultErrorLevel() const override { return 0.5; }
         void setData(const RooAbsData &data) ;
         virtual RooArgSet* getObservables(const RooArgSet* depList, Bool_t valueOnly = kTRUE) const ;
-        virtual RooArgSet* getParameters(const RooArgSet* depList, Bool_t stripDisconnected = kTRUE) const ;
+#if ROOT_VERSION_CODE < ROOT_VERSION(6,26,0)
+        RooArgSet* getParameters(const RooArgSet* depList, Bool_t stripDisconnected = kTRUE) const override;
+#else
+        bool getParameters(const RooArgSet* depList, RooArgSet& outputSet, bool stripDisconnected=true) const override;
+#endif
         double  sumWeights() const { return sumWeights_; }
         const RooAbsPdf *pdf() const { return pdf_; }
         void setZeroPoint() ;
@@ -168,8 +172,11 @@ class CachingSimNLL  : public RooAbsReal {
         Double_t defaultErrorLevel() const override { return 0.5; }
         void setData(const RooAbsData &data) ;
         virtual RooArgSet* getObservables(const RooArgSet* depList, Bool_t valueOnly = kTRUE) const ;
-        virtual RooArgSet* getParameters(const RooArgSet* depList, Bool_t stripDisconnected = kTRUE) const ;
+#if ROOT_VERSION_CODE < ROOT_VERSION(6,26,0)
+        RooArgSet* getParameters(const RooArgSet* depList, Bool_t stripDisconnected = kTRUE) const override;
+#else
         bool getParameters(const RooArgSet* depList, RooArgSet& outputSet, bool stripDisconnected=true) const override;
+#endif
         void splitWithWeights(const RooAbsData &data, const RooAbsCategory& splitCat, Bool_t createEmptyDataSets) ;
         static void setNoDeepLogEvalError(bool noDeep) { noDeepLEE_ = noDeep; }
         void setZeroPoint() ; 

diff --git a/src/CachingNLL.cc b/src/CachingNLL.cc
@@ -842,13 +842,27 @@ cacheutils::CachingAddNLL::getObservables(const RooArgSet* depList, Bool_t value
     return new RooArgSet();
 }
 
+// ROOT 6.26 changed the signature of getParameters to avoid heap allocation,
+// and especially returning an owning pointer that people tend to forget to
+// delete.
+#if ROOT_VERSION_CODE < ROOT_VERSION(6,26,0)
 RooArgSet* 
 cacheutils::CachingAddNLL::getParameters(const RooArgSet* depList, Bool_t stripDisconnected) const 
 {
     RooArgSet *ret = new RooArgSet(params_);
     ret->add(catParams_);
     return ret;
 }
+#else
+bool cacheutils::CachingAddNLL::getParameters(const RooArgSet* depList,
+                                              RooArgSet& outputSet,
+                                              bool stripDisconnected) const
+{
+    outputSet.add(params_);
+    outputSet.add(catParams_);
+    return true;
+}
+#endif
 
 
 cacheutils::CachingSimNLL::CachingSimNLL(RooSimultaneous *pdf, RooAbsData *data, const RooArgSet *nuis) :
@@ -1270,6 +1284,10 @@ cacheutils::CachingSimNLL::getObservables(const RooArgSet* depList, Bool_t value
     return new RooArgSet();
 }
 
+// ROOT 6.26 changed the signature of getParameters to avoid heap allocation,
+// and especially returning an owning pointer that people tend to forget to
+// delete.
+#if ROOT_VERSION_CODE < ROOT_VERSION(6,26,0)
 RooArgSet* 
 cacheutils::CachingSimNLL::getParameters(const RooArgSet* depList, Bool_t stripDisconnected) const 
 {
@@ -1284,14 +1302,22 @@ cacheutils::CachingSimNLL::getParameters(const RooArgSet* depList, Bool_t stripD
     if (hideConstants_) RooStats::RemoveConstantParameters(ret);
     return ret;
 }
-
+#else
 bool cacheutils::CachingSimNLL::getParameters(const RooArgSet* depList,
-                                                              RooArgSet& outputSet,
-                                                              bool stripDisconnected) const {
-  RooArgSet tempList = *getParameters(depList);
-  outputSet.add(tempList);
-  return true;
+                                              RooArgSet& outputSet,
+                                              bool stripDisconnected) const
+{
+    if (internalMasks_.empty()) {
+        outputSet.add(params_);
+        if (!hideRooCategories_) outputSet.add(catParams_);
+    } else {
+        outputSet.add(activeParameters_);
+        if (!hideRooCategories_) outputSet.add(activeCatParameters_);
+    }
+    if (hideConstants_) RooStats::RemoveConstantParameters(&outputSet);
+    return true;
 }
+#endif
 
 void cacheutils::CachingSimNLL::setMaskConstraints(bool flag) {
     double nllBefore = evaluate();