Skip to content

Commit

Permalink
[RF] Fix problems in RooAddPdf.
Browse files Browse the repository at this point in the history 
- Improve input validation.
RooAddPdf can only work with PDFs as arguments, but wasn't properly
checking that only PDFs are passed.

[ROOT-6008] Replace fixed-size array by vector in RooAddPdf.
RooAddPdf was using a fixed-size array of 100 elements to store
coefficients. This is obviously not safe, and therefore needed to be
replaced by something with dynamic allocation.

(cherry picked from commit 31e637e)
  • Loading branch information
@hageboeck
hageboeck committed Nov 3, 2019
1 parent dc087f1 commit 8adf4e1
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 54 deletions.
2 changes: 1 addition & 1 deletion roofit/roofitcore/inc/RooAddPdf.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class RooAddPdf : public RooAbsPdf {
mutable TNamed* _refCoefRangeName ; // Reference range name for coefficient interpreation

Bool_t _projectCoefs ; // If true coefficients need to be projected for use in evaluate()
mutable Double_t* _coefCache ; //! Transiet cache with transformed values of coefficients
std::vector<double> _coefCache; //! Transient cache with transformed values of coefficients


class CacheElem : public RooAbsCacheElement {
Expand Down
8 changes: 7 additions & 1 deletion roofit/roofitcore/src/RooAddGenContext.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,13 @@ RooAddGenContext::RooAddGenContext(const RooAddPdf &model, const RooArgSet &vars
_vars = (RooArgSet*) vars.snapshot(kFALSE) ;

for (const auto arg : model._pdfList) {
auto pdf = static_cast<const RooAbsPdf *>(arg);
auto pdf = dynamic_cast<const RooAbsPdf *>(arg);
if (!pdf) {
coutF(Generation) << "Cannot generate events from an object that is not a PDF.\n\t"
<< "The offending object is a " << arg->IsA()->GetName() << " named '" << arg->GetName() << "'." << std::endl;
throw std::invalid_argument("Trying to generate events from on object that is not a PDF.");
}

RooAbsGenContext* cx = pdf->genContext(vars,prototype,auxProto,verbose) ;
_gcList.push_back(cx) ;
}
Expand Down
94 changes: 42 additions & 52 deletions roofit/roofitcore/src/RooAddPdf.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ RooAddPdf::RooAddPdf() :
_snormList(0),
_recursive(kFALSE)
{
_coefCache = new Double_t[100] ;
_coefErrCount = _errorCount ;
TRACE_CREATE
}
Expand All @@ -120,7 +119,6 @@ RooAddPdf::RooAddPdf(const char *name, const char *title) :
_allExtendable(kFALSE),
_recursive(kFALSE)
{
_coefCache = new Double_t[100] ;
_coefErrCount = _errorCount ;
TRACE_CREATE
}
Expand Down Expand Up @@ -148,7 +146,7 @@ RooAddPdf::RooAddPdf(const char *name, const char *title,
_pdfList.add(pdf2) ;
_coefList.add(coef1) ;

_coefCache = new Double_t[_pdfList.getSize()] ;
_coefCache.resize(_pdfList.size());
_coefErrCount = _errorCount ;
TRACE_CREATE
}
Expand Down Expand Up @@ -191,28 +189,24 @@ RooAddPdf::RooAddPdf(const char *name, const char *title, const RooArgList& inPd
}

// Constructor with N PDFs and N or N-1 coefs
TIterator* pdfIter = inPdfList.createIterator() ;
TIterator* coefIter = inCoefList.createIterator() ;
RooAbsPdf* pdf ;
RooAbsReal* coef ;

RooArgList partinCoefList ;

Bool_t first(kTRUE) ;

while((coef = (RooAbsPdf*)coefIter->Next())) {
pdf = (RooAbsPdf*) pdfIter->Next() ;
if (!pdf) {
for (auto i = 0u; i < inCoefList.size(); ++i) {
auto coef = dynamic_cast<RooAbsReal*>(inCoefList.at(i));
auto pdf = dynamic_cast<RooAbsPdf*>(inPdfList.at(i));
if (inPdfList.at(i) == nullptr) {
coutE(InputArguments) << "RooAddPdf::RooAddPdf(" << GetName()
<< ") number of pdfs and coefficients inconsistent, must have Npdf=Ncoef or Npdf=Ncoef+1" << endl ;
assert(0) ;
}
if (!dynamic_cast<RooAbsReal*>(coef)) {
coutE(InputArguments) << "RooAddPdf::RooAddPdf(" << GetName() << ") coefficient " << coef->GetName() << " is not of type RooAbsReal, ignored" << endl ;
if (!coef) {
coutE(InputArguments) << "RooAddPdf::RooAddPdf(" << GetName() << ") coefficient " << (coef ? coef->GetName() : "") << " is not of type RooAbsReal, ignored" << endl ;
continue ;
}
if (!dynamic_cast<RooAbsReal*>(pdf)) {
coutE(InputArguments) << "RooAddPdf::RooAddPdf(" << GetName() << ") pdf " << pdf->GetName() << " is not of type RooAbsPdf, ignored" << endl ;
if (!pdf) {
coutE(InputArguments) << "RooAddPdf::RooAddPdf(" << GetName() << ") pdf " << (pdf ? pdf->GetName() : "") << " is not of type RooAbsPdf, ignored" << endl ;
continue ;
}
_pdfList.add(*pdf) ;
Expand Down Expand Up @@ -240,11 +234,12 @@ RooAddPdf::RooAddPdf(const char *name, const char *title, const RooArgList& inPd
}
}

pdf = (RooAbsPdf*) pdfIter->Next() ;
if (pdf) {
if (!dynamic_cast<RooAbsReal*>(pdf)) {
coutE(InputArguments) << "RooAddPdf::RooAddPdf(" << GetName() << ") last pdf " << coef->GetName() << " is not of type RooAbsPdf, fatal error" << endl ;
assert(0) ;
if (inPdfList.size() == inCoefList.size() + 1) {
auto pdf = dynamic_cast<RooAbsPdf*>(inPdfList.at(inCoefList.size()));

if (!pdf) {
coutF(InputArguments) << "RooAddPdf::RooAddPdf(" << GetName() << ") last pdf " << pdf->GetName() << " is not of type RooAbsPdf, fatal error" << endl ;
throw std::invalid_argument("Last argument for RooAddPdf is not a PDF.");
}
_pdfList.add(*pdf) ;

Expand All @@ -265,10 +260,7 @@ RooAddPdf::RooAddPdf(const char *name, const char *title, const RooArgList& inPd
_haveLastCoef=kTRUE ;
}

delete pdfIter ;
delete coefIter ;

_coefCache = new Double_t[_pdfList.getSize()] ;
_coefCache.resize(_pdfList.size());
_coefErrCount = _errorCount ;
_recursive = recursiveFractions ;

Expand Down Expand Up @@ -296,12 +288,11 @@ RooAddPdf::RooAddPdf(const char *name, const char *title, const RooArgList& inPd
_recursive(kFALSE)
{
// Constructor with N PDFs
TIterator* pdfIter = inPdfList.createIterator() ;
RooAbsPdf* pdf ;
while((pdf = (RooAbsPdf*) pdfIter->Next())) {

if (!dynamic_cast<RooAbsReal*>(pdf)) {
coutE(InputArguments) << "RooAddPdf::RooAddPdf(" << GetName() << ") pdf " << pdf->GetName() << " is not of type RooAbsPdf, ignored" << endl ;
for (const auto pdfArg : inPdfList) {
auto pdf = dynamic_cast<const RooAbsPdf*>(pdfArg);

if (!pdf) {
coutE(InputArguments) << "RooAddPdf::RooAddPdf(" << GetName() << ") pdf " << (pdf ? pdf->GetName() : "") << " is not of type RooAbsPdf, ignored" << endl ;
continue ;
}
if (!pdf->canBeExtended()) {
Expand All @@ -311,9 +302,7 @@ RooAddPdf::RooAddPdf(const char *name, const char *title, const RooArgList& inPd
_pdfList.add(*pdf) ;
}

delete pdfIter ;

_coefCache = new Double_t[_pdfList.getSize()] ;
_coefCache.resize(_pdfList.size());
_coefErrCount = _errorCount ;
TRACE_CREATE
}
Expand All @@ -337,7 +326,7 @@ RooAddPdf::RooAddPdf(const RooAddPdf& other, const char* name) :
_allExtendable(other._allExtendable),
_recursive(other._recursive)
{
_coefCache = new Double_t[_pdfList.getSize()] ;
_coefCache.resize(_pdfList.size());
_coefErrCount = _errorCount ;
TRACE_CREATE
}
Expand All @@ -349,7 +338,6 @@ RooAddPdf::RooAddPdf(const RooAddPdf& other, const char* name) :

RooAddPdf::~RooAddPdf()
{
if (_coefCache) delete[] _coefCache ;
TRACE_DESTROY
}

Expand Down Expand Up @@ -656,7 +644,9 @@ RooAddPdf::CacheElem* RooAddPdf::getProjCache(const RooArgSet* nset, const RooAr

void RooAddPdf::updateCoefficients(CacheElem& cache, const RooArgSet* nset) const
{
// cxcoutD(ChangeTracking) << "RooAddPdf::updateCoefficients(" << GetName() << ") update coefficients" << endl ;
// Since this function updates the cache, it obviously needs write access:
auto& myCoefCache = const_cast<std::vector<double>&>(_coefCache);
myCoefCache.resize(_haveLastCoef ? _coefList.size() : _pdfList.size(), 0.);

// Straight coefficients
if (_allExtendable) {
Expand All @@ -666,16 +656,16 @@ void RooAddPdf::updateCoefficients(CacheElem& cache, const RooArgSet* nset) cons
std::size_t i = 0;
for (auto arg : _pdfList) {
auto pdf = static_cast<RooAbsPdf*>(arg);
_coefCache[i] = pdf->expectedEvents(_refCoefNorm.getSize()>0?&_refCoefNorm:nset) ;
coefSum += _coefCache[i] ;
myCoefCache[i] = pdf->expectedEvents(_refCoefNorm.getSize()>0?&_refCoefNorm:nset) ;
coefSum += myCoefCache[i] ;
i++ ;
}

if (coefSum==0.) {
coutW(Eval) << "RooAddPdf::updateCoefCache(" << GetName() << ") WARNING: total number of expected events is 0" << endl ;
} else {
for (int j=0; j < _pdfList.getSize(); j++) {
_coefCache[j] /= coefSum ;
myCoefCache[j] /= coefSum ;
}
}

Expand All @@ -687,14 +677,14 @@ void RooAddPdf::updateCoefficients(CacheElem& cache, const RooArgSet* nset) cons
std::size_t i=0;
for (auto coefArg : _coefList) {
auto coef = static_cast<RooAbsReal*>(coefArg);
_coefCache[i] = coef->getVal(nset) ;
coefSum += _coefCache[i++];
myCoefCache[i] = coef->getVal(nset) ;
coefSum += myCoefCache[i++];
}
if (coefSum==0.) {
coutW(Eval) << "RooAddPdf::updateCoefCache(" << GetName() << ") WARNING: sum of coefficients is zero 0" << endl ;
} else {
for (int j=0; j < _coefList.getSize(); j++) {
_coefCache[j] /= coefSum;
myCoefCache[j] /= coefSum;
}
}
} else {
Expand All @@ -704,12 +694,12 @@ void RooAddPdf::updateCoefficients(CacheElem& cache, const RooArgSet* nset) cons
std::size_t i=0;
for (auto coefArg : _coefList) {
auto coef = static_cast<RooAbsReal*>(coefArg);
_coefCache[i] = coef->getVal(nset) ;
//cxcoutD(Caching) << "SYNC: orig coef[" << i << "] = " << _coefCache[i] << endl ;
lastCoef -= _coefCache[i++];
myCoefCache[i] = coef->getVal(nset) ;
//cxcoutD(Caching) << "SYNC: orig coef[" << i << "] = " << myCoefCache[i] << endl ;
lastCoef -= myCoefCache[i++];
}
_coefCache[_coefList.getSize()] = lastCoef ;
//cxcoutD(Caching) << "SYNC: orig coef[" << _coefList.getSize() << "] = " << _coefCache[_coefList.getSize()] << endl ;
myCoefCache[_coefList.getSize()] = lastCoef ;
//cxcoutD(Caching) << "SYNC: orig coef[" << _coefList.getSize() << "] = " << myCoefCache[_coefList.getSize()] << endl ;


// Warn about coefficient degeneration
Expand Down Expand Up @@ -763,20 +753,20 @@ void RooAddPdf::updateCoefficients(CacheElem& cache, const RooArgSet* nset) cons

RooAbsPdf::globalSelectComp(_tmp) ;

_coefCache[i] *= proj ;
coefSum += _coefCache[i] ;
myCoefCache[i] *= proj ;
coefSum += myCoefCache[i] ;
}


if ((RooMsgService::_debugCount>0) && RooMsgService::instance().isActive(this,RooFit::Caching,RooFit::DEBUG)) {
for (int i=0; i < _pdfList.getSize(); ++i) {
ccoutD(Caching) << " ALEX: POST-SYNC coef[" << i << "] = " << _coefCache[i]
<< " ( _coefCache[i]/coefSum = " << _coefCache[i]*coefSum << "/" << coefSum << " ) "<< endl ;
ccoutD(Caching) << " ALEX: POST-SYNC coef[" << i << "] = " << myCoefCache[i]
<< " ( _coefCache[i]/coefSum = " << myCoefCache[i]*coefSum << "/" << coefSum << " ) "<< endl ;
}
}

for (int i=0; i < _pdfList.getSize(); i++) {
_coefCache[i] /= coefSum ;
myCoefCache[i] /= coefSum ;
}


Expand Down

0 comments on commit 8adf4e1

Please sign in to comment.