CA for Phase2 tracking + few more smaller-impact optimizations #18728

VinInn · 2017-05-14T16:48:09Z

This PR is one more major step toward an affordable tracking at Run4
It sports

Cellular Automaton as in Phase1
retuning of iterations bringing closer to Phase1
Minor optimization of OT CPE
Introduction of window search in OT MeasurementDet (as already done in IT)

It includes #18663 as it targets mostly D11

Performances

Timing Tracking Only (seeds+building+fitting)
3.8GHz machine, 8 core, 8 Threads

Phase1 PU50: 3.8 sec/ev ~1700 reco-tracks/ev

Phase2 D11 PU200 current: 27.0 sec/ev

Phase2 D11 PU200 This PR: 14.8 sec/ev ~7000 reco-track/ev

MTV

SingleMu 10GeV D11
http://innocent.home.cern.ch/innocent/RelVal/SingleMuonD11_CA8a/

ttbar 200PU D11
http://innocent.home.cern.ch/innocent/RelVal/pu200D11_CA8a/

cmsbuild · 2017-05-14T16:48:30Z

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

It involves the following packages:

DataFormats/Phase2TrackerCluster
RecoLocalTracker/Phase2TrackerRecHits
RecoLocalTracker/SiPhase2Clusterizer
RecoPixelVertexing/PixelLowPtUtilities
RecoPixelVertexing/PixelTriplets
RecoTracker/IterativeTracking
RecoTracker/MeasurementDet

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

cms-bot commands are listed here

VinInn · 2017-05-14T16:49:26Z

@cmsbuild , please test

cmsbuild · 2017-05-14T16:49:42Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19809/console Started: 2017/05/14 20:55

VinInn · 2017-05-14T16:51:05Z

@boudoul @atricomi @venturia FYI

VinInn · 2017-05-14T16:58:22Z

Slides...

venturia · 2017-05-14T17:15:31Z

No chance to have it presented in tomorrow upgrade simulation meeting? It would be a shame not to do so given the fact it is Tracker week

VinInn · 2017-05-14T17:27:20Z

@venturia
slides are ready (not by chance). I just need a slot....

boudoul · 2017-05-14T17:30:55Z

seems that you have one .. : https://indico.cern.ch/event/636871/

atricomi · 2017-05-14T17:31:30Z

Not anymore.. You got it ;-)

…

Il giorno 14 mag 2017, alle ore 19:27, Vincenzo Innocente ***@***.***> ha scritto: @venturia <https://github.com/venturia> slides are ready (not by chance). I just need a slot.... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18728 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE88kXf_AhIWKGAHP_7j0Jy7ev1eb9Czks5r5zl5gaJpZM4Nabh0>.

cmsbuild · 2017-05-14T21:00:33Z

+1
Tested at: c76bb9c
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18728/19809/summary.html

cmsbuild · 2017-05-14T21:00:36Z

Comparison job queued.

cmsbuild · 2017-05-14T23:17:34Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 6624 differences found in the comparisons
DQMHistoTests: Total files compared: 24
DQMHistoTests: Total histograms compared: 1830696
DQMHistoTests: Total failures: 38417
DQMHistoTests: Total nulls: 775
DQMHistoTests: Total successes: 1791324
DQMHistoTests: Total skipped: 180
DQMHistoTests: Total Missing objects: 0
Checked 98 log files, 14 edm output root files, 24 DQM output files

ebrondol · 2017-05-15T11:25:55Z

Thanks @VinInn !

kpedro88 · 2017-05-15T14:55:25Z

RecoLocalTracker/Phase2TrackerRecHits/src/Phase2StripCPE.cc

