Fix QuickTrackAssociatorByHits for composite hits #20364

makortel · 2017-09-04T09:39:15Z

This PR fixes a long-standing bug in QuickTrackAssociatorByHits regarding composite hits (=hits having >= 2 clusters). Currently the "matched hit fraction" is in fact calculated as

N(clusters of track matched to a TrackingParticle)
--------------------------------------------------
                N(hits of track)

so hits with 2 cluster are ounted as 2 in the numerator and 1 in denominator. The bug does not affect the offline track monitoring as in offline the strip matched hits are split in the final fit. The affected cases are

HLT track monitoring (strip matched hits are not split)
Strip seed monitoring (strip matched hits are not split)
VectorHits when they get integrated (in fact this was the context that revealed the bug)

The fix is to change the denominator to N(clusters of track) so one could say that the association is now actually done "by clusters". Note that this is done only for ClusterTPAssociation mode, the TrackerHitAssociator mode is kept in counting hits.

Here are MTV plots for phase1
https://cms-tracking-validation.web.cern.ch/cms-tracking-validation/PR/CMSSW_9_3_0_pre3_PR20364/
showing no effect for tracks
https://cms-tracking-validation.web.cern.ch/cms-tracking-validation/PR/CMSSW_9_3_0_pre3_PR20364/phase1_ttbar_pu35_summary/summary.pdf
and decrease in efficiency of seeds with strip hits
https://cms-tracking-validation.web.cern.ch/cms-tracking-validation/PR/CMSSW_9_3_0_pre3_PR20364/phase1_ttbar_pu35_seeding_summary_seeds/summary.pdf

Plots for HLT (thanks to @JanFSchulte) can be found from
https://cms-tracking-validation.web.cern.ch/cms-tracking-validation/PR/CMSSW_9_3_0_pre3_PR20364_HLT/
showing decrease in efficiency (especially in |eta| > 1) and increase in fake rates.

Tested in 9_3_0_pre3, expecting no changes in offline track plots, and decrease in efficiency and increase in fake rates for HLT track plots.

@rovere @VinInn @ebrondol @JanFSchulte @mtosi @slava77 @perrotta

…instead of hits Otherwise composite hits will always be treated as "duplicates".

cmsbuild · 2017-09-04T09:39:37Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-09-04T09:39:38Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

SimTracker/TrackAssociatorProducers

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@sdevissc, @GiacomoSguazzoni, @rovere, @VinInn, @ebrondol, @threus, @dgulhan this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

cmsbuild · 2017-09-04T09:42:49Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20364/478

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-20364/478/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20364/478/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly (this will soon be required for PRs to be considered)

cmsbuild · 2017-09-04T09:51:19Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-09-04T09:51:19Z

Pull request #20364 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again.

cmsbuild · 2017-09-04T09:52:56Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20364/479

makortel · 2017-09-04T09:54:23Z

@cmsbuild, please test

cmsbuild · 2017-09-04T09:54:42Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22688/console Started: 2017/09/04 11:55

cmsbuild · 2017-09-04T10:57:26Z

-1

Tested at: d3e1f4a

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
0770659
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20364/22688/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20364/22688/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20364/22688/summary.html

I found follow errors while testing this PR

Failed tests: RelVals AddOn

RelVals:

When I ran the RelVals I found an error in the following worklfows:
5.1 step1

runTheMatrix-results/5.1_TTbar+TTbarFS+HARVESTFS/step1_TTbar+TTbarFS+HARVESTFS.log

135.4 step1

runTheMatrix-results/135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step1_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log

AddOn:

I found errors in the following addon tests:

cmsDriver.py TTbar_13TeV_TuneCUETP8M1_cfi --conditions auto:run2_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot NominalCollision2015 --era Run2_25ns : FAILED - time: date Mon Sep 4 12:32:50 2017-date Mon Sep 4 12:29:10 2017 s - exit: 35584
cmsDriver.py TTbar_13TeV_TuneCUETP8M1_cfi --conditions auto:run2_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot NominalCollision2015 --era Run2_2016 : FAILED - time: date Mon Sep 4 12:33:36 2017-date Mon Sep 4 12:29:11 2017 s - exit: 35584
cmsDriver.py TTbar_8TeV_TuneCUETP8M1_cfi --conditions auto:run1_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot Realistic8TeVCollision : FAILED - time: date Mon Sep 4 12:33:36 2017-date Mon Sep 4 12:29:29 2017 s - exit: 35584

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
0770659
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20364/22688/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20364/22688/git-merge-result

cmsbuild · 2017-09-04T10:57:28Z

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

cmsbuild · 2017-09-04T14:53:56Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-09-04T14:53:57Z

Pull request #20364 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again.

cmsbuild · 2017-09-04T14:56:03Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20364/489

makortel · 2017-09-04T14:57:04Z

@cmsbuild, please test

Fixed FastSim by keeping TrackerHitAssociator-mode counting hits. I start to feel, however, that a more proper way would be to separate the counting-by-clusters with ClusterTPAssociation to a separate module (e.g. "QuickTrackAssociatorByClusters"), but for such refactoring I prefer a separate PR.

cmsbuild · 2017-09-04T14:57:20Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22695/console Started: 2017/09/04 16:57

cmsbuild · 2017-09-04T16:29:23Z

+1
Tested at: 330377f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20364/22695/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
0770659
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20364/22695/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20364/22695/git-merge-result

cmsbuild · 2017-09-04T16:29:26Z

Comparison job queued.

cmsbuild · 2017-09-04T18:07:58Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20364/22695/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 26
DQMHistoTests: Total histograms compared: 2656522
DQMHistoTests: Total failures: 2688
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2653645
DQMHistoTests: Total skipped: 189
DQMHistoTests: Total Missing objects: 0
Checked 107 log files, 14 edm output root files, 26 DQM output files

civanch · 2017-09-04T20:24:35Z

+1

cmsbuild · 2017-09-04T20:24:55Z

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, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2)

makortel · 2017-09-05T08:06:17Z

I wonder why there are changes in PixelPhase1V/TrackingParticle? I took a look on the code Validation/SiPixelPhase1TrackingParticleV/src/SiPixelPhase1TrackingParticleV.cc but didn't spot any obvious reason as nothing seems to depend on the track<->TrackingParticle association. @fioriNTU any ideas?

davidlange6 · 2017-09-05T09:50:13Z

+1

ebrondol and others added 3 commits August 12, 2017 13:34

using numberOfValidTrackClusters instead of numberOfValidTrackHits

287c26a

consistent nomenclature

963631f

In getDoubleCount, count the number of duplicate cluster->TP matches …

1a8c051

…instead of hits Otherwise composite hits will always be treated as "duplicates".

cmsbuild added this to the CMSSW_9_3_X milestone Sep 4, 2017

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures simulation-pending tests-pending labels Sep 4, 2017

cmsbuild added code-checks-rejected and removed code-checks-pending labels Sep 4, 2017

Code style checks

d3e1f4a

cmsbuild added code-checks-pending and removed code-checks-rejected labels Sep 4, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 4, 2017

cmsbuild added tests-started and removed tests-pending labels Sep 4, 2017

cmsbuild removed the comparison-pending label Sep 4, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 4, 2017

cmsbuild added tests-started and removed tests-pending labels Sep 4, 2017

cmsbuild added tests-approved and removed tests-started labels Sep 4, 2017

cmsbuild added comparison-available and removed comparison-pending labels Sep 4, 2017

cmsbuild added fully-signed simulation-approved and removed pending-signatures simulation-pending labels Sep 4, 2017

cmsbuild added orp-approved and removed orp-pending labels Sep 5, 2017

cmsbuild merged commit 4998ca6 into cms-sw:master Sep 5, 2017

ebrondol mentioned this pull request Sep 12, 2017

adding LogError for quality computation #20469

Merged

makortel mentioned this pull request Sep 15, 2017

Refactor track MVA classifier code #20429

Merged

makortel deleted the ebrondol_tracking_fixQualityClus branch February 12, 2018 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix QuickTrackAssociatorByHits for composite hits #20364

Fix QuickTrackAssociatorByHits for composite hits #20364

makortel commented Sep 4, 2017 •

edited

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

makortel commented Sep 4, 2017

cmsbuild commented Sep 4, 2017 •

edited

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

makortel commented Sep 4, 2017

cmsbuild commented Sep 4, 2017 •

edited

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

civanch commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

makortel commented Sep 5, 2017

davidlange6 commented Sep 5, 2017

Fix QuickTrackAssociatorByHits for composite hits #20364

Fix QuickTrackAssociatorByHits for composite hits #20364

Conversation

makortel commented Sep 4, 2017 • edited

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

makortel commented Sep 4, 2017

cmsbuild commented Sep 4, 2017 • edited

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

makortel commented Sep 4, 2017

cmsbuild commented Sep 4, 2017 • edited

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

civanch commented Sep 4, 2017

cmsbuild commented Sep 4, 2017

makortel commented Sep 5, 2017

davidlange6 commented Sep 5, 2017

makortel commented Sep 4, 2017 •

edited

cmsbuild commented Sep 4, 2017 •

edited

cmsbuild commented Sep 4, 2017 •

edited