Fix calculation of speed in CloseByParticleGunProducer#50241
Fix calculation of speed in CloseByParticleGunProducer#50241felicepantaleo wants to merge 6 commits intocms-sw:masterfrom
Conversation
|
@cmsbuild please test |
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50241/48231 |
|
A new Pull Request was created by @felicepantaleo for master. It involves the following packages:
@cmsbuild, @lviliani, @mkirsano, @sensrcn, @theofil can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
I performed the validation of the proposed fix. Time Validation
Response Validation (Run3 2025, no PU):
Both the CloseBy timing and response now behave as expected. See below the same plots for a SingleElectron gun for comparison (the high-energy tail is likely due to interactions with the tracker): 
|
|
+1 Size: This PR adds an extra 24KB to repository Comparison SummarySummary:
|
| @@ -239,11 +239,12 @@ void CloseByParticleGunProducer::produce(Event& e, const EventSetup& es) { | |||
|
|
|||
| // compute correct path assuming uniform magnetic field in CMS | |||
| double pathLength = 0.; | |||
| const double speed = p.pz() / p.e() * c_light / CLHEP::cm; | |||
| const double pabs = std::sqrt(p.px() * p.px() + p.py() * p.py() + p.pz() * p.pz()); | |||
There was a problem hiding this comment.
One could use p.mag() instead of pabs [reference].
There was a problem hiding this comment.
After some discussion with @AuroraPerego, we realized there is ambiguity in the definitions of px() and x() (and the same for the other components). Still, using .mag() for the case at hand should be correct.
To be noted that here we should use .setPx() and friends instead of .setX() and friends, for clarity. They do exactly the same operation, though.
There was a problem hiding this comment.
hi @bfonta
mag() was replaced by rho() in the version of hepmc we are using in cmssw. std::hypot is more stable, I moved to use that.
There was a problem hiding this comment.
@felicepantaleo Note that std::hypot will be slower than std::sqrt(x*x + y*y) due to overflow and underflow guarantees.
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-approved
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50241/48260 |
|
@cmsbuild please test |
|
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. @ftenchini, @mandrenguyen, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
|
@felicepantaleo The problem is not who can merge the PR. The problem is it has to be fully signed before it can be merged. This is done now, so once you fix the conflicts, we will merge it (assuming the tests still pass). |
felicepantaleo
force-pushed
the
fix_closeby_time_16_1_0_pre2
branch
from
a50d0b0 to
0c0a881
Compare
|
@cmsbuild please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50241/49141 |
|
+1 Size: This PR adds an extra 24KB to repository Comparison SummarySummary:
|
|
@bfonta can you post again the plots with this rebase? |
|
Yes, but only once I've understood the segmentation violation that arises when fixing the conflicts in #50292. |
6 participants
This PR fixes the time-of-flight calculation in
CloseByParticleGunProducerby using the full particle speed (|p|/E * c) instead of the z-velocity component (pz/E * c).Using
pz/Eintroduced an unphysical dependence on the sign ofpz, which can lead to asymmetries in timing vsη.The patch also updates the magnetic bending radius calculation to use the momentum magnitude and absolute charge.
This makes the path-length/time computation physically consistent for charged particles.
No interface changes; only the internal timing/path calculation is corrected.
@bfonta @AuroraPerego