[RF] Speed up getting weights from RooDataHist#7859
[RF] Speed up getting weights from RooDataHist#7859guitargeek merged 5 commits intoroot-project:masterfrom
Conversation
|
Starting build on |
|
Build failed on windows10/cxx14. Failing tests: |
hageboeck
left a comment
hageboeck
left a comment
There was a problem hiding this comment.
Nice to see that you find some opportunities for speed improvements!
| // Asserts commented out because the code is performance critical. | ||
|
|
||
| // With fast, caller promises that layout of "coords" is identical to our internal "vars". | ||
| // assert(!fast || _vars.size() == coords.size()); |
There was a problem hiding this comment.
Check this out: assert
It's the magic weapon for things that "should", but you cannot be sure that they "are", unless you get c++ contracts.
Note that there is no run time overhead in release builds - they don't generate any code.
Now, imagine the following:
A user comes to the forum and tells you that xy yields wrong results when they do A, but with B it works. It doesn't crash, the results are just wrong, and nobody knows where the error happens.
Those are the bugs that take long to track down.
You ask for the code, run it in your debug build that you should always have around, the assert fails and you tell the user what's wrong or fix the bug.
Long story short:
The asserts should stay in, and please use them to assert all things where you go "hmm, this should be OK if people (or you) used it correctly".
For things that are not performance critical, it's of course even better to validate inputs before you start a fit etc.
There was a problem hiding this comment.
For things that are not performance critical, it's of course even better to validate inputs before you start a fit etc.
I mean run-time checks that are active also in release builds, but this is mostly for the initialisation phase, while the asserts are better in the execution phase when you want speed.
| // If fast is on, users promise that the sets have the same layout. | ||
| // assert(!fast || strcmp(internalVar->GetName(), theVar->GetName()) == 0); |
guitargeek
force-pushed
the
RooDataHist_1
branch
from
ee051dc to
58d01e8
Compare
|
Starting build on |
guitargeek
changed the title
guitargeek
force-pushed
the
RooDataHist_1
branch
from
58d01e8 to
15667a1
Compare
|
Starting build on |
guitargeek
changed the title
guitargeek
force-pushed
the
RooDataHist_1
branch
from
15667a1 to
a3c9080
Compare
|
Starting build on |
|
Build failed on ROOT-debian10-i386/cxx14. Failing tests: |
|
Build failed on windows10/cxx14. Failing tests: |
guitargeek
force-pushed
the
RooDataHist_1
branch
from
a3c9080 to
07820b6
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
|
Okay, I can update the code with less |
guitargeek
force-pushed
the
RooDataHist_1
branch
from
07820b6 to
38b3da7
Compare
|
Starting build on |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Failing tests: |
guitargeek
force-pushed
the
RooDataHist_1
branch
from
38b3da7 to
712fe8b
Compare
|
Starting build on |
guitargeek
force-pushed
the
RooDataHist_1
branch
from
712fe8b to
b443d89
Compare
|
Starting build on |
|
Hi! Some updates on this PR:
|
hageboeck
left a comment
hageboeck
left a comment
There was a problem hiding this comment.
It's looking good, but I have some suggestions to take it all the way.
Sorry that I insist on documentation, but you will never be so close to getting the docs done, because you should still remember what the functions and parameters mean.
In 3 months, this documentation won't get touched again.
Where is the schema evolution read test that you mentioned? Is it one of the tests that I implemented?
roofit/roofitcore/inc/RooDataHist.h
Outdated
| double* _binv {nullptr}; //[_arrSize] Bin volume array | ||
|
|
||
| RooArgSet _realVars ; // Real dimensions of the dataset | ||
| RooArgSet _realVars ; // Real dimensions of the dataset |
There was a problem hiding this comment.
Try to keep formatting changes and code changes separate. (No need to change anything in the commit, though. 👍 )
| //////////////////////////////////////////////////////////////////////////////// | ||
| /// A faster version of RooDataHist::weight that assumes the passed arguments | ||
| /// are aligned with the histogram variables. | ||
| /// |
There was a problem hiding this comment.
Whenever you touch a comment, try to write full doxygen style with documentation of the parameters.
Random example:
root/roofit/roofitcore/src/RooBinSamplingPdf.cxx
Lines 122 to 126 in bd49fae
I know it's more work, but if you don't do it right now, it won't get done in a long time.
There was a problem hiding this comment.
Yes, I can do that.
roofit/roofitcore/src/RooHistPdf.cxx
Outdated
| // cout << "RooHistPdf::evaluate(" << GetName() << ") ret = " << ret << " "; | ||
| // cout << _histObsList[0] << " "; | ||
| // _histObsList[0]->Print(""); |
There was a problem hiding this comment.
Kill useless code when you get close to it.
| VarInfo const& varInfo = getVarInfo(); | ||
|
|
||
| Double_t wInt(0) ; | ||
| auto centralIdx = calcTreeIndex(bin, true); |
There was a problem hiding this comment.
That's very optional, but you could make temporary stuff const. You don't inadvertently overwrite it and make sure that it gets optimised out. (It will of course also be optimised out when it's not const, unless you write to it by accident.)
There was a problem hiding this comment.
Sure, that's a good thing to do!
| hist.Fill(1.5, 1.5); | ||
| } | ||
|
|
||
| hist.Print(); |
There was a problem hiding this comment.
We try to be reasonably silent in unit tests.
There was a problem hiding this comment.
Okay, I'll delete that print statement!
|
|
||
| RooDataHist dataHist("data", "dataset with (x,y)", RooArgList(x, y), &hist); | ||
|
|
||
| dataHist.Print(); |
There was a problem hiding this comment.
Also silence this if possible.
There was a problem hiding this comment.
Okay, I'll delete that print statement!
Retreiving weights from a RooDataHist can be sped up signifcantly if it is assumed that parameters passed to `RooDataHist::weight` are aligned with the histogram variables. This commit introduces a `RooDataHist::weightFast` function that makes this assumption. It is used by RooHistFunc and RooHistPdf.
Getting the weights from a RooDataHist is now also sped up for the interpolation case with the assumption that the arguemtns passed to `RooDataHist::weightFast` are aligned with the histogram variables. To achieve this, the interpolation routines were changed such that the value of the parameters is never set.
guitargeek
force-pushed
the
RooDataHist_1
branch
from
b443d89 to
e9ea120
Compare
|
Starting build on |
|
Hi @hageboeck! For the schema evolution from the previous version, I just made a test myself where I wrote a RooDataHist to a file with ROOT 6.24 and read it back with ROOT master + this PR. Do you think I should implement a unit test for that? I didn't think this was necessary, after all the change is rather trivial (removal of |
3 participants
Retreiving weights from a RooDataHist can be sped up signifcantly if it
is assumed that parameters passed to
RooDataHist::weightare alignedwith the histogram variables.
This commit introduces a
RooDataHist::weightFastfunction that makesthis assumption. It is used by RooHistFunc and RooHistPdf.
This was benchmarked with the same hist factory example as in #7838:
The effect is a 15 % speedup of the HistFactory example.