-
Notifications
You must be signed in to change notification settings - Fork 1
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
Updated Occupancies #147
base: master
Are you sure you want to change the base?
Updated Occupancies #147
Conversation
/run all |
There was a problem while building and running with CMSSW. The logs can be found here. |
@ariostas Seems like an issue with the CMSSW tests? Something about alpaka math? |
The PR was built and ran successfully in standalone mode. Here are some of the comparison plots. The full set of validation and comparison plots can be found here. Here is a timing comparison:
|
/run standalone lowpt |
Oh that's a package that was recently introduced by Manos. You could cherry-pick that commit or see if by early next week the CMSSW PR finally gets merged so that you can rebase |
The PR was built and ran successfully in standalone mode (low pT setup). Here are some of the comparison plots. The full set of validation and comparison plots can be found here. Here is a timing comparison:
|
constexpr int p08_occupancy_matrix[4][4] = { | ||
{336, 414, 231, 146}, // category 0 | ||
{161, 129, 135, 132}, // category 0 |
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.
this is a pretty large drop: does this rely on the pending T3 DNN?
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.
I also don't think the T3 DNN will affect the T5 occupancies much. The T5 fake rate is already pretty low compared to the T3 fake rate, so a cleaner set of T3 candidates shouldn't lower the T5 occupancies much.
edit: Maybe this is from the T5 DNN? These occupancies were selected before the T5 DNN was added to the code. The location seems correct.
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.
" 'md_occupancies', 'sg_occupancies', 't3_occupancies', 't5_occupancies']\n", | ||
"\n", | ||
"# Root file generated with compile -d option turned on to generate relevant occupancy branches\n", | ||
"file_path = \"occ_1000_p06.root\"\n", |
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.
am I guessing correctly that the old derivation was based on 1000 events and the new on 500?
Considering some relatively large changes at 99.99% quantile, I suspect that this reduction in the occupancies may be chasing some tails of occasional combinatorial explosions. Are there always enough entries in the remaining 0.01% to make the decision? Something >>10 (e.g. > 30) would be good.
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.
Yes, in the slides Yonsi sent me it said she used a 500 event sample for the 0.8 occupancies, so I tried to match that here. I will check.
{0, 38, 46, 39} // category 3 | ||
{668, 271, 105, 59}, // category 0 | ||
{738, 310, 0, 0}, // category 1 | ||
{0, 13, 5, 0}, // category 2 |
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.
which kinematic regions are these 4 bins? (to follow up on the discussion during the meeting for why earlier zeroes are now not)
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.
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.
this still requires parsing: what are the two middle elements out of 4 in the category 2?
Is Category 2 defined by radius and z range or by the disk and ring index in the endcap?
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.
Definition of Category 2: (module_layers >= 4) & (module_subdets == 5)
Definition of Eta ranges:
eta_numbers[module_eta < 0.75] = 0
eta_numbers[(module_eta >= 0.75) & (module_eta < 1.5)] = 1
eta_numbers[(module_eta >= 1.5) & (module_eta < 2.25)] = 2
eta_numbers[(module_eta >= 2.25) & (module_eta < 3)] = 3
2f6fd57
to
9893cdf
Compare
/run all |
There was a problem while building and running with CMSSW. The logs can be found here. |
The PR was built and ran successfully in standalone mode. Here are some of the comparison plots. The full set of validation and comparison plots can be found here. Here is a timing comparison:
|
/run standalone lowpt |
The PR was built and ran successfully in standalone mode (low pT setup). Here are some of the comparison plots. The full set of validation and comparison plots can be found here. Here is a timing comparison:
|
01dceba
to
f81b81b
Compare
/run standalone |
There was a problem while building and running in standalone mode. The logs can be found here. |
f81b81b
to
9893cdf
Compare
Moving the dynamic memory allocation to #148 |
I think it makes sense to merge #148 first and then quickly reevaluate the occupancy thresholds. It's likely we can increase the caps now without a huge increase in memory in order to decrease truncation. |
Updated occupancies for all LST objects, and small updates to the notebook for printing them.
This PR Timing (0.8 GeV):
Original Timing (0.8 GeV):
This PR Timing (0.6 GeV):
Original Timing (0.6 GeV):