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

[ntuple] Add a join processing tutorial #17137

Merged
merged 2 commits into from
Nov 30, 2024

Conversation

enirolf
Copy link
Contributor

@enirolf enirolf commented Nov 29, 2024

This PR adds a tutorial for using the RNTupleProcessor to join two RNTuples. Once RNTupleProcessors are fully composable (see #17132), we might consider merging it with ntpl012_processor_chain.C.

@enirolf
Copy link
Contributor Author

enirolf commented Nov 29, 2024

The failing clang-format is not actually a formatting failure (not sure why the job failed..). I ran clang-format locally before pushing, so that should all be ok 🤷‍♀️.

Copy link

github-actions bot commented Nov 29, 2024

Test Results

    18 files      18 suites   4d 2h 42m 38s ⏱️
 2 688 tests  2 687 ✅ 0 💤 1 ❌
46 547 runs  46 546 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 2d11817.

♻️ This comment has been updated with latest results.

Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

Nice thanks! Some minor comments from my side

tutorials/v7/ntuple/ntpl012_processor_chain.C Outdated Show resolved Hide resolved
tutorials/v7/ntuple/ntpl015_processor_join.C Outdated Show resolved Hide resolved
/// \macro_code
///
/// \date November 2024
/// \author The ROOT Team
Copy link
Member

Choose a reason for hiding this comment

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

I would say your name is better suited here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All other tutorials also have "The ROOT team", in this case I would prefer to keep this consistency.

auto ntuple = RNTupleWriter::Recreate(std::move(model), ntupleName, ntupleFileName);

// The main ntuple only contains a subset of the entries present in the auxiliary ntuple.
for (int i = 0; i < kNEvents; i += gRandom->Integer(5)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would say we don't want to have a random number of entries in a tutorial?

// Fields from the main ntuple are accessed by their original name.
px = *entry.GetPtr<float>("vpx");
// Fields from auxiliary ntuples are accessed by prepending the name of the auxiliary ntuple.
py = *entry.GetPtr<float>(kAuxNTupleName + ".vpy");
Copy link
Member

Choose a reason for hiding this comment

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

But in this case it could also be accessed without the auxiliary name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No; in the current implementation fields in auxiliary fields always have to be prefixed even if they are unique. Apart from the fact that it would be non-trivial to change this I'm also not sure if we would even want that, I think it would only lead to unwanted ambiguities.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, this is a different behaviour than TTree. Not wrong per se, but we can keep it in mind if discussion arises at some point

tutorials/v7/ntuple/ntpl015_processor_join.C Outdated Show resolved Hide resolved
A tutorial for join-based processing is currently provided in
`ntpl015_processor_join.C`.
@enirolf enirolf force-pushed the ntuple-join-tutorial branch from 31f74e0 to 2d11817 Compare November 29, 2024 16:31
@enirolf enirolf requested a review from vepadulano November 29, 2024 16:32
Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

Thank you!

@enirolf enirolf merged commit b8a8f39 into root-project:master Nov 30, 2024
18 of 21 checks passed
@enirolf enirolf deleted the ntuple-join-tutorial branch December 2, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants