-
Notifications
You must be signed in to change notification settings - Fork 54
Stop pretending to simulate the PPS #672
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
Conversation
marcodeltutto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense :D Thank you!
|
trigger build LArSoft/lar*@LARSOFT_SUITE_v10_04_03 SBNSoftware/sbnanaobj@v09_23_03 SBNSoftware/sbnobj@v10_00_04 SBNSoftware/sbncode@v10_04_03 |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build SBND phase logs parent CI build details are available through the CI dashboard |
|
🚨 For more details about the warning phase, check the ci_tests SBND phase logs parent CI build details are available through the CI dashboard |
|
@henrylay97 can you verify these product size changes are expected? |
|
Agree - I would naively not expect any change so will investigate. |
|
Recreated offline with same differences. The reason for the difference is the removal of the random throw line. There are other random throw lines in the algorithm (see L540-547 and L573-580) which replicate the resolution of our understanding of the detector effects. These random throws do impact the creation of AuxDetIDEs & FEBDatas. By removing one of the random throws we change the evolution of the set seed in the CI and thus the output of the other throw lines. TL;DR this is understood and is okay. |
|
Approved |
Description
We don't need to pretend to simulate the PPS - assigning it to a random value for every piece of activity doesn't replicate what actually happens in data.
In data reconstruction we reference the T0 to the event trigger, thus replicating closely what T1 represents in the simulation. In aid of simplifying concepts and making data / MC comparisons easier I am suggesting we move to make T0 & T1 the same in MC.
Checklist
Reviewers,AssigneesDevelopementRelevant PR links (optional)
N/A
Link(s) to docdb describing changes (optional)
N/A