-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
correct the time propagation and add options in CloseByParticleGunProducer #42487
correct the time propagation and add options in CloseByParticleGunProducer #42487
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42487/36493
|
A new Pull Request was created by @AuroraPerego (Aurora Perego) for master. It involves the following packages:
@SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @alberto-sanchez, @menglu21, @GurpreetSinghChahal can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cc8299/34141/summary.html Comparison SummarySummary:
|
@SiewYan @mkirsano @Saptaparna @alberto-sanchez @menglu21 @GurpreetSinghChahal a kind reminder in case you forgot to sign this. We need it for a sample production. |
@AuroraPerego I apologize if this is simple PR is not getting even looked at for the last 2 months. @cms-sw/orp-l2 is there any way we can proceed with this? |
double pathLength = 0.; | ||
const double speed = p.pz() / p.e() * c_light / cm; | ||
if (PData->charge()) { | ||
const double radius = sqrt(p.px() * p.px() + p.py() * p.py()) * 87.78f; // cm |
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.
To be extremely picky:
- The math is wrong for loopers, right? Maybe they are not relevant in general, but it would be wise to say something or put some protection or do the math right for all cases.
- Would it make sense to ask the value of the magnetic field and act accordingly? With the code as is, all generations with anything but 3.8T will be wrong.
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've added a check for loopers and now the magnetic field is taken from the Event Setup
Added error when energy is less than 1 GeV, time may not be accurate in this case (e.g. loopers)
1d8b2b9
to
34c93e0
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42487/37061
|
Pull request #42487 was updated. @cmsbuild, @menglu21, @SiewYan, @GurpreetSinghChahal, @bbilin, @alberto-sanchez, @mkirsano can you please check and sign again. |
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cc8299/34986/summary.html Comparison SummarySummary:
|
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Before this PR the time was propagated back to the vertex in a straight line at the speed of light.
These changes propagate the time back at the speed of the particle and following a curved path if the particle is charged.
In addition to that, two options have been added:
By default both the offset and the dt are set to 0.
The
fillDescriptions
method has been added as well.All of this is used in studies regarding timing in HGCAL.
PR validation:
Tested on CloseBy photons and pions.
@waredjeb @felicepantaleo @rovere