[RF] Add RooAbsArg::getParameters RooAbsArg::getObservables overloads that takes output parameter #8142
Conversation
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Failing tests:
|
|
Build failed on mac11.0/cxx17. Failing tests:
|
guitargeek
force-pushed
the
fillObservablesParameters_1
branch
from
e650d89
to
b6b9b52
Compare
|
Starting build on |
There was a problem hiding this comment.
Looks good to me !
I think is nice to have this new API for avoid creating all the time new RooArgset on the heap.
Only general comment thjat in general such functions in RooFit returns a Bool value , that is True if the function result in an error. Maybe we should change this to be consistent.
| return getParameters(&data,stripDisconnected) ; | ||
| } | ||
| /// Return the parameters of the p.d.f given the provided set of observables | ||
| RooArgSet* getParameters(const RooArgSet& observables, Bool_t stripDisconnected=kTRUE) const { | ||
| RooArgSet* getParameters(const RooArgSet& observables, bool stripDisconnected=true) const { |
There was a problem hiding this comment.
should we add also a void getParameters also in the RooAbsData case ?
I would not add for the other cases where a reference instead of a pointer is passed.
Same below in case of getObservables
There was a problem hiding this comment.
I chose to not do this to not blow up the interface too much. The caller can just do data.get() to get a RooArgSet to use with the new void getParameters/Observables. If we see at some point that this becomes too verbose we can still add a void getParameters for the RooAbsData case, but I think it doesn't have to be done now. Is that okay for you?
roofit/roofitcore/src/RooAbsArg.cxx
Outdated
| } | ||
|
|
||
|
|
||
| void RooAbsArg::getParameters(const RooArgSet* nset, RooArgSet& out, bool stripDisconnected) const |
There was a problem hiding this comment.
I would maybe call maybe the output set outList or outSet or as before parList
roofit/roofitcore/src/RooAbsArg.cxx
Outdated
| out.clear(); | ||
| out.setName("dependents"); | ||
|
|
||
| if (!dataList) return; |
There was a problem hiding this comment.
Should we return something here from the function (like False) or True ?
See for example bool RooAbsCollection::snapshot
guitargeek
force-pushed
the
fillObservablesParameters_1
branch
from
b6b9b52
to
772c0f7
Compare
|
Starting build on |
guitargeek
force-pushed
the
fillObservablesParameters_1
branch
from
772c0f7
to
e6aee2f
Compare
|
Starting build on |
Creating RooArgSets on the heap should be avoided. The existing `RooAbsArg::getParameters` created a RooArgSet calling `operator new`. This commit adds an overload of `RooAbsArg::getParameters` that takes an output parameter such that the output RooArgSet can also be created on the stack.
Creating RooArgSets on the heap should be avoided. The existing `RooAbsArg::getObservables` created a RooArgSet calling `operator new`. This commit adds an overload of `RooAbsArg::getObservables` that takes an output parameter such that the output RooArgSet can also be created on the stack.
With this fix, the calls to getObservables and getParameters are replaced with calls to the overloads that fill the RooArgSet in place. This avoids the creation of RooArgSets on the heap, which should be avoided to not let the MemPoolForRooSets grow too much.
guitargeek
force-pushed
the
fillObservablesParameters_1
branch
from
e6aee2f
to
d55568e
Compare
|
Starting build on |
Creating RooArgSets on the heap should be avoided. The existing
RooAbsArg::getParameters/Observablescreated a RooArgSet callingoperator new.This commit adds an overload of
RooAbsArg::getParameters/Observablesthat takes anoutput parameter such that the output RooArgSet can also be created on
the stack.
Using these new methods, a memory leak in
RooAbsReal::evaluateSpanis fixed. This leak was recently introduced in PR #7742 and backported to 6.24.Therefore, this fix should also be backported to 6.24.