Speed up Phase2TrackerClusterizer by a factor 1000 #16815

VinInn · 2016-11-30T13:13:25Z

Introduced a Trivial sequential algorithm acting on pre-sorted Digis
Verified comparing cluster by cluster for 10 ttbar events w/r/t the previous algorithm (see code
protected by VERIFY_PH2_TK_CLUS)

Regressions:
the limit on the size of the cluster has been removed: better to split using track-info if needed, than at random after N strips...

a bug in the previous algorithm (affecting HIP bit) has been fixed

N.B.
operator< in Phase2TrackerDigi has been modified to ignore the ot_bit
this affects Step2 as well

side gains : the algo is now stateless

cmsbuild · 2016-11-30T13:13:42Z

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_9_0_X.

It involves the following packages:

DataFormats/Phase2TrackerDigi
RecoLocalTracker/SiPhase2Clusterizer

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @gpetruc, @threus this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

VinInn · 2016-11-30T13:14:18Z

RecoLocalTracker/SiPhase2Clusterizer/plugins/Phase2TrackerClusterizerAlgorithm.cc

@@ -71,6 +71,7 @@ void Phase2TrackerClusterizerAlgorithm::clusterizeDetUnit(const edm::DetSet< Pha
                sizeCluster = 0;
                // Increase the number of clusters
                ++numberClusters;
+                HIPbit = false;


Thanks!... my bad.

VinInn · 2016-11-30T13:14:48Z

@cmsbuild , please test

cmsbuild · 2016-11-30T13:15:05Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16682/console

VinInn · 2016-11-30T13:38:01Z

@boudoul , apparently not manager of the whole RecoLocalTracker

cmsbuild · 2016-11-30T14:48:16Z

+1
Tested at: 0d28f30
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16815/16682/summary.html

cmsbuild · 2016-11-30T14:48:19Z

Comparison job queued.

cmsbuild · 2016-11-30T15:23:32Z

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

The workflows 1003.0, 1001.0, 1000.0, 140.53, 136.731, 4.22 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

slava77 · 2016-12-10T01:40:57Z

RecoLocalTracker/SiPhase2Clusterizer/python/phase2TrackerClusterizer_cfi.py

@@ -3,7 +3,7 @@
 # Clusterizer options
 siPhase2Clusters = cms.EDProducer('Phase2TrackerClusterizer',
    src = cms.InputTag("mix", "Tracker"),
-    maxClusterSize = cms.uint32(8),
+    maxClusterSize = cms.uint32(0), # was 8


if I'm not mistaken, this is a bogus parameter now (unless VERIFY_PH2_TK_CLUS is set).
It may be worth to leave a note about it here.

slava77 · 2016-12-10T17:33:02Z

Technical performance improves as expected.
In PU200 time of siPhase2Clusters reduced by ~x400.

Curiously, we have an increase in the number of clusters overall,
including a pretty significant increase in size=1 clusters in addition to appearance of longer clusters

as a more extreme ttbar PU200 in D4 (I only ran trackerlocalreco, didn't check the performance in higher level products)
[the plot is inverted: black is new with this PR and red is the baseline; done in 900pre1]

there are ~20% more size=1
also, we have clusters up to size 500.

It's unclear from the PR description if size=1 cluster increase is expected.

Given that there is no cluster splitting currently in place, it seems like in busy events in jets we will start loosing efficiency. This effect for the OT is probably small enough that this will not be an issue (even high-pt jets fan out fairly well and will not hit the same module).

What are the limits for clustering size on the planned readout side (IIRC, clustering is done on the detector/DAQ)?

I need some feedback before signing off (mainly concerning size=1).

VinInn · 2016-12-11T06:18:45Z

Sent from my iPad

On Dec 10, 2016, at 18:33, Slava Krutelyov ***@***.***> wrote: as a more extreme ttbar PU200 in D4 (I only ran trackerlocalreco, didn't check the performance in higher level products)

did you run step2? this PR affects digitization as well, this is Why it Must go in pre2. splitting at 8 make no sense, we are not running jetcore in phase2, a track based cluster splitting shall eventually be developed,

slava77 · 2016-12-11T07:11:49Z

I didn't run step2.
I'll rerun and if this fixes size=1, will sign off.

slava77 · 2016-12-11T17:02:43Z

things are much cleaner and size<=8 population is moving in the right direction of reduction, and the total number of clusters is smaller as expected from the feature change of clusters now unlimited in size (the plot is still with this PR in black)

MTV plots on QCD600to800 21253.0, ZEE 21246.0, and TenMu 21211 do not show any significant changes.

slava77 · 2016-12-11T17:05:50Z

@cmsbuild please test
old files are gone by now

cmsbuild · 2016-12-11T17:06:06Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16957/console Started: 2016/12/11 19:01

cmsbuild · 2016-12-11T21:38:33Z

+1
Tested at: 0d28f30
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16815/16957/summary.html

cmsbuild · 2016-12-11T21:38:36Z

Comparison job queued.

cmsbuild · 2016-12-11T23:25:13Z

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

slava77 · 2016-12-11T23:37:29Z

+1

for #16815 0d28f30

code changes are OK; extremely large cluster size is still to be dealt with at some point in the future
jenkins tests passed comparisons show that this change affects only 2023 workflows as expected; changes are small.
local tests show expected changes in siPhase2Clusters (longer clusters) and performance in tests looks unchanged (not identical due to rerunning step2 in multiple threads).

cmsbuild · 2016-12-11T23:37:48Z

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. @slava77, @davidlange6, @smuzaffar

davidlange6 · 2016-12-12T12:47:58Z

+1

VinInn added 5 commits November 29, 2016 16:42

inline Phase2TrackerClusterizer

7501aa9

inline Phase2TrackerClusterizer and move in plugins

ae386e1

do not use ot bit in comparison

22780a3

new fast sequential algo

9ca66c4

fix bug in old clusterizer

0d28f30

cmsbuild added this to the Next CMSSW_9_0_X milestone Nov 30, 2016

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Nov 30, 2016

VinInn commented Nov 30, 2016

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Nov 30, 2016

cmsbuild added tests-approved and removed tests-started labels Nov 30, 2016

cmsbuild added comparison-available and removed comparison-pending labels Nov 30, 2016

slava77 reviewed Dec 10, 2016

View reviewed changes

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-approved labels Dec 11, 2016

cmsbuild added tests-started and removed tests-pending labels Dec 11, 2016

cmsbuild added tests-approved and removed tests-started labels Dec 11, 2016

cmsbuild added comparison-available and removed comparison-pending labels Dec 11, 2016

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Dec 11, 2016

cmsbuild added orp-approved and removed orp-pending labels Dec 12, 2016

cmsbuild merged commit 7d4dc66 into cms-sw:CMSSW_9_0_X Dec 12, 2016

smuzaffar mentioned this pull request Dec 13, 2016

add the missing dependency for plugin #17004

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up Phase2TrackerClusterizer by a factor 1000 #16815

Speed up Phase2TrackerClusterizer by a factor 1000 #16815

VinInn commented Nov 30, 2016 •

edited

Loading

cmsbuild commented Nov 30, 2016

VinInn Nov 30, 2016

delaere Nov 30, 2016

VinInn commented Nov 30, 2016

cmsbuild commented Nov 30, 2016 •

edited

Loading

VinInn commented Nov 30, 2016

cmsbuild commented Nov 30, 2016

cmsbuild commented Nov 30, 2016

cmsbuild commented Nov 30, 2016

slava77 Dec 10, 2016

slava77 commented Dec 10, 2016

VinInn commented Dec 11, 2016 via email

slava77 commented Dec 11, 2016

slava77 commented Dec 11, 2016

slava77 commented Dec 11, 2016

cmsbuild commented Dec 11, 2016 •

edited

Loading

cmsbuild commented Dec 11, 2016

cmsbuild commented Dec 11, 2016

cmsbuild commented Dec 11, 2016

slava77 commented Dec 11, 2016

cmsbuild commented Dec 11, 2016

davidlange6 commented Dec 12, 2016

Speed up Phase2TrackerClusterizer by a factor 1000 #16815

Speed up Phase2TrackerClusterizer by a factor 1000 #16815

Conversation

VinInn commented Nov 30, 2016 • edited Loading

cmsbuild commented Nov 30, 2016

VinInn Nov 30, 2016

Choose a reason for hiding this comment

delaere Nov 30, 2016

Choose a reason for hiding this comment

VinInn commented Nov 30, 2016

cmsbuild commented Nov 30, 2016 • edited Loading

VinInn commented Nov 30, 2016

cmsbuild commented Nov 30, 2016

cmsbuild commented Nov 30, 2016

cmsbuild commented Nov 30, 2016

slava77 Dec 10, 2016

Choose a reason for hiding this comment

slava77 commented Dec 10, 2016

VinInn commented Dec 11, 2016 via email

slava77 commented Dec 11, 2016

slava77 commented Dec 11, 2016

slava77 commented Dec 11, 2016

cmsbuild commented Dec 11, 2016 • edited Loading

cmsbuild commented Dec 11, 2016

cmsbuild commented Dec 11, 2016

cmsbuild commented Dec 11, 2016

slava77 commented Dec 11, 2016

cmsbuild commented Dec 11, 2016

davidlange6 commented Dec 12, 2016

VinInn commented Nov 30, 2016 •

edited

Loading

cmsbuild commented Nov 30, 2016 •

edited

Loading

cmsbuild commented Dec 11, 2016 •

edited

Loading