[v626][RF] Backports of RooFit PRs to v6-26-00-patches: Part 19
#11133
Conversation
This is done to easier cherry-pick any commits that come after it.
guitargeek
force-pushed
the
v6-26-00-patches_roofit_backports_19
branch
from
59f952d
to
4bc5063
Compare
|
Starting build on |
|
Build failed on windows10/cxx14. Errors:
|
guitargeek
force-pushed
the
v6-26-00-patches_roofit_backports_19
branch
from
4bc5063
to
a856aa8
Compare
|
Starting build on |
|
Build failed on windows10/cxx14. Errors:
|
The RooLagrangianMorphFunc was a desaster concerning memory safety. This commit fixes almost all of the memory leaks that were easy to spot because of the usage of `new`. One leak that still remains are the `RooDataHist`s that underlies the `RooHistFunc`s. But to fix that, it would be important to first come up with an elegant way to have the RooHistFunc own their underlying RooDataHist.
* correctly initialize `RooAbsRealWrapper::_ownsDriver`
* remove observables from output parameters in
`RooAbsRealWrapper::getParameters()`
Integrating a RooAbsRealLValue like a RooRealVar doesn't work in RooFit,
which one can check with this code:
```C++
RooRealVar x{"x", "x", 2.0, -5.0, 5.0};
std::unique_ptr<RooAbsReal> xint{x.createIntegral(x)};
xint->Print();
```
The integral of x from -5 to 5 should be zero, but the integral object
only returns the current value of the variable.
Some users expect the integral to work, and give the same result as
this, which correctly prints out zero:
```C++
RooRealVar x{"x", "x", 2.0, -5.0, 5.0};
RooProduct xId{"xId", "xId", RooArgList{x}};
std::unique_ptr<RooAbsReal> xint{xId.createIntegral(x)};
xint->Print();
```
This is assumed in two RooFit unit tests:
* [testRooWrapperPdf](https://github.com/root-project/root/blob/master/roofit/roofitcore/test/testRooWrapperPdf.cxx#L27)
* [testNestedPDFs](https://github.com/guitargeek/roottest/blob/master/root/roofitstats/vectorisedPDFs/testNestedPDFs.cxx#L45) in roottest
Both tests **work only by chance** because the stored x value is the
same as its integral! As soon as the x value or limits would change, the
results don't make sense anymore.
As the integration of RooAbsRealLValues never worked correctly and was
not used anywhere outside artificial unit tests, this commit suggests so
prohibit the integration of RooAbsRealLValues by throwing an exception
if `RooAbsRealLValue::createIntegral()` is called.
There is no reason to hide these member functions from the user, which are `const` getters to values that the user was setting themselves to begin with.
In pass through mode, the RooRealIntegral should have registered the function as a value server, because we directly depend on its value. As it is only determined later in the constructor if a given integral is in pass through mode, we also have to delay the construction of the function proxy object now. It's important to do this correctly, because the new BatchMode uses the value server interface to analyze the computation graph.
The new RooFit batchMode skipped zero-weight events to optimize the likelihood calculation. However, this should not be done in general, because it is unexpected to users is the output of batched computations is not aligned with the original dataset.
Add the arg from the actual node list in the computation graph. Like this, we don't accidentally add internal variable clones that the client args returned. Looking this up is fast because of the name pointer hash map optimization. This is done because it will prevent crashes in future RooFit developments that yet have to be commited.
In the BatchModeHelpers that are used to create the NLL object with the BatchMode, the PDF is cloned if it is a RooSimultaneous. After cloning, the cloned parameters are reattached to the original model. However, it should be the other way around because otherwise the original model points to invalid parameter args when the fit is done.
This is not a fix to any issue, just a refactoring to avoid manual memory management in the BatchModeHelpers.
In the `RooFitDriver::NodeInfo` destructor, the data tokens for all nodes are reset. However, if this happens after the destruction of the normalization integrals in the computation graph, it tries to reset the data token for nodes that don't exist anymore. Hence, the data token resetting is better done at the beginning of the RooFitDriver destructor.
In ROOT 6.26, the default constructor of the RooEffProd was accidentally removed. This commit is bringing it back, and it also needs to be backported to the 6.26 development branch. Thanks to this forum post for noticing the problem: https://root-forum.cern.ch/t/trouble-with-rooworkspace-since-rooeffprod-default-constructor-is-deleted-in-v6-26/50577
If a RooProdPdf is created with the `Conditional` command argument to do a conditional fit, there is a flag `depsAreCond` to the command argument that specifies if the passed variable set corresponds to the normalization set of the conditional PDF (default), or the conditional observables. In the new BatchMode, this flag was not considered, which is fixed by this PR. A new unit test that checks that the `depsAreCond` parameter is correctly considered in both the BatchMode and the old RooFit is also implemented, based on the issue reproducer code that was kindly provided by @elusian in root-project#11332. This commit also merges the `testRooProductPdf` and `testRooProdPdf` files, because they are both testing the RooProdPdf. Closes root-project#11332.
guitargeek
force-pushed
the
v6-26-00-patches_roofit_backports_19
branch
from
a856aa8
to
062e44b
Compare
|
Starting build on |
|
Build failed on ROOT-debian10-i386/soversion. Errors:
|
|
Build failed on mac1015/cxx17. Errors:
|
guitargeek
changed the title
v6-26-00-patches: Part 19
|
Starting build on |
|
Build failed on ROOT-ubuntu2004/python3. Errors:
|
|
Build failed on ROOT-debian10-i386/soversion. Errors:
|
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
|
|
Build failed on mac11/cxx14. Errors:
|
|
Build failed on mac1015/cxx17. Errors:
|
This is a backport of all the relevant bugfix RooFit PRs that were recently merged to
mastertov6-26-00-patches(in the right order, to not have the commit history diverge too much).Only the last commit, which has not been backported for the last patch release yet (the rest of PR was backported before the 6.26.04 release)
Only the first commit that relates to the batch mode
RooAbsRealLValue#11162Only the first two commits that are note just code modernization
depsAreCondparameter for conditional fits in BatchMode #11343Only the first commit that is not code modernization
Furthermore, an initial commit is added to sync the RooLagrangianMorphFunc with the
masterbefore the first backported commit, in order to make the cherry-picking of backport commits easierAddresses #11059.