From c512572f997347c5f9bce4884fd4009c29a59e5a Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 7 Mar 2023 11:53:01 +0100 Subject: [PATCH] [RF] Rewrite `RooProdPdf.TestGetPartIntList` unit test In the ROOT 6.26 development cycle, the RooProdPdf was partially rewritten in moden C++ with less manual memory allocation to improve performance (PR #7907). In that PR, a unit test that verifies the RooProdPdf can correctly deal with factorizing PDFs was implemented. However, that test used an arbitrary PDF where the correct factorization was checked in a rather crude way: check by hashing the content of the RooProdPdf cache element for a given normalization set that said PR doesn't change any behavior (the reference hash was hardcoded in the unit test). This commit suggests a better alternative for the unit test, checking for a multidimensional product pdf of factorizing uniform pdfs that the pdf values for differenc normalization sets is as expected. This should cover the same functionality and is less fragile and implementation dependend than hashing the cache elements. This closes GitHub issue #12430, as the rewritten test is not affected anymore by the problem reported in that issue. --- roofit/roofitcore/inc/RooProdPdf.h | 2 - roofit/roofitcore/src/RooProdPdf.cxx | 4 -- roofit/roofitcore/test/testRooProdPdf.cxx | 88 +++++++---------------- 3 files changed, 26 insertions(+), 68 deletions(-) diff --git a/roofit/roofitcore/inc/RooProdPdf.h b/roofit/roofitcore/inc/RooProdPdf.h index 3d779a4901cec..0a76c839aa018 100644 --- a/roofit/roofitcore/inc/RooProdPdf.h +++ b/roofit/roofitcore/inc/RooProdPdf.h @@ -96,8 +96,6 @@ class RooProdPdf : public RooAbsPdf { RooArgSet* findPdfNSet(RooAbsPdf const& pdf) const ; - void writeCacheToStream(std::ostream& os, RooArgSet const* nset) const; - std::unique_ptr compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileContext & ctx) const override; private: diff --git a/roofit/roofitcore/src/RooProdPdf.cxx b/roofit/roofitcore/src/RooProdPdf.cxx index 876f6946df758..75fa2c15003cb 100644 --- a/roofit/roofitcore/src/RooProdPdf.cxx +++ b/roofit/roofitcore/src/RooProdPdf.cxx @@ -2318,10 +2318,6 @@ void RooProdPdf::CacheElem::writeToStream(std::ostream& os) const { } } -void RooProdPdf::writeCacheToStream(std::ostream& os, RooArgSet const* nset) const { - getCacheElem(nset)->writeToStream(os); -} - std::unique_ptr RooProdPdf::fillNormSetForServer(RooArgSet const &normSet, RooAbsArg const &server) const { if (normSet.empty()) diff --git a/roofit/roofitcore/test/testRooProdPdf.cxx b/roofit/roofitcore/test/testRooProdPdf.cxx index d794fb415a4f1..ff334211d23da 100644 --- a/roofit/roofitcore/test/testRooProdPdf.cxx +++ b/roofit/roofitcore/test/testRooProdPdf.cxx @@ -74,76 +74,40 @@ INSTANTIATE_TEST_SUITE_P(RooProdPdf, TestProdPdf, return ss.str(); }); -std::vector> allPossibleSubset(RooAbsCollection const &arr) +TEST(RooProdPdf, TestGetPartIntList) { - std::vector> out; - std::size_t n = arr.size(); - - std::size_t count = std::pow(2, n); - for (std::size_t i = 0; i < count; i++) { - out.emplace_back(); - auto &back = out.back(); - for (std::size_t j = 0; j < n; j++) { - if ((i & (1 << j)) != 0) { - back.push_back(arr[j]); - } - } - } - return out; -} + RooHelpers::LocalChangeMsgLevel chmsglvl1{RooFit::ERROR, 0u, RooFit::InputArguments, true}; + RooHelpers::LocalChangeMsgLevel chmsglvl2{RooFit::WARNING, 0u, RooFit::NumIntegration, true}; -// Hash the integral configuration for all possible normalization sets. -unsigned int hashRooProduct(RooProdPdf const &prod) -{ - RooArgSet params; - prod.getParameters(nullptr, params); - auto subsets = allPossibleSubset(params); + // This test checks if RooProdPdf::getPartIntList factorizes the integrals + // as expected, for the example of a three dimensional RooProdPdf. - std::stringstream ss; + RooWorkspace ws; - for (auto const &subset : subsets) { - // this can't be on the stack, otherwise we will always get the same - // address and therefore get wrong cache hits! - auto nset = std::make_unique(subset.begin(), subset.end()); - prod.writeCacheToStream(ss, nset.get()); - } + double a = 10.; + double b = 4.; + double c = 2.5; - std::string s = ss.str(); - return TString::Hash(s.c_str(), s.size()); -} + auto &x = static_cast(*ws.factory("x[0, 0, " + std::to_string(a) + "]")); + auto &y = static_cast(*ws.factory("y[0, 0, " + std::to_string(b) + "]")); + auto &z = static_cast(*ws.factory("z[0, 0, " + std::to_string(c) + "]")); -TEST(RooProdPdf, TestGetPartIntList) -{ - // This test checks if RooProdPdf::getPartIntList factorizes the integrals - // as expected. - // Instead of trying to construct tests for all possible cases by hand, - // this test creates a product where the factors have different patters of - // overlapping parameters. To make sure all possible cases are covered, we - // are using all possible subsets of the parameters one after the other to - // create the reference test result. - - RooRealVar x{"x", "x", 1., 0, 10}; - RooRealVar y{"y", "y", 1., 0, 10}; - RooRealVar z{"z", "z", 1., 0, 10}; - - RooRealVar m1{"m1", "m1", 1., 0, 10}; - RooRealVar m2{"m2", "m2", 1., 0, 10}; - RooRealVar m3{"m3", "m3", 1., 0, 10}; - - RooGenericPdf gauss1{"gauss1", "gauss1", "x+m1", {x, m1}}; - RooGenericPdf gauss2{"gauss2", "gauss2", "x+m2", {x, m2}}; - RooGenericPdf gauss3{"gauss3", "gauss3", "y+m3", {y, m3}}; - RooGenericPdf gauss4{"gauss4", "gauss4", "z+m1", {z, m1}}; - RooGenericPdf gauss5{"gauss5", "gauss5", "x+m1", {x, m1}}; + // Factorize the product in one 1D and one 2D pdf to get a more complicated + // and complete test case. + ws.factory("Uniform::pdf1({x})"); + ws.factory("Uniform::pdf2({y, z})"); // Product of all the pdfs. - RooProdPdf prod{"prod", "prod", RooArgList{gauss1, gauss2, gauss3, gauss4, gauss5}}; - - // We hash the string serializations of caches for all possible - // normalization sets and compare it to the expected hash. - // This value must be updated if the convention for integral names in - // RooProdPdf changes. - EXPECT_EQ(hashRooProduct(prod), 2448666198); + 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); } TEST(RooProdPdf, TestDepsAreCond)