From d2ed30aecf61a2548b3cd13794e5b14fed37e0c1 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Wed, 27 Nov 2024 14:34:11 +0100 Subject: [PATCH] [RF] Forbid calling `getVal()` with r-value references to norm set Calling RooAbsReal::getVal() with r-value references to the normalization set (i.e. with RooArgSets that are created in place) is a bad idea, because it breaks RooFits caching logic and potentially introduces significant overhead. There was even a dedicated Pythonization to deal with the related memory problems. Since to avoid the problems and the need for a workaround in Python, this commit simply suggests to forbid calling getVal() with r-value references at runtime by printing a clear exception. --- .../_pythonization/_roofit/_rooabsreal.py | 16 ------------ roofit/histfactory/test/testHistFactory.cxx | 4 ++- roofit/roofit/test/testRooPoisson.cxx | 4 ++- roofit/roofitcore/inc/RooAbsReal.h | 2 ++ roofit/roofitcore/src/RooAbsReal.cxx | 26 +++++++++++++++++++ roofit/roofitcore/test/testRooAbsPdf.cxx | 4 ++- roofit/roofitcore/test/testRooDataHist.cxx | 3 ++- roofit/roofitcore/test/testRooProdPdf.cxx | 25 ++++++++++++------ roofit/roofitcore/test/testRooWrapperPdf.cxx | 3 ++- 9 files changed, 58 insertions(+), 29 deletions(-) diff --git a/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabsreal.py b/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabsreal.py index 5ff1986129f67..60f6839a05262 100644 --- a/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabsreal.py +++ b/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabsreal.py @@ -116,22 +116,6 @@ def chi2FitTo(self, *args, **kwargs): args, kwargs = _kwargs_to_roocmdargs(*args, **kwargs) return self._chi2FitTo(*args, **kwargs) - def getVal(self, normalizationSet=None): - # We do the conversion to RooArgSet now, such that we can keep alive - # the normalization set by setting it as an attribute of this - # RooAbsReal. - if isinstance(normalizationSet, (set, list, tuple)): - import ROOT - - normalizationSet = ROOT.RooArgSet(normalizationSet) - # With the pythonizations, we have the opportunity to use the Python - # reference counting to make sure the last normalization set doesn't - # get deleted under our feet (RooFit tries to use it by pointer when - # you call getVal() without any normalization set the next time). - if normalizationSet: - self._getVal_normSet = normalizationSet - return self._getVal(normalizationSet) if normalizationSet else self._getVal() - def setEvalErrorLoggingMode(m): import ROOT diff --git a/roofit/histfactory/test/testHistFactory.cxx b/roofit/histfactory/test/testHistFactory.cxx index 035d3c2ae355e..d82d05f6a3dd4 100644 --- a/roofit/histfactory/test/testHistFactory.cxx +++ b/roofit/histfactory/test/testHistFactory.cxx @@ -40,13 +40,15 @@ const bool writeJsonFiles = false; std::vector getValues(RooAbsReal const &real, RooRealVar &obs, bool normalize, bool useBatchMode) { + RooArgSet normSet{obs}; + std::vector out; // We want to evaluate the function at the bin centers std::vector binCenters(obs.numBins()); for (int iBin = 0; iBin < obs.numBins(); ++iBin) { obs.setBin(iBin); binCenters[iBin] = obs.getVal(); - out.push_back(normalize ? real.getVal(obs) : real.getVal()); + out.push_back(normalize ? real.getVal(normSet) : real.getVal()); } if (useBatchMode == false) { diff --git a/roofit/roofit/test/testRooPoisson.cxx b/roofit/roofit/test/testRooPoisson.cxx index 3f378ff0c5157..aa757027ea429 100644 --- a/roofit/roofit/test/testRooPoisson.cxx +++ b/roofit/roofit/test/testRooPoisson.cxx @@ -15,11 +15,13 @@ TEST(RooPoisson, Bare) { RooRealVar lambda("lambda", "lambda", 1); RooPoisson pois("pois", "pois", x, lambda); + RooArgSet normSet{x}; + auto Poisson = [&](int val, double lambdaVal, double target) { x = val; lambda = lambdaVal; - EXPECT_NEAR(pois.getVal(x), target, 5.E-16) + EXPECT_NEAR(pois.getVal(normSet), target, 5.E-16) << "Test was Pois(" << val << " | " << lambdaVal << ")"; }; diff --git a/roofit/roofitcore/inc/RooAbsReal.h b/roofit/roofitcore/inc/RooAbsReal.h index 5300c32c3b84a..61a28dbe41ef8 100644 --- a/roofit/roofitcore/inc/RooAbsReal.h +++ b/roofit/roofitcore/inc/RooAbsReal.h @@ -131,6 +131,8 @@ class RooAbsReal : public RooAbsArg { return _fast ? _value : getValV(normalisationSet.empty() ? nullptr : &normalisationSet) ; } + double getVal(RooArgSet &&) const; + virtual double getValV(const RooArgSet* normalisationSet = nullptr) const ; double getPropagatedError(const RooFitResult &fr, const RooArgSet &nset = {}) const; diff --git a/roofit/roofitcore/src/RooAbsReal.cxx b/roofit/roofitcore/src/RooAbsReal.cxx index f1bb7c1db4a7d..defbad2c803bb 100644 --- a/roofit/roofitcore/src/RooAbsReal.cxx +++ b/roofit/roofitcore/src/RooAbsReal.cxx @@ -4475,3 +4475,29 @@ void RooAbsReal::enableOffsetting(bool flag) RooAbsReal::Ref::Ref(double val) : _ref{RooFit::RooConst(val)} {} + +//////////////////////////////////////////////////////////////////////////////// +/// Calling RooAbsReal::getVal() with an r-value reference is a common +/// performance trap, because this usually happens when implicitly constructing +/// the RooArgSet to be used as the parameter (for example, in calls like +/// `pdf.getVal(x)`). +/// +/// Creating the RooArgSet can cause quite some overhead, especially when the +/// evaluated object is just a simple variable. Even worse, many RooFit objects +/// internally cache information using the uniqueId() of the normalization set +/// as the key. So by constructing normalization sets in place, RooFits caching +/// logic is broken. +/// +/// To avoid these kind of problems, getVal() will just throw an error when +/// it's called with an r-value reference. This also catches the cases where +/// one uses it in Python, implicitly creating the normalization set from a +/// Python list or set. +double RooAbsReal::getVal(RooArgSet &&) const +{ + std::stringstream errMsg; + errMsg << "calling RooAbsReal::getVal() with r-value references to the normalization set is not allowed, because " + "it breaks RooFits caching logic and potentially introduces significant overhead. Please explicitly " + "create the RooArgSet outside the call to getVal()."; + coutF(Eval) << errMsg.str() << std::endl; + throw std::runtime_error(errMsg.str()); +} diff --git a/roofit/roofitcore/test/testRooAbsPdf.cxx b/roofit/roofitcore/test/testRooAbsPdf.cxx index 0ac357bdd897f..814a6c0eb1898 100644 --- a/roofit/roofitcore/test/testRooAbsPdf.cxx +++ b/roofit/roofitcore/test/testRooAbsPdf.cxx @@ -389,8 +389,10 @@ TEST(RooAbsPdf, NormSetChange) RooAddition add("add", "add", {gauss}); + RooArgSet normSet{x}; + double v1 = add.getVal(); - double v2 = add.getVal(x); + double v2 = add.getVal(normSet); // The change of normalization set should trigger a recomputation of the // value, so val2 should be different from val1. } diff --git a/roofit/roofitcore/test/testRooDataHist.cxx b/roofit/roofitcore/test/testRooDataHist.cxx index 8581b98265197..fccaa2749dbd0 100644 --- a/roofit/roofitcore/test/testRooDataHist.cxx +++ b/roofit/roofitcore/test/testRooDataHist.cxx @@ -740,9 +740,10 @@ TEST_P(WeightsTest, VectorizedWeights) } std::vector weightsGetVal(nVals); + RooArgSet normSet{x}; for (std::size_t i = 0; i < nVals; ++i) { x.setVal(xVals[i]); - weightsGetVal[i] = absReal->getVal(x); + weightsGetVal[i] = absReal->getVal(normSet); } x.setVal(0.0); diff --git a/roofit/roofitcore/test/testRooProdPdf.cxx b/roofit/roofitcore/test/testRooProdPdf.cxx index 34fb559cd1173..4eb3997604e83 100644 --- a/roofit/roofitcore/test/testRooProdPdf.cxx +++ b/roofit/roofitcore/test/testRooProdPdf.cxx @@ -99,14 +99,23 @@ TEST(RooProdPdf, TestGetPartIntList) // Product of all the pdfs. auto &prod = static_cast(*ws.factory("PROD::prod(pdf1, pdf2)")); - EXPECT_DOUBLE_EQ(prod.getVal({}), 1.0); - EXPECT_DOUBLE_EQ(prod.getVal({x}), 1. / a); - EXPECT_DOUBLE_EQ(prod.getVal({y}), 1. / b); - EXPECT_DOUBLE_EQ(prod.getVal({z}), 1. / c); - EXPECT_DOUBLE_EQ(prod.getVal({x, y}), 1. / a / b); - EXPECT_DOUBLE_EQ(prod.getVal({x, z}), 1. / a / c); - EXPECT_DOUBLE_EQ(prod.getVal({y, z}), 1. / b / c); - EXPECT_DOUBLE_EQ(prod.getVal({x, y, z}), 1. / a / b / c); + RooArgSet normSetNada{}; + RooArgSet normSetX{x}; + RooArgSet normSetY{y}; + RooArgSet normSetZ{z}; + RooArgSet normSetXY{x, y}; + RooArgSet normSetXZ{x, z}; + RooArgSet normSetYZ{y, z}; + RooArgSet normSetXYZ{x, y, z}; + + EXPECT_DOUBLE_EQ(prod.getVal(normSetNada), 1.0); + EXPECT_DOUBLE_EQ(prod.getVal(normSetX), 1. / a); + EXPECT_DOUBLE_EQ(prod.getVal(normSetY), 1. / b); + EXPECT_DOUBLE_EQ(prod.getVal(normSetZ), 1. / c); + EXPECT_DOUBLE_EQ(prod.getVal(normSetXY), 1. / a / b); + EXPECT_DOUBLE_EQ(prod.getVal(normSetXZ), 1. / a / c); + EXPECT_DOUBLE_EQ(prod.getVal(normSetYZ), 1. / b / c); + EXPECT_DOUBLE_EQ(prod.getVal(normSetXYZ), 1. / a / b / c); } TEST(RooProdPdf, TestDepsAreCond) diff --git a/roofit/roofitcore/test/testRooWrapperPdf.cxx b/roofit/roofitcore/test/testRooWrapperPdf.cxx index 29ff9066fc62e..48a6603be76fa 100644 --- a/roofit/roofitcore/test/testRooWrapperPdf.cxx +++ b/roofit/roofitcore/test/testRooWrapperPdf.cxx @@ -29,7 +29,8 @@ TEST(RooWrapperPdf, Basics) RooWrapperPdf polPdf("polPdf", "polynomial PDF", pol); - EXPECT_GT(pol.getVal(x)*1.05, polPdf.getVal(x)) << "Wrapper pdf normalises."; + RooArgSet normSet{x}; + EXPECT_GT(pol.getVal(normSet)*1.05, polPdf.getVal(normSet)) << "Wrapper pdf normalises."; RooArgSet intSet(x); RooArgSet numSet;