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

Add track tree writer #503

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

beomki-yeo
Copy link
Contributor

@beomki-yeo beomki-yeo commented Nov 27, 2023

This PR adds an event tree writer to records the value of various properties (truth particle information, fitting information, etc.)

One of the motivations is to compare the fitted momentum distribution between CPU and GPU:

image

In this PR, I only added trees for truth particle information and fitting results. We can add more in the future if necessary

@beomki-yeo beomki-yeo marked this pull request as draft November 27, 2023 23:22
@beomki-yeo beomki-yeo marked this pull request as ready for review November 28, 2023 18:41
@beomki-yeo beomki-yeo changed the title WIP: Add event tree writer Add event tree writer Nov 28, 2023
@beomki-yeo
Copy link
Contributor Author

@krasznaa Could you take a look?

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

🤔 I believe the code is a bit too over-complicated right now.

But let's start at the beginning. Let's not use a generic name like event_tree_writer for this class. It is doing a very specific thing. It writes data about the truth and reconstructed tracks. So let's call it something like: traccc::root::tracking_performance_file_writer. (I'm very open to a better name. My point is just to give it a little more concrete of a name.)

Then... I am on board with the general API of the current traccc::event_tree_writer class. But its implementation is unnecessarily complicated. Just make traccc::details::event_tree_writer_data (or whatever you rename it to) do the heavy lifting itself. There's no need for an extra layer of classes under that data class. It can just be the "data class", which itself would both hold the primitive variables that the output TTree is written with, and the code that deals with the TTree.

The "cache concept" for this writer is just not necessary. 🤔

But sure, with a better name, and some simplification, I'm very open to adding this to the project.

@beomki-yeo beomki-yeo changed the title Add event tree writer Add track tree writer Apr 13, 2024
@beomki-yeo
Copy link
Contributor Author

Thanks for the comments! I simplified the code now with different name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants