From 9f7f1bd52e32258bee6929e33f4ce87828a95a0d Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Fri, 15 Sep 2023 13:34:00 +0200 Subject: [PATCH 1/4] [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 8dfc8916490a9..9c0e0fef35269 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 @@ -347,7 +389,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); From 8fa8cc05e1837ae6a37748daa9451e65f473f0cf Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Fri, 15 Sep 2023 14:52:22 +0200 Subject: [PATCH 2/4] [RF] Avoid separate RooXYChi2Var constructors for pdfs and functions It can be checked at runtime if a given `RooAbsReal` is a pdf or not. Like this, we also don't need a separate override of `createChi2(RooDataSet &)` in RooAbsPdf. --- .../ROOT/_pythonization/_roofit/_rooabspdf.py | 15 +-- roofit/roofitcore/inc/RooAbsPdf.h | 6 - roofit/roofitcore/inc/RooXYChi2Var.h | 2 - roofit/roofitcore/src/RooAbsPdf.cxx | 36 ----- roofit/roofitcore/src/RooAbsReal.cxx | 2 +- roofit/roofitcore/src/RooXYChi2Var.cxx | 126 ++++++------------ 6 files changed, 45 insertions(+), 142 deletions(-) diff --git a/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabspdf.py b/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabspdf.py index 3427dc60ab7f4..9c90ca93e2680 100644 --- a/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabspdf.py +++ b/bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabspdf.py @@ -43,7 +43,7 @@ def _pack_cmd_args(*args, **kwargs): class RooAbsPdf(RooAbsReal): r"""Some member functions of RooAbsPdf that take a RooCmdArg as argument also support keyword arguments. So far, this applies to RooAbsPdf::fitTo, RooAbsPdf::plotOn, RooAbsPdf::generate, RooAbsPdf::paramOn, RooAbsPdf::createCdf, - RooAbsPdf::generateBinned, RooAbsPdf::createChi2, RooAbsPdf::prepareMultiGen and RooAbsPdf::createNLL. + RooAbsPdf::generateBinned, RooAbsPdf::prepareMultiGen and RooAbsPdf::createNLL. For example, the following code is equivalent in PyROOT: \code{.py} # Directly passing a RooCmdArg: @@ -115,19 +115,6 @@ def createNLL(self, *args, **kwargs): # Redefinition of `RooAbsPdf.createNLL` for keyword arguments. return self._createNLL(args[0], _pack_cmd_args(*args[1:], **kwargs)) - @cpp_signature( - "RooAbsReal *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={}) ;" - ) - def createChi2(self, *args, **kwargs): - r"""The RooAbsPdf::createChi2() function is pythonized with the command argument pythonization. - The keywords must correspond to the CmdArgs of the function. - """ - # Redefinition of `RooAbsPdf.createChi2` for keyword arguments. - args, kwargs = _kwargs_to_roocmdargs(*args, **kwargs) - return self._createChi2(*args, **kwargs) - @cpp_signature( "RooAbsReal *RooAbsPdf::createCdf(const RooArgSet& iset, const RooCmdArg& arg1, const RooCmdArg& arg2={}," " const RooCmdArg& arg3={}, const RooCmdArg& arg4={}," diff --git a/roofit/roofitcore/inc/RooAbsPdf.h b/roofit/roofitcore/inc/RooAbsPdf.h index c42acf65b9a13..9afac5c6f693a 100644 --- a/roofit/roofitcore/inc/RooAbsPdf.h +++ b/roofit/roofitcore/inc/RooAbsPdf.h @@ -202,12 +202,6 @@ class RooAbsPdf : public RooAbsReal { return createNLL(data, *RooFit::Detail::createCmdList(&arg1, &args...)); } - // Chi^2 fits to histograms - using RooAbsReal::createChi2 ; - - // Chi^2 fits to X-Y datasets - RooFit::OwningPtr createChi2(RooDataSet& data, const RooLinkedList& cmdList) override ; - // Constraint management virtual RooArgSet* getConstraints(const RooArgSet& /*observables*/, RooArgSet& /*constrainedParams*/, bool /*stripDisconnected*/, bool /*removeConstraintsFromPdf*/=false) const diff --git a/roofit/roofitcore/inc/RooXYChi2Var.h b/roofit/roofitcore/inc/RooXYChi2Var.h index 17d098b4a8ff8..97f82632ec61f 100644 --- a/roofit/roofitcore/inc/RooXYChi2Var.h +++ b/roofit/roofitcore/inc/RooXYChi2Var.h @@ -32,8 +32,6 @@ class RooXYChi2Var : public RooAbsOptTestStatistic { // Constructors, assignment etc RooXYChi2Var(const char *name, const char* title, RooAbsReal& func, RooDataSet& data, bool integrate=false) ; RooXYChi2Var(const char *name, const char* title, RooAbsReal& func, RooDataSet& data, RooRealVar& yvar, bool integrate=false) ; - RooXYChi2Var(const char *name, const char* title, RooAbsPdf& extPdf, RooDataSet& data, bool integrate=false) ; - RooXYChi2Var(const char *name, const char* title, RooAbsPdf& extPdf, RooDataSet& data, RooRealVar& yvar, bool integrate=false) ; RooXYChi2Var(const RooXYChi2Var& other, const char* name=nullptr); TObject* clone(const char* newname) const override { return new RooXYChi2Var(*this,newname); } diff --git a/roofit/roofitcore/src/RooAbsPdf.cxx b/roofit/roofitcore/src/RooAbsPdf.cxx index db905c97d36f6..6941ca38869d8 100644 --- a/roofit/roofitcore/src/RooAbsPdf.cxx +++ b/roofit/roofitcore/src/RooAbsPdf.cxx @@ -161,7 +161,6 @@ called for each data event. #include "RooFitResult.h" #include "RooNumGenConfig.h" #include "RooCachedReal.h" -#include "RooXYChi2Var.h" #include "RooChi2Var.h" #include "RooMinimizer.h" #include "RooRealIntegral.h" @@ -1796,41 +1795,6 @@ RooFit::OwningPtr RooAbsPdf::fitTo(RooAbsData& data, const RooLink } -//////////////////////////////////////////////////////////////////////////////// -/// Argument-list version of RooAbsPdf::createChi2() - -RooFit::OwningPtr RooAbsPdf::createChi2(RooDataSet& data, const RooLinkedList& cmdList) -{ - // Select the pdf-specific commands - RooCmdConfig pc("RooAbsPdf::createChi2(" + std::string(GetName()) + ")"); - - pc.defineInt("integrate","Integrate",0,0) ; - pc.defineObject("yvar","YVar",0,0) ; - - // Process and check varargs - pc.process(cmdList) ; - if (!pc.ok(true)) { - return nullptr; - } - - // Decode command line arguments - bool integrate = pc.getInt("integrate") ; - RooRealVar* yvar = (RooRealVar*) pc.getObject("yvar") ; - - std::string name = "chi2_" + std::string(GetName()) + "_" + std::string(data.GetName()); - - std::unique_ptr out; - if (yvar) { - out = std::make_unique(name.c_str(),name.c_str(),*this,data,*yvar,integrate) ; - } else { - out = std::make_unique(name.c_str(),name.c_str(),*this,data,integrate) ; - } - return RooFit::Detail::owningPtr(std::move(out)); -} - - - - //////////////////////////////////////////////////////////////////////////////// /// Print value of p.d.f, also print normalization integral that was last used, if any diff --git a/roofit/roofitcore/src/RooAbsReal.cxx b/roofit/roofitcore/src/RooAbsReal.cxx index 5a5726ce7f8a8..67812e4b783d3 100644 --- a/roofit/roofitcore/src/RooAbsReal.cxx +++ b/roofit/roofitcore/src/RooAbsReal.cxx @@ -4376,7 +4376,7 @@ RooFit::OwningPtr RooAbsReal::createChi2(RooDataSet& data, const Roo RooFit::OwningPtr RooAbsReal::createChi2(RooDataSet& data, const RooLinkedList& cmdList) { // Select the pdf-specific commands - RooCmdConfig pc("RooAbsPdf::fitTo(" + std::string(GetName()) + ")"); + RooCmdConfig pc("RooAbsReal::createChi2(" + std::string(GetName()) + ")"); pc.defineInt("integrate","Integrate",0,0) ; pc.defineObject("yvar","YVar",0,nullptr) ; diff --git a/roofit/roofitcore/src/RooXYChi2Var.cxx b/roofit/roofitcore/src/RooXYChi2Var.cxx index 327fbdd39d0a7..1c4ace635e870 100644 --- a/roofit/roofitcore/src/RooXYChi2Var.cxx +++ b/roofit/roofitcore/src/RooXYChi2Var.cxx @@ -60,7 +60,11 @@ namespace { //////////////////////////////////////////////////////////////////////////////// /// -/// RooXYChi2Var constructor with function and X-Y values dataset +/// RooXYChi2Var constructor with function and X-Y values dataset +/// +/// If the function is a pdf, it must be extendable. in this case, hhe value of +/// the function that defines the chi^2 in this form is takes as the p.d.f. +/// times the expected number of events /// /// An X-Y dataset is a weighted dataset with one or more observables X where the weight is interpreted /// as the Y value and the weight error is interpreted as the Y value error. The weight must have an @@ -73,55 +77,37 @@ namespace { /// are the double values that correspond to the Y and its error /// -RooXYChi2Var::RooXYChi2Var(const char *name, const char* title, RooAbsReal& func, RooDataSet& xydata, bool integrate) : - RooAbsOptTestStatistic(name,title,func,xydata,RooArgSet(),makeRooAbsTestStatisticCfg()), - _extended(false), - _integrate(integrate), - _intConfig(*defaultIntegratorConfig()) +RooXYChi2Var::RooXYChi2Var(const char *name, const char *title, RooAbsReal &func, RooDataSet &xydata, bool integrate) + : RooAbsOptTestStatistic(name, title, func, xydata, RooArgSet(), makeRooAbsTestStatisticCfg()), + _integrate(integrate), + _intConfig(*defaultIntegratorConfig()) { - _extended = false ; - _yvar = nullptr ; - - initialize() ; -} + bool isPdf = dynamic_cast(&func) != nullptr; + if (isPdf) { + auto &extPdf = static_cast(func); + if (!extPdf.canBeExtended()) { + throw std::runtime_error( + Form("RooXYChi2Var::RooXYChi2Var(%s) ERROR: Input p.d.f. must be extendible", GetName())); + } + } -//////////////////////////////////////////////////////////////////////////////// -/// -/// RooXYChi2Var constructor with function and X-Y values dataset -/// -/// An X-Y dataset is a weighted dataset with one or more observables X where given yvar is interpreted -/// as the Y value. The Y variable must have a non-zero error defined at each point for the chi^2 calculation to be meaningful. -/// -/// To store errors associated with the x and y values in a RooDataSet, call RooRealVar::setAttribute("StoreError") -/// on each X-type observable for which the error should be stored and add datapoints to the dataset as follows -/// -/// RooDataSet::add(xset,yval,yerr) where xset is the RooArgSet of x observables (with or without errors) and yval and yerr -/// are the double values that correspond to the Y and its error -/// - -RooXYChi2Var::RooXYChi2Var(const char *name, const char* title, RooAbsReal& func, RooDataSet& xydata, RooRealVar& yvar, bool integrate) : - RooAbsOptTestStatistic(name,title,func,xydata,RooArgSet(),makeRooAbsTestStatisticCfg()), - _extended(false), - _integrate(integrate), - _intConfig(*defaultIntegratorConfig()) -{ - _extended = false ; - _yvar = (RooRealVar*) _dataClone->get()->find(yvar.GetName()) ; + _extended = isPdf; + _yvar = nullptr; - initialize() ; + initialize(); } - //////////////////////////////////////////////////////////////////////////////// /// -/// RooXYChi2Var constructor with an extended p.d.f. and X-Y values dataset -/// The value of the function that defines the chi^2 in this form is takes as -/// the p.d.f. times the expected number of events +/// RooXYChi2Var constructor with function and X-Y values dataset. /// -/// An X-Y dataset is a weighted dataset with one or more observables X where the weight is interpreted -/// as the Y value and the weight error is interpreted as the Y value error. The weight must have an -/// non-zero error defined at each point for the chi^2 calculation to be meaningful. +/// If the function is a pdf, it must be extendable. in this case, hhe value of +/// the function that defines the chi^2 in this form is takes as the p.d.f. +/// times the expected number of events +/// +/// An X-Y dataset is a weighted dataset with one or more observables X where given yvar is interpreted +/// as the Y value. The Y variable must have a non-zero error defined at each point for the chi^2 calculation to be meaningful. /// /// To store errors associated with the x and y values in a RooDataSet, call RooRealVar::setAttribute("StoreError") /// on each X-type observable for which the error should be stored and add datapoints to the dataset as follows @@ -130,55 +116,29 @@ RooXYChi2Var::RooXYChi2Var(const char *name, const char* title, RooAbsReal& func /// are the double values that correspond to the Y and its error /// -RooXYChi2Var::RooXYChi2Var(const char *name, const char* title, RooAbsPdf& extPdf, RooDataSet& xydata, bool integrate) : - RooAbsOptTestStatistic(name,title,extPdf,xydata,RooArgSet(),makeRooAbsTestStatisticCfg()), - _extended(true), - _integrate(integrate), - _intConfig(*defaultIntegratorConfig()) +RooXYChi2Var::RooXYChi2Var(const char *name, const char *title, RooAbsReal &func, RooDataSet &xydata, RooRealVar &yvar, + bool integrate) + : RooAbsOptTestStatistic(name, title, func, xydata, RooArgSet(), makeRooAbsTestStatisticCfg()), + _integrate(integrate), + _intConfig(*defaultIntegratorConfig()) { - if (!extPdf.canBeExtended()) { - throw std::runtime_error(Form("RooXYChi2Var::RooXYChi2Var(%s) ERROR: Input p.d.f. must be extendible",GetName())); - } - _yvar = nullptr ; - initialize() ; -} - + bool isPdf = dynamic_cast(&func) != nullptr; + if (isPdf) { + auto &extPdf = static_cast(func); + if (!extPdf.canBeExtended()) { + throw std::runtime_error( + Form("RooXYChi2Var::RooXYChi2Var(%s) ERROR: Input p.d.f. must be extendible", GetName())); + } + } + _extended = isPdf; + _yvar = static_cast(_dataClone->get()->find(yvar.GetName())); -//////////////////////////////////////////////////////////////////////////////// -/// -/// RooXYChi2Var constructor with an extended p.d.f. and X-Y values dataset -/// The value of the function that defines the chi^2 in this form is takes as -/// the p.d.f. times the expected number of events -/// -/// An X-Y dataset is a weighted dataset with one or more observables X where the weight is interpreted -/// as the Y value and the weight error is interpreted as the Y value error. The weight must have an -/// non-zero error defined at each point for the chi^2 calculation to be meaningful. -/// -/// To store errors associated with the x and y values in a RooDataSet, call RooRealVar::setAttribute("StoreError") -/// on each X-type observable for which the error should be stored and add datapoints to the dataset as follows -/// -/// RooDataSet::add(xset,yval,yerr) where xset is the RooArgSet of x observables (with or without errors) and yval and yerr -/// are the double values that correspond to the Y and its error -/// - -RooXYChi2Var::RooXYChi2Var(const char *name, const char* title, RooAbsPdf& extPdf, RooDataSet& xydata, RooRealVar& yvar, bool integrate) : - RooAbsOptTestStatistic(name,title,extPdf,xydata,RooArgSet(),makeRooAbsTestStatisticCfg()), - _extended(true), - _integrate(integrate), - _intConfig(*defaultIntegratorConfig()) -{ - if (!extPdf.canBeExtended()) { - throw std::runtime_error(Form("RooXYChi2Var::ctor(%s) ERROR: Input p.d.f. must be an extendible",GetName())); - } - _yvar = (RooRealVar*) _dataClone->get()->find(yvar.GetName()) ; - initialize() ; + initialize(); } - - //////////////////////////////////////////////////////////////////////////////// /// Copy constructor From 7b7b05b90a0f831ef93d04e1deb3c15f28fcb575 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Fri, 15 Sep 2023 15:29:49 +0200 Subject: [PATCH 3/4] [RF] Support `NumCPU()` and `Range()` in createChi2() for RooDataSets This is achieved in the same was as in `createNLL()`, by forwarding the configuration options to the `RooAbsOptTestStatistic` base class. --- roofit/roofitcore/inc/RooXYChi2Var.h | 5 ++ roofit/roofitcore/src/RooAbsReal.cxx | 85 +++++++++++++++----------- roofit/roofitcore/src/RooXYChi2Var.cxx | 34 ++++------- 3 files changed, 68 insertions(+), 56 deletions(-) diff --git a/roofit/roofitcore/inc/RooXYChi2Var.h b/roofit/roofitcore/inc/RooXYChi2Var.h index 97f82632ec61f..10aaf3b1cc02f 100644 --- a/roofit/roofitcore/inc/RooXYChi2Var.h +++ b/roofit/roofitcore/inc/RooXYChi2Var.h @@ -32,6 +32,11 @@ class RooXYChi2Var : public RooAbsOptTestStatistic { // Constructors, assignment etc RooXYChi2Var(const char *name, const char* title, RooAbsReal& func, RooDataSet& data, bool integrate=false) ; RooXYChi2Var(const char *name, const char* title, RooAbsReal& func, RooDataSet& data, RooRealVar& yvar, bool integrate=false) ; + /// \cond ROOFIT_INTERNAL + // For internal use in RooAbsReal::createChi2(). + RooXYChi2Var(const char *name, const char *title, RooAbsReal& func, RooAbsData& data, RooRealVar *yvar, bool integrate, + RooAbsTestStatistic::Configuration const& cfg); + /// \endcond ROOFIT_INTERNAL RooXYChi2Var(const RooXYChi2Var& other, const char* name=nullptr); TObject* clone(const char* newname) const override { return new RooXYChi2Var(*this,newname); } diff --git a/roofit/roofitcore/src/RooAbsReal.cxx b/roofit/roofitcore/src/RooAbsReal.cxx index 67812e4b783d3..a2710cc0affab 100644 --- a/roofit/roofitcore/src/RooAbsReal.cxx +++ b/roofit/roofitcore/src/RooAbsReal.cxx @@ -4326,17 +4326,18 @@ RooFit::OwningPtr RooAbsReal::chi2FitTo(RooDataSet& xydata, const //////////////////////////////////////////////////////////////////////////////// /// \copydoc RooAbsReal::chi2FitTo(RooDataSet&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&) -RooFit::OwningPtr RooAbsReal::chi2FitTo(RooDataSet& xydata, const RooLinkedList& cmdList) +RooFit::OwningPtr RooAbsReal::chi2FitTo(RooDataSet &xydata, 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,"YVar,Integrate") ; + // Pull arguments to be passed to chi2 construction from list + RooLinkedList fitCmdList(cmdList); + auto createChi2DataSetCmdArgs = "YVar,Integrate,RangeWithName,NumCPU,Verbose"; + RooLinkedList chi2CmdList = pc.filterCmdList(fitCmdList, createChi2DataSetCmdArgs); - std::unique_ptr xychi2{createChi2(xydata,chi2CmdList)}; - return chi2FitDriver(*xychi2,fitCmdList) ; + std::unique_ptr xychi2{createChi2(xydata, chi2CmdList)}; + return chi2FitDriver(*xychi2, fitCmdList); } @@ -4369,44 +4370,57 @@ RooFit::OwningPtr RooAbsReal::createChi2(RooDataSet& data, const Roo } - //////////////////////////////////////////////////////////////////////////////// /// See RooAbsReal::createChi2(RooDataSet&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&,const RooCmdArg&) -RooFit::OwningPtr RooAbsReal::createChi2(RooDataSet& data, const RooLinkedList& cmdList) +RooFit::OwningPtr RooAbsReal::createChi2(RooDataSet &data, const RooLinkedList &cmdList) { - // Select the pdf-specific commands - RooCmdConfig pc("RooAbsReal::createChi2(" + std::string(GetName()) + ")"); - - pc.defineInt("integrate","Integrate",0,0) ; - pc.defineObject("yvar","YVar",0,nullptr) ; + // Select the pdf-specific commands + RooCmdConfig pc("RooAbsReal::createChi2(" + std::string(GetName()) + ")"); + + pc.defineInt("integrate", "Integrate", 0, 0); + pc.defineObject("yvar", "YVar", 0, nullptr); + pc.defineString("rangeName", "RangeWithName", 0, "", true); + pc.defineInt("numcpu", "NumCPU", 0, 1); + pc.defineInt("interleave", "NumCPU", 1, 0); + pc.defineInt("verbose", "Verbose", 0, 0); + + // Process and check varargs + pc.process(cmdList); + if (!pc.ok(true)) { + return nullptr; + } - // Process and check varargs - pc.process(cmdList) ; - if (!pc.ok(true)) { - return nullptr; - } + // Decode command line arguments + bool integrate = pc.getInt("integrate"); + RooRealVar *yvar = static_cast(pc.getObject("yvar")); + const char *rangeName = pc.getString("rangeName", 0, true); + Int_t numcpu = pc.getInt("numcpu"); + Int_t numcpu_strategy = pc.getInt("interleave"); + // strategy 3 works only for RooSimultaneous. + if (numcpu_strategy == 3 && !this->InheritsFrom("RooSimultaneous")) { + coutW(Minimization) << "Cannot use a NumCpu Strategy = 3 when the pdf is not a RooSimultaneous, " + "falling back to default strategy = 0" + << endl; + numcpu_strategy = 0; + } + RooFit::MPSplit interl = (RooFit::MPSplit)numcpu_strategy; + bool verbose = pc.getInt("verbose"); - // Decode command line arguments - bool integrate = pc.getInt("integrate") ; - RooRealVar* yvar = (RooRealVar*) pc.getObject("yvar") ; + RooAbsTestStatistic::Configuration cfg; + cfg.rangeName = rangeName ? rangeName : ""; + cfg.nCPU = numcpu; + cfg.interleave = interl; + cfg.verbose = verbose; + cfg.verbose = false; - std::string name = "chi2_" + std::string(GetName()) + "_" + data.GetName(); + std::string name = "chi2_" + std::string(GetName()) + "_" + data.GetName(); - std::unique_ptr out; - if (yvar) { - out = std::make_unique(name.c_str(),name.c_str(),*this,data,*yvar,integrate) ; - } else { - out = std::make_unique(name.c_str(),name.c_str(),*this,data,integrate) ; - } - return RooFit::Detail::owningPtr(std::move(out)); + return RooFit::Detail::owningPtr( + std::make_unique(name.c_str(), name.c_str(), *this, data, yvar, integrate, cfg)); } - - - - //////////////////////////////////////////////////////////////////////////////// /// Internal driver function for chi2 fits @@ -4430,6 +4444,7 @@ RooFit::OwningPtr RooAbsReal::chi2FitDriver(RooAbsReal& fcn, RooLi pc.defineString("mintype","Minimizer",0,"") ; pc.defineString("minalg","Minimizer",1,"minuit") ; pc.defineSet("minosSet","Minos",0,nullptr) ; + pc.allowUndefined() ; // Process and check varargs pc.process(cmdList) ; diff --git a/roofit/roofitcore/src/RooXYChi2Var.cxx b/roofit/roofitcore/src/RooXYChi2Var.cxx index 1c4ace635e870..0b62812fe6aeb 100644 --- a/roofit/roofitcore/src/RooXYChi2Var.cxx +++ b/roofit/roofitcore/src/RooXYChi2Var.cxx @@ -78,24 +78,8 @@ namespace { /// RooXYChi2Var::RooXYChi2Var(const char *name, const char *title, RooAbsReal &func, RooDataSet &xydata, bool integrate) - : RooAbsOptTestStatistic(name, title, func, xydata, RooArgSet(), makeRooAbsTestStatisticCfg()), - _integrate(integrate), - _intConfig(*defaultIntegratorConfig()) + : RooXYChi2Var{name, title, func, xydata, nullptr, integrate, makeRooAbsTestStatisticCfg()} { - bool isPdf = dynamic_cast(&func) != nullptr; - - if (isPdf) { - auto &extPdf = static_cast(func); - if (!extPdf.canBeExtended()) { - throw std::runtime_error( - Form("RooXYChi2Var::RooXYChi2Var(%s) ERROR: Input p.d.f. must be extendible", GetName())); - } - } - - _extended = isPdf; - _yvar = nullptr; - - initialize(); } //////////////////////////////////////////////////////////////////////////////// @@ -118,7 +102,16 @@ RooXYChi2Var::RooXYChi2Var(const char *name, const char *title, RooAbsReal &func RooXYChi2Var::RooXYChi2Var(const char *name, const char *title, RooAbsReal &func, RooDataSet &xydata, RooRealVar &yvar, bool integrate) - : RooAbsOptTestStatistic(name, title, func, xydata, RooArgSet(), makeRooAbsTestStatisticCfg()), + : RooXYChi2Var{name, title, func, xydata, &yvar, integrate, makeRooAbsTestStatisticCfg()} +{ +} + + +/// \cond ROOFIT_INTERNAL +// For internal use in RooAbsReal::createChi2(). +RooXYChi2Var::RooXYChi2Var(const char *name, const char *title, RooAbsReal &func, RooAbsData &data, RooRealVar *yvar, + bool integrate, RooAbsTestStatistic::Configuration const &cfg) + : RooAbsOptTestStatistic(name, title, func, data, RooArgSet(), cfg), _integrate(integrate), _intConfig(*defaultIntegratorConfig()) { @@ -133,10 +126,11 @@ RooXYChi2Var::RooXYChi2Var(const char *name, const char *title, RooAbsReal &func } _extended = isPdf; - _yvar = static_cast(_dataClone->get()->find(yvar.GetName())); + _yvar = yvar ? static_cast(_dataClone->get()->find(yvar->GetName())) : nullptr; initialize(); } +/// \endcond ROOFIT_INTERNAL //////////////////////////////////////////////////////////////////////////////// @@ -154,8 +148,6 @@ RooXYChi2Var::RooXYChi2Var(const RooXYChi2Var& other, const char* name) : } - - //////////////////////////////////////////////////////////////////////////////// /// Common constructor initialization From dc693dc5cc898021f766b0498cc475978107f3b7 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Fri, 15 Sep 2023 16:01:42 +0200 Subject: [PATCH 4/4] [RF] Bugfix in `RooDataSet::reduce()` by also copying the stored errors After adding support for subrange fits with the `RooXYChi2Var`, it initially didn't work because the y-value errors were missing in the dataset. This is because `RooDataSet::reduce()` is not copying the errors over to the reduced dataset, which is a bug. Fortunately, there is a straighforward solution. To create the reduced `RooDataSet`, it now doesn't use some buggy private constructor (that is now removed), but instead the trusty `RooAbsData::emptyClone()` method, for which I fixed the problem with the errors not being copied already some time ago. --- roofit/roofitcore/inc/RooDataSet.h | 3 -- roofit/roofitcore/src/RooDataSet.cxx | 74 +++++++++++----------------- 2 files changed, 30 insertions(+), 47 deletions(-) diff --git a/roofit/roofitcore/inc/RooDataSet.h b/roofit/roofitcore/inc/RooDataSet.h index ec3cabd370f9f..d8a346451ef55 100644 --- a/roofit/roofitcore/inc/RooDataSet.h +++ b/roofit/roofitcore/inc/RooDataSet.h @@ -166,9 +166,6 @@ class RooDataSet : public RooAbsData, public RooDirItem { // Cache copy feature is not publicly accessible std::unique_ptr reduceEng(const RooArgSet& varSubset, const RooFormulaVar* cutVar, const char* cutRange=nullptr, std::size_t nStart=0, std::size_t nStop = std::numeric_limits::max()) override; - RooDataSet(RooStringView name, RooStringView title, RooDataSet *ntuple, - const RooArgSet& vars, const RooFormulaVar* cutVar, const char* cutRange, - std::size_t nStart, std::size_t nStop); RooArgSet _varsNoWgt; ///< Vars without weight variable RooRealVar *_wgtVar = nullptr; ///< Pointer to weight variable (if set) diff --git a/roofit/roofitcore/src/RooDataSet.cxx b/roofit/roofitcore/src/RooDataSet.cxx index 7ba6a0db95d6d..638f98ca73b6d 100644 --- a/roofit/roofitcore/src/RooDataSet.cxx +++ b/roofit/roofitcore/src/RooDataSet.cxx @@ -741,23 +741,6 @@ RooDataSet::RooDataSet(RooDataSet const & other, const char* newname) : TRACE_CREATE; } -//////////////////////////////////////////////////////////////////////////////// -/// Protected constructor for internal use only - -RooDataSet::RooDataSet(RooStringView name, RooStringView title, RooDataSet *dset, - const RooArgSet& vars, const RooFormulaVar* cutVar, const char* cutRange, - std::size_t nStart, std::size_t nStop) : - RooAbsData(name,title,vars) -{ - _dstore = dset->_dstore->reduce(name, title, _vars, cutVar, cutRange, nStart, nStop); - - _cachedVars.add(_dstore->cachedVars()); - - appendToDir(this, true); - initialize(dset->_wgtVar ? dset->_wgtVar->GetName() : nullptr); - TRACE_CREATE; -} - //////////////////////////////////////////////////////////////////////////////// /// Return an empty clone of this dataset. If vars is not null, only the variables in vars @@ -832,37 +815,40 @@ void RooDataSet::initialize(const char* wgtVarName) //////////////////////////////////////////////////////////////////////////////// /// Implementation of RooAbsData virtual method that drives the RooAbsData::reduce() methods -std::unique_ptr RooDataSet::reduceEng(const RooArgSet& varSubset, const RooFormulaVar* cutVar, const char* cutRange, - std::size_t nStart, std::size_t nStop) +std::unique_ptr RooDataSet::reduceEng(const RooArgSet &varSubset, const RooFormulaVar *cutVar, + const char *cutRange, std::size_t nStart, std::size_t nStop) { - checkInit() ; - RooArgSet tmp(varSubset) ; - if (_wgtVar) { - tmp.add(*_wgtVar) ; - } + checkInit(); + RooArgSet tmp(varSubset); + if (_wgtVar) { + tmp.add(*_wgtVar); + } - std::unique_ptr out; + auto createEmptyClone = [&]() { return emptyClone(GetName(), GetTitle(), &tmp); }; - if (!cutRange || strchr(cutRange,',')==nullptr) { - out.reset(new RooDataSet(GetName(), GetTitle(), this, tmp, cutVar, cutRange, nStart, nStop)); - } else { - // Composite case: multiple ranges - auto tokens = ROOT::Split(cutRange, ","); - if (RooHelpers::checkIfRangesOverlap(tmp, tokens)) { - std::stringstream errMsg; - errMsg << "Error in RooAbsData::reduce! The ranges " << cutRange << " are overlapping!"; - throw std::runtime_error(errMsg.str()); - } - for (const auto& token : tokens) { - if(!out) { - out.reset(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); + std::unique_ptr out{createEmptyClone()}; + + if (!cutRange || strchr(cutRange, ',') == nullptr) { + auto &ds = static_cast(*out); + ds._dstore = _dstore->reduce(ds.GetName(), ds.GetTitle(), ds._vars, cutVar, cutRange, nStart, nStop); + ds._cachedVars.add(_dstore->cachedVars()); + } else { + // Composite case: multiple ranges + auto tokens = ROOT::Split(cutRange, ","); + if (RooHelpers::checkIfRangesOverlap(tmp, tokens)) { + std::stringstream errMsg; + errMsg << "Error in RooAbsData::reduce! The ranges " << cutRange << " are overlapping!"; + throw std::runtime_error(errMsg.str()); } - } - } - return out; + for (const auto &token : tokens) { + std::unique_ptr appendedData{createEmptyClone()}; + auto &ds = static_cast(*appendedData); + ds._dstore = _dstore->reduce(ds.GetName(), ds.GetTitle(), ds._vars, cutVar, token.c_str(), nStart, nStop); + ds._cachedVars.add(_dstore->cachedVars()); + static_cast(*out).append(ds); + } + } + return out; }