Skip to content

[WIP][RF] Remove all code related to constant term optimization #21831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
guitargeek wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[WIP][RF] Remove all code related to constant term optimization #21831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 58 additions & 73 deletions roofit/histfactory/test/testHistFactory.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,82 +590,67 @@ TEST_P(HFFixtureFit, Fit)
auto mc = dynamic_cast<RooStats::ModelConfig *>(ws->obj("ModelConfig"));
ASSERT_NE(mc, nullptr);

// This tests both correct pre-caching of constant terms and (if false) that all doEval() are correct.
for (bool constTermOptimization : {true, false}) {
std::unique_ptr<RooArgSet> pars(simPdf->getParameters(*data));
// Kick parameters:
for (auto par : *pars) {
auto real = dynamic_cast<RooAbsRealLValue *>(par);
if (real && !real->isConstant())
real->setVal(real->getVal() * 0.95);
}
if (makeModelMode == MakeModelMode::StatSyst) {
auto poi = dynamic_cast<RooRealVar *>(pars->find("SigXsecOverSM"));
ASSERT_NE(poi, nullptr);
poi->setVal(2.);
poi->setConstant();
}

// constTermOptimization makes only sense in the legacy backend
if (constTermOptimization && evalBackend != RooFit::EvalBackend::Legacy()) {
continue;
}
SCOPED_TRACE(constTermOptimization ? "const term optimisation" : "No const term optimisation");

// Stop if one of the previous runs had a failure to keep the terminal clean.
if (HasFailure())
break;

std::unique_ptr<RooArgSet> pars(simPdf->getParameters(*data));
// Kick parameters:
for (auto par : *pars) {
auto real = dynamic_cast<RooAbsRealLValue *>(par);
if (real && !real->isConstant())
real->setVal(real->getVal() * 0.95);
}
if (makeModelMode == MakeModelMode::StatSyst) {
auto poi = dynamic_cast<RooRealVar *>(pars->find("SigXsecOverSM"));
ASSERT_NE(poi, nullptr);
poi->setVal(2.);
poi->setConstant();
using namespace RooFit;
std::unique_ptr<RooFitResult> fitResult{simPdf->fitTo(
*data, evalBackend, GlobalObservables(*mc->GetGlobalObservables()), Save(), PrintLevel(verbose ? 1 : -1))};
ASSERT_NE(fitResult, nullptr);
if (verbose)
fitResult->Print("v");
EXPECT_EQ(fitResult->status(), 0);

auto checkParam = [&](const std::string &param, double target, double absPrecision = 1.e-2) {
auto par = dynamic_cast<RooRealVar *>(fitResult->floatParsFinal().find(param.c_str()));
if (!par) {
// Parameter was constant in this fit
par = dynamic_cast<RooRealVar *>(fitResult->constPars().find(param.c_str()));
ASSERT_NE(par, nullptr) << param;
EXPECT_DOUBLE_EQ(par->getVal(), target) << "Constant parameter " << param << " is off target.";
} else {
EXPECT_NEAR(par->getVal(), target, par->getError())
<< "Parameter " << param << " close to target " << target << " within uncertainty";
EXPECT_NEAR(par->getVal(), target, absPrecision) << "Parameter " << param << " close to target " << target;
}
};

using namespace RooFit;
std::unique_ptr<RooFitResult> fitResult{simPdf->fitTo(*data, evalBackend, Optimize(constTermOptimization),
GlobalObservables(*mc->GetGlobalObservables()), Save(),
PrintLevel(verbose ? 1 : -1))};
ASSERT_NE(fitResult, nullptr);
if (verbose)
fitResult->Print("v");
EXPECT_EQ(fitResult->status(), 0);

auto checkParam = [&](const std::string &param, double target, double absPrecision = 1.e-2) {
auto par = dynamic_cast<RooRealVar *>(fitResult->floatParsFinal().find(param.c_str()));
if (!par) {
// Parameter was constant in this fit
par = dynamic_cast<RooRealVar *>(fitResult->constPars().find(param.c_str()));
ASSERT_NE(par, nullptr) << param;
EXPECT_DOUBLE_EQ(par->getVal(), target) << "Constant parameter " << param << " is off target.";
} else {
EXPECT_NEAR(par->getVal(), target, par->getError())
<< "Parameter " << param << " close to target " << target << " within uncertainty";
EXPECT_NEAR(par->getVal(), target, absPrecision) << "Parameter " << param << " close to target " << target;
}
};

if (makeModelMode == MakeModelMode::OverallSyst) {
// Model is set up such that background scale factors should be close to 1, and signal == 2
checkParam("SigXsecOverSM", 2.);
checkParam("alpha_syst2", 0.);
checkParam("alpha_syst3", 0.);
checkParam("alpha_syst4", 0.);
checkParam("gamma_stat_channel1_bin_0", 1.);
checkParam("gamma_stat_channel1_bin_1", 1.);
} else if (makeModelMode == MakeModelMode::HistoSyst) {
// Model is set up with a -1 sigma pull on the signal shape parameter.
checkParam("SigXsecOverSM", 2., 1.1E-1); // Higher tolerance: Expect a pull due to shape syst.
checkParam("gamma_stat_channel1_bin_0", 1.);
checkParam("gamma_stat_channel1_bin_1", 1.);
checkParam("alpha_SignalShape", -0.9, 5.E-2); // Pull slightly lower than 1 because of constraint term
} else if (makeModelMode == MakeModelMode::StatSyst) {
// Model is set up with a -1 sigma pull on the signal shape parameter.
checkParam("SigXsecOverSM", 2., 1.1E-1); // Higher tolerance: Expect a pull due to shape syst.
checkParam("gamma_stat_channel1_bin_0", 1.09); // This should be pulled
checkParam("gamma_stat_channel1_bin_1", 1.);
} else if (makeModelMode == MakeModelMode::ShapeSyst) {
// This should be pulled down
checkParam("gamma_background1Shape_bin_0", 0.8866, 0.03);
// This should be pulled up, but not so much because the free signal
// strength will fit the excess in this bin.
checkParam("gamma_background2Shape_bin_1", 1.0250, 0.03);
}
if (makeModelMode == MakeModelMode::OverallSyst) {
// Model is set up such that background scale factors should be close to 1, and signal == 2
checkParam("SigXsecOverSM", 2.);
checkParam("alpha_syst2", 0.);
checkParam("alpha_syst3", 0.);
checkParam("alpha_syst4", 0.);
checkParam("gamma_stat_channel1_bin_0", 1.);
checkParam("gamma_stat_channel1_bin_1", 1.);
} else if (makeModelMode == MakeModelMode::HistoSyst) {
// Model is set up with a -1 sigma pull on the signal shape parameter.
checkParam("SigXsecOverSM", 2., 1.1E-1); // Higher tolerance: Expect a pull due to shape syst.
checkParam("gamma_stat_channel1_bin_0", 1.);
checkParam("gamma_stat_channel1_bin_1", 1.);
checkParam("alpha_SignalShape", -0.9, 5.E-2); // Pull slightly lower than 1 because of constraint term
} else if (makeModelMode == MakeModelMode::StatSyst) {
// Model is set up with a -1 sigma pull on the signal shape parameter.
checkParam("SigXsecOverSM", 2., 1.1E-1); // Higher tolerance: Expect a pull due to shape syst.
checkParam("gamma_stat_channel1_bin_0", 1.09); // This should be pulled
checkParam("gamma_stat_channel1_bin_1", 1.);
} else if (makeModelMode == MakeModelMode::ShapeSyst) {
// This should be pulled down
checkParam("gamma_background1Shape_bin_0", 0.8866, 0.03);
// This should be pulled up, but not so much because the free signal
// strength will fit the excess in this bin.
checkParam("gamma_background2Shape_bin_1", 1.0250, 0.03);
}

if (false) {
Expand Down
2 changes: 1 addition & 1 deletion roofit/roofit/test/vectorisedPDFs/VectorisedPDFTests.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ std::unique_ptr<RooFitResult> PDFTest::runBatchFit(RooAbsPdf *pdf)

MyTimer batchTimer("Fitting batch mode " + _name);
std::unique_ptr<RooFitResult> result{pdf->fitTo(*_dataFit, RooFit::EvalBackend::Cpu(), RooFit::SumW2Error(false),
RooFit::Optimize(1), RooFit::PrintLevel(_printLevel), RooFit::Save(),
RooFit::PrintLevel(_printLevel), RooFit::Save(),
_multiProcess > 0 ? RooFit::NumCPU(_multiProcess) : RooCmdArg())};
std::cout << batchTimer;
EXPECT_NE(result, nullptr);
Expand Down
2 changes: 1 addition & 1 deletion roofit/roofit/test/vectorisedPDFs/testGaussBinned.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ TEST_P(GaussBinnedFit, BatchFit)
m.setVal(-1.);
s.setVal(3.);
MyTimer timer(evalBackend == EvalBackend::Value::Cpu ? "BatchBinned" : "ScalarBinned");
gaus.fitTo(*dataHist, EvalBackend(evalBackend), PrintLevel(-1), Optimize(0));
gaus.fitTo(*dataHist, EvalBackend(evalBackend), PrintLevel(-1));
timer.interval();
std::cout << timer << std::endl;
EXPECT_NEAR(m.getVal(), 1., m.getError());
Expand Down
1 change: 0 additions & 1 deletion roofit/roofitcore/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,6 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooFitCore
src/RooVectorDataStore.cxx
src/RooWorkspace.cxx
src/RooWrapperPdf.cxx
src/TestStatistics/ConstantTermsOptimizer.cxx
src/TestStatistics/LikelihoodGradientWrapper.cxx
src/TestStatistics/LikelihoodSerial.cxx
src/TestStatistics/LikelihoodWrapper.cxx
Expand Down
3 changes: 0 additions & 3 deletions roofit/roofitcore/inc/RooAbsArg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,9 +340,6 @@ class RooAbsArg : public TNamed, public RooPrintable {
bool findConstantNodes(const RooArgSet &observables, RooArgSet &cacheList);
bool findConstantNodes(const RooArgSet &observables, RooArgSet &cacheList, RooLinkedList &processedNodes);

// constant term optimization
virtual void constOptimizeTestStatistic(ConstOpCode opcode, bool doAlsoTrackingOpt = true);

virtual CacheMode canNodeBeCached() const { return Always; }
virtual void setCacheAndTrackHints(RooArgSet & /*trackNodes*/) {};

Expand Down
8 changes: 0 additions & 8 deletions roofit/roofitcore/inc/RooAbsData.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,9 +304,6 @@ class RooAbsData : public TNamed, public RooPrintable {
double corrcov(const RooRealVar& x, const RooRealVar& y, const char* cutSpec, const char* cutRange, bool corr) const ;
RooFit::OwningPtr<TMatrixDSym> corrcovMatrix(const RooArgList& vars, const char* cutSpec, const char* cutRange, bool corr) const ;

virtual void optimizeReadingWithCaching(RooAbsArg& arg, const RooArgSet& cacheList, const RooArgSet& keepObsList) ;
bool allClientsCached(RooAbsArg*, const RooArgSet&) ;

struct PlotOpt {
const char* cuts = "";
Option_t* drawOptions = "P";
Expand Down Expand Up @@ -341,11 +338,6 @@ class RooAbsData : public TNamed, public RooPrintable {
// for access into copied dataset:
friend class RooFit::TestStatistics::RooAbsL;

virtual void cacheArgs(const RooAbsArg* owner, RooArgSet& varSet, const RooArgSet* nset=nullptr, bool skipZeroWeights=false) ;
virtual void resetCache() ;
virtual void setArgStatus(const RooArgSet& set, bool active) ;
virtual void attachCache(const RooAbsArg* newOwner, const RooArgSet& cachedVars) ;

virtual std::unique_ptr<RooAbsData> reduceEng(const RooArgSet& varSubset, const RooFormulaVar* cutVar, const char* cutRange=nullptr,
std::size_t nStart = 0, std::size_t = std::numeric_limits<std::size_t>::max()) const = 0 ;

Expand Down
11 changes: 0 additions & 11 deletions roofit/roofitcore/inc/RooAbsDataStore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,6 @@ class RooAbsDataStore : public TNamed, public RooPrintable {
int defaultPrintContents(Option_t* /*opt*/) const override { return kName|kClassName|kArgs|kValue ; }


// Constant term optimizer interface
virtual void cacheArgs(const RooAbsArg* cacheOwner, RooArgSet& varSet, const RooArgSet* nset=nullptr, bool skipZeroWeights=false) = 0 ;
virtual const RooAbsArg* cacheOwner() = 0 ;
virtual void attachCache(const RooAbsArg* newOwner, const RooArgSet& cachedVars) = 0 ;
virtual void setArgStatus(const RooArgSet& set, bool active) = 0 ;
const RooArgSet& cachedVars() const { return _cachedVars ; }
virtual void resetCache() = 0 ;
virtual void recalculateCache(const RooArgSet* /*proj*/, Int_t /*firstEvent*/, Int_t /*lastEvent*/, Int_t /*stepSize*/, bool /* skipZeroWeights*/) {} ;

virtual void setDirtyProp(bool flag) { _doDirtyProp = flag ; }
bool dirtyProp() const { return _doDirtyProp ; }

Expand All @@ -142,8 +133,6 @@ class RooAbsDataStore : public TNamed, public RooPrintable {
virtual void loadValues(const RooAbsDataStore *tds, const RooFormulaVar* select=nullptr, const char* rangeName=nullptr,
std::size_t nStart=0, std::size_t nStop = std::numeric_limits<std::size_t>::max()) = 0 ;

virtual void forceCacheUpdate() {} ;

protected:

RooArgSet _vars;
Expand Down
13 changes: 0 additions & 13 deletions roofit/roofitcore/inc/RooCompositeDataStore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,20 +89,9 @@ class RooCompositeDataStore : public RooAbsDataStore {
void attachBuffers(const RooArgSet& extObs) override ;
void resetBuffers() override ;

// Constant term optimizer interface
void cacheArgs(const RooAbsArg* owner, RooArgSet& varSet, const RooArgSet* nset=nullptr, bool skipZeroWeights=false) override ;
const RooAbsArg* cacheOwner() override { return nullptr ; }
void setArgStatus(const RooArgSet& set, bool active) override ;
void resetCache() override ;

void recalculateCache(const RooArgSet* /*proj*/, Int_t /*firstEvent*/, Int_t /*lastEvent*/, Int_t /*stepSize*/, bool /*skipZeroWeights*/) override ;
bool hasFilledCache() const override ;

void loadValues(const RooAbsDataStore *tds, const RooFormulaVar* select=nullptr, const char* rangeName=nullptr,
std::size_t nStart=0, std::size_t nStop = std::numeric_limits<std::size_t>::max()) override;

void forceCacheUpdate() override ;

RooAbsData::RealSpans getBatches(std::size_t first, std::size_t len) const override {
//TODO
std::cerr << "This functionality is not yet implemented for composite data stores." << std::endl;
Expand All @@ -115,8 +104,6 @@ class RooCompositeDataStore : public RooAbsDataStore {

protected:

void attachCache(const RooAbsArg* newOwner, const RooArgSet& cachedVars) override ;

std::map<Int_t,RooAbsDataStore*> _dataMap ;
RooCategory* _indexCat = nullptr;
mutable RooAbsDataStore* _curStore = nullptr; ///<! Datastore associated with current event
Expand Down
3 changes: 0 additions & 3 deletions roofit/roofitcore/inc/RooEvaluatorWrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,6 @@ class RooEvaluatorWrapper final : public RooAbsReal {
_evaluator->print(os);
}

/// The RooFit::Evaluator is dealing with constant terms itself.
void constOptimizeTestStatistic(ConstOpCode /*opcode*/, bool /*doAlsoTrackingOpt*/) override {}

bool hasGradient() const override;
bool hasHessian() const override;

Expand Down
1 change: 0 additions & 1 deletion roofit/roofitcore/inc/RooFit/TestStatistics/LikelihoodWrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ class LikelihoodWrapper {
virtual void updateMinuitExternalParameterValues(const std::vector<double> &minuit_external_x);

// The following functions are necessary from MinuitFcnGrad to reach likelihood properties:
void constOptimizeTestStatistic(RooAbsArg::ConstOpCode opcode, bool doAlsoTrackingOpt);
double defaultErrorLevel() const;
virtual std::string GetName() const;
virtual std::string GetTitle() const;
Expand Down
6 changes: 0 additions & 6 deletions roofit/roofitcore/inc/RooFit/TestStatistics/RooAbsL.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,6 @@ class RooAbsL {
// necessary from MinuitFcnGrad to reach likelihood properties:
virtual std::unique_ptr<RooArgSet> getParameters();

/// \brief Interface function signaling a request to perform constant term optimization.
///
/// The default implementation takes no action other than to forward the calls to all servers. May be overridden in
/// likelihood classes without a cached dataset, like RooSubsidiaryL.
virtual void constOptimizeTestStatistic(RooAbsArg::ConstOpCode opcode, bool doAlsoTrackingOpt);

virtual std::string GetName() const;
virtual std::string GetTitle() const;
virtual std::string GetInfo() const { return GetClassName() + "::" + pdf_->GetName(); }
Expand Down
2 changes: 0 additions & 2 deletions roofit/roofitcore/inc/RooFit/TestStatistics/RooSubsidiaryL.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ class RooSubsidiaryL : public RooAbsL {
return 0;
}

void constOptimizeTestStatistic(RooAbsArg::ConstOpCode opcode, bool doAlsoTrackingOpt) override;

private:
std::string parent_pdf_name_;
RooArgList subsidiary_pdfs_{"subsidiary_pdfs"}; ///< Set of subsidiary PDF or "constraint" terms
Expand Down
2 changes: 0 additions & 2 deletions roofit/roofitcore/inc/RooFit/TestStatistics/RooSumL.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ class RooSumL : public RooAbsL {
// necessary only for legacy offsetting mode in LikelihoodWrapper; TODO: remove this if legacy mode is ever removed
ROOT::Math::KahanSum<double> getSubsidiaryValue();

void constOptimizeTestStatistic(RooAbsArg::ConstOpCode opcode, bool doAlsoTrackingOpt) override;

std::string GetClassName() const override { return "RooSumL"; }

const std::vector<std::unique_ptr<RooAbsL>> &GetComponents() const { return components_; };
Expand Down
5 changes: 0 additions & 5 deletions roofit/roofitcore/inc/RooGlobalFunc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,11 +253,6 @@ RooCmdArg IntegrateBins(double precision);

// RooAbsPdf::fitTo arguments
RooCmdArg PrefitDataFraction(double data_ratio = 0.0) ;
RooCmdArg Optimize(Int_t flag=2)
#ifndef ROOFIT_BUILDS_ITSELF
R__DEPRECATED(6, 42, "Please use the default \"cpu\" likelihood evaluation backend if you want all optimizations.")
#endif
;

class EvalBackend : public RooCmdArg {
public:
Expand Down
5 changes: 0 additions & 5 deletions roofit/roofitcore/inc/RooMinimizer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,6 @@ class RooMinimizer : public TObject {
void setPrintLevel(int newLevel);

// Setters on _fcn
void optimizeConst(int flag)
#ifndef ROOFIT_BUILDS_ITSELF
R__DEPRECATED(6, 42, "Please use the default \"cpu\" likelihood evaluation backend if you want all optimizations.")
#endif
;
void setEvalErrorWall(bool flag) { _cfg.doEEWall = flag; }
void setRecoverFromNaNStrength(double strength);
void setOffsetting(bool flag);
Expand Down
9 changes: 0 additions & 9 deletions roofit/roofitcore/inc/RooTreeDataStore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,6 @@ class RooTreeDataStore : public RooAbsDataStore {
TTree* tree() { return _tree ; }
TTree const *tree() const { return _tree ; }

// Constant term optimizer interface
void cacheArgs(const RooAbsArg* owner, RooArgSet& varSet, const RooArgSet* nset=nullptr, bool skipZeroWeights=false) override ;
const RooAbsArg* cacheOwner() override { return _cacheOwner ; }
void setArgStatus(const RooArgSet& set, bool active) override ;
void resetCache() override ;

void loadValues(const TTree *t, const RooFormulaVar* select=nullptr, const char* rangeName=nullptr, Int_t nStart=0, Int_t nStop=2000000000) ;
void loadValues(const RooAbsDataStore *tds, const RooFormulaVar* select=nullptr, const char* rangeName=nullptr,
std::size_t nStart=0, std::size_t nStop = std::numeric_limits<std::size_t>::max()) override;
Expand All @@ -138,7 +132,6 @@ class RooTreeDataStore : public RooAbsDataStore {
RooRealVar* weightVar(const RooArgSet& allVars, const char* wgtName=nullptr) ;

void initialize();
void attachCache(const RooAbsArg* newOwner, const RooArgSet& cachedVars) override ;

// TTree Branch buffer size control
void setBranchBufferSize(Int_t size) { _defTreeBufSize = size ; }
Expand All @@ -150,8 +143,6 @@ class RooTreeDataStore : public RooAbsDataStore {

void createTree(RooStringView name, RooStringView title) ;
TTree *_tree = nullptr; // TTree holding the data points
TTree *_cacheTree = nullptr; ///<! TTree holding the cached function values
const RooAbsArg* _cacheOwner = nullptr; ///<! Object owning cache contents
mutable bool _defCtor = false; ///<! Was object constructed with default ctor?

RooArgSet _varsww ;
Expand Down
Loading
Loading