[RF] Some performance improvements to RooFit likelihood creation and BatchMode #12051
Conversation
|
Starting build on |
guitargeek
changed the title
|
Build failed on mac12/noimt. Failing tests:
|
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Failing tests:
|
In `RooProdPdf::getConstraints()`, there is a place where it checks if the parameters of one component pdf are overlapping with the observables or all parameters of the model. This can get expensive for models with a huge number of parameters. For the ATLAS Higgs combination workspace I have, it is a significant fraction of `createNLL()`, which takes 100 seconds total. We can greatly speed up this check by caching the ordered name pointers of the parameters and observables collections. This makes the time spent in `getConstraints()` negligible for now (5 out of the remaining 80 seconds in `createNLL` for the ATLAS Higgs combination). It was important to optimize this part, because the exact same code is also used in the BatchMode, for which I aim to make `createNLL` much faster than 80 seconds in total.
|
Build failed on ROOT-debian10-i386/soversion. Failing tests:
|
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests:
|
guitargeek
force-pushed
the
devs_roofit
branch
from
fda7cae to
1c27063
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
|
|
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
|
Build failed on ROOT-debian10-i386/soversion. Failing tests: |
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
LGTM!
Thanks Jonas for the nice improvements when fitting the ATLAS Higgs examples
It's to expensive to do for large graphs, too complicated to implement, an not necessary anymore because also in BatchMode the full computation graph is cloned. This also means that the `RooAbsReal` that is evaluated in `RooAbsReal::getValues()` also needs to be cloned, but this is okay because the interface is so far only used in unit tests and not performance critical. It was actually necessary to do the cloning there to begin with, since the BatchMode was not correctly reverting all modifications to the computation graph anyway.
guitargeek
force-pushed
the
devs_roofit
branch
from
1c27063 to
0827d42
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
|
|
Build failed on ROOT-debian10-i386/soversion. Failing tests: |
3 participants
RooProdPdf::getConstraints()More detail in the commit descriptions.