Skip to content
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

[TTree] when merging, ignore trees without branches #17652

Merged
merged 4 commits into from
Feb 28, 2025

Conversation

ferdymercury
Copy link
Contributor

@ferdymercury ferdymercury commented Feb 6, 2025

This Pull request:

Alternative to #17650 as suggested by pcanal

Changes or fixes:

Fixes #14558
Fixes #12510

@ferdymercury ferdymercury requested a review from pcanal as a code owner February 6, 2025 18:14
@ferdymercury ferdymercury marked this pull request as draft February 7, 2025 07:48
Copy link

github-actions bot commented Feb 7, 2025

Test Results

    18 files      18 suites   4d 6h 29m 39s ⏱️
 2 719 tests  2 719 ✅ 0 💤 0 ❌
47 253 runs  47 253 ✅ 0 💤 0 ❌

Results for commit 7f6cce8.

♻️ This comment has been updated with latest results.

@pcanal
Copy link
Member

pcanal commented Feb 7, 2025

Part of my confusion is that #12510 is indeed about a file with branches and no entries.
In this case, the problem is not that there is missing branches or missing entries but rather that the first entry (number 0) in the output file is corrupted (has the wrong values).

With the files https://github.com/root-project/root/files/11333378/dummyFiles.zip, we get:

$ root.exe -b -l -q dummyFiles/dummyFile0.root -e 'dummyTree->Scan("*","","",2,0);'

Attaching file dummyFiles/dummyFile0.root as _file0...
(TFile *) 0x22986b0
************************************
*    Row   * index.ind * dummyVari *
************************************
*        0 *         0 * 0.2612867 *
*        1 *         1 * 0.7484587 *
************************************
$ hadd -f dummyFile10.root dummyFiles/dummyFile1.root dummyFiles/dummyFile0.root 
Info in <hadd>: target file: dummyFile10.root
Info in <hadd>: compression setting for all output: 101
hadd Source file 1: dummyFiles/dummyFile1.root
hadd Source file 2: dummyFiles/dummyFile0.root
hadd Target path: dummyFile10.root:/
$ root.exe -b -l -q dummyFile10.root -e 'dummyTree->Scan("*","","",2,0);'

Attaching file dummyFile10.root as _file0...
(TFile *) 0x2f39310
************************************
*    Row   * index.ind * dummyVari *
************************************
*        0 *         0 * 2.13e-306 *
*        1 *         1 * 0.7484587 *
************************************

I.e. Row 0 is wrong.

Nonetheless, it might (or might not) be fixed by this PR, in particular in my last proposal the part:

     if (newtree && nentries == 0)
        continue; // If we already have the structure and we have no entry, save time and skip

i.e. cloning the structure even if there is no entries, might solve the issue.

@ferdymercury ferdymercury changed the title [TTree] when merging, do not clone branch structure if no entries [TTree] when merging, ignore trees without branches Feb 8, 2025
@ferdymercury ferdymercury marked this pull request as ready for review February 8, 2025 15:56
@ferdymercury ferdymercury requested a review from pcanal February 12, 2025 15:46
[TTree] when merging, do not clone branch structure if no entries

Fixes root-project#14558
Fixes root-project#12510

[test] add fully qualified tree

[tree] skip trees with zero branches in static merge

By pcanal

[test] amend tests with cases

[tree] restore prev behavior

[tree] skip "this" when Merge starts with empty-branch tree

as implemented by pcanal

[skip-ci] clarify comments

[tree] early exit if all empty

[test] fix merge substitution
Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@pcanal pcanal merged commit 57425dd into root-project:master Feb 28, 2025
20 of 21 checks passed
@ferdymercury ferdymercury deleted the emptytree branch March 1, 2025 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

[ROOT-4716] TTree merging problems when including empty trees Issue with hadd when first file has empty tree
2 participants