-
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
Change pT5 rz chi2 cut definition into helix approximation (rebase PR40) #128
Change pT5 rz chi2 cut definition into helix approximation (rebase PR40) #128
Conversation
/run all |
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:
|
The PR was built and ran successfully with CMSSW. Here are some plots. OOTB All TracksThe full set of validation and comparison plots can be found here. |
@@ -641,10 +642,9 @@ namespace ALPAKA_ACCELERATOR_NAMESPACE::lst { | |||
float diffz1 = alpaka::math::abs(acc, solz1 - zsi) * 100; | |||
float diffz2 = alpaka::math::abs(acc, solz2 - zsi) * 100; | |||
diffz = alpaka::math::min(acc, diffz1, diffz2); | |||
residual = diffz; |
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.
It is a bit weird that this line and the Line 628 are added into this PR, it seems pure pT3 rzchi2 work.
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.
Thank you, @YonsiG. Seems like I might have messed something up. I'll fix it.
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.
Actually, if you look at #40, those changes were made for pT3 rzchi2. Either way, those changes shouldn't affect anything.
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, those are for pT3 rzchi2. The order doesn't affect much as you said.
I think to reproduce PR #40 , this PR looks good to me. The plot before this PR baseline is a bit different. Other than that, this PR should be good to merge. |
…x approximation Co-authored-by: YonsiG <[email protected]>
fae9939
to
d343907
Compare
I switched the changes to /run standalone |
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:
|
Thank you Andres, these looks good and I will merge it |
2e0a3c1
into
CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel_rebased
This is a rebase of PR #40. Since it changes the performance I made it as its own PR so that we can compare the plots with the original PR.