+   auto const & dus = geom_.detUnits();
+   m_off = dus.size();
+   // skip Barrel and Foward pixels...
+   for(unsigned int i=3;i<7;++i) {


explanation of magic numbers 3 and 7?

at the moment there is no better way (see the other two CPE)
anyhow BPIX =1, FPIX=2, TIB=3, TID=4,TOB=5,TEC=6

Could an enum be used, or are these indices expected to change frequently?

These numbers never changed since last century and will never change.
And those working in the tracker-tracking better know them by hearth.
A smarter code in PixelCPE is "broken" in Phase2 as it was looking for non strip detector.
in Phase2 all dets answer "isPixel".
So for the time being better NOT to be smart.
(here and elsewhere we assume for instance that IT and OT (or pixel and strip) are built contiguously and ones after the other. AND better to be like this in future).
We rely on ordering, sorting, numbering. We cannot verify this each time.
Things have to be done properly, once, in a single place. Everybody else just rely that things are correct.
And possibly developers should know what they are dealing with, inform themselves, read code.

I am skeptical that an enum comparison has any noticeable expense.

it will be more confusing and error prone...
anyhow given that similar code is all other CPEs let's take note, open an issue, and fix all CPEs (and other places at once).

opened a Jira Ticket
https://its.cern.ch/jira/browse/CMSTRACK-154

in any case, we are discussing of this
https://cmssdt.cern.ch/dxr/CMSSW/source/Geometry/CommonDetUnit/interface/GeomDetEnumerators.h#15
which is immutable, we need anyhow to know that the first valid is "1" and last valid is "6".
Yes, we can search (using an unclear enumerator to me) the location of "TIB" and we need to know "7" (or we need to add "7" to GeomDetEnumerators.
at the end what one is asking is to search for TIB instead of writing 3 whitch btw in int(GeomDetEnumerators::TIB)
In my opinion all this is auto-referencing, no added knowledge as the developers HAS to know the content of GeomDetEnumerators anyhow, that better not change.
There is always at some level some hidden knowledge required, no matter what code we right.

I personally find MUCH more confusing that on phase2 isTrackerPixel answers true for OT as well

Okay, we can try to work on improving this in the longer term.

kpedro88 · 2017-05-15T14:56:33Z

RecoLocalTracker/Phase2TrackerRecHits/src/Phase2StripCPE.cc

+       if(geom_.offsetDU(GeomDetEnumerators::tkDetEnum[i]) < m_off) m_off = geom_.offsetDU(GeomDetEnumerators::tkDetEnum[i]);
+     }
+   }
+   LogDebug("LookingForFirstPahse2OT") << " Chosen offset: " << m_off;


typo: Pahse -> Phase

kpedro88 · 2017-05-15T15:02:39Z

RecoTracker/MeasurementDet/plugins/TkPhase2OTMeasurementDet.cc

+  auto lastCluster = detSet.end();
+
+  // do not use this as it does not account for APE...
+  // auto xyLimits = est.maximalLocalDisplacement(stateOnThisDet,fastGeomDet().specificSurface());


Should this line be deleted?

For documentation. In the old-tymes (previous century) this method was there to deliver the search limits. Unfortunately does not account for APE.
This code (comments included) has been adapted from IT (read Pixel) MeasurementDet asis in the current realease

venturia · 2017-05-15T15:46:04Z

If you have the TrackerGeometry available you can use const GeomDetEnumerators::SubDetector geomDetSubDetector(int subdet) const;
to convert 1,2,3,4,5,6 into an enumerator which tells you which subdetector is and check if it is GeomDetEnumerators::P2OTB or GeomDetEnumerators::P2OTEC (if the target is to select OT subdetectors).
These enumerators are described here:
https://github.com/cms-sw/cmssw/tree/master/Geometry/TrackerGeometryBuilder in the README part

cmsbuild · 2017-05-22T08:31:19Z

Pull request #18728 was updated. @perrotta, @cmsbuild, @slava77, @kpedro88, @davidlange6 can you please check and sign again.

VinInn · 2017-05-22T08:34:04Z

@cmsbuild, please test

cmsbuild · 2017-05-22T08:34:23Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20016/console Started: 2017/05/22 10:34

cmsbuild · 2017-05-22T10:35:41Z

+1
Tested at: 8e1ceb4
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18728/20016/summary.html

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

cmsbuild · 2017-05-22T10:35:44Z

Comparison job queued.

cmsbuild · 2017-05-22T12:40:50Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 6428 differences found in the comparisons
DQMHistoTests: Total files compared: 24
DQMHistoTests: Total histograms compared: 1832990
DQMHistoTests: Total failures: 52939
DQMHistoTests: Total nulls: 1034
DQMHistoTests: Total successes: 1778837
DQMHistoTests: Total skipped: 180
DQMHistoTests: Total Missing objects: 0
Checked 98 log files, 14 edm output root files, 24 DQM output files

slava77 · 2017-05-22T14:17:10Z

+1

for #18728 8e1ceb4

changes are in line with the description and affect only phase-2 workflows
jenkins tests pass and comparisons with baseline show differences only in phase-2 workflows as expected
a more detailed summary of changes in performance based on CA for Phase2 tracking + few more smaller-impact optimizations #18728 916b79b is available earlier in this thread (CA for Phase2 tracking + few more smaller-impact optimizations #18728 (comment)).
- after the rebase, the first range 42866e3...1363115 is identical to the previous version and the added changes in 1363115...8e1ceb4 are only in commented out lines (removal)

kpedro88 · 2017-05-22T14:45:20Z

+1

cmsbuild · 2017-05-22T14:45:40Z

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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

davidlange6 · 2017-05-23T06:03:32Z

+1

cmsbuild added this to the CMSSW_9_2_X milestone May 14, 2017

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending upgrade-pending labels May 14, 2017

cmsbuild added tests-started and removed tests-pending labels May 14, 2017

VinInn mentioned this pull request May 14, 2017

New Calibration for D11 Geometry #18663

Merged

cmsbuild added tests-approved and removed tests-started labels May 14, 2017

cmsbuild added comparison-available and removed comparison-pending labels May 14, 2017

kpedro88 reviewed May 15, 2017

View reviewed changes

cmsbuild removed the comparison-available label May 15, 2017

cmsbuild added tests-started and removed tests-pending labels May 22, 2017

remove comments

8e1ceb4

cmsbuild added tests-pending and removed tests-started labels May 22, 2017

cmsbuild added tests-started and removed tests-pending labels May 22, 2017

cmsbuild added tests-approved and removed tests-started labels May 22, 2017

cmsbuild added comparison-available and removed comparison-pending labels May 22, 2017

cmsbuild added reconstruction-approved and removed reconstruction-pending labels May 22, 2017

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels May 22, 2017

cmsbuild added orp-approved and removed orp-pending labels May 23, 2017

cmsbuild merged commit 6ce8de5 into cms-sw:master May 23, 2017

CA for Phase2 tracking + few more smaller-impact optimizations #18728

CA for Phase2 tracking + few more smaller-impact optimizations #18728

Conversation

VinInn commented May 14, 2017 • edited

cmsbuild commented May 14, 2017

VinInn commented May 14, 2017

cmsbuild commented May 14, 2017 • edited

VinInn commented May 14, 2017

VinInn commented May 14, 2017 • edited

venturia commented May 14, 2017

VinInn commented May 14, 2017

boudoul commented May 14, 2017

atricomi commented May 14, 2017 via email

cmsbuild commented May 14, 2017

cmsbuild commented May 14, 2017

cmsbuild commented May 14, 2017

ebrondol commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn May 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn May 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn May 15, 2017 • edited

Choose a reason for hiding this comment

venturia commented May 15, 2017 • edited

cmsbuild commented May 22, 2017

VinInn commented May 22, 2017

cmsbuild commented May 22, 2017 • edited

cmsbuild commented May 22, 2017

cmsbuild commented May 22, 2017

cmsbuild commented May 22, 2017

slava77 commented May 22, 2017

kpedro88 commented May 22, 2017

cmsbuild commented May 22, 2017

davidlange6 commented May 23, 2017

VinInn commented May 14, 2017 •

edited

cmsbuild commented May 14, 2017 •

edited

VinInn commented May 14, 2017 •

edited

VinInn May 15, 2017 •

edited

VinInn May 15, 2017 •

edited

VinInn May 15, 2017 •

edited

venturia commented May 15, 2017 •

edited

cmsbuild commented May 22, 2017 •

edited