Remove check on TrackingParticle::numberOfHits()==0 from QuickTrackAssociatorByHits #17338
Conversation
…yHits In TrackingTruthAccumulator, the numberOfHits is calculated from a subset of the SimHits of the SimTracks of a TrackingParticle (essentially limiting the processType and particleType to those of the "first" hit, and particleType to the pdgId of the SimTrack). But, here the association between tracks and TrackingParticles is done with *all* the hits of TrackingParticle, so we should not rely on the numberOfHits() calculated with a subset of SimHits.
|
A new Pull Request was created by @makortel (Matti Kortelainen) for CMSSW_9_0_X. It involves the following packages: SimTracker/TrackAssociatorProducers @civanch, @mdhildreth, @dmitrijus, @cmsbuild, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
|
Not sure if the |
|
+1 |
|
Comparison job queued. |
|
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
|
|
+1 |
cmsbuild
added
fully-signed
simulation-approved
and removed
pending-signatures
simulation-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
|
+1 |
|
The "another PR" mentioned in the PR description is #17382. |
Currently
QuickTrackAssociatorByHitsignores TrackingParticles withnumberOHits()==0from track-TrackingParticle association. While this seems like a nice performance optimization, it does cause some track-TrackingParticle pairs to not be associated because of how thenumberOfHits()is calculated inTrackingTruthAccumulator:particleTypeas theSimTrack'spdgIdand the sameparticleTypeandprocessTypeas the "first hit" are countedfixing that issueimproving the situation, the PR is Fix calculation of TrackingParticle::numberOfHits() #17382).Because of 1, I propose to include also TrackingParticles with
numberOfHits()==0in the track-TrackingParticle association, because the association is (effectively) done with all SimHits of a TrackingParticle (via the Pixel/StripDigiSimLinks)."Accidentally", this PR also fixes the problem caused of 2.
Here are MTV plots in CMSSW_9_0_X_2017-01-23-1100 for ttbar+35PU for phase0, phase1, and phase1 with CA seeding, and CMSSW_9_0_0_pre2 for phase2 (D4) ttbar+PU200
https://mkortela.web.cern.ch/mkortela/tracking/validation/CMSSW_9_0_X_phase2debug/
and here for phase2 (D4) ttbar noPU in CMSSW_9_0_X_2017-01-23-1100 (thanks to @ebrondol)
http://ebrondol.web.cern.ch/ebrondol/Phase2Tracking/fixFakes/20170125_TTbar/
The effect on efficiency in phase0, phase1, phase2 PU200 is tiny; phase2 noPU shows few percent increase in |eta| 2-3. The effect on fake rate in phase0, phase1, phase2 PU200 is a 1-2 % overall decrease (can be more in specific phase space regions); phase2 noPU shows ~40 % overall decrease.
Tested in CMSSW_9_0_X_2017-01-23-1100 and CMSSW_9_0_0_pre2, expecting changes like above in the track-TrackingParticle association (no change expected e.g. on RECO itself).
@VinInn @rovere @ebrondol @slava77