Replace deprecated legacy iterators #874
Conversation
| RooArgSet *pdfPars = pdf->getParameters((const RooArgSet*)0); | ||
| std::unique_ptr<TIterator> iter_v(pdfPars->createIterator()); | ||
| for (RooAbsArg *a = (RooAbsArg *) iter_v->Next(); a != 0; a = (RooAbsArg *) iter_v->Next()) { | ||
| std::unique_ptr<RooArgSet> pdfPars{pdf->getParameters((const RooArgSet*)0)}; |
There was a problem hiding this comment.
A memory leak is fixed here. Note that getParameters returns an owning pointer that needs to be deleted.
| @@ -1,4 +1,4 @@ | |||
| #include "HiggsAnalysis/CombinedLimit/interface/RooEFTScalingFunction.h" | |||
| #include "../interface/RooEFTScalingFunction.h" | |||
There was a problem hiding this comment.
This change should be fine, because outside the classes.h file, the src/*.cc sources always use relative imports for combine headers already.
| @@ -21,8 +21,7 @@ RooAbsData *asimovutils::asimovDatasetNominal(RooStats::ModelConfig *mc, double | |||
|
|
|||
| if (verbose>2) { | |||
| Logger::instance().log(std::string(Form("AsimovUtils.cc: %d -- Parameters after fit for asimov dataset",__LINE__)),Logger::kLogLevelInfo,__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)}) { | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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) {
}There was a problem hiding this comment.
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
guitargeek
force-pushed
the
legacy_iterators
branch
from
91a8cdf
to
3664ef7
Compare
| while ( (fVar = (RooAbsReal*)varIter->Next()) ){ | ||
| pars.add(*fVar); | ||
| } | ||
| pars.add(_pars); |
There was a problem hiding this comment.
This fixes a memory leak because the TIterator *varIter was not deleted originally.
guitargeek
force-pushed
the
legacy_iterators
branch
9 times, most recently
from
7088f22
to
8e065b2
Compare
guitargeek
force-pushed
the
legacy_iterators
branch
2 times, most recently
from
599ad18
to
c960b9b
Compare
guitargeek
force-pushed
the
legacy_iterators
branch
from
c960b9b
to
5804339
Compare
The legacy iterators are deprecated since ROOT 6.18 and they will be removed at some point. This commit also fixes a few memory leaks, and it might also benefit the performance a bit because the legacy iterators had quite some overhead.
guitargeek
force-pushed
the
legacy_iterators
branch
from
5804339
to
cc0514d
Compare
|
Rebased to fix conflicts after #878 was merged |
|
Hi @guitargeek Thanks for the heads up; I will propagate the point about TIterators to classes removed in #878. Since pdf data members have changed, would it be possible to update their versions in the ClassDefs as well? Otherwise, the pdfs saved in workspaces, e.g., VerticalInterpPdf, would be inconsistent with the definitions in the loaded Combine library, and ROOT begins to complain with both workspaces and the Combine library loaded (as a side note, I found out that ROOT apparently also complains about the content of the comment lines after whitespace). |
|
Hi @usarica, thanks for the comments! There was no case where updating the version in the ClassDef was necessary. The only members that changed were transient members that are not read or written to disk (which is indicated to ROOT by this |
|
@guitargeek Ok, so that's what I saw before then. Thanks for clarifying! |
|
Shall we merge this? |
|
Yes please, merging this soon would help me to get the warnings with ROOT master out of the way, and do further developments without causing conflicts in this PR every time 🙂 |
| @@ -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)}) { | |||
There was a problem hiding this comment.
@nucleosynthesis If it turns out that these inlined getParameters() are problematic, this needs to be changed too.
The legacy iterators are deprecated since ROOT 6.18 and they will be removed at some point.
This commit also fixes a few memory leaks, and it might also benefit the performance a bit because the legacy iterators had quite some overhead.
After being work in progress for a bit, this PR is now done from my side and ready for review by @kcormi and @anigamova.