Skip to content

Commit

Permalink
[RF] Remove multi-range chi-square fit logic from RooAbsPdf
Browse files Browse the repository at this point in the history 
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 root-project#11455 (commit fa10523 in particular),
where the same change was already make for regular likelihoods.
  • Loading branch information
@guitargeek
guitargeek committed Sep 17, 2023
1 parent e1c1002 commit 9f7f1bd
Show file tree
Hide file tree
Showing 7 changed files with 71 additions and 181 deletions.
11 changes: 0 additions & 11 deletions roofit/roofitcore/inc/RooAbsPdf.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,21 +204,10 @@ class RooAbsPdf : public RooAbsReal {

// Chi^2 fits to histograms
using RooAbsReal::createChi2 ;
RooFit::OwningPtr<RooAbsReal> 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<RooAbsReal> 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
Expand Down
7 changes: 0 additions & 7 deletions roofit/roofitcore/inc/RooAbsReal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<RooAbsReal> createProfile(const RooArgSet& paramsOfInterest) ;


Expand Down
5 changes: 0 additions & 5 deletions roofit/roofitcore/inc/RooChi2Var.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
78 changes: 0 additions & 78 deletions roofit/roofitcore/src/RooAbsPdf.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1796,84 +1796,6 @@ RooFit::OwningPtr<RooFitResult> 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<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)
{
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<RooChi2Var>((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<RooAbsReal>{chi2});
}




////////////////////////////////////////////////////////////////////////////////
/// Argument-list version of RooAbsPdf::createChi2()

Expand Down
33 changes: 22 additions & 11 deletions roofit/roofitcore/src/RooAbsReal.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4200,17 +4200,20 @@ RooFit::OwningPtr<RooFitResult> 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<RooFitResult> RooAbsReal::chi2FitTo(RooDataHist& data, const RooLinkedList& cmdList)
RooFit::OwningPtr<RooFitResult> 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<RooAbsReal> 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<RooAbsReal> chi2{createChi2(data, chi2CmdList)};
return chi2FitDriver(*chi2, fitCmdList);
}


Expand All @@ -4232,10 +4235,18 @@ RooFit::OwningPtr<RooAbsReal> 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<RooChi2Var>(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<RooChi2Var>(name.c_str(), name.c_str(), *this, data, arg1, arg2,
arg3, arg4, arg5, arg6, arg7, arg8));
return RooFit::Detail::owningPtr(std::move(chi2));
}


Expand Down
66 changes: 2 additions & 64 deletions roofit/roofitcore/src/RooChi2Var.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,28 +67,21 @@ using namespace std;

namespace {
template<class ...Args>
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...);
cfg.interleave = RooFit::Interleave;
cfg.verbose = static_cast<bool>(RooCmdConfig::decodeIntOnTheFly("RooChi2Var::RooChi2Var","Verbose",0,1,args...));
cfg.cloneInputData = false;
cfg.integrateOverBinsPrecision = RooCmdConfig::decodeDoubleOnTheFly("RooChi2Var::RooChi2Var", "IntegrateBins", 0, -1., {args...});
return cfg;
}

template<class ...Args>
RooAbsTestStatistic::Configuration makeRooAbsTestStatisticCfgForPdf(Args const& ... args) {
auto cfg = makeRooAbsTestStatisticCfgForFunc(args...);
cfg.addCoefRangeName = RooCmdConfig::decodeStringOnTheFly("RooChi2Var::RooChi2Var","AddCoefRange",0,"",args...);
cfg.splitCutRange = static_cast<bool>(RooCmdConfig::decodeIntOnTheFly("RooChi2Var::RooChi2Var","SplitRange",0,0,args...));
return cfg;
}
}

ClassImp(RooChi2Var);
;

RooArgSet RooChi2Var::_emptySet ;

Expand Down Expand Up @@ -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) ;
Expand All @@ -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.
/// <table>
/// <tr><th> Argument <th> Effect
/// <tr><td>
/// Extended() <td> Include extended term in calculation
/// <tr><td>
/// DataError() <td> Choose between Poisson errors and Sum-of-weights errors
/// <tr><td>
/// NumCPU() <td> Activate parallel processing feature
/// <tr><td>
/// Range() <td> Fit only selected region
/// <tr><td>
/// SumCoefRange() <td> Set the range in which to interpret the coefficients of RooAddPdf components
/// <tr><td>
/// SplitRange() <td> Fit range is split by index category of simultaneous PDF
/// <tr><td>
/// ConditionalObservables() <td> Define projected observables
/// <tr><td>
/// Verbose() <td> Verbose output of GOF framework
/// <tr><td>
/// IntegrateBins() <td> 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

Expand Down
52 changes: 47 additions & 5 deletions roofit/roofitcore/test/testRooAbsPdf.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,11 @@ TEST_P(FitTest, MultiRangeFit)
nsig.setError(0.0);
};

// loop over non-extended and extended fit
for (auto *model : {static_cast<RooAbsPdf *>(&modelSimple), static_cast<RooAbsPdf *>(&modelExtended)}) {
std::unique_ptr<RooAbsData> dataSet{modelSimple.generate(x, nEvents)};
std::unique_ptr<RooAbsData> dataHist{static_cast<RooDataSet &>(*dataSet).binnedClone()};

std::unique_ptr<RooAbsData> dataSet{model->generate(x, nEvents)};
std::unique_ptr<RooAbsData> dataHist{static_cast<RooDataSet &>(*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()}) {
Expand All @@ -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<RooDataHist &>(*dataHist);

// loop over non-extended and extended fit
for (auto *model : {&modelSimple, &modelExtended}) {

// full range
resetValues();
std::unique_ptr<RooFitResult> 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<RooFitResult> 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)
Expand Down Expand Up @@ -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<RooFitResult> 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<RooFitResult> 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
Expand Down Expand Up @@ -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<FitTest::ParamType> const &paramInfo) {
std::stringstream ss;
ss << "BatchMode" << std::get<0>(paramInfo.param);
Expand Down

0 comments on commit 9f7f1bd

Please sign in to comment.