-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix cluster-to-TrackingParticle associations for duplicate entries #19637
Conversation
The duplicates cause problems in certain kind of bookkeeping.
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages: SimTracker/TrackerHitAssociation @cmsbuild, @civanch, @mdhildreth, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild, please tesst |
@cmsbuild, please test (hopefully without the typo this time...) |
The tests are being triggered in jenkins. |
type bug-fix |
-1 Tested at: 734f8c6 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see the results of the tests here: I found follow errors while testing this PR Failed tests: AddOn
I found errors in the following addon tests: cmsDriver.py TTbar_Tauola_13TeV_TuneCUETP8M1_cfi -s GEN,SIM,DIGI,L1,DIGI2RAW --mc --scenario=pp -n 10 --conditions auto:run2_mc_GRun --relval 9000,50 --datatier "GEN-SIM-RAW" --eventcontent RAWSIM --customise=HLTrigger/Configuration/CustomConfigs.L1T --era Run2_2017 --magField 38T_PostLS1 --fileout file:RelVal_Raw_GRun_MC.root : FAILED - time: date Mon Jul 10 15:26:18 2017-date Mon Jul 10 15:24:41 2017 s - exit: 21248 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
@cmsbuild, please test Let's try again |
The tests are being triggered in jenkins. |
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+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. @davidlange6, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
I noticed the
ClusterTPAssociation
can contain multiple associations from a cluster to a single TrackingParticle. This can happen despite ofClusterTPAssociationProducer
already having already a logic for adding only one entry for each cluster-to-SimTrack association. But electron TrackingParticles can consist of multiple SimTracks, and therefore the existing logic is not enough. I decided to enhance the sort comparison function and usestd::unique
at the end as that's likely faster than doing any additional existence checks before the insertion, especially given that it does not happen very often.While e.g. the track-to-TrackingParticle association (i.e.
QuickTrackAssociatorByHits
) is not affected, some other analyses may be affected because of the duplicated entries. E.g. here is one affected piece from standard workflows (FYI @smoortga)https://github.com/cms-sw/cmssw/blob/master/Validation/RecoB/plugins/BDHadronTrackMonitoringAnalyzer.cc#L218-L246
(this PR provides a bug fix and in all cases the number of clusters / TrackingParticle should decrease)
Tested in CMSSW_9_1_0_pre3, expecting changes in the BDHadron monitoring.
@rovere @VinInn