-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RF] Skip out-of-range events in RDataFrame to RooDataSet Helper #11018
Conversation
Starting build on |
Build failed on ROOT-debian10-i386/soversion. Failing tests: |
Build failed on ROOT-ubuntu18.04/nortcxxmod. Failing tests: |
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests:
|
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
The RDataFrameHelper should be consistent with creating a RooDataSet from a TTree, meaning out-of-range events should be skipped. This is implemented in this commit, borrowing the logic from `RooTreeDataStore::loadValues()`. A unit test is also implemented. The previous logic of just taking just all values to fill the dataset was very dangerous, because these values then clipped to the RooRealVar limits and biased the number of events observed at the boundaries. Closes root-project#11017.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
Thank you for the fix!
The RDataFrameHelper should be consistent with creating a RooDataSet
from a TTree, meaning out-of-range events should be skipped. This is
implemented in this commit, borrowing the logic from
RooTreeDataStore::loadValues()
. A unit test is also implemented.The previous logic of just taking just all values to fill the dataset
was very dangerous, because these values then clipped to the RooRealVar
limits and biased the number of events observed at the boundaries.
Closes #11017.