From 24a85a10ef05e0babd885640e91b2a941ece994d Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 8 Jun 2021 02:14:25 +0200 Subject: [PATCH 1/2] [RF] Handle nullptr or empty norm set ambiguity in RooAbsReal::getVal The downsteam code, -- like RooAddPdf::getValV for example -- assume that a nullptr is passed when no normalization is requested. The case of an empty norm set is not handled correctly in RooAddPdf::getValV, leading to wrong results. However, some calling code passes an empty norm set to RooAbsReal::getVal instead of a `nullptr` in an attempt to disable normalization. This commit suggests to solve this ambiguity at the highest possible level: right at the beginning of RooAbsReal::getVal. If the normalization set is empty, the pointer pointing to it will be set to `nullptr`. This fixes issue #8307. --- roofit/roofitcore/inc/RooAbsReal.h | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/roofit/roofitcore/inc/RooAbsReal.h b/roofit/roofitcore/inc/RooAbsReal.h index 465407e5325b2..53eb63fea1119 100644 --- a/roofit/roofitcore/inc/RooAbsReal.h +++ b/roofit/roofitcore/inc/RooAbsReal.h @@ -89,6 +89,14 @@ class RooAbsReal : public RooAbsArg { /// These are integrated over their current ranges to compute the normalisation constant, /// and the unnormalised result is divided by this value. inline Double_t getVal(const RooArgSet* normalisationSet = nullptr) const { + // Sometimes, the calling code uses an empty RooArgSet to request evaluation + // without normalization set instead of following the `nullptr` convention. + // To remove this ambiguity which might not always be correctly handled in + // downstream code, we set `normalisationSet` to nullptr if it is pointing + // to an empty set. + if(normalisationSet && normalisationSet->empty()) { + normalisationSet = nullptr; + } #ifdef ROOFIT_CHECK_CACHED_VALUES return _DEBUG_getVal(normalisationSet); #else @@ -103,7 +111,13 @@ class RooAbsReal : public RooAbsArg { } /// Like getVal(const RooArgSet*), but always requires an argument for normalisation. - inline Double_t getVal(const RooArgSet& normalisationSet) const { return _fast ? _value : getValV(&normalisationSet) ; } + inline Double_t getVal(const RooArgSet& normalisationSet) const { + // Sometimes, the calling code uses an empty RooArgSet to request evaluation + // without normalization set instead of following the `nullptr` convention. + // To remove this ambiguity which might not always be correctly handled in + // downstream code, we set `normalisationSet` to nullptr if it is an empty set. + return _fast ? _value : getValV(normalisationSet.empty() ? nullptr : &normalisationSet) ; + } virtual Double_t getValV(const RooArgSet* normalisationSet = nullptr) const ; From b771e112d3b60acde90101b29d75529a2fc76046 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 8 Jun 2021 02:21:59 +0200 Subject: [PATCH 2/2] [RF] Add unit test for RooSimultaneous that covers #8307 --- roofit/roofitcore/test/CMakeLists.txt | 1 + .../roofitcore/test/testRooSimultaneous.cxx | 49 +++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 roofit/roofitcore/test/testRooSimultaneous.cxx diff --git a/roofit/roofitcore/test/CMakeLists.txt b/roofit/roofitcore/test/CMakeLists.txt index 7b55450808091..85e69a85a6afc 100644 --- a/roofit/roofitcore/test/CMakeLists.txt +++ b/roofit/roofitcore/test/CMakeLists.txt @@ -37,4 +37,5 @@ if(NOT MSVC OR win_broken_tests) endif() ROOT_ADD_GTEST(testRooProductPdf testRooProductPdf.cxx LIBRARIES RooFitCore) ROOT_ADD_GTEST(testNaNPacker testNaNPacker.cxx LIBRARIES RooFitCore) +ROOT_ADD_GTEST(testRooSimultaneous testRooSimultaneous.cxx LIBRARIES RooFitCore RooFit) diff --git a/roofit/roofitcore/test/testRooSimultaneous.cxx b/roofit/roofitcore/test/testRooSimultaneous.cxx new file mode 100644 index 0000000000000..1d9afb7b9297c --- /dev/null +++ b/roofit/roofitcore/test/testRooSimultaneous.cxx @@ -0,0 +1,49 @@ +// Tests for the RooSimultaneous +// Authors: Jonas Rembser, CERN 06/2021 + +#include "RooAddPdf.h" +#include "RooConstVar.h" +#include "RooCategory.h" +#include "RooDataSet.h" +#include "RooGaussian.h" +#include "RooRealVar.h" +#include "RooSimultaneous.h" +#include "RooProdPdf.h" + +#include "gtest/gtest.h" + +#include + +/// GitHub issue #8307. +/// A likelihood with a model wrapped in a RooSimultaneous ith one category +/// should give the same results as the likelihood with the model directly. +TEST(RooSimultaneous, ImportFromTreeWithCut) +{ + using namespace RooFit; + + RooRealVar x("x", "x", 0, 10); + RooRealVar mean("mean", "mean", 1., 0, 10); + RooRealVar width("width", "width", 1, 0.1, 10); + RooRealVar nsig("nsig", "nsig", 500, 100, 1000); + + RooGaussian gauss1("gauss1", "gauss1", x, mean, width); + RooGaussian fconstraint("fconstraint", "fconstraint", mean, RooConst(2.0), RooConst(0.2)); + + RooAddPdf model("model", "model", RooArgList(gauss1), RooArgList(nsig)); + RooProdPdf modelConstrained("modelConstrained", "modelConstrained", RooArgSet(model, fconstraint)); + + RooCategory cat("cat", "cat"); + cat.defineType("physics"); + + RooSimultaneous modelSim("modelSim", "modelSim", RooArgList{modelConstrained}, cat); + + std::unique_ptr data{model.generate(x)}; + RooDataSet combData("combData", "combData", x, Index(cat), Import("physics", *data)); + + RooArgSet constraints{fconstraint}; + + std::unique_ptr nllDirect{modelConstrained.createNLL(combData, Constrain(constraints))}; + std::unique_ptr nllSimWrapped{modelSim.createNLL(combData, Constrain(constraints))}; + + EXPECT_FLOAT_EQ(nllDirect->getVal(), nllSimWrapped->getVal()); +}