From 345e7261c96302befefced4daa89f2a5b7d0a625 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 29 Sep 2022 16:10:18 +0200 Subject: [PATCH 1/9] [RF] Add `print` function for debugging of `AddCacheElem` This commit doesn't change any behavior. It only adds a new helper function that can be used for debugging the `RooAddPdf` cache elements. --- roofit/roofitcore/src/RooAddHelpers.cxx | 20 ++++++++++++++++++++ roofit/roofitcore/src/RooAddHelpers.h | 2 ++ 2 files changed, 22 insertions(+) diff --git a/roofit/roofitcore/src/RooAddHelpers.cxx b/roofit/roofitcore/src/RooAddHelpers.cxx index f59668d09670c..be86593577cc5 100644 --- a/roofit/roofitcore/src/RooAddHelpers.cxx +++ b/roofit/roofitcore/src/RooAddHelpers.cxx @@ -206,6 +206,26 @@ AddCacheElem::AddCacheElem(RooAbsPdf const &addPdf, RooArgList const &pdfList, R } } +void AddCacheElem::print() const +{ + auto printVector = [](auto const &vec, const char *name) { + std::cout << "+++ " << name << ":" << std::endl; + for (auto const &arg : vec) { + std::cout << " "; + if (arg) + arg->Print(); + else + std::cout << "nullptr" << std::endl; + } + }; + + printVector(_suppNormList, "_suppNormList"); + printVector(_projList, "_projList"); + printVector(_suppProjList, "_suppProjList"); + printVector(_refRangeProjList, "_refRangeProjList"); + printVector(_rangeProjList, "_rangeProjList"); +} + //////////////////////////////////////////////////////////////////////////////// /// List all RooAbsArg derived contents in this cache element diff --git a/roofit/roofitcore/src/RooAddHelpers.h b/roofit/roofitcore/src/RooAddHelpers.h index 20e1294e02fed..473bcb5b5d262 100644 --- a/roofit/roofitcore/src/RooAddHelpers.h +++ b/roofit/roofitcore/src/RooAddHelpers.h @@ -39,6 +39,8 @@ class AddCacheElem : public RooAbsCacheElement { inline double rangeProjScaleFactor(std::size_t idx) const { return rangeProjVal(idx) / refRangeProjVal(idx); } + void print() const; + private: inline double refRangeProjVal(std::size_t idx) const { From d5755de9315726c781667769434ab8132f2a315d Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 29 Sep 2022 15:54:09 +0200 Subject: [PATCH 2/9] [RF] Fix RooAddPdf coefficient cache for custom `normRange()` cases The function that created the RooAddPdf cache elements was completely broken once either the AddPdf or one of its components had a custom normalization set defined via `RooAbsPdf::setNormSet()`. To make this work, I was rethinking and rewriting the cache creation logic to hopefully cover more cases correctly. There are some PRs coming up where I will use `RooAbsPdf::setNormSet()` for all ranged fits, so the changes in this commit will get a lot of test coverage from that. --- roofit/roofitcore/inc/RooAddModel.h | 2 +- roofit/roofitcore/inc/RooAddPdf.h | 2 +- roofit/roofitcore/src/RooAddGenContext.cxx | 2 +- roofit/roofitcore/src/RooAddHelpers.cxx | 126 +++++++-------------- roofit/roofitcore/src/RooAddHelpers.h | 24 ++-- roofit/roofitcore/src/RooAddModel.cxx | 12 +- roofit/roofitcore/src/RooAddPdf.cxx | 16 +-- 7 files changed, 68 insertions(+), 116 deletions(-) diff --git a/roofit/roofitcore/inc/RooAddModel.h b/roofit/roofitcore/inc/RooAddModel.h index d279aa3a89db8..216b168c874c9 100644 --- a/roofit/roofitcore/inc/RooAddModel.h +++ b/roofit/roofitcore/inc/RooAddModel.h @@ -99,7 +99,7 @@ class RooAddModel : public RooResolutionModel { mutable RooObjCacheManager _projCacheMgr ; ///getProjCache(_vars.get()) ; } else { RooAddPdf* apdf = (RooAddPdf*) _pdf ; - _pcache = apdf->getProjCache(_vars.get(),nullptr,"FULL_RANGE_ADDGENCONTEXT") ; + _pcache = apdf->getProjCache(_vars.get(),nullptr) ; } // Forward initGenerator call to all components diff --git a/roofit/roofitcore/src/RooAddHelpers.cxx b/roofit/roofitcore/src/RooAddHelpers.cxx index be86593577cc5..590eea730b667 100644 --- a/roofit/roofitcore/src/RooAddHelpers.cxx +++ b/roofit/roofitcore/src/RooAddHelpers.cxx @@ -16,15 +16,20 @@ #include #include #include +#include //////////////////////////////////////////////////////////////////////////////// /// Create a RooAddPdf cache element for a given normalization set and /// projection configuration. AddCacheElem::AddCacheElem(RooAbsPdf const &addPdf, RooArgList const &pdfList, RooArgList const &coefList, - const RooArgSet *nset, const RooArgSet *iset, const char *rangeName, bool projectCoefs, - RooArgSet const &refCoefNorm, TNamed const *refCoefRangeName, int verboseEval) + const RooArgSet *nset, const RooArgSet *iset, bool projectCoefs, + RooArgSet const &refCoefNormSet, std::string const &refCoefNormRange, int verboseEval) { + // We put the normRange into a std::string to not have to deal with + // nullptr vs. "" ambiguities + const std::string normRange = addPdf.normRange() ? addPdf.normRange() : ""; + _suppNormList.reserve(pdfList.size()); // *** PART 1 : Create supplemental normalization list *** @@ -36,11 +41,15 @@ AddCacheElem::AddCacheElem(RooAbsPdf const &addPdf, RooArgList const &pdfList, R fullDepList.remove(*iset, true, true); } + bool hasPdfWithCustomRange = false; + // Fill with dummy unit RRVs for now for (std::size_t i = 0; i < pdfList.size(); ++i) { auto pdf = static_cast(pdfList.at(i)); auto coef = static_cast(coefList.at(i)); + hasPdfWithCustomRange |= pdf->normRange() != nullptr; + // Start with full list of dependents RooArgSet supNSet(fullDepList); @@ -63,6 +72,17 @@ AddCacheElem::AddCacheElem(RooAbsPdf const &addPdf, RooArgList const &pdfList, R << " making supplemental normalization set " << supNSet << " for pdf component " << pdf->GetName() << std::endl; } + + if (!normRange.empty()) { + auto snormTerm = std::unique_ptr(pdf->createIntegral(*nset, *nset, normRange.c_str())); + if (snorm) { + auto oldSnorm = std::move(snorm); + snorm = std::make_unique("snorm", "snorm", *oldSnorm.get(), *snormTerm.get()); + snorm->addOwnedComponents(std::move(snormTerm), std::move(oldSnorm)); + } else { + snorm = std::move(snormTerm); + } + } _suppNormList.emplace_back(std::move(snorm)); } @@ -74,12 +94,8 @@ AddCacheElem::AddCacheElem(RooAbsPdf const &addPdf, RooArgList const &pdfList, R // *** PART 2 : Create projection coefficients *** - // cout << " this = " << this << " (" << GetName() << ")" << std::endl ; - // cout << "projectCoefs = " << (_projectCoefs?"T":"F") << std::endl ; - // cout << "_normRange.Length() = " << _normRange.Length() << std::endl ; - // If no projections required stop here - if (!projectCoefs && !rangeName) { + if (!projectCoefs && !hasPdfWithCustomRange) { // cout << " no projection required" << std::endl ; return; } @@ -94,36 +110,26 @@ AddCacheElem::AddCacheElem(RooAbsPdf const &addPdf, RooArgList const &pdfList, R << ")::getPC nset = " << (nset ? *nset : RooArgSet()) << " nset2 = " << nset2 << std::endl; - if (nset2.empty() && !refCoefNorm.empty()) { + if (nset2.empty() && !refCoefNormSet.empty()) { // cout << "WVE: evaluating RooAddPdf without normalization, but have reference normalization for coefficient // definition" << std::endl ; - nset2.add(refCoefNorm); - if (refCoefRangeName) { - rangeName = RooNameReg::str(refCoefRangeName); - } + nset2.add(refCoefNormSet); } - // Check if requested transformation is not identity - if (!nset2.equals(refCoefNorm) || refCoefRangeName != 0 || rangeName != 0 || addPdf.normRange()) { - - oocxcoutD(&addPdf, Caching) << "ALEX: " << addPdf.ClassName() << "::syncCoefProjList(" << addPdf.GetName() - << ") projecting coefficients from " << nset2 << (rangeName ? ":" : "") - << (rangeName ? rangeName : "") << " to " - << ((!refCoefNorm.empty()) ? refCoefNorm : nset2) << (refCoefRangeName ? ":" : "") - << (refCoefRangeName ? RooNameReg::str(refCoefRangeName) : "") << std::endl; + if (!nset2.equals(refCoefNormSet) || !refCoefNormRange.empty() || !normRange.empty() || hasPdfWithCustomRange) { // Recalculate projection integrals of PDFs for (auto *pdf : static_range_cast(pdfList)) { // Calculate projection integral std::unique_ptr pdfProj; - if (!nset2.equals(refCoefNorm)) { - pdfProj = std::unique_ptr{pdf->createIntegral(nset2, refCoefNorm, addPdf.normRange())}; + if (!refCoefNormSet.empty() && !nset2.equals(refCoefNormSet)) { + pdfProj = std::unique_ptr{pdf->createIntegral(nset2, refCoefNormSet, normRange.c_str())}; pdfProj->setOperMode(addPdf.operMode()); oocxcoutD(&addPdf, Caching) << addPdf.ClassName() << "(" << addPdf.GetName() << ")::getPC nset2(" << nset2 - << ")!=_refCoefNorm(" << refCoefNorm << ") --> pdfProj = " << pdfProj->GetName() - << std::endl; + << ")!=_refCoefNormSet(" << refCoefNormSet + << ") --> pdfProj = " << pdfProj->GetName() << std::endl; oocxcoutD(&addPdf, Caching) << " " << addPdf.ClassName() << "::syncCoefProjList(" << addPdf.GetName() << ") PP = " << pdfProj->GetName() << std::endl; } @@ -131,13 +137,13 @@ AddCacheElem::AddCacheElem(RooAbsPdf const &addPdf, RooArgList const &pdfList, R _projList.emplace_back(std::move(pdfProj)); // Calculation optional supplemental normalization term - RooArgSet supNormSet(refCoefNorm); + RooArgSet supNormSet(refCoefNormSet); auto deps = std::unique_ptr{pdf->getParameters(RooArgSet())}; supNormSet.remove(*deps, true, true); std::unique_ptr snorm; auto name = std::string(addPdf.GetName()) + "_" + pdf->GetName() + "_ProjSupNorm"; - if (!supNormSet.empty() && !nset2.equals(refCoefNorm)) { + if (!supNormSet.empty() && !nset2.equals(refCoefNormSet)) { snorm = std::make_unique(name.c_str(), "Projection Supplemental normalization integral", RooRealConstant::value(1.0), supNormSet); oocxcoutD(&addPdf, Caching) << " " << addPdf.ClassName() << "::syncCoefProjList(" << addPdf.GetName() @@ -145,62 +151,17 @@ AddCacheElem::AddCacheElem(RooAbsPdf const &addPdf, RooArgList const &pdfList, R } _suppProjList.emplace_back(std::move(snorm)); - // Calculate reference range adjusted projection integral - std::unique_ptr rangeProj1; - - // cout << "ALEX >>>> RooAddPdf(" << GetName() << ")::getPC refCoefRangeName WVE = " - // <<(refCoefRangeName?":":"") << (refCoefRangeName?RooNameReg::str(refCoefRangeName):"") - // <<" refCoefRangeName AK = " << (refCoefRangeName?refCoefRangeName->GetName():"") - // << " && _refCoefNorm" << _refCoefNorm << " with size = _refCoefNorm.size() " << _refCoefNorm.size() << - // std::endl ; - - // Check if refCoefRangeName is identical to default range for all observables, - // If so, substitute by unit integral - - // ---------- - RooArgSet tmpObs; - pdf->getObservables(&refCoefNorm, tmpObs); - bool allIdent = true; - for (auto *rvarg : dynamic_range_cast(tmpObs)) { - if (rvarg) { - if (rvarg->getMin(RooNameReg::str(refCoefRangeName)) != rvarg->getMin() || - rvarg->getMax(RooNameReg::str(refCoefRangeName)) != rvarg->getMax()) { - allIdent = false; - } - } - } - // ------------- - - if (refCoefRangeName && !refCoefNorm.empty() && !allIdent) { - - RooArgSet tmp; - pdf->getObservables(&refCoefNorm, tmp); - rangeProj1 = std::unique_ptr{pdf->createIntegral(tmp, tmp, RooNameReg::str(refCoefRangeName))}; - - // rangeProj1->setOperMode(operMode()) ; - oocxcoutD(&addPdf, Caching) << " " << addPdf.ClassName() << "::syncCoefProjList(" << addPdf.GetName() - << ") R1 = " << rangeProj1->GetName() << std::endl; - } - _refRangeProjList.emplace_back(std::move(rangeProj1)); - // Calculate range adjusted projection integral std::unique_ptr rangeProj2; - oocxcoutD(&addPdf, Caching) << addPdf.ClassName() << "::syncCoefProjList(" << addPdf.GetName() - << ") rangename = " << (rangeName ? rangeName : "") - << " nset = " << (nset ? *nset : RooArgSet()) << std::endl; - if (rangeName && !refCoefNorm.empty()) { - - rangeProj2 = std::unique_ptr{pdf->createIntegral(refCoefNorm, refCoefNorm, rangeName)}; - // rangeProj2->setOperMode(operMode()) ; - - } else if (addPdf.normRange()) { - + if (normRange != refCoefNormRange) { RooArgSet tmp; - pdf->getObservables(&refCoefNorm, tmp); - rangeProj2 = std::unique_ptr{pdf->createIntegral(tmp, tmp, addPdf.normRange())}; - oocxcoutD(&addPdf, Caching) << " " << addPdf.ClassName() << "::syncCoefProjList(" << addPdf.GetName() - << ") R2 = " << rangeProj2->GetName() << std::endl; + pdf->getObservables(refCoefNormSet.empty() ? nset : &refCoefNormSet, tmp); + auto int1 = std::unique_ptr{pdf->createIntegral(tmp, tmp, normRange.c_str())}; + auto int2 = std::unique_ptr{pdf->createIntegral(tmp, tmp, refCoefNormRange.c_str())}; + rangeProj2 = std::make_unique("rangeProj", "rangeProj", *int1, *int2); + rangeProj2->addOwnedComponents(std::move(int1), std::move(int2)); } + _rangeProjList.emplace_back(std::move(rangeProj2)); } } @@ -222,7 +183,6 @@ void AddCacheElem::print() const printVector(_suppNormList, "_suppNormList"); printVector(_projList, "_projList"); printVector(_suppProjList, "_suppProjList"); - printVector(_refRangeProjList, "_refRangeProjList"); printVector(_rangeProjList, "_rangeProjList"); } @@ -241,10 +201,6 @@ RooArgList AddCacheElem::containedArgs(Action) if (arg) allNodes.add(*arg); } - for (auto const &arg : _refRangeProjList) { - if (arg) - allNodes.add(*arg); - } for (auto const &arg : _rangeProjList) { if (arg) allNodes.add(*arg); @@ -264,7 +220,7 @@ RooArgList AddCacheElem::containedArgs(Action) void RooAddHelpers::updateCoefficients(RooAbsPdf const &addPdf, std::vector &coefCache, RooArgList const &pdfList, bool haveLastCoef, AddCacheElem &cache, - const RooArgSet *nset, bool projectCoefs, RooArgSet const &refCoefNorm, + const RooArgSet *nset, bool projectCoefs, RooArgSet const &refCoefNormSet, bool allExtendable, int &coefErrCount) { // Straight coefficients @@ -275,7 +231,7 @@ void RooAddHelpers::updateCoefficients(RooAbsPdf const &addPdf, std::vector(arg); - coefCache[i] = pdf->expectedEvents(!refCoefNorm.empty() ? &refCoefNorm : nset); + coefCache[i] = pdf->expectedEvents(!refCoefNormSet.empty() ? &refCoefNormSet : nset); coefSum += coefCache[i]; i++; } diff --git a/roofit/roofitcore/src/RooAddHelpers.h b/roofit/roofitcore/src/RooAddHelpers.h index 473bcb5b5d262..9949153858198 100644 --- a/roofit/roofitcore/src/RooAddHelpers.h +++ b/roofit/roofitcore/src/RooAddHelpers.h @@ -21,8 +21,8 @@ class RooArgSet; class AddCacheElem : public RooAbsCacheElement { public: AddCacheElem(RooAbsPdf const &addPdf, RooArgList const &pdfList, RooArgList const &coefList, const RooArgSet *nset, - const RooArgSet *iset, const char *rangeName, bool projectCoefs, RooArgSet const &refCoefNorm, - TNamed const *refCoefRangeName, int verboseEval); + const RooArgSet *iset, bool projectCoefs, RooArgSet const &refCoefNormSet, + std::string const &refCoefNormRange, int verboseEval); RooArgList containedArgs(Action) override; @@ -37,35 +37,27 @@ class AddCacheElem : public RooAbsCacheElement { return _suppProjList[idx] ? _suppProjList[idx]->getVal() : 1.0; } - inline double rangeProjScaleFactor(std::size_t idx) const { return rangeProjVal(idx) / refRangeProjVal(idx); } - - void print() const; - -private: - inline double refRangeProjVal(std::size_t idx) const - { - return _refRangeProjList[idx] ? _refRangeProjList[idx]->getVal() : 1.0; - } - - inline double rangeProjVal(std::size_t idx) const + inline double rangeProjScaleFactor(std::size_t idx) const { return _rangeProjList[idx] ? _rangeProjList[idx]->getVal() : 1.0; } + void print() const; + +private: using OwningArgVector = std::vector>; OwningArgVector _suppNormList; ///< Supplemental normalization list OwningArgVector _projList; ///< Projection integrals to be multiplied with coefficients OwningArgVector _suppProjList; ///< Projection integrals to multiply with coefficients for supplemental normalization - OwningArgVector _refRangeProjList; ///< Range integrals to be multiplied with coefficients (reference range) - OwningArgVector _rangeProjList; ///< Range integrals to be multiplied with coefficients (target range) + OwningArgVector _rangeProjList; ///< Range integrals to be multiplied with coefficients (reference to target range) }; class RooAddHelpers { public: static void updateCoefficients(RooAbsPdf const &addPdf, std::vector &coefCache, RooArgList const &pdfList, bool haveLastCoef, AddCacheElem &cache, const RooArgSet *nset, bool projectCoefs, - RooArgSet const &refCoefNorm, bool allExtendable, int &coefErrCount); + RooArgSet const &refCoefNormSet, bool allExtendable, int &coefErrCount); }; #endif diff --git a/roofit/roofitcore/src/RooAddModel.cxx b/roofit/roofitcore/src/RooAddModel.cxx index 9734c7bd2a649..2650877eee321 100644 --- a/roofit/roofitcore/src/RooAddModel.cxx +++ b/roofit/roofitcore/src/RooAddModel.cxx @@ -311,19 +311,21 @@ Int_t RooAddModel::basisCode(const char* name) const /// integrals to calculated transformed fraction coefficients when a frozen reference frame is provided /// and projection integrals for similar transformations when a frozen reference range is provided. -AddCacheElem* RooAddModel::getProjCache(const RooArgSet* nset, const RooArgSet* iset, const char* rangeName) const +AddCacheElem* RooAddModel::getProjCache(const RooArgSet* nset, const RooArgSet* iset) const { // Check if cache already exists - auto cache = static_cast(_projCacheMgr.getObj(nset,iset,0,rangeName)); + auto cache = static_cast(_projCacheMgr.getObj(nset,iset,0,normRange())); if (cache) { return cache ; } //Create new cache - cache = new AddCacheElem{*this, _pdfList, _coefList, nset, iset, rangeName, - _projectCoefs, _refCoefNorm, _refCoefRangeName, _verboseEval}; + cache = new AddCacheElem{*this, _pdfList, _coefList, nset, iset, + _projectCoefs, _refCoefNorm, + _refCoefRangeName ? RooNameReg::str(_refCoefRangeName) : "", + _verboseEval}; - _projCacheMgr.setObj(nset,iset,cache,RooNameReg::ptr(rangeName)) ; + _projCacheMgr.setObj(nset,iset,cache,RooNameReg::ptr(normRange())) ; return cache; } diff --git a/roofit/roofitcore/src/RooAddPdf.cxx b/roofit/roofitcore/src/RooAddPdf.cxx index aaaf63b872860..ea37f9e80b329 100644 --- a/roofit/roofitcore/src/RooAddPdf.cxx +++ b/roofit/roofitcore/src/RooAddPdf.cxx @@ -356,19 +356,21 @@ void RooAddPdf::fixCoefRange(const char* rangeName) /// - Projection integrals to calculate transformed fraction coefficients when a frozen reference frame is provided /// - Projection integrals for similar transformations when a frozen reference range is provided. -AddCacheElem* RooAddPdf::getProjCache(const RooArgSet* nset, const RooArgSet* iset, const char* rangeName) const +AddCacheElem* RooAddPdf::getProjCache(const RooArgSet* nset, const RooArgSet* iset) const { // Check if cache already exists - auto cache = static_cast(_projCacheMgr.getObj(nset,iset,0,rangeName)); + auto cache = static_cast(_projCacheMgr.getObj(nset,iset,0,normRange())); if (cache) { return cache ; } //Create new cache - cache = new AddCacheElem{*this, _pdfList, _coefList, nset, iset, rangeName, - _projectCoefs, _refCoefNorm, _refCoefRangeName, _verboseEval}; + cache = new AddCacheElem{*this, _pdfList, _coefList, nset, iset, + _projectCoefs, _refCoefNorm, + _refCoefRangeName ? RooNameReg::str(_refCoefRangeName) : "", + _verboseEval}; - _projCacheMgr.setObj(nset,iset,cache,RooNameReg::ptr(rangeName)) ; + _projCacheMgr.setObj(nset,iset,cache,RooNameReg::ptr(normRange())) ; return cache; } @@ -476,7 +478,7 @@ double RooAddPdf::getValV(const RooArgSet* normSet) const _value = 0.0; for (unsigned int i=0; i < _pdfList.size(); ++i) { - const auto& pdf = static_cast(_pdfList[i]); + auto& pdf = static_cast(_pdfList[i]); double snormVal = 1.; snormVal = cache->suppNormVal(i); @@ -658,7 +660,7 @@ double RooAddPdf::analyticalIntegralWN(Int_t code, const RooArgSet* normSet, con normSet = &_refCoefNorm ; } - AddCacheElem* cache = getProjCache(normSet,intSet,0) ; // WVE rangename here? + AddCacheElem* cache = getProjCache(normSet,intSet); updateCoefficients(*cache,normSet); // Calculate the current value of this object From fcec4542bc1885427ff9d386a4fcfe656b6d7c08 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 29 Sep 2022 14:03:32 +0200 Subject: [PATCH 3/9] [RF] Use explicit normalization set in RooBinSamplingPdf In the `RooBinSamplingPdf`, the underlying PDF is evaluated with a call to `getVal()` without normalization set. This is not a good idea, becaues it implicitly assumes that the PDF was evaluated with the right normalization range the last time and breaks the case where you set the normalization range of the PDF with `setNormRange()`. It is better to evaluate the PDF explicitly normalized and then declate the RooBinSamplingPdf as self normalized. Some unnecessary printouts in the corresponding unit tests are also removed. --- roofit/roofitcore/inc/RooBinSamplingPdf.h | 2 +- roofit/roofitcore/src/RooBinSamplingPdf.cxx | 2 +- roofit/roofitcore/test/testRooBinSamplingPdf.cxx | 3 --- roofit/roofitcore/test/testTestStatistics.cxx | 16 +++++++++------- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/roofit/roofitcore/inc/RooBinSamplingPdf.h b/roofit/roofitcore/inc/RooBinSamplingPdf.h index b6509d17ab392..82adb5c940c8f 100644 --- a/roofit/roofitcore/inc/RooBinSamplingPdf.h +++ b/roofit/roofitcore/inc/RooBinSamplingPdf.h @@ -62,7 +62,7 @@ class RooBinSamplingPdf : public RooAbsPdf { } /// Forwards to the PDF's implementation. - bool selfNormalized() const override { return _pdf->selfNormalized(); } + bool selfNormalized() const override { return true; } /// Forwards to the PDF's implementation. RooAbsReal* createIntegral(const RooArgSet& iset, diff --git a/roofit/roofitcore/src/RooBinSamplingPdf.cxx b/roofit/roofitcore/src/RooBinSamplingPdf.cxx index 0abdbd0f29826..b1a3d677459a2 100644 --- a/roofit/roofitcore/src/RooBinSamplingPdf.cxx +++ b/roofit/roofitcore/src/RooBinSamplingPdf.cxx @@ -294,7 +294,7 @@ std::unique_ptr& RooBinSamplingPdf::integrator() c /// Binding used by the integrator to evaluate the PDF. double RooBinSamplingPdf::operator()(double x) const { _observable->setVal(x); - return _pdf->getVal(); + return _pdf; } diff --git a/roofit/roofitcore/test/testRooBinSamplingPdf.cxx b/roofit/roofitcore/test/testRooBinSamplingPdf.cxx index 798052527e44c..0999a2cc727b3 100644 --- a/roofit/roofitcore/test/testRooBinSamplingPdf.cxx +++ b/roofit/roofitcore/test/testRooBinSamplingPdf.cxx @@ -95,7 +95,6 @@ TEST(RooBinSamplingPdf, CheckConsistentNormalization) // An integral over the normalization set normalized by an integral over the // normalization set should be unity by definition. std::unique_ptr int1{binSamplingPdf.createIntegral(normSet, &normSet)}; - std::cout << int1->getVal() << std::endl; EXPECT_FLOAT_EQ(int1->getVal(), 1.0); // Evaluating the pdf with a given normalization set should not unexpectedly @@ -103,7 +102,5 @@ TEST(RooBinSamplingPdf, CheckConsistentNormalization) std::unique_ptr int2{binSamplingPdf.createIntegral(normSet)}; binSamplingPdf.getVal(normSet); std::unique_ptr int3{binSamplingPdf.createIntegral(normSet)}; - std::cout << int2->getVal() << std::endl; - std::cout << int3->getVal() << std::endl; EXPECT_FLOAT_EQ(int2->getVal(), int3->getVal()); } diff --git a/roofit/roofitcore/test/testTestStatistics.cxx b/roofit/roofitcore/test/testTestStatistics.cxx index d63bb405ef066..774d2fbf5ed15 100644 --- a/roofit/roofitcore/test/testTestStatistics.cxx +++ b/roofit/roofitcore/test/testTestStatistics.cxx @@ -18,6 +18,8 @@ #include +const bool batchMode = true; + TEST(RooNLLVar, IntegrateBins) { RooRandom::randomGenerator()->SetSeed(1337ul); @@ -48,7 +50,7 @@ TEST(RooNLLVar, IntegrateBins) { a.setVal(3.); std::unique_ptr fit2( pdf.fitTo(data, RooFit::Save(), RooFit::PrintLevel(-1), - RooFit::BatchMode(true), + RooFit::BatchMode(batchMode), RooFit::IntegrateBins(1.E-3)) ); pdf.plotOn(frame.get(), RooFit::LineColor(kBlue), RooFit::Name("highRes")); @@ -107,14 +109,14 @@ TEST(RooNLLVar, IntegrateBins_SubRange) { std::unique_ptr fit1( pdf.fitTo(data, RooFit::Save(), RooFit::PrintLevel(-1), RooFit::Optimize(0), RooFit::Range("range"), - RooFit::BatchMode(true)) ); + RooFit::BatchMode(batchMode)) ); pdf.plotOn(frame.get(), RooFit::LineColor(kRed), RooFit::Name("standard")); a.setVal(3.); std::unique_ptr fit2( pdf.fitTo(data, RooFit::Save(), RooFit::PrintLevel(-1), RooFit::Optimize(0), RooFit::Range("range"), - RooFit::BatchMode(true), + RooFit::BatchMode(batchMode), RooFit::IntegrateBins(1.E-3)) ); pdf.plotOn(frame.get(), RooFit::LineColor(kBlue), RooFit::Name("highRes")); @@ -179,7 +181,7 @@ TEST(RooNLLVar, IntegrateBins_CustomBinning) { a.setVal(3.); std::unique_ptr fit2( pdf.fitTo(data, RooFit::Save(), RooFit::PrintLevel(-1), RooFit::Optimize(0), - RooFit::BatchMode(true), + RooFit::BatchMode(batchMode), RooFit::IntegrateBins(1.E-3)) ); pdf.plotOn(frame.get(), RooFit::LineColor(kBlue), RooFit::Name("highRes")); @@ -226,14 +228,14 @@ TEST(RooNLLVar, IntegrateBins_RooDataHist) { a.setVal(3.); std::unique_ptr fit1( pdf.fitTo(*data, RooFit::Save(), RooFit::PrintLevel(-1), - RooFit::BatchMode(true), + RooFit::BatchMode(batchMode), RooFit::IntegrateBins(-1.) // Disable forcefully ) ); pdf.plotOn(frame.get(), RooFit::LineColor(kRed), RooFit::Name("standard")); a.setVal(3.); std::unique_ptr fit2( pdf.fitTo(*data, RooFit::Save(), RooFit::PrintLevel(-1), - RooFit::BatchMode(true), + RooFit::BatchMode(batchMode), RooFit::IntegrateBins(0.) // Auto-enable for all RooDataHists. ) ); pdf.plotOn(frame.get(), RooFit::LineColor(kBlue), RooFit::Name("highRes")); @@ -283,7 +285,7 @@ TEST(RooChi2Var, IntegrateBins) { a.setVal(3.); std::unique_ptr fit2( pdf.chi2FitTo(*dataH, RooFit::Save(), RooFit::PrintLevel(-1), - RooFit::BatchMode(true), + RooFit::BatchMode(batchMode), RooFit::IntegrateBins(1.E-3)) ); pdf.plotOn(frame.get(), RooFit::LineColor(kBlue), RooFit::Name("highRes")); From bd4d37c2634949971e2583c47182229c7fcaf451 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 29 Sep 2022 07:01:32 +0200 Subject: [PATCH 4/9] [RF] Use `RooAbsPdf::setNormRange()` for BatchMode Now that defining the normalization range via `RooAbsPdf::setNormRange` works, we can directly use it to create the right normalization integral for the top-level PDF for the new BatchMode, and we don't need to create any further correction integrals. --- roofit/roofitcore/res/RooNLLVarNew.h | 3 +- roofit/roofitcore/src/BatchModeHelpers.cxx | 15 +++++-- roofit/roofitcore/src/RooNLLVarNew.cxx | 52 ++-------------------- roofit/roofitcore/src/RooNormalizedPdf.h | 4 +- 4 files changed, 17 insertions(+), 57 deletions(-) diff --git a/roofit/roofitcore/res/RooNLLVarNew.h b/roofit/roofitcore/res/RooNLLVarNew.h index 9b815fa2b0db4..fa588b3958581 100644 --- a/roofit/roofitcore/res/RooNLLVarNew.h +++ b/roofit/roofitcore/res/RooNLLVarNew.h @@ -34,7 +34,7 @@ class RooNLLVarNew : public RooAbsReal { RooNLLVarNew(){}; RooNLLVarNew(const char *name, const char *title, RooAbsPdf &pdf, RooArgSet const &observables, bool isExtended, - std::string const &rangeName, bool doOffset); + bool doOffset); RooNLLVarNew(const RooNLLVarNew &other, const char *name = nullptr); TObject *clone(const char *newname) const override { return new RooNLLVarNew(*this, newname); } @@ -68,7 +68,6 @@ class RooNLLVarNew : public RooAbsReal { std::string _prefix; RooTemplateProxy _weightVar; RooTemplateProxy _weightSquaredVar; - std::unique_ptr> _fractionInRange; mutable std::vector _binw; /// _logProbasBuffer; /// _offset = 0.0; /// createSimultaneousNLL(RooSimultaneous const &simPdf, auto const &catName = catItem.first; if (RooAbsPdf *pdf = simPdf.getPdf(catName.c_str())) { auto name = std::string("nll_") + pdf->GetName(); - auto nll = std::make_unique(name.c_str(), name.c_str(), *pdf, observables, isExtended, rangeName, - doOffset); + if (!rangeName.empty()) { + pdf->setNormRange(rangeName.c_str()); + } + auto nll = std::make_unique(name.c_str(), name.c_str(), *pdf, observables, isExtended, doOffset); // Rename the observables and weights newObservables.add(nll->prefixObservableAndWeightNames(std::string("_") + catName + "_")); nllTerms.addOwned(std::move(nll)); @@ -152,6 +154,11 @@ RooFit::BatchModeHelpers::createNLL(RooAbsPdf &pdf, RooAbsData &data, std::uniqu observables.remove(projDeps, true, true); obsClones.remove(projDeps, true, true); + // Set the normalization range + if (!rangeName.empty()) { + pdfClone->setNormRange(rangeName.c_str()); + } + oocxcoutI(&pdf, Fitting) << "RooAbsPdf::fitTo(" << pdf.GetName() << ") fixing normalization set for coefficient determination to observables in data" << "\n"; @@ -183,8 +190,8 @@ RooFit::BatchModeHelpers::createNLL(RooAbsPdf &pdf, RooAbsData &data, std::uniqu // Warning! This mutates "obsClones" nllTerms.addOwned(createSimultaneousNLL(*simPdf, obsClones, isExtended, rangeName, doOffset)); } else { - nllTerms.addOwned(std::make_unique("RooNLLVarNew", "RooNLLVarNew", finalPdf, obsClones, isExtended, - rangeName, doOffset)); + nllTerms.addOwned( + std::make_unique("RooNLLVarNew", "RooNLLVarNew", finalPdf, obsClones, isExtended, doOffset)); } if (constraints) { nllTerms.addOwned(std::move(constraints)); diff --git a/roofit/roofitcore/src/RooNLLVarNew.cxx b/roofit/roofitcore/src/RooNLLVarNew.cxx index f65423c2bb33d..4784acb8345c6 100644 --- a/roofit/roofitcore/src/RooNLLVarNew.cxx +++ b/roofit/roofitcore/src/RooNLLVarNew.cxx @@ -51,13 +51,6 @@ constexpr const char *RooNLLVarNew::weightVarNameSumW2; namespace { -std::unique_ptr -createFractionInRange(RooAbsPdf const &pdf, RooArgSet const &observables, std::string const &rangeNames) -{ - return std::unique_ptr{ - pdf.createIntegral(observables, &observables, pdf.getIntegratorConfig(), rangeNames.c_str())}; -} - template double kahanSum(Input const &input) { @@ -79,10 +72,9 @@ RooArgSet getObservablesInPdf(RooAbsPdf const &pdf, RooArgSet const &observables \param pdf The pdf for which the nll is computed for \param observables The observabes of the pdf \param isExtended Set to true if this is an extended fit -\param rangeName the range name **/ RooNLLVarNew::RooNLLVarNew(const char *name, const char *title, RooAbsPdf &pdf, RooArgSet const &observables, - bool isExtended, std::string const &rangeName, bool doOffset) + bool isExtended, bool doOffset) : RooAbsReal(name, title), _pdf{"pdf", "pdf", this, pdf}, _observables{getObservablesInPdf(pdf, observables)}, _isExtended{isExtended}, _doOffset{doOffset}, _weightVar{"weightVar", "weightVar", this, *new RooRealVar(weightVarName, weightVarName, 1.0), true, false, true}, @@ -133,13 +125,6 @@ RooNLLVarNew::RooNLLVarNew(const char *name, const char *title, RooAbsPdf &pdf, } } - if (!rangeName.empty()) { - auto term = createFractionInRange(*actualPdf, _observables, rangeName); - _fractionInRange = - std::make_unique>("_fractionInRange", "_fractionInRange", this, *term); - addOwnedComponents(std::move(term)); - } - resetWeightVarNames(); } @@ -150,9 +135,6 @@ RooNLLVarNew::RooNLLVarNew(const RooNLLVarNew &other, const char *name) this, other._weightSquaredVar} { - if (other._fractionInRange) - _fractionInRange = - std::make_unique>("_fractionInRange", this, *other._fractionInRange); } /** Compute multiple negative logs of propabilities @@ -225,34 +207,12 @@ void RooNLLVarNew::computeBatch(cudaStream_t * /*stream*/, double *output, size_ _logProbasBuffer.resize(nEvents); (*_pdf).getLogProbabilities(probas, _logProbasBuffer.data()); - if ((_isExtended || _fractionInRange) && _sumWeight == 0.0) { + if (_isExtended && _sumWeight == 0.0) { _sumWeight = weights.size() == 1 ? weights[0] * nEvents : kahanSum(weights); } - if ((_isExtended || _fractionInRange) && _weightSquared && _sumWeight2 == 0.0) { + if (_isExtended && _weightSquared && _sumWeight2 == 0.0) { _sumWeight2 = weights.size() == 1 ? weightsSumW2[0] * nEvents : kahanSum(weightsSumW2); } - double sumCorrectionTerm = 0; - if (_fractionInRange) { - auto fractionInRangeSpan = dataMap.at(*_fractionInRange); - if (fractionInRangeSpan.size() == 1) { - sumCorrectionTerm = (_weightSquared ? _sumWeight2 : _sumWeight) * std::log(fractionInRangeSpan[0]); - } else { - if (weightSpan.size() == 1) { - double fractionInRangeLogSum = 0.0; - for (std::size_t i = 0; i < fractionInRangeSpan.size(); ++i) { - fractionInRangeLogSum += std::log(fractionInRangeSpan[i]); - } - sumCorrectionTerm = weightSpan[0] * fractionInRangeLogSum; - } else { - // We don't need to use the library for now because the weights and - // correction term integrals are always in the CPU map. - sumCorrectionTerm = 0.0; - for (std::size_t i = 0; i < nEvents; ++i) { - sumCorrectionTerm += weightSpan[i] * std::log(fractionInRangeSpan[i]); - } - } - } - } ROOT::Math::KahanSum kahanProb; RooNaNPacker packedNaN(0.f); @@ -277,14 +237,8 @@ void RooNLLVarNew::computeBatch(cudaStream_t * /*stream*/, double *output, size_ if (_isExtended) { assert(_sumWeight != 0.0); double expected = _pdf->expectedEvents(&_observables); - if (_fractionInRange) { - expected *= dataMap.at(*_fractionInRange)[0]; - } kahanProb += _pdf->extendedTerm(_sumWeight, expected, _weightSquared ? _sumWeight2 : 0.0); } - if (_fractionInRange) { - kahanProb += sumCorrectionTerm; - } // Check if value offset flag is set. if (_doOffset) { diff --git a/roofit/roofitcore/src/RooNormalizedPdf.h b/roofit/roofitcore/src/RooNormalizedPdf.h index f30ed5e8747ba..764d930b319dc 100644 --- a/roofit/roofitcore/src/RooNormalizedPdf.h +++ b/roofit/roofitcore/src/RooNormalizedPdf.h @@ -21,7 +21,7 @@ class RooNormalizedPdf : public RooAbsPdf { RooNormalizedPdf(RooAbsPdf &pdf, RooArgSet const &normSet) : _pdf("numerator", "numerator", this, pdf), _normIntegral("denominator", "denominator", this, - *pdf.createIntegral(normSet, *pdf.getIntegratorConfig(), nullptr), true, false, true), + *pdf.createIntegral(normSet, *pdf.getIntegratorConfig(), pdf.normRange()), true, false, true), _normSet{normSet} { auto name = std::string(pdf.GetName()) + "_over_" + _normIntegral->GetName(); @@ -59,7 +59,7 @@ class RooNormalizedPdf : public RooAbsPdf { } protected: - void computeBatch(cudaStream_t *, double *output, size_t size, RooFit::Detail::DataMap const&) const override; + void computeBatch(cudaStream_t *, double *output, size_t size, RooFit::Detail::DataMap const &) const override; double evaluate() const override { // Evaluate() should not be called in the BatchMode, but we still need it From 3240b309203e9e42c1ae3b659f91b05df72476de Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Wed, 28 Sep 2022 12:24:36 +0200 Subject: [PATCH 5/9] [RF] Replace multi-range fit logic in RooAbsPdf Multi-range fits in RooFit are more complicated than they should be. In principle, all that is required is to change the normalization range of the PDF to the union of the ranges. There is a RooAbdPdf interface suggesting that this could be done easily like this: ```C++ pdf.setNormRange("range1,range2") ``` But this didn't wor for RooAddPdfs, which is probably why it was chosen to implement mulit-range fits as a sum of separate RooNLLVars. In the old test statistics framework. But in this case, the PDFs are normalized separately, and extra terms need to be introduced to correct for that. This resulted in lots of complicated code, and still there are issues like #11447, i.e. is still doesn't work for simultaneous fits. Now that in the previous commits the `RooAddPdf` behavior for `setNormRange()` was fixed, one can actually use comma-separated normalization ranges for a single PDF in a multi-range fit. With this commit, this is done in the old RooFit test statistics classes. This has sevaral advantages: 1. Fixes issues with multi-range simultaneous fits 2. It's now in harmony with the logic in the new BatchMode 3. Some speedup because there are less nodes in the computation graph 4. Less code required 5. Logic is easier to understand Maybe there are also new bugs now, but they can be fixed later. I am sure that already now the commit fixes more issues that it creates. --- .../roofitcore/src/RooAbsOptTestStatistic.cxx | 50 +++++------ roofit/roofitcore/src/RooAbsPdf.cxx | 85 +------------------ roofit/roofitcore/src/RooAbsTestStatistic.cxx | 18 ++-- 3 files changed, 33 insertions(+), 120 deletions(-) diff --git a/roofit/roofitcore/src/RooAbsOptTestStatistic.cxx b/roofit/roofitcore/src/RooAbsOptTestStatistic.cxx index 0246c3e3f086c..f2d63cb7e4205 100644 --- a/roofit/roofitcore/src/RooAbsOptTestStatistic.cxx +++ b/roofit/roofitcore/src/RooAbsOptTestStatistic.cxx @@ -61,6 +61,8 @@ parallelized calculation of test statistics. #include "RooVectorDataStore.h" #include "RooBinSamplingPdf.h" +#include "ROOT/StringUtils.hxx" + using namespace std; ClassImp(RooAbsOptTestStatistic); @@ -290,16 +292,27 @@ void RooAbsOptTestStatistic::initSlave(RooAbsReal& real, RooAbsData& indata, con // ****************************************************************** std::unique_ptr origObsSet( real.getObservables(indata) ); - RooArgSet* dataObsSet = (RooArgSet*) _dataClone->get() ; if (rangeName && strlen(rangeName)) { cxcoutI(Fitting) << "RooAbsOptTestStatistic::ctor(" << GetName() << ") constructing test statistic for sub-range named " << rangeName << endl ; - bool observablesKnowRange = false; + if(auto pdfClone = dynamic_cast(_funcClone)) { + pdfClone->setNormRange(rangeName); + } + // Adjust FUNC normalization ranges to requested fitRange, store original ranges for RooAddPdf coefficient interpretation for (const auto arg : *_funcObsSet) { - RooRealVar* realObs = dynamic_cast(arg) ; - if (realObs) { + if (auto realObs = dynamic_cast(arg)) { + + auto tokens = ROOT::Split(rangeName, ","); + for(std::string const& token : tokens) { + if(!realObs->hasRange(token.c_str())) { + std::stringstream errMsg; + errMsg << "The observable \"" << realObs->GetName() << "\" doesn't define the requested range \"" + << token << "\". Replacing it with the default range." << std::endl; + coutW(Fitting) << errMsg.str() << std::endl; + } + } auto transferRangeAndBinning = [&](RooRealVar & toVar, const char* toName, const char* fromName) { toVar.setRange(toName, realObs->getMin(fromName),realObs->getMax(fromName)); @@ -312,22 +325,11 @@ void RooAbsOptTestStatistic::initSlave(RooAbsReal& real, RooAbsData& indata, con } }; - observablesKnowRange |= realObs->hasRange(rangeName); - - // If no explicit range is given for RooAddPdf coefficients, create explicit named range equivalent to original observables range - if (!(addCoefRangeName && strlen(addCoefRangeName))) { - transferRangeAndBinning(*realObs, Form("NormalizationRangeFor%s",rangeName), nullptr); - } - - // Adjust range of function observable to those of given named range - transferRangeAndBinning(*realObs, nullptr, rangeName); - - // Adjust range of data observable to those of given named range - RooRealVar* dataObs = (RooRealVar*) dataObsSet->find(realObs->GetName()) ; - transferRangeAndBinning(*dataObs, nullptr, rangeName); - - // Keep track of list of fit ranges in string attribute fit range of original p.d.f. - if (!_splitRange) { + // Keep track of list of fit ranges in string attribute fit range of + // original PDF, but only do so if we don't do multi-range + // normalization. In this case, we could not define an equivalend + // single fit range. + if (!_splitRange && strchr(rangeName,',') == 0) { const std::string fitRangeName = std::string("fit_") + GetName(); const char* origAttrib = real.getStringAttribute("fitrange") ; std::string newAttr = origAttrib ? origAttrib : ""; @@ -343,9 +345,6 @@ void RooAbsOptTestStatistic::initSlave(RooAbsReal& real, RooAbsData& indata, con } } } - - if (!observablesKnowRange) - coutW(Fitting) << "None of the fit observables seem to know the range '" << rangeName << "'. This means that the full range will be used." << std::endl; } @@ -366,10 +365,6 @@ void RooAbsOptTestStatistic::initSlave(RooAbsReal& real, RooAbsData& indata, con cxcoutI(Fitting) << "RooAbsOptTestStatistic::ctor(" << GetName() << ") fixing interpretation of coefficients of any RooAddPdf component to range " << addCoefRangeName << endl ; _funcClone->fixAddCoefRange(addCoefRangeName,false) ; - } else { - cxcoutI(Fitting) << "RooAbsOptTestStatistic::ctor(" << GetName() - << ") fixing interpretation of coefficients of any RooAddPdf to full domain of observables " << endl ; - _funcClone->fixAddCoefRange(Form("NormalizationRangeFor%s",rangeName),false) ; } } @@ -859,3 +854,4 @@ void RooAbsOptTestStatistic::setUpBinSampling() { const char* RooAbsOptTestStatistic::cacheUniqueSuffix() const { return Form("_%lx", _dataClone->uniqueId().value()) ; } + diff --git a/roofit/roofitcore/src/RooAbsPdf.cxx b/roofit/roofitcore/src/RooAbsPdf.cxx index 855ac6f1fdbb1..2a68a7d3b11e5 100644 --- a/roofit/roofitcore/src/RooAbsPdf.cxx +++ b/roofit/roofitcore/src/RooAbsPdf.cxx @@ -938,51 +938,6 @@ RooAbsReal* RooAbsPdf::createNLL(RooAbsData& data, const RooCmdArg& arg1, const return createNLL(data,l) ; } -namespace { - -std::unique_ptr createMultiRangeNLLCorrectionTerm( - RooAbsPdf const &pdf, RooAbsData const &data, std::string const &baseName, std::string const &rangeNames) -{ - double sumEntriesTotal = 0.0; - - RooArgList termList; - RooArgList integralList; - - for (const auto ¤tRangeName : ROOT::Split(rangeNames, ",")) { - const std::string currentName = baseName + "_" + currentRangeName; - - auto sumEntriesCurrent = data.sumEntries("1", currentRangeName.c_str()); - sumEntriesTotal += sumEntriesCurrent; - - RooArgSet depList; - pdf.getObservables(data.get(), depList); - auto pdfIntegralCurrent = pdf.createIntegral(depList, &depList, nullptr, currentRangeName.c_str()); - - auto term = new RooFormulaVar((currentName + "_correctionTerm").c_str(), - (std::string("-(") + std::to_string(sumEntriesCurrent) + " * log(x[0]))").c_str(), - RooArgList(*pdfIntegralCurrent)); - - termList.add(*term); - integralList.add(*pdfIntegralCurrent); - } - - auto integralFull = new RooAddition((baseName + "_correctionFullIntegralTerm").c_str(), - "integral", - integralList, - true); - - auto fullRangeTerm = new RooFormulaVar((baseName + "_foobar").c_str(), - (std::string("(") + std::to_string(sumEntriesTotal) + " * log(x[0]))").c_str(), - RooArgList(*integralFull)); - - termList.add(*fullRangeTerm); - return std::unique_ptr{ - new RooAddition((baseName + "_correction").c_str(), "correction", termList, true)}; -} - - -} // namespace - //////////////////////////////////////////////////////////////////////////////// /// Construct representation of -log(L) of PDFwith given dataset. If dataset is unbinned, an unbinned likelihood is constructed. If the dataset @@ -1129,43 +1084,9 @@ RooAbsReal* RooAbsPdf::createNLL(RooAbsData& data, const RooLinkedList& cmdList) cfg.integrateOverBinsPrecision = pc.getDouble("IntegrateBins"); cfg.binnedL = false; cfg.takeGlobalObservablesFromData = takeGlobalObservablesFromData; - if (!rangeName || strchr(rangeName,',')==0) { - // Simple case: default range, or single restricted range - //cout<<"FK: Data test 1: "<(baseName.c_str(),"-log(likelihood)",*this,data,projDeps, ext, cfg); - static_cast(*nll).batchMode(batchMode == RooFit::BatchModeOption::Old); - } else { - // Composite case: multiple ranges - RooArgList nllList ; - auto tokens = ROOT::Split(rangeName, ","); - if (RooHelpers::checkIfRangesOverlap(*this, data, tokens, cfg.splitCutRange)) { - throw std::runtime_error( - std::string("Error in RooAbsPdf::createNLL! The ranges ") + rangeName + " are overlapping!"); - } - for (const auto& token : tokens) { - cfg.rangeName = token; - auto nllComp = std::make_unique((baseName + "_" + token).c_str(),"-log(likelihood)", - *this,data,projDeps,ext,cfg); - nllComp->batchMode(pc.getInt("BatchMode")); - nllList.addOwned(std::move(nllComp)) ; - } - - if (!ext) { - // Each RooNLLVar was created with the normalization set corresponding to - // the subrange, not the union range like it should be. We have to add an - // extra term to cancel this normalization problem. However, this is - // only necessarry for the non-extended case, because adding an extension - // term to the individual NLLs as done here is mathematicall equivalent - // to adding the normalization correction terms plus a global extension - // term. - nllList.addOwned(createMultiRangeNLLCorrectionTerm(*this, data, baseName, rangeName)); - } - - nll = std::make_unique(baseName.c_str(),"-log(likelihood)",nllList) ; - nll->addOwnedComponents(std::move(nllList)); - } + cfg.rangeName = rangeName ? rangeName : ""; + nll = std::make_unique(baseName.c_str(),"-log(likelihood)",*this,data,projDeps, ext, cfg); + static_cast(*nll).batchMode(batchMode == RooFit::BatchModeOption::Old); RooAbsReal::setEvalErrorLoggingMode(RooAbsReal::PrintErrors) ; // Include constraints, if any, in likelihood diff --git a/roofit/roofitcore/src/RooAbsTestStatistic.cxx b/roofit/roofitcore/src/RooAbsTestStatistic.cxx index 145016755a7a5..7f3522058e3a3 100644 --- a/roofit/roofitcore/src/RooAbsTestStatistic.cxx +++ b/roofit/roofitcore/src/RooAbsTestStatistic.cxx @@ -49,6 +49,8 @@ combined in the main thread. #include "RooRealSumPdf.h" #include "RooAbsCategoryLValue.h" +#include "ROOT/StringUtils.hxx" + #include "TTimeStamp.h" #include "TClass.h" #include @@ -107,16 +109,6 @@ RooAbsTestStatistic::RooAbsTestStatistic(const char *name, const char *title, Ro { // Register all parameters as servers _paramSet.add(*std::unique_ptr{real.getParameters(&data)}); - - if (cfg.rangeName.find(',') != std::string::npos) { - auto errorMsg = std::string("Ranges ") + cfg.rangeName - + " were passed to the RooAbsTestStatistic with name \"" + name + "\", " - + "but it doesn't support multiple comma-separated fit ranges!\n" + - + "Instead, one should combine multiple RooAbsTestStatistic objects " - + "(see RooAbsPdf::createNLL for an example with RooNLLVar)."; - coutE(InputArguments) << errorMsg << std::endl; - throw std::invalid_argument(errorMsg); - } } @@ -535,7 +527,11 @@ void RooAbsTestStatistic::initSimMode(RooSimultaneous* simpdf, RooAbsData* data, cfg.integrateOverBinsPrecision = thisAsRooAbsOptTestStatistic->_integrateBinsPrecision; } if (_splitRange && !rangeName.empty()) { - cfg.rangeName = rangeName + "_" + catName; + auto tokens = ROOT::Split(rangeName, ","); + for(std::string const& token : tokens) { + cfg.rangeName += token + "_" + catName + ","; + } + cfg.rangeName.pop_back(); // to remove the last comma cfg.nCPU = _nCPU*(_mpinterl?-1:1); } else { cfg.rangeName = rangeName; From 5a8ed9b28c554e57b5c15ad8bd20455b8e6c14c5 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Sat, 1 Oct 2022 02:23:32 +0200 Subject: [PATCH 6/9] [RF] Add unit test for RooAddPdf coefficient projection With the recent refactoring of ranged fits, the RooAddPdf configuration when you do a ranged fits changed in the construction of the test statistic. Before: reset default range of the observables to the fitting range and create named range for the coefficient normalization. After: keep default range of the observables and coefficient normalization, and only change the normalization range of the PDF. With the refactoring, there is no test coverage for the first case anymore. That's why this commit suggests a new unit test to cover the RooAddPdf configurations that were covered by the test statistics formally. --- roofit/roofitcore/test/testRooAddPdf.cxx | 58 ++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/roofit/roofitcore/test/testRooAddPdf.cxx b/roofit/roofitcore/test/testRooAddPdf.cxx index f3501bd370fc6..b6221643e4e78 100644 --- a/roofit/roofitcore/test/testRooAddPdf.cxx +++ b/roofit/roofitcore/test/testRooAddPdf.cxx @@ -3,8 +3,10 @@ #include #include +#include #include #include +#include #include #include #include @@ -179,3 +181,59 @@ TEST(RooAddPdf, RecursiveCoefficients) EXPECT_TRUE(curve2->isIdentical(*curve1)); } + +class ProjCacheTest : public testing::TestWithParam> { + void SetUp() override { _fixCoefNormalization = std::get<0>(GetParam()); } + + void TearDown() override {} + +protected: + bool _fixCoefNormalization = false; +}; + +/// Verify that the coefficient projection works for different configurations +/// of the RooAddPdf. +TEST_P(ProjCacheTest, Test) +{ + RooRealVar x{"x", "x", 0, 4}; + x.setBins(4); + x.setRange("B1", 0, 2); + x.setRange("B2", 2, 4); + x.setRange("FULL", 0, 4); + + RooDataHist dataHist1{"dh1", "dh1", x}; + RooDataHist dataHist2{"dh2", "dh2", x}; + + for (int i = 0; i < 4; ++i) { + dataHist1.set(i, i + 1, 0.0); + dataHist2.set(i, 4. - i, 0.0); + } + + RooHistPdf pdf1{"pdf1", "pdf1", x, dataHist1}; + RooHistPdf pdf2{"pdf2", "pdf2", x, dataHist2}; + + RooRealVar frac{"frac", "frac", 0.5, 0, 1}; + + RooAddPdf pdf{"pdf", "pdf", {pdf1, pdf2}, frac}; + + RooArgSet normSet{x}; + + x.setRange(2, 4); + pdf.fixCoefRange("FULL"); + if (_fixCoefNormalization) { + pdf.fixCoefNormalization(normSet); + } + + x.setVal(2.5); + EXPECT_FLOAT_EQ(pdf.getVal(normSet), 0.5); + x.setVal(3.5); + EXPECT_FLOAT_EQ(pdf.getVal(normSet), 0.5); +} + +INSTANTIATE_TEST_SUITE_P(RooAddPdf, ProjCacheTest, + testing::Values(ProjCacheTest::ParamType{false}, ProjCacheTest::ParamType{true}), + [](testing::TestParamInfo const ¶mInfo) { + std::stringstream ss; + ss << (std::get<0>(paramInfo.param) ? "fixCoefNorm" : "floatingCoefNorm"); + return ss.str(); + }); From 7245ab5d914114abe4731f6047545140c2301410 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 29 Sep 2022 14:31:32 +0200 Subject: [PATCH 7/9] [RF] Update `RooHelpers::checkIfRangesOverlap` Before, the `checkIfRangesOverlap` function was used in `RooAbsPdf::createNLL`, which is why it had some command arguments that accomodated specifically for that usecase. This is not necessary anymore. --- roofit/roofitcore/inc/RooHelpers.h | 5 ++-- roofit/roofitcore/src/RooHelpers.cxx | 34 +++------------------------- 2 files changed, 5 insertions(+), 34 deletions(-) diff --git a/roofit/roofitcore/inc/RooHelpers.h b/roofit/roofitcore/inc/RooHelpers.h index 30d0430e22ef4..12b0908b0545c 100644 --- a/roofit/roofitcore/inc/RooHelpers.h +++ b/roofit/roofitcore/inc/RooHelpers.h @@ -132,10 +132,9 @@ class ChangeOperModeRAII { std::pair getRangeOrBinningInterval(RooAbsArg const* arg, const char* rangeName); -bool checkIfRangesOverlap(RooAbsPdf const& pdf, +bool checkIfRangesOverlap(RooArgSet const& observables, RooAbsData const& data, - std::vector const& rangeNames, - bool splitRange); + std::vector const& rangeNames); std::string getColonSeparatedNameString(RooArgSet const& argSet); RooArgSet selectFromArgSet(RooArgSet const&, std::string const& names); diff --git a/roofit/roofitcore/src/RooHelpers.cxx b/roofit/roofitcore/src/RooHelpers.cxx index e0ec13a386d1c..ed7c0aa850708 100644 --- a/roofit/roofitcore/src/RooHelpers.cxx +++ b/roofit/roofitcore/src/RooHelpers.cxx @@ -181,41 +181,13 @@ std::pair getRangeOrBinningInterval(RooAbsArg const* arg, const /// Check if there is any overlap when a list of ranges is applied to a set of observables. -/// \param[in] pdf the PDF +/// \param[in] observables The observables to check for overlap /// \param[in] data RooAbsCollection with the observables to check for overlap. /// \param[in] rangeNames The names of the ranges. -/// \param[in] splitRange If `true`, each component of a RooSimultaneous will -/// be checked individually for overlaps, with the range -/// names in that component suffixed by `_`. -/// See the `SplitRange()` command argument of -/// RooAbsPdf::fitTo()` to understand where this is used. -bool checkIfRangesOverlap(RooAbsPdf const& pdf, +bool checkIfRangesOverlap(RooArgSet const& observables, RooAbsData const& data, - std::vector const& rangeNames, - bool splitRange) + std::vector const& rangeNames) { - // If the PDF is a RooSimultaneous and the `splitRange` option is set, we - // have to check each component PDF with a different set of rangeNames, each - // suffixed by the category name. - if(splitRange && dynamic_cast(&pdf)) { - auto const& simPdf = static_cast(pdf); - bool hasOverlap = false; - std::vector rangeNamesSplit; - for (const auto& catState : simPdf.indexCat()) { - const std::string& catName = catState.first; - for(std::string const& rangeName : rangeNames) { - rangeNamesSplit.emplace_back(rangeName + "_" + catName); - } - hasOverlap |= checkIfRangesOverlap(*simPdf.getPdf(catName.c_str()), data, rangeNamesSplit, false); - - rangeNamesSplit.clear(); - } - - return hasOverlap; - } - - auto observables = *pdf.getObservables(data); - auto getLimits = [&](RooAbsRealLValue const& rlv, const char* rangeName) { // RooDataHistCase From f9797217df8a3e5fc4d52e6ef2a0754ec6f377d0 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 29 Sep 2022 14:32:58 +0200 Subject: [PATCH 8/9] [RF] Support comma-separated range names in RooDataSet reduction The new implementation of multi-ranged fits is not relying on the `RooAbsData::reduce()` function to work with multiple comma-separated ranges. Hence, this is implemented now. --- roofit/roofitcore/src/RooAbsData.cxx | 6 +----- roofit/roofitcore/src/RooDataSet.cxx | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/roofit/roofitcore/src/RooAbsData.cxx b/roofit/roofitcore/src/RooAbsData.cxx index c8d9e9d08a1d7..dfef90dbbbc79 100644 --- a/roofit/roofitcore/src/RooAbsData.cxx +++ b/roofit/roofitcore/src/RooAbsData.cxx @@ -498,13 +498,9 @@ RooAbsData* RooAbsData::reduce(const RooCmdArg& arg1,const RooCmdArg& arg2,const RooFormulaVar cutVarTmp(cutSpec,cutSpec,*get()) ; ret = reduceEng(varSubset,&cutVarTmp,cutRange,nStart,nStop) ; - } else if (cutVar) { - - ret = reduceEng(varSubset,cutVar,cutRange,nStart,nStop) ; - } else { - ret = reduceEng(varSubset,0,cutRange,nStart,nStop) ; + ret = reduceEng(varSubset,cutVar,cutRange,nStart,nStop) ; } diff --git a/roofit/roofitcore/src/RooDataSet.cxx b/roofit/roofitcore/src/RooDataSet.cxx index b3763d4eaf4df..40c4e0b0fa8df 100644 --- a/roofit/roofitcore/src/RooDataSet.cxx +++ b/roofit/roofitcore/src/RooDataSet.cxx @@ -100,6 +100,7 @@ the new `RooAbsData::uniqueId()`. #include "RooCompositeDataStore.h" #include "RooSentinel.h" #include "RooTrace.h" +#include "RooHelpers.h" #include "ROOT/StringUtils.hxx" @@ -886,7 +887,28 @@ RooAbsData* RooDataSet::reduceEng(const RooArgSet& varSubset, const RooFormulaVa if (_wgtVar) { tmp.add(*_wgtVar) ; } + + if (!cutRange || strchr(cutRange,',')==0) { return new RooDataSet(GetName(), GetTitle(), this, tmp, cutVar, cutRange, nStart, nStop) ; + } else { + // Composite case: multiple ranges + auto tokens = ROOT::Split(cutRange, ","); + if (RooHelpers::checkIfRangesOverlap(tmp, *this, tokens)) { + std::stringstream errMsg; + errMsg << "Error in RooAbsData::reduce! The ranges " << cutRange << " are overlapping!"; + throw std::runtime_error(errMsg.str()); + } + RooDataSet * out = nullptr; + for (const auto& token : tokens) { + if(!out) { + out = new RooDataSet(GetName(), GetTitle(), this, tmp, cutVar, token.c_str(), nStart, nStop); + } else { + RooDataSet appendedData{GetName(), GetTitle(), this, tmp, cutVar, token.c_str(), nStart, nStop}; + out->append(appendedData); + } + } + return out; + } } From a3bfd679f071d51bb4823bf54f905ff77b175770 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 29 Sep 2022 15:39:22 +0200 Subject: [PATCH 9/9] [RF] Enable closure check in multi-range RooSimultaneous test Now that the multi-ranged fits for RooSimultaneous got re-implemented in a better way with hopefully less bugs, the closure check for multi-range fits in `testRooSimultaneous` succeeds, so it should be done in the test. Closes #11447. --- .../roofitcore/test/testRooSimultaneous.cxx | 44 ++++++++----------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/roofit/roofitcore/test/testRooSimultaneous.cxx b/roofit/roofitcore/test/testRooSimultaneous.cxx index aad39051e2c67..e6194268e182d 100644 --- a/roofit/roofitcore/test/testRooSimultaneous.cxx +++ b/roofit/roofitcore/test/testRooSimultaneous.cxx @@ -1,6 +1,7 @@ // Tests for the RooSimultaneous // Authors: Jonas Rembser, CERN 06/2021 +#include #include #include #include @@ -140,7 +141,7 @@ TEST(RooSimultaneous, MultiRangeFitWithSplitRange) int nEventsInCat = 11000; RooWorkspace wsCat1{"wsCat1"}; - wsCat1.factory("Gaussian::pdf_cat1(x_cat1[0,10],muCat1[4,0,10],sigma_cat1[1.0,0.1,10.0])"); + wsCat1.factory("Gaussian::pdf_cat1(x_cat1[0,10],mu_cat1[4,0,10],sigma_cat1[1.0,0.1,10.0])"); RooAbsPdf &pdfCat1 = *wsCat1.pdf("pdf_cat1"); RooRealVar &xCat1 = *wsCat1.var("x_cat1"); xCat1.setRange("SideBandLo_cat1", 0, 2); @@ -148,12 +149,12 @@ TEST(RooSimultaneous, MultiRangeFitWithSplitRange) std::unique_ptr dsCat1{pdfCat1.generate(xCat1, nEventsInCat)}; RooWorkspace wsCat2{"wsCat2"}; - wsCat2.factory("Gaussian::pdf_cat2(x_cat2[0,10],muCat2[6,0,10],sigma_cat2[1.0,0.1,10.0])"); + wsCat2.factory("Gaussian::pdf_cat2(x_cat2[0,10],mu_cat2[6,0,10],sigma_cat2[1.0,0.1,10.0])"); RooAbsPdf &pdfCat2 = *wsCat2.pdf("pdf_cat2"); RooRealVar &xCat2 = *wsCat2.var("x_cat2"); xCat2.setRange("SideBandLo_cat2", 0, 4); xCat2.setRange("SideBandHi_cat2", 8, 10); - std::unique_ptr dsCat2{pdfCat1.generate(xCat2, nEventsInCat)}; + std::unique_ptr dsCat2{pdfCat2.generate(xCat2, nEventsInCat)}; RooCategory indexCat{"cat", "cat"}; indexCat.defineType("cat1"); @@ -167,33 +168,24 @@ TEST(RooSimultaneous, MultiRangeFitWithSplitRange) RooDataSet combData{"combData", "", {xCat1, xCat2}, Index(indexCat), Import(dsmap)}; - std::unique_ptr nll1{pdfCat1.createNLL(*dsCat1, Range("SideBandLo_cat1,SideBandHi_cat1"))}; - std::unique_ptr nll2{pdfCat2.createNLL(*dsCat2, Range("SideBandLo_cat2,SideBandHi_cat2"))}; + const char *cutRange1 = "SideBandLo_cat1,SideBandHi_cat1"; + const char *cutRange2 = "SideBandLo_cat2,SideBandHi_cat2"; std::unique_ptr nllSim{simPdf.createNLL(combData, Range("SideBandLo,SideBandHi"), SplitRange())}; - // For now, we can't cross check the likelihood value and the unit test - // only checks whether the simultaneous likelihood can be created with the - // SplitRange() argument without the range overlap checks failing. Once the - // issue with multi-range simultaneous likelihood values is fixed, the rest - // of the test can be commented out: - - //const double nll1Val = nll1->getVal(); - //const double nll2Val = nll2->getVal(); // In simultaneous PDFs, the probability is normalized over the categories, // so we have to do that as well when computing the reference value. Since // we do a ranged fit, we have to consider the ranges when calculating the // number of events in data. - //double nCat1InRange = std::unique_ptr - //{ - //dsCat1->reduce(CutRange("SideBandLo_cat1,SideBandHi_cat1")) - //} -> sumEntries(); - //double nCat2InRange = std::unique_ptr - //{ - //dsCat2->reduce(CutRange("SideBandLo_cat2,SideBandHi_cat2")) - //} -> sumEntries(); - //const double nllSimRefVal = (nCat1InRange + nCat2InRange) * std::log(2) + nll1Val + nll2Val; - - //const double nllSimVal = nllSim->getVal(); - - //EXPECT_FLOAT_EQ(nllSimVal, nllSimRefVal); + double n1 = std::unique_ptr(dsCat1->reduce(CutRange(cutRange1)))->sumEntries(); + double n2 = std::unique_ptr(dsCat2->reduce(CutRange(cutRange2)))->sumEntries(); + const double normTerm = (n1 + n2) * std::log(2); + + std::unique_ptr nll1{pdfCat1.createNLL(*dsCat1, Range(cutRange1))}; + std::unique_ptr nll2{pdfCat2.createNLL(*dsCat2, Range(cutRange2))}; + RooAddition nllSimRef{"nllSimRef", "nllSimRef", {*nll1, *nll2, RooConst(normTerm)}}; + + const double nllSimRefVal = nllSimRef.getVal(); + const double nllSimVal = nllSim->getVal(); + + EXPECT_FLOAT_EQ(nllSimVal, nllSimRefVal); }