[RF] Fix recently-introduced RooAbsReal::getPropagatedError problems and add several new unit tests
#11445
Conversation
The unit test checks whether a RooAddPdf defined with recursive coefficients is equivalent to mathematically equivalent RooAddPdf defined without the recursive coefficient flag.
|
Starting build on |
The recent commit `565e9aa19a` updated the `getPropagatedError` with some additional checks, but these checks caused segfaults in some specifit usecases: * where the `RooAbsReal` is a parameter in the fit result itself * where the `RooAbsReal` is a variable unrelated to the fit result * where the `RooAbsReal` depends only on some of the parameters in the fit result This commit fixes these usecases again. A unit test that checks that these usecases don't fail anymore is now implemented in `testRooAbsReal`.
guitargeek
force-pushed
the
roofit_fixes_2
branch
from
4e61a64
to
0cf6207
Compare
|
Starting build on |
guitargeek
changed the title
RooAbsReal::getPropagatedError problems and several new unit testsRooAbsReal::getPropagatedError problems and add several new unit tests
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Warnings:
|
|
Build failed on ROOT-ubuntu2004/python3. Warnings:
Failing tests: |
|
Build failed on ROOT-debian10-i386/soversion. Warnings:
|
guitargeek
force-pushed
the
roofit_fixes_2
branch
from
0cf6207
to
be6bfb2
Compare
|
Starting build on |
|
The last push didn't change much, just commenting out the unused variables |
roofit/roofitcore/src/RooAbsReal.cxx
Outdated
| @@ -2660,9 +2673,9 @@ double RooAbsReal::getPropagatedError(const RooFitResult &fr, const RooArgSet &n | |||
| TMatrixDSym C(paramList.getSize()) ; | |||
| vector<double> errVec(paramList.getSize()) ; | |||
There was a problem hiding this comment.
Okay! I have done the same for all std:: classes in the file
roofit/roofitcore/src/RooAbsReal.cxx
Outdated
| if(rrvFitRes->namePtr() == namePtr()) return rrvFitRes->getError(); | ||
|
|
||
| // Strip out parameters with zero error | ||
| if (rrvFitRes->getError() <= 1e-20) continue; |
There was a problem hiding this comment.
why hard coding here 1.E-20 ? Maybe use numerical precision epsilon ?
There was a problem hiding this comment.
Yes, that is indeed a better choice than this magic number. I have updated it here and also in another place in that file
|
Starting build on |
The recent commit 922dec5 fixed GitHub issue root-project#11396, but there was still no unit test implemented with the reproducer code from that issue. This commit is implementing such a unit test.
guitargeek
force-pushed
the
roofit_fixes_2
branch
from
a0566ba
to
d533b13
Compare
|
Starting build on |
In some places in RooAbsReal, the magic constant 1e-20 was used to denote a negligibly small error greater than zero. Instead of using that arbitrary value, one should use the variable value times the `std::numeric_limits<double>::epsilon()`, which is the correct way to figure out how small the error needs to be to have numerically no effect on the value.
The `binBoundaries` function that was used in RooAbsReal returns an owning pointer, and in the code it was forgotten to delete that pointer later. This is fixed now by using a smart pointer.
guitargeek
force-pushed
the
roofit_fixes_2
branch
from
d533b13
to
920783b
Compare
|
Starting build on |
|
Build failed on mac11/cxx14. Failing tests: |
The recent commit
565e9aa19aupdated thegetPropagatedErrorwithsome additional checks, but these checks caused segfaults in some
specifit usecases:
where the
RooAbsRealis a parameter in the fit result itselfwhere the
RooAbsRealis a variable unrelated to the fit resultwhere the
RooAbsRealdepends only on some of the parameters in thefit result
This commit fixes these usecases again.
A unit test that checks that these usecases don't fail anymore is now
implemented in
testRooAbsReal.This PR also adds some other new unit tests that cover recent PRs and issues.