From 69b3c77e70c6785ef1aba6ecde23653e1450b564 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Fri, 15 Sep 2023 13:34:00 +0200 Subject: [PATCH] [RF] Remove multi-range chi-square fit logic from RooAbsPdf Almost a year ago, I fixed the support for comma-separated normalization ranges for pdfs, e.g. `pdf.setNormRange("range1,range2")` was fixed also for RooAddPdfs. As a result, the logic for multi-range likelihood fits was removed from `createNLL()`, because the multi-range fit didn't have to be treated as a special case anymore. The same applies also to chi-square fits. In fact, the reason why the `RooChi2Var` constructor and `RooAbsReal::createChi2()` methods had a `RooAbsPdf` overload was *only* this workaround! For regular RooAbsReals, the workaround was not necessary, because there is no normalization. Therefore, quite a few functions were removed in this commit. The multi-range chi2 fit is now also validated by the multi-range fit unit test in `testRooAbsPdf`. This is a follow-up to PR #11455 (commit fa10523706 in particular), where the same change was already make for regular likelihoods. --- roofit/roofitcore/inc/RooAbsPdf.h | 11 ---- roofit/roofitcore/inc/RooAbsReal.h | 7 --- roofit/roofitcore/inc/RooChi2Var.h | 5 -- roofit/roofitcore/src/RooAbsPdf.cxx | 78 ------------------------ roofit/roofitcore/src/RooAbsReal.cxx | 33 ++++++---- roofit/roofitcore/src/RooChi2Var.cxx | 66 +------------------- roofit/roofitcore/test/testRooAbsPdf.cxx | 52 ++++++++++++++-- 7 files changed, 71 insertions(+), 181 deletions(-) diff --git a/roofit/roofitcore/inc/RooAbsPdf.h b/roofit/roofitcore/inc/RooAbsPdf.h index 4796a21c03111..c42acf65b9a13 100644 --- a/roofit/roofitcore/inc/RooAbsPdf.h +++ b/roofit/roofitcore/inc/RooAbsPdf.h @@ -204,21 +204,10 @@ class RooAbsPdf : public RooAbsReal { // Chi^2 fits to histograms using RooAbsReal::createChi2 ; - RooFit::OwningPtr createChi2(RooDataHist& data, const RooCmdArg& arg1={}, const RooCmdArg& arg2={}, - const RooCmdArg& arg3={}, const RooCmdArg& arg4={}, const RooCmdArg& arg5={}, - const RooCmdArg& arg6={}, const RooCmdArg& arg7={}, const RooCmdArg& arg8={}) override ; // Chi^2 fits to X-Y datasets RooFit::OwningPtr createChi2(RooDataSet& data, const RooLinkedList& cmdList) override ; - /// \cond ROOFIT_INTERNAL - std::string createChi2DataHistCmdArgs() const override - { - return "Range,RangeWithName,NumCPU,Optimize,ProjectedObservables," - "AddCoefRange,SplitRange,DataError,Extended,IntegrateBins"; - } - /// \endcond ROOFIT_INTERNAL - // Constraint management virtual RooArgSet* getConstraints(const RooArgSet& /*observables*/, RooArgSet& /*constrainedParams*/, bool /*stripDisconnected*/, bool /*removeConstraintsFromPdf*/=false) const diff --git a/roofit/roofitcore/inc/RooAbsReal.h b/roofit/roofitcore/inc/RooAbsReal.h index 0cdcba7a52dba..d7e7a6a9b3617 100644 --- a/roofit/roofitcore/inc/RooAbsReal.h +++ b/roofit/roofitcore/inc/RooAbsReal.h @@ -195,13 +195,6 @@ class RooAbsReal : public RooAbsArg { const RooCmdArg& arg3={}, const RooCmdArg& arg4={}, const RooCmdArg& arg5={}, const RooCmdArg& arg6={}, const RooCmdArg& arg7={}, const RooCmdArg& arg8={}) ; - /// \cond ROOFIT_INTERNAL - /// If the overriding classes support more command args in the createChi2() - /// overload for RooDataHist, they can override this (like RooAbsPdf). - virtual std::string createChi2DataHistCmdArgs() const {return "Range,RangeWithName,NumCPU,Optimize,IntegrateBins"; } - /// \endcond ROOFIT_INTERNAL - - virtual RooFit::OwningPtr createProfile(const RooArgSet& paramsOfInterest) ; diff --git a/roofit/roofitcore/inc/RooChi2Var.h b/roofit/roofitcore/inc/RooChi2Var.h index e0a1e048f317a..5382f244f1cca 100644 --- a/roofit/roofitcore/inc/RooChi2Var.h +++ b/roofit/roofitcore/inc/RooChi2Var.h @@ -31,11 +31,6 @@ class RooChi2Var : public RooAbsOptTestStatistic { const RooCmdArg& arg4={}, const RooCmdArg& arg5={},const RooCmdArg& arg6={}, const RooCmdArg& arg7={}, const RooCmdArg& arg8={},const RooCmdArg& arg9={}) ; - RooChi2Var(const char *name, const char* title, RooAbsPdf& pdf, RooDataHist& data, - const RooCmdArg& arg1={}, const RooCmdArg& arg2={},const RooCmdArg& arg3={}, - const RooCmdArg& arg4={}, const RooCmdArg& arg5={},const RooCmdArg& arg6={}, - const RooCmdArg& arg7={}, const RooCmdArg& arg8={},const RooCmdArg& arg9={}) ; - enum FuncMode { Function, Pdf, ExtendedPdf } ; RooChi2Var(const RooChi2Var& other, const char* name=nullptr); diff --git a/roofit/roofitcore/src/RooAbsPdf.cxx b/roofit/roofitcore/src/RooAbsPdf.cxx index 57d0c21fad5ea..db905c97d36f6 100644 --- a/roofit/roofitcore/src/RooAbsPdf.cxx +++ b/roofit/roofitcore/src/RooAbsPdf.cxx @@ -1796,84 +1796,6 @@ RooFit::OwningPtr RooAbsPdf::fitTo(RooAbsData& data, const RooLink } -//////////////////////////////////////////////////////////////////////////////// -/// Create a \f$ \chi^2 \f$ from a histogram and this function. -/// -/// Options to control construction of the chi-square -/// ------------------------------------------ -/// The list of supported command arguments is given in the documentation for -/// RooChi2Var::RooChi2Var(const char *name, const char* title, RooAbsReal& func, RooDataHist& hdata, const RooCmdArg&,const RooCmdArg&,const RooCmdArg&, const RooCmdArg&,const RooCmdArg&,const RooCmdArg&, const RooCmdArg&,const RooCmdArg&,const RooCmdArg&). - -RooFit::OwningPtr RooAbsPdf::createChi2(RooDataHist& data, const RooCmdArg& arg1, const RooCmdArg& arg2, - const RooCmdArg& arg3, const RooCmdArg& arg4, const RooCmdArg& arg5, - const RooCmdArg& arg6, const RooCmdArg& arg7, const RooCmdArg& arg8) -{ - RooLinkedList cmdList ; - cmdList.Add((TObject*)&arg1) ; cmdList.Add((TObject*)&arg2) ; - cmdList.Add((TObject*)&arg3) ; cmdList.Add((TObject*)&arg4) ; - cmdList.Add((TObject*)&arg5) ; cmdList.Add((TObject*)&arg6) ; - cmdList.Add((TObject*)&arg7) ; cmdList.Add((TObject*)&arg8) ; - - RooCmdConfig pc("RooAbsPdf::createChi2(" + std::string(GetName()) + ")"); - pc.defineString("rangeName","RangeWithName",0,"",true) ; - pc.allowUndefined(true) ; - pc.process(cmdList) ; - if (!pc.ok(true)) { - return nullptr; - } - const char* rangeName = pc.getString("rangeName",0,true) ; - - // Construct Chi2 - RooAbsReal::setEvalErrorLoggingMode(RooAbsReal::CollectErrors) ; - RooAbsReal* chi2 ; - std::string baseName = "chi2_" + std::string(GetName()) + "_" + std::string(data.GetName()); - - // Clear possible range attributes from previous fits. - removeStringAttribute("fitrange"); - - if (!rangeName || strchr(rangeName,',')==0) { - // Simple case: default range, or single restricted range - - chi2 = new RooChi2Var(baseName.c_str(),baseName.c_str(),*this,data,arg1,arg2,arg3,arg4,arg5,arg6,arg7,arg8) ; - - } else { - - // Find which argument is RangeWithName - const RooCmdArg* rarg(0) ; - string rcmd = "RangeWithName" ; - if (arg1.GetName()==rcmd) rarg = &arg1 ; - if (arg2.GetName()==rcmd) rarg = &arg2 ; - if (arg3.GetName()==rcmd) rarg = &arg3 ; - if (arg4.GetName()==rcmd) rarg = &arg4 ; - if (arg5.GetName()==rcmd) rarg = &arg5 ; - if (arg6.GetName()==rcmd) rarg = &arg6 ; - if (arg7.GetName()==rcmd) rarg = &arg7 ; - if (arg8.GetName()==rcmd) rarg = &arg8 ; - - // Composite case: multiple ranges - RooArgList chi2List ; - for (std::string& token : ROOT::Split(rangeName, ",")) { - RooCmdArg subRangeCmd = RooFit::Range(token.c_str()) ; - // Construct chi2 while substituting original RangeWithName argument with subrange argument created above - auto chi2Comp = std::make_unique((baseName + "_" + token).c_str(), "chi^2", *this, data, - &arg1==rarg?subRangeCmd:arg1,&arg2==rarg?subRangeCmd:arg2, - &arg3==rarg?subRangeCmd:arg3,&arg4==rarg?subRangeCmd:arg4, - &arg5==rarg?subRangeCmd:arg5,&arg6==rarg?subRangeCmd:arg6, - &arg7==rarg?subRangeCmd:arg7,&arg8==rarg?subRangeCmd:arg8) ; - chi2List.addOwned(std::move(chi2Comp)); - } - chi2 = new RooAddition(baseName.c_str(),"chi^2",chi2List); - chi2->addOwnedComponents(std::move(chi2List)); - } - RooAbsReal::setEvalErrorLoggingMode(RooAbsReal::PrintErrors) ; - - - return RooFit::Detail::owningPtr(std::unique_ptr{chi2}); -} - - - - //////////////////////////////////////////////////////////////////////////////// /// Argument-list version of RooAbsPdf::createChi2() diff --git a/roofit/roofitcore/src/RooAbsReal.cxx b/roofit/roofitcore/src/RooAbsReal.cxx index 5e3b5b18132ee..5a5726ce7f8a8 100644 --- a/roofit/roofitcore/src/RooAbsReal.cxx +++ b/roofit/roofitcore/src/RooAbsReal.cxx @@ -4200,17 +4200,20 @@ RooFit::OwningPtr RooAbsReal::chi2FitTo(RooDataHist& data, const R /// RooAbsReal::chi2FitTo(RooDataHist&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&) and /// RooAbsPdf::chi2FitTo(RooDataHist&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&). -RooFit::OwningPtr RooAbsReal::chi2FitTo(RooDataHist& data, const RooLinkedList& cmdList) +RooFit::OwningPtr RooAbsReal::chi2FitTo(RooDataHist &data, const RooLinkedList &cmdList) { - // Select the pdf-specific commands - RooCmdConfig pc("RooAbsPdf::chi2FitTo(" + std::string(GetName()) + ")"); + // Select the pdf-specific commands + RooCmdConfig pc("RooAbsPdf::chi2FitTo(" + std::string(GetName()) + ")"); - // Pull arguments to be passed to chi2 construction from list - RooLinkedList fitCmdList(cmdList) ; - RooLinkedList chi2CmdList = pc.filterCmdList(fitCmdList, createChi2DataHistCmdArgs().c_str()); + // Pull arguments to be passed to chi2 construction from list + RooLinkedList fitCmdList(cmdList); - std::unique_ptr chi2{createChi2(data,chi2CmdList)}; - return chi2FitDriver(*chi2,fitCmdList) ; + auto createChi2DataHistCmdArgs = "Range,RangeWithName,NumCPU,Optimize,IntegrateBins,ProjectedObservables," + "AddCoefRange,SplitRange,DataError,Extended"; + RooLinkedList chi2CmdList = pc.filterCmdList(fitCmdList, createChi2DataHistCmdArgs); + + std::unique_ptr chi2{createChi2(data, chi2CmdList)}; + return chi2FitDriver(*chi2, fitCmdList); } @@ -4232,10 +4235,18 @@ RooFit::OwningPtr RooAbsReal::createChi2(RooDataHist &data, const Ro const RooCmdArg &arg5, const RooCmdArg &arg6, const RooCmdArg &arg7, const RooCmdArg &arg8) { - std::string name = "chi2_" + std::string(GetName()) + "_" + data.GetName(); + // Construct Chi2 + RooAbsReal::setEvalErrorLoggingMode(RooAbsReal::CollectErrors); + std::string baseName = "chi2_" + std::string(GetName()) + "_" + data.GetName(); + + // Clear possible range attributes from previous fits. + removeStringAttribute("fitrange"); + + auto chi2 = std::make_unique(baseName.c_str(), baseName.c_str(), *this, data, arg1, arg2, arg3, arg4, + arg5, arg6, arg7, arg8); + RooAbsReal::setEvalErrorLoggingMode(RooAbsReal::PrintErrors); - return RooFit::Detail::owningPtr(std::make_unique(name.c_str(), name.c_str(), *this, data, arg1, arg2, - arg3, arg4, arg5, arg6, arg7, arg8)); + return RooFit::Detail::owningPtr(std::move(chi2)); } diff --git a/roofit/roofitcore/src/RooChi2Var.cxx b/roofit/roofitcore/src/RooChi2Var.cxx index 0802f2a51b1d7..933ca8b79dd95 100644 --- a/roofit/roofitcore/src/RooChi2Var.cxx +++ b/roofit/roofitcore/src/RooChi2Var.cxx @@ -67,7 +67,7 @@ using namespace std; namespace { template - RooAbsTestStatistic::Configuration makeRooAbsTestStatisticCfgForFunc(Args const& ... args) { + RooAbsTestStatistic::Configuration makeRooAbsTestStatisticCfg(Args const& ... args) { RooAbsTestStatistic::Configuration cfg; cfg.rangeName = RooCmdConfig::decodeStringOnTheFly("RooChi2Var::RooChi2Var","RangeWithName",0,"",args...); cfg.nCPU = RooCmdConfig::decodeIntOnTheFly("RooChi2Var::RooChi2Var","NumCPU",0,1,args...); @@ -75,12 +75,6 @@ namespace { cfg.verbose = static_cast(RooCmdConfig::decodeIntOnTheFly("RooChi2Var::RooChi2Var","Verbose",0,1,args...)); cfg.cloneInputData = false; cfg.integrateOverBinsPrecision = RooCmdConfig::decodeDoubleOnTheFly("RooChi2Var::RooChi2Var", "IntegrateBins", 0, -1., {args...}); - return cfg; - } - - template - RooAbsTestStatistic::Configuration makeRooAbsTestStatisticCfgForPdf(Args const& ... args) { - auto cfg = makeRooAbsTestStatisticCfgForFunc(args...); cfg.addCoefRangeName = RooCmdConfig::decodeStringOnTheFly("RooChi2Var::RooChi2Var","AddCoefRange",0,"",args...); cfg.splitCutRange = static_cast(RooCmdConfig::decodeIntOnTheFly("RooChi2Var::RooChi2Var","SplitRange",0,0,args...)); return cfg; @@ -88,7 +82,6 @@ namespace { } ClassImp(RooChi2Var); -; RooArgSet RooChi2Var::_emptySet ; @@ -140,7 +133,7 @@ RooChi2Var::RooChi2Var(const char *name, const char* title, RooAbsReal& func, Ro const RooCmdArg& arg4,const RooCmdArg& arg5,const RooCmdArg& arg6, const RooCmdArg& arg7,const RooCmdArg& arg8,const RooCmdArg& arg9) : RooAbsOptTestStatistic(name,title,func,hdata,_emptySet, - makeRooAbsTestStatisticCfgForFunc(arg1,arg2,arg3,arg4,arg5,arg6,arg7,arg8,arg9)) + makeRooAbsTestStatisticCfg(arg1,arg2,arg3,arg4,arg5,arg6,arg7,arg8,arg9)) { RooCmdConfig pc("RooChi2Var::RooChi2Var") ; pc.defineInt("etype","DataError",0,(Int_t)RooDataHist::Auto) ; @@ -165,61 +158,6 @@ RooChi2Var::RooChi2Var(const char *name, const char* title, RooAbsReal& func, Ro } -//////////////////////////////////////////////////////////////////////////////// -/// RooChi2Var constructor. Optional arguments taken -/// -/// \param[in] name Name of the PDF -/// \param[in] title Title for plotting etc. -/// \param[in] pdf PDF to fit -/// \param[in] hdata Data histogram -/// \param[in] arg1,arg2,arg3,arg4,arg5,arg6,arg7,arg8,arg9 Optional arguments according to table below. -/// -///
Argument Effect -///
-/// Extended() Include extended term in calculation -///
-/// DataError() Choose between Poisson errors and Sum-of-weights errors -///
-/// NumCPU() Activate parallel processing feature -///
-/// Range() Fit only selected region -///
-/// SumCoefRange() Set the range in which to interpret the coefficients of RooAddPdf components -///
-/// SplitRange() Fit range is split by index category of simultaneous PDF -///
-/// ConditionalObservables() Define projected observables -///
-/// Verbose() Verbose output of GOF framework -///
-/// IntegrateBins() Integrate PDF within each bin. This sets the desired precision. - -RooChi2Var::RooChi2Var(const char *name, const char* title, RooAbsPdf& pdf, RooDataHist& hdata, - const RooCmdArg& arg1,const RooCmdArg& arg2,const RooCmdArg& arg3, - const RooCmdArg& arg4,const RooCmdArg& arg5,const RooCmdArg& arg6, - const RooCmdArg& arg7,const RooCmdArg& arg8,const RooCmdArg& arg9) : - RooAbsOptTestStatistic(name,title,pdf,hdata, - *RooCmdConfig::decodeSetOnTheFly("RooChi2Var::RooChi2Var","ProjectedObservables",0,&_emptySet, - arg1,arg2,arg3,arg4,arg5,arg6,arg7,arg8,arg9), - makeRooAbsTestStatisticCfgForPdf(arg1,arg2,arg3,arg4,arg5,arg6,arg7,arg8,arg9)) -{ - RooCmdConfig pc("RooChi2Var::RooChi2Var") ; - pc.defineInt("extended","Extended",0,RooAbsPdf::extendedFitDefault) ; - pc.defineInt("etype","DataError",0,(Int_t)RooDataHist::Auto) ; - pc.allowUndefined() ; - - pc.process(arg1) ; pc.process(arg2) ; pc.process(arg3) ; - pc.process(arg4) ; pc.process(arg5) ; pc.process(arg6) ; - pc.process(arg7) ; pc.process(arg8) ; pc.process(arg9) ; - - _funcMode = pdf.interpretExtendedCmdArg(pc.getInt("extended")) ? ExtendedPdf : Pdf ; - _etype = (RooDataHist::ErrorType) pc.getInt("etype") ; - if (_etype==RooAbsData::Auto) { - _etype = hdata.isNonPoissonWeighted()? RooAbsData::SumW2 : RooAbsData::Expected ; - } -} - - //////////////////////////////////////////////////////////////////////////////// /// Copy constructor diff --git a/roofit/roofitcore/test/testRooAbsPdf.cxx b/roofit/roofitcore/test/testRooAbsPdf.cxx index 66e13b10067bf..afc8cd6fe6f2e 100644 --- a/roofit/roofitcore/test/testRooAbsPdf.cxx +++ b/roofit/roofitcore/test/testRooAbsPdf.cxx @@ -192,11 +192,11 @@ TEST_P(FitTest, MultiRangeFit) nsig.setError(0.0); }; - // loop over non-extended and extended fit - for (auto *model : {static_cast(&modelSimple), static_cast(&modelExtended)}) { + std::unique_ptr dataSet{modelSimple.generate(x, nEvents)}; + std::unique_ptr dataHist{static_cast(*dataSet).binnedClone()}; - std::unique_ptr dataSet{model->generate(x, nEvents)}; - std::unique_ptr dataHist{static_cast(*dataSet).binnedClone()}; + // loop over non-extended and extended fit + for (auto *model : {&modelSimple, &modelExtended}) { // loop over binned fit and unbinned fit for (auto *data : {dataSet.get(), dataHist.get()}) { @@ -214,6 +214,29 @@ TEST_P(FitTest, MultiRangeFit) << "Results of fitting " << model->GetName() << " to a " << data->ClassName() << " should be very similar."; } } + + // If the BatchMode is off, we are doing the same cross-check also with the + // chi-square fit on the RooDataHist. + if (_batchMode == "off") { + + auto &dh = static_cast(*dataHist); + + // loop over non-extended and extended fit + for (auto *model : {&modelSimple, &modelExtended}) { + + // full range + resetValues(); + std::unique_ptr fitResultFull{model->chi2FitTo(dh, Range("full"), Save(), PrintLevel(-1))}; + + // part (side band fit, but the union of the side bands is the full range) + resetValues(); + std::unique_ptr fitResultPart{model->chi2FitTo(dh, Range("low,high"), Save(), PrintLevel(-1))}; + + EXPECT_TRUE(fitResultPart->isIdentical(*fitResultFull)) + << "Results of fitting " << model->GetName() + << " to a RooDataHist should be very similar also for chi2FitTo()."; + } + } } // ROOT-9530: RooFit side-band fit inconsistent with fit to full range (2D case) @@ -287,6 +310,25 @@ TEST_P(FitTest, MultiRangeFit2D) EXPECT_TRUE(fitResultPart->isIdentical(*fitResultFull)) << "Results of fitting " << model.GetName() << " to a " << data->ClassName() << " should be very similar."; } + + // If the BatchMode is off, we are doing the same cross-check also with the + // chi-square fit on the RooDataHist. + if (_batchMode == "off") { + + // full range + resetValues(); + std::unique_ptr fitResultFull{ + model.fitTo(*dataHist, Range("FULL"), Save(), PrintLevel(-1), BatchMode(_batchMode))}; + + // part (side band fit, but the union of the side bands is the full range) + resetValues(); + std::unique_ptr fitResultPart{ + model.fitTo(*dataHist, Range("SB1,SB2,SIG"), Save(), PrintLevel(-1), BatchMode(_batchMode))}; + + EXPECT_TRUE(fitResultPart->isIdentical(*fitResultFull)) + << "Results of fitting " << model.GetName() + << " to a RooDataHist should be very similar also for chi2FitTo()."; + } } // This test will crash if the cached normalization sets are not reset @@ -349,7 +391,7 @@ TEST(RooAbsPdf, NormSetChange) EXPECT_NE(v1, v2); } -INSTANTIATE_TEST_SUITE_P(RooAbsPdf, FitTest, testing::Values("Off", "Cpu"), +INSTANTIATE_TEST_SUITE_P(RooAbsPdf, FitTest, testing::Values("off", "cpu"), [](testing::TestParamInfo const ¶mInfo) { std::stringstream ss; ss << "BatchMode" << std::get<0>(paramInfo.param);