Fixes for TTree with complex friend hierarchies #20222
Conversation
vepadulano
added
in:TTree
experiment
Test Results 22 files 22 suites 3d 19h 24m 43s ⏱️ Results for commit aacd4c1. ♻️ This comment has been updated with latest results. |
vepadulano
changed the title
There was a problem hiding this comment.
Thanks a lot!
Some mini-observations:
- There are more places where fixing the potential integer overflow is advisable, see https://github.com/root-project/root/pull/20227/files, in TTreePlayer, but also in TSpider, TChain
- precalculating lastentry is already being used in three other places in the ROOT tree code, so the current solution (entry - firstentry < nentries), while it works perfectly, is a bit less homogeneous with the existing than my proposal (entry <= lastentry)
Thanks for pointing these out! I acknowledge there may be more places to fix. For each of those, we need a corresponding reproducer that fails without the fix and works with the fix. For now those places are not interfering immediately with the code changes needed for this PR, so I would in principle tackle them separately. I'm still developing more tests offline which will end up in this PR. If while developing those tests I see that some other integer overflow is the problem, then I will create the corresponding issue+commit in this PR.
Where exactly? I can only see in |
Thanks, got it. An easy failures-reproducer of two of the locations: TTreeplayer lines 237 and 620: change line 83 of tutorials/io/tree/tree109_friend.C with (For the other locations I do not have a repro.) |
vepadulano
force-pushed
the
gh-20033
branch
2 times, most recently
from
97a4b1e to
68c8968
Compare
|
@pcanal I addressed all your comments. I also added in the last commit an extension to one of the testing suites that I introduced in an earlier commit, now it considers all cases in which a call to TChain::Scan is accessing one of the friend chains with a name of chain that may be different than the name of tree it holds. ATOW, this PR introduces the following amount of tests:
|
|
As discussed privately with @pcanal, I have reorganised the commit history of this PR to have all the code changes at the beginning, and separate every type of new test scenario added in a different commit, appended after the changes. Another PR has been opened just with the test commits to see the failures in isolation --> #20263 |
vepadulano
added
the
test coverage
|
Pasting here errors from the tests seen in the other PR. Regression testsNew tests for complex friendship hierarchies scenarios |
|
Here is a link to the test runs in the other PR https://github.com/root-project/root/actions/runs/18983943721?pr=20263 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #20222 +/- ##
==========================================
- Coverage 34.98% 34.96% -0.03%
==========================================
Files 3391 3392 +1
Lines 493265 493394 +129
Branches 243723 243756 +33
==========================================
- Hits 172547 172493 -54
- Misses 207301 207558 +257
+ Partials 113417 113343 -74
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vepadulano
force-pushed
the
gh-20033
branch
2 times, most recently
from
4d3992d to
1c8c12b
Compare
TTree::FindBranch already took into account the case of the user wanting to find a branch in the tree by preceding the branch name with the tree name itself. The same logic was not applied symmetrically in TChain::FindBranch, so it's now introduced. The logic is written such that it only performs a copy of the lookup string after having checked that it starts with the name of the chain itself followed by a dot.
TTree::FindLeaf already took into account the case of the user wanting to find a leaf in the tree by preceding the branch name with the tree name itself. The same logic was not applied symmetrically in TChain::FindLeaf, so it's now introduced. The logic is written such that it only performs a copy of the lookup string after having checked that it starts with the name of the chain itself followed by a dot.
The general idea for this test suite is to have one single source of data, a TChain, that is befriended by a long enough friendship hierarchy, namely four consecutive layers of friends. The outermost dataset is the one that drives the event loop, either via SetBranchAddress + raw loop or via TTree::Scan. The data comes from the innermost TChain. All the combinations of TTree/TChain at the various steps of the hierarchy are tried. Furthermore, TTree::Scan is called with an increasingly larger starting entry to ensure that even when starting e.g. from an entry of the second tree of the innermost chain the Scan works well.
Add a test suite for the scenario of multiple levels of friendship hierarchy where all TTree/TChain datasets in the hierarchy have a branch with exactly the same name. Call TTree::Scan and make sure that the correct branch from the correct TTree is accessed with every possible combination of using TTree/TChain to steer the execution or only one or more of the branches of the hierarchy. This scenario represents the use case of comparing the same quantity which can pass through various derivation steps and each step must be compared with the other ones.
Introduce a test with a long friendship hierarchy where data is accessed via TTree::Scan. Every dataset in the hierarchy has the same branch name. The Scan expression uses different branch names from every dataset in the same component of the expression. This checks that the branch addresses are properly set and they don't get mixed up.
Tests that a TChain with a name different than its inner TTree can find a branch of the TTree when the user passes a branch lookup string of the form "chainName.branchName".
Tests that a TTree that has a TChain friend can retrieve a branch from the inner TTree of the friend TChain by prefixing the branch name with the chain name, even if the inner TTree of the friend TChain might have a different name than the chain name.
|
The latest force-push is just to introduce a small improvement. In TChain::FindBranch and TChain::FindLeaf we don't need the copy of the std::string_view to ensure the character array is null-terminated. That's because we're only truncating from the beginning of the view (removing the leading |
4 participants
This PR collects the fixes and correlated testing in the latest campaign to fix the CMS DAS MiniAOD analysis framework, the only MiniAOD analysis system supported by the CMS Common Analysis Tools group (see https://indico.cern.ch/event/1330797/papers/5796853/files/13266-CAT_ACAT24_proceeding.pdf)