[RF] Consider depsAreCond parameter for conditional fits in BatchMode #11343
[RF] Consider depsAreCond parameter for conditional fits in BatchMode #11343guitargeek merged 3 commits intoroot-project:masterfrom
depsAreCond parameter for conditional fits in BatchMode #11343Conversation
guitargeek
requested review from
bellenot,
hageboeck and
lmoneta
as code owners
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests: |
|
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
|
Build failed on mac11/cxx14. Failing tests: |
|
Hello, thank you for the PR! |
|
Yes you're right. Thanks! |
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.
Improving the memory management of the RooProdPdf by using more smart pointers. In particular, a memory leak of the return values of the private functions `RooProdPdf::makeCondPdfRatioCorr` is fixed.
guitargeek
force-pushed
the
issue-11332
branch
from
27a7136 to
29dc67d
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests: |
|
Build failed on mac11/cxx14. Failing tests: |
|
Build failed on mac1015/cxx17. Failing tests: |
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
LGTM !
Thank you for fixing this Jonas.
I have just one comment, is this flag really needed ? I think it could cause some confusion, one should just pass the correct observable to normalise in the constructor of the RooProdPdf via the Conditional common, what is the reason to interpret them in some case in the opposite way, i.e. the depSet as conditional observable rather than normalising observables ?
|
I don't know. Why this option exists. I agree, it is confusing. I think what we can do is to merge this PR, do the backport as a bugfix, and then only in ROOT |
4 participants
If a RooProdPdf is created with the
Conditionalcommand argument to doa conditional fit, there is a flag
depsAreCondto the command argumentthat 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
depsAreCondparameter iscorrectly 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 #11332.
This PR also merges the
testRooProductPdfandtestRooProdPdffiles, because they are both testing the RooProdPdf.
Closes #11332.
A secondary commit with some code improvements to the RooProdPdf class is also included in this PR.