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

Update EPD hit IO classes to include the DEP information #501

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

plexoos
Copy link
Member

@plexoos plexoos commented Mar 17, 2023

No description provided.

@plexoos plexoos requested a review from klendathu2k as a code owner March 17, 2023 14:42
Copy link
Contributor

@klendathu2k klendathu2k left a comment

Choose a reason for hiding this comment

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

I don't see any issues with the code, save for it breaking StEpdHitMaker. Please revise StEpdHitMaker (and any other dependencies) to reflect the changes made to the constructor, and submit as part of this PR.

@plexoos
Copy link
Member Author

plexoos commented Mar 17, 2023

I don't see any issues with the code,

Do you know if the automatic ROOT schema evolution can handle a renamed data member?

@klendathu2k
Copy link
Contributor

Now that you mention it... I'm not certain. So probably better to leave the name of the pre-existing data members in tact. Otherwise need to test.

@plexoos
Copy link
Member Author

plexoos commented Mar 17, 2023

I think so too, it is safer to keep the old name. I did a test once removing and adding members and it worked well but renaming may need some magic with pragma in LinkDef.h

@genevb
Copy link
Contributor

genevb commented Mar 17, 2023

Do you know if the automatic ROOT schema evolution can handle a renamed data member?

Good catch! I think if you write your own Streamer(), there is probably a way to do this, but I believe that incrementing the ClassDef otherwise makes it think a member with one name has been dropped and a member with a different name has been added and should be filled with default zeroes when reading a file with an old version.

@plexoos plexoos changed the title Update StEpdHit to include the DEP information Update EPD hit IO classes to include the DEP information Mar 24, 2023
@plexoos plexoos marked this pull request as draft March 24, 2023 21:17
@akioogawa
Copy link
Contributor

At few places in comments, it says "DEP data from TriggerData". DEP data is not in Trigger data but in FCS data.

@Femtoscopist
Copy link

Femtoscopist commented Aug 18, 2023 via email

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.

5 participants