root/root-RF-Rewrite-RooProdPdf.TestGetPartIntList-unit-test.patch

From c20004230608a5679ee6c0a7648af5f190c415c9 Mon Sep 17 00:00:00 2001
From: Jonas Rembser <jonas.rembser@cern.ch>
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 3d779a4901..0a76c839aa 100644
--- a/roofit/roofitcore/inc/RooProdPdf.h
+++ b/roofit/roofitcore/inc/RooProdPdf.h
@@ -96,8 +96,6 @@ public:
 
   RooArgSet* findPdfNSet(RooAbsPdf const& pdf) const ;
 
-  void writeCacheToStream(std::ostream& os, RooArgSet const* nset) const;
-
   std::unique_ptr<RooAbsArg> 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 8c8c7e01c6..6af7279603 100644
--- a/roofit/roofitcore/src/RooProdPdf.cxx
+++ b/roofit/roofitcore/src/RooProdPdf.cxx
@@ -2324,10 +2324,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<RooArgSet> 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 d794fb415a..ff334211d2 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<std::vector<RooAbsArg *>> allPossibleSubset(RooAbsCollection const &arr)
+TEST(RooProdPdf, TestGetPartIntList)
 {
-   std::vector<std::vector<RooAbsArg *>> 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<RooArgSet>(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<RooRealVar &>(*ws.factory("x[0, 0, " + std::to_string(a) + "]"));
+   auto &y = static_cast<RooRealVar &>(*ws.factory("y[0, 0, " + std::to_string(b) + "]"));
+   auto &z = static_cast<RooRealVar &>(*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<RooProdPdf &>(*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)
-- 
2.39.2