Skip to content
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

Replace deprecated legacy iterators #874

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Replace deprecated legacy iterators #874

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
3 changes: 0 additions & 3 deletions interface/AtlasPdfs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,9 +372,6 @@ class RooStarMomentMorph : public RooAbsPdf {
mutable std::vector<int> _nnuis;
mutable std::vector<double> _nref;

TIterator* _parItr ; //! do not persist
TIterator* _obsItr ; //! do not persist
TIterator* _pdfItr ; //!
mutable TMatrixD* _M; //!

Setting _setting;
Expand Down
5 changes: 1 addition & 4 deletions interface/FastTemplateFunc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,12 @@ template <typename T> class FastTemplateFunc_t : public RooAbsReal{
virtual inline ~FastTemplateFunc_t(){}

void setProxyList(RooListProxy& proxyList, RooArgList& varList){
TIterator* varIter = varList.createIterator();
RooAbsArg* var;
while ((var = (RooAbsArg*)varIter->Next())) {
for (RooAbsArg *var : varList) {
if (!dynamic_cast<RooAbsReal*>(var)) {
assert(0);
}
proxyList.add(*var);
}
delete varIter;
}

virtual TObject* clone(const char* newname) const = 0;
Expand Down
1 change: 0 additions & 1 deletion interface/HZZ4L_RooSpinZeroPdf_1D.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ class HZZ4L_RooSpinZeroPdf_1D : public RooAbsPdf {
// RooRealProxy ksmd ;
RooRealProxy fai ;
RooListProxy _coefList ; // List of funcficients
TIterator* _coefIter ; //! Iterator over funcficient lis
Double_t evaluate() const ;
public:
HZZ4L_RooSpinZeroPdf_1D() {} ;
Expand Down
1 change: 0 additions & 1 deletion interface/ProcessNormalization.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#ifndef HiggsAnalysis_CombinedLimit_ProcessNormalization_h
#define HiggsAnalysis_CombinedLimit_ProcessNormalization_h

#include <TIterator.h>
#include <RooAbsReal.h>
#include "RooListProxy.h"

Expand Down
1 change: 0 additions & 1 deletion interface/RooMultiPdf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include "RooConstVar.h"


#include "TIterator.h"
#include "RooListProxy.h"

#include <iostream>
Expand Down
1 change: 0 additions & 1 deletion interface/SimpleCacheSentry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

#include "RooRealVar.h"
#include "RooSetProxy.h"
#include "TIterator.h"

class SimpleCacheSentry : public RooAbsArg {
public:
Expand Down
4 changes: 0 additions & 4 deletions interface/VerticalInterpHistPdf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ class VerticalInterpHistPdf : public RooAbsPdf {
RooListProxy _coefList ; // List of coefficients
Double_t _smoothRegion;
Int_t _smoothAlgo;
TIterator* _funcIter ; //! Iterator over FUNC list
TIterator* _coefIter ; //! Iterator over coefficient list

// TH1 containing the histogram of this pdf
mutable SimpleCacheSentry _sentry; // !not to be serialized
Expand Down Expand Up @@ -93,8 +91,6 @@ class FastVerticalInterpHistPdfBase : public RooAbsPdf {
RooListProxy _coefList ; // List of coefficients
Double_t _smoothRegion;
Int_t _smoothAlgo;
TIterator* _funcIter ; //! Iterator over FUNC list
TIterator* _coefIter ; //! Iterator over coefficient list

// TH1 containing the histogram of this pdf
mutable bool _init; //! not to be serialized
Expand Down
2 changes: 0 additions & 2 deletions interface/VerticalInterpPdf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ class VerticalInterpPdf : public RooAbsPdf {
RooListProxy _coefList ; // List of coefficients
Double_t _quadraticRegion;
Int_t _quadraticAlgo;
TIterator* _funcIter ; //! Iterator over FUNC list
TIterator* _coefIter ; //! Iterator over coefficient list

Double_t _pdfFloorVal; // PDF floor should be customizable, default is 1e-15
Double_t _integralFloorVal; // PDF integral floor should be customizable, default is 1e-10
Expand Down
22 changes: 8 additions & 14 deletions src/AsimovUtils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

#include <memory>
#include <stdexcept>
#include <TIterator.h>
#include <RooAbsData.h>
#include <RooArgSet.h>
#include <RooProdPdf.h>
Expand All @@ -21,8 +20,7 @@ RooAbsData *asimovutils::asimovDatasetNominal(RooStats::ModelConfig *mc, double

if (verbose>2) {
CombineLogger::instance().log("AsimovUtils.cc",__LINE__,"Parameters after fit for asimov dataset",__func__);
std::unique_ptr<TIterator> iter(mc->GetPdf()->getParameters((const RooArgSet*) 0)->createIterator());
for (RooAbsArg *a = (RooAbsArg *) iter->Next(); a != 0; a = (RooAbsArg *) iter->Next()) {
for (RooAbsArg *a : *std::unique_ptr<RooArgSet>{mc->GetPdf()->getParameters((const RooArgSet*) 0)}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that a memory leak is fixed here by wrapping the return value of getParameters in a smart pointer (getParameters returns a pointer that needs to be deleted)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change has introduced a seg fault when running with -v 3, try for example

combine data/tutorials/CAT23001/parametric-analysis-datacard.txt -m 125 -v 3

Any thoughts @guitargeek ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be that some compilers don't like that this std::unique_ptr<RooArgSet> objects lifetime is only defined by the scope of the loop.

What happens if you give this a name?

std::unique_ptr<RooArgSet> paramsTmp{mc->GetPdf()->getParameters((const RooArgSet*) 0)};
for (RooAbsArg *a : *paramsTmp) {
}

Copy link
Contributor Author

@guitargeek guitargeek Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively you can try the getParameters overload with an output parameter, which doesn't do a heap allocation so in principle it's even better:

RooArgSet paramsTmp;
mc->GetPdf()->getParameters(nullptr, paramsTmp);
for (RooAbsArg *a : paramsTmp) {
}

Let me know if these patterns avoid the segfault

TString varstring = utils::printRooArgAsString(a);
CombineLogger::instance().log("AsimovUtils.cc",__LINE__,std::string(Form("%s",varstring.Data())),__func__);
}
Expand All @@ -45,8 +43,7 @@ RooAbsData *asimovutils::asimovDatasetWithFit(RooStats::ModelConfig *mc, RooAbsD
} else {
// Do we have free parameters anyway that need fitting?
std::unique_ptr<RooArgSet> params(mc->GetPdf()->getParameters(realdata));
std::unique_ptr<TIterator> iter(params->createIterator());
for (RooAbsArg *a = (RooAbsArg *) iter->Next(); a != 0; a = (RooAbsArg *) iter->Next()) {
for (RooAbsArg *a : *params) {
RooRealVar *rrv = dynamic_cast<RooRealVar *>(a);
if ( rrv != 0 && rrv->isConstant() == false ) { needsFit &= true; break; }
}
Expand All @@ -67,8 +64,7 @@ RooAbsData *asimovutils::asimovDatasetWithFit(RooStats::ModelConfig *mc, RooAbsD

if (verbose>2) {
CombineLogger::instance().log("AsimovUtils.cc",__LINE__,"Parameters after fit for asimov dataset",__func__);
std::unique_ptr<TIterator> iter(mc->GetPdf()->getParameters(realdata)->createIterator());
for (RooAbsArg *a = (RooAbsArg *) iter->Next(); a != 0; a = (RooAbsArg *) iter->Next()) {
for (RooAbsArg *a : *std::unique_ptr<RooArgSet>{mc->GetPdf()->getParameters(realdata)}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nucleosynthesis If it turns out that these inlined getParameters() are problematic, this needs to be changed too.

TString varstring = utils::printRooArgAsString(a);
CombineLogger::instance().log("AsimovUtils.cc",__LINE__,std::string(Form("%s",varstring.Data())),__func__);
}
Expand All @@ -92,8 +88,7 @@ RooAbsData *asimovutils::asimovDatasetWithFit(RooStats::ModelConfig *mc, RooAbsD
std::unique_ptr<RooAbsPdf> nuispdf(utils::makeNuisancePdf(*mc));
RooProdPdf *prod = dynamic_cast<RooProdPdf *>(nuispdf.get());
if (prod == 0) throw std::runtime_error("AsimovUtils: the nuisance pdf is not a RooProdPdf!");
std::unique_ptr<TIterator> iter(prod->pdfList().createIterator());
for (RooAbsArg *a = (RooAbsArg *) iter->Next(); a != 0; a = (RooAbsArg *) iter->Next()) {
for (RooAbsArg *a : prod->pdfList()) {
RooAbsPdf *cterm = dynamic_cast<RooAbsPdf *>(a);
if (!cterm) throw std::logic_error("AsimovUtils: a factor of the nuisance pdf is not a Pdf!");
if (!cterm->dependsOn(nuis)) continue; // dummy constraints
Expand All @@ -109,8 +104,7 @@ RooAbsData *asimovutils::asimovDatasetWithFit(RooStats::ModelConfig *mc, RooAbsD
if (cpars->getSize() == 1) {
match = dynamic_cast<RooAbsReal *>(cpars->first());
} else {
std::unique_ptr<TIterator> iter2(cpars->createIterator());
for (RooAbsArg *a2 = (RooAbsArg *) iter2->Next(); a2 != 0; a2 = (RooAbsArg *) iter2->Next()) {
for (RooAbsArg *a2 : *cpars) {
RooRealVar *rrv2 = dynamic_cast<RooRealVar *>(a2);
if (rrv2 != 0 && !rrv2->isConstant()) {
if (match != 0) throw std::runtime_error(Form("AsimovUtils: constraint term %s has multiple floating params", cterm->GetName()));
Expand Down Expand Up @@ -138,10 +132,10 @@ RooAbsData *asimovutils::asimovDatasetWithFit(RooStats::ModelConfig *mc, RooAbsD
// we want to set the global obs to a value for which the current value
// of the nuisance is the best fit one.
// best fit x = (k-1)*theta ----> k = x/theta + 1
RooArgList leaves; cterm->leafNodeServerList(&leaves);
std::unique_ptr<TIterator> iter2(leaves.createIterator());
RooArgList leaves;
cterm->leafNodeServerList(&leaves);
RooAbsReal *match2 = 0;
for (RooAbsArg *a2 = (RooAbsArg *) iter2->Next(); a2 != 0; a2 = (RooAbsArg *) iter2->Next()) {
for (RooAbsArg *a2 : leaves) {
RooAbsReal *rar = dynamic_cast<RooAbsReal *>(a2);
if (rar == 0 || rar == match || rar == &rrv) continue;
if (!rar->isConstant()) throw std::runtime_error(Form("AsimovUtils: extra floating parameter %s of RooGamma %s.", rar->GetName(), cterm->GetName()));
Expand Down
6 changes: 2 additions & 4 deletions src/AsymptoticLimits.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,7 @@ bool AsymptoticLimits::run(RooWorkspace *w, RooStats::ModelConfig *mc_s, RooStat
*/
hasDiscreteParams_ = false;
if (params_.get() == 0) params_.reset(mc_s->GetPdf()->getParameters(data));
std::unique_ptr<TIterator> itparam(params_->createIterator());
for (RooAbsArg *a = (RooAbsArg *) itparam->Next(); a != 0; a = (RooAbsArg *) itparam->Next()) {
for (RooAbsArg *a : *params_) {
if (a->IsA()->InheritsFrom(RooCategory::Class())) { hasDiscreteParams_ = true; break; }
}

Expand Down Expand Up @@ -164,8 +163,7 @@ bool AsymptoticLimits::runLimit(RooWorkspace *w, RooStats::ModelConfig *mc_s, Ro
if (params_.get() == 0) params_.reset(mc_s->GetPdf()->getParameters(data));

hasFloatParams_ = false;
std::unique_ptr<TIterator> itparam(params_->createIterator());
for (RooAbsArg *a = (RooAbsArg *) itparam->Next(); a != 0; a = (RooAbsArg *) itparam->Next()) {
for (RooAbsArg *a : *params_) {
RooRealVar *rrv = dynamic_cast<RooRealVar *>(a);
if ( rrv != 0 && rrv != r && rrv->isConstant() == false ) { hasFloatParams_ = true; break; }
}
Expand Down
66 changes: 10 additions & 56 deletions src/AtlasPdfs.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,10 +433,8 @@ RooBSpline::RooBSpline(const char* name, const char* title,

//bool even = fabs(_n/2-_n/2.) < 0.0000000001;
bool first=1;
TIterator* pointIter = controlPoints.createIterator() ;
RooAbsArg* point ;
RooAbsArg* lastPoint=NULL ;
while((point = (RooAbsArg*)pointIter->Next())) {
for (RooAbsArg * point : controlPoints) {
if (!dynamic_cast<RooAbsReal*>(point)) {
coutE(InputArguments) << "RooBSpline::ctor(" << GetName() << ") ERROR: control point " << point->GetName()
<< " is not of type RooAbsReal" << std::endl ;
Expand All @@ -456,19 +454,9 @@ RooBSpline::RooBSpline(const char* name, const char* title,
lastPoint=point;
}
for (int i=0;i<(_n)/2;i++) _controlPoints.add(*lastPoint);
delete pointIter ;


TIterator* varItr = vars.createIterator();
RooAbsArg* arg;
while ((arg=(RooAbsArg*)varItr->Next())) {
//std::cout << "======== Adding "<<arg->GetName()<<" to list of _vars of "<<name<<"." << std::endl;
_vars.add(*arg);
}
// std::cout << "all _vars: " << std::endl;
// _vars.Print("V");
delete varItr;
//std::cout << "out Ctor" << std::endl;
_vars.add(vars);
}

//_____________________________________________________________________________
Expand Down Expand Up @@ -556,10 +544,8 @@ void RooBSpline::setWeights(const RooArgList& weights)
{
_weights.removeAll();
bool first=1;
TIterator* pointIter = weights.createIterator() ;
RooAbsArg* point ;
RooAbsArg* lastPoint=NULL ;
while((point = (RooAbsArg*)pointIter->Next())) {
for (RooAbsArg *point : weights) {
if (!dynamic_cast<RooAbsReal*>(point)) {
coutE(InputArguments) << "RooBSpline::ctor(" << GetName() << ") ERROR: control point " << point->GetName()
<< " is not of type RooAbsReal" << std::endl ;
Expand All @@ -577,7 +563,6 @@ void RooBSpline::setWeights(const RooArgList& weights)
lastPoint=point;
}
for (int i=0;i<(_n+1)/2;i++) _weights.add(*lastPoint);
delete pointIter;
}


Expand Down Expand Up @@ -1235,9 +1220,6 @@ RooStarMomentMorph::RooStarMomentMorph() :
{

// coverity[UNINIT_CTOR]
_parItr = _parList.createIterator() ;
_obsItr = _obsList.createIterator() ;
_pdfItr = _pdfList.createIterator() ;
}


Expand Down Expand Up @@ -1269,45 +1251,31 @@ RooStarMomentMorph::RooStarMomentMorph(const char *name, const char *title,
// CTOR

// fit parameters
TIterator* parItr = parList.createIterator() ;
RooAbsArg* par ;
for (Int_t i=0; (par = (RooAbsArg*)parItr->Next()); ++i) {
for (RooAbsArg *par : parList) {
if (!dynamic_cast<RooAbsReal*>(par)) {
coutE(InputArguments) << "RooStarMomentMorph::ctor(" << GetName() << ") ERROR: parameter " << par->GetName() << " is not of type RooAbsReal" << endl ;
throw std::string("RooStarMomentMorh::ctor() ERROR parameter is not of type RooAbsReal") ;
}
_parList.add(*par) ;
}
delete parItr ;

// observables
TIterator* obsItr = obsList.createIterator() ;
RooAbsArg* var ;
for (Int_t i=0; (var = (RooAbsArg*)obsItr->Next()); ++i) {
for (RooAbsArg *var : obsList) {
if (!dynamic_cast<RooAbsReal*>(var)) {
coutE(InputArguments) << "RooStarMomentMorph::ctor(" << GetName() << ") ERROR: variable " << var->GetName() << " is not of type RooAbsReal" << endl ;
throw std::string("RooStarMomentMorh::ctor() ERROR variable is not of type RooAbsReal") ;
}
_obsList.add(*var) ;
}
delete obsItr ;

// reference p.d.f.s
TIterator* pdfItr = pdfList.createIterator() ;
RooAbsPdf* pdf ;
for (Int_t i=0; (pdf = dynamic_cast<RooAbsPdf*>(pdfItr->Next())); ++i) {
if (!pdf) {
for (RooAbsArg *pdf : pdfList) {
if (!dynamic_cast<RooAbsPdf*>(pdf)) {
coutE(InputArguments) << "RooStarMomentMorph::ctor(" << GetName() << ") ERROR: pdf " << pdf->GetName() << " is not of type RooAbsPdf" << endl ;
throw std::string("RooStarMomentMorph::ctor() ERROR pdf is not of type RooAbsPdf") ;
}
_pdfList.add(*pdf) ;
}
delete pdfItr ;

_parItr = _parList.createIterator() ;
_obsItr = _obsList.createIterator() ;
_pdfItr = _pdfList.createIterator() ;


// initialization
initialize();
Expand All @@ -1329,12 +1297,6 @@ RooStarMomentMorph::RooStarMomentMorph(const RooStarMomentMorph& other, const ch
_nnuisvar(other._nnuisvar),
_useHorizMorph(other._useHorizMorph)
{


_parItr = _parList.createIterator() ;
_obsItr = _obsList.createIterator() ;
_pdfItr = _pdfList.createIterator() ;

// nref is resized in initialize, so reduce the size here
if (_nref.size()>0) {
_nref.resize( _nref.size()-1 );
Expand All @@ -1347,9 +1309,6 @@ RooStarMomentMorph::RooStarMomentMorph(const RooStarMomentMorph& other, const ch
//_____________________________________________________________________________
RooStarMomentMorph::~RooStarMomentMorph()
{
if (_parItr) delete _parItr;
if (_obsItr) delete _obsItr;
if (_pdfItr) delete _pdfItr;
if (_M) delete _M;
}

Expand Down Expand Up @@ -1512,15 +1471,11 @@ RooStarMomentMorph::CacheElem* RooStarMomentMorph::getCache(const RooArgSet* /*n
//}

// construction of unit pdfs
_pdfItr->Reset();
RooAbsPdf* pdf;
RooArgList transPdfList;

for (Int_t i=0; i<nPdf; ++i) {
_obsItr->Reset() ;
RooRealVar* var ;

pdf = (RooAbsPdf*)_pdfItr->Next();
RooAbsPdf* pdf = &(RooAbsPdf&)_pdfList[i];

std::string pdfName = Form("pdf_%d",i);
RooCustomizer cust(*pdf,pdfName.c_str());
Expand All @@ -1545,7 +1500,7 @@ RooStarMomentMorph::CacheElem* RooStarMomentMorph::getCache(const RooArgSet* /*n
ownedComps.add(RooArgSet(*slope[sij(i,j)],*offsetVar[sij(i,j)])) ;

// linear transformations, so pdf can be renormalized easily
var = (RooRealVar*)(_obsItr->Next());
RooRealVar *var = (RooRealVar*)(_obsList[j]);
std::string transVarName = Form("%s_transVar_%d_%d",GetName(),i,j);
transVar[sij(i,j)] = new RooLinearVar(transVarName.c_str(),transVarName.c_str(),*var,*slope[sij(i,j)],*offsetVar[sij(i,j)]);

Expand Down Expand Up @@ -1680,7 +1635,6 @@ void RooStarMomentMorph::CacheElem::calculateFractions(const RooStarMomentMorph&
//int nObs=self._obsList.getSize();

// loop over parList
self._parItr->Reset();
int nnuis=0;

// zero all fractions
Expand All @@ -1691,7 +1645,7 @@ void RooStarMomentMorph::CacheElem::calculateFractions(const RooStarMomentMorph&
}
for (Int_t j=0; j<self._parList.getSize(); j++) {

RooRealVar* m = (RooRealVar*)(self._parItr->Next());
RooRealVar* m = &(RooRealVar&)self._parList[j];
double m0=m->getVal();

if (m0==0.) continue;
Expand Down
Loading
Loading