Vertex Reconstruction with Timing 810 #15710

lgray · 2016-09-02T18:27:17Z

This is a forward port of:

Baseline for Hermetic Timing Demo (SLHC) #12290 & Hermetic Timing SLHC27 (fix crashes in RECO so we can use the usual workflows) #12962

It implements in reco::Vertex and the vertex finding/fitting machinery the ability to cluster in time in addition to space.

The track times are extracted from the matched sim-track of the reconstructed track and saved as ValueMaps.

In order to seemlessly switch between timing and no timing I have implemented the underlying GlobalError and Positions as four vectors instead of three vectors. Extending an interface to deal with the new dimension, and using the old interface for the original 3D algorithms.

The physics performance of the 3D algorithms (i.e. present LHC vertex reco) should be (and from local tests is) unaffected by this change. There may be minor performance drawbacks due to the way the backwards compatibility is kept. Alternative recommendations are welcome.

The timing is integrated directly into the phase 2 tracking reconstruction era. The overhead is fairly small.

@bendavid @kpedro88

cmsbuild · 2016-09-02T18:27:45Z

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_8_1_X.

It involves the following packages:

DataFormats/GeometryCommonDetAlgo
DataFormats/ParticleFlowReco
DataFormats/PatCandidates
DataFormats/VertexReco
IOMC/RandomEngine
RecoParticleFlow/Configuration
RecoParticleFlow/PFProducer
RecoVertex/Configuration
RecoVertex/GaussianSumVertexFit
RecoVertex/PrimaryVertexProducer
RecoVertex/VertexPrimitives
SimTracker/TrackAssociation
TrackingTools/TransientTrack

@smuzaffar, @civanch, @Dr15Jones, @cvuosalo, @mdhildreth, @monttj, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@mmarionncern, @sdevissc, @wmtan, @makortel, @trocino, @abbiendi, @GiacomoSguazzoni, @rafaellopesdesa, @jhgoh, @VinInn, @bellan, @HuguesBrun, @wddgit, @rovere, @cbernet, @gpetruc, @battibass, @threus, @dgulhan, @bachtis this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

lgray · 2016-09-02T18:27:56Z

@cmsbuild please test

cmsbuild · 2016-09-02T18:28:06Z

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

slava77 · 2016-09-02T20:42:47Z

DataFormats/VertexReco/interface/Vertex.h

@@ -43,19 +43,27 @@ namespace reco {
    typedef math::Error<dimension>::type Error;


better defind dimension4D = 4 enum and use it instead of "dimension+1" everywhere

this comment was not addressed

slava77 · 2016-09-02T21:16:36Z

I think we will have to see detailed cost estimates from changes in the data formats before this can go in.

For the TransientTrack* edits, my first thought is that they are not necessary and the "(time, timeError)" should instead be carried in parallel to the methods that need them (only vertexing, apparently).

lgray · 2016-09-02T21:26:28Z

@slava77 If we want to really calculate covariances and do a proper vertex fit the time will need to be in the TransientTracks anyway.

lgray · 2016-09-02T21:26:53Z

@slava77 Of course, right now, that is not done and something approximate is used.

cmsbuild · 2016-09-02T21:28:05Z

-1

Tested at: 975fed1

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

I found follow errors while testing this PR

Failed tests: RelVals

RelVals:

When I ran the RelVals I found an error in the following worklfows:
1003.0 step2

runTheMatrix-results/1003.0_RunMinBias2012A+RunMinBias2012A+RECODDQM+HARVESTDDQM/step2_RunMinBias2012A+RunMinBias2012A+RECODDQM+HARVESTDDQM.log

1000.0 step2

runTheMatrix-results/1000.0_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT/step2_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT.log

1001.0 step2

runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4+ALCAHARVD5/step2_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4+ALCAHARVD5.log

1306.0 step3

runTheMatrix-results/1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15/step3_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15.log

10021.0 step3

runTheMatrix-results/10021.0_TenMuE_0_200+TenMuE_0_200_pythia8_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017/step3_TenMuE_0_200+TenMuE_0_200_pythia8_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017.log

1330.0 step3

runTheMatrix-results/1330.0_ZMM_13+ZMM_13+DIGIUP15+RECOUP15+HARVESTUP15/step3_ZMM_13+ZMM_13+DIGIUP15+RECOUP15+HARVESTUP15.log

136.731 step3

runTheMatrix-results/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_HIPM+HARVESTDR2/step3_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_HIPM+HARVESTDR2.log

9.0 step3

runTheMatrix-results/9.0_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST/step3_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST.log

10424.0 step3

runTheMatrix-results/10424.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2023D1_GenSimFull+DigiFull_2023D1+RecoFullGlobal_2023D1+HARVESTFullGlobal_2023D1/step3_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2023D1_GenSimFull+DigiFull_2023D1+RecoFullGlobal_2023D1+HARVESTFullGlobal_2023D1.log

25.0 step3

runTheMatrix-results/25.0_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT/step3_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT.log

10024.0 step3

runTheMatrix-results/10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017/step3_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017.log

50202.0 step3

runTheMatrix-results/50202.0_TTbar_13+TTbar_13+DIGIUP15_PU50+RECOUP15_PU50+HARVESTUP15_PU50/step3_TTbar_13+TTbar_13+DIGIUP15_PU50+RECOUP15_PU50+HARVESTUP15_PU50.log

25202.0 step3

runTheMatrix-results/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/step3_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25.log

11224.0 step3

runTheMatrix-results/11224.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2023D3_GenSimFull+DigiFull_2023D3+RecoFullGlobal_2023D3+HARVESTFullGlobal_2023D3/step3_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2023D3_GenSimFull+DigiFull_2023D3+RecoFullGlobal_2023D3+HARVESTFullGlobal_2023D3.log

lgray · 2016-09-02T21:28:25Z

OK... fun. that ran locally.

lgray · 2016-09-02T21:28:57Z

Anyway, I'll come to it over the weekend, tell me what size impact estimates you want to see and in what samples you want to see them.

slava77 · 2016-09-02T21:59:34Z

On 9/2/16 2:28 PM, Lindsey Gray wrote:

Anyway, I'll come to it over the weekend, tell me what size impact
estimates you want to see and in what samples you want to see them.

memory peak
plain CPU cost increase
disk size [likely less of an issue due to compression]
The above, more interesting for production-like processing (no
validation/mixing)
with PU35 in 2016 and with PU200 for e.g. 2023 D1.
I'd say, a 1% cost increase to any of the above is a potential show-stopper.

GlobalError is used in alignment. I recall it's a memory-demanding workflow.
If that memory use is driven by instances of GlobalError, it's a problem.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15710 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbpe38AWFVxJqICwLbXepKjXKXu7hks5qmJUagaJpZM4Jz-C-.

slava77 · 2016-09-02T22:13:24Z

IB is broken, apparently.

lgray · 2016-09-03T00:12:38Z

Are there any runTheMatrix WFs for production processing or can you toss me configs?

cmsbuild · 2016-09-20T14:18:08Z

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

cmsbuild · 2016-09-20T16:28:30Z

+1
Tested at: f07727f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15710/15289/summary.html

lgray · 2016-09-20T16:36:36Z

Hi everyone, OK - looks like we're OK to go here with the new IB. Please resign!

Dr15Jones · 2016-09-20T16:45:02Z

+1

cmsbuild · 2016-09-20T18:05:29Z

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

The workflows 140.53, 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-09-20T18:43:26Z

+1

for #15710 f07727f

technical update since the last approval by reco to use std::unique_ptr std::auto_ptr
jenkins tests pass (compilation is enough)

lgray · 2016-09-20T18:45:21Z

@davidlange6 Please merge this when able.

lgray · 2016-09-20T19:53:14Z

FYI This was previously signed off by simulation as well. The last commit didn't touch simulation packages.

lgray · 2016-09-20T21:12:28Z

@davidlange6 (for the morning) I was aiming for pre12 with this PR and it is able to go in (no conflicts or adverse effects...). Please include it in the release. Thanks!

monttj · 2016-09-21T02:18:50Z

+1
There seems no issues from analysis side with this PR.
It is about adding vector type in the PAT candidate dictionary.

davidlange6 · 2016-09-21T08:30:42Z

@lgray - two things for a future PR - I echo @slava77 comments about dimension+1 becoming a constant instead of repeatedly appearing as dimension+1. Second, naively seems better to use floats instead of doubles in the data formats unless there is some reason to expect the need for the extra precision on the timing variables (or that some piece is already committed to using doubles...)

lgray · 2016-09-21T11:18:17Z

Yep, agreed with Slava to put this in another PR. Along with remaining code
review from the ECAL timing. It will be there for pre13.

-L

(Sent from my Nexus 6)

On Sep 21, 2016 3:30 AM, "David Lange" notifications@github.com wrote:

@lgray https://github.com/lgray - two things for a future PR - I echo
@slava77 https://github.com/slava77 comments about dimension+1 becoming
a constant instead of repeatedly appearing as dimension+1. Second, naively
seems better to use floats instead of doubles in the data formats unless
there is some reason to expect the need for the extra precision on the
timing variables (or that some piece is already committed to using
doubles...)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15710 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABBMOfLwv74jaJMOMlFah7nzHS3DP7IUks5qsOs2gaJpZM4Jz-C-
.

lgray · 2016-09-21T17:53:25Z

@davidlange6 Concerning time in double: x,y,z position is also in double because it's an math::XYZPoint. Should I change vertex to use math::XYZPointF in my follow up PR?

lgray · 2016-09-21T17:53:44Z

More or less for consistency in precision...

cmsbuild added this to the Next CMSSW_8_1_X milestone Sep 2, 2016

cmsbuild added analysis-pending comparison-pending core-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending labels Sep 2, 2016

cmsbuild added tests-started and removed tests-pending labels Sep 2, 2016

slava77 reviewed Sep 2, 2016
View reviewed changes

cmsbuild added tests-rejected and removed tests-started labels Sep 2, 2016

cmsbuild added tests-pending and removed tests-rejected labels Sep 20, 2016

cmsbuild added tests-started and removed tests-pending labels Sep 20, 2016

cmsbuild added tests-approved and removed tests-started labels Sep 20, 2016

cmsbuild added core-approved and removed core-pending labels Sep 20, 2016

cmsbuild added comparison-available and removed comparison-pending labels Sep 20, 2016

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Sep 20, 2016

cmsbuild added analysis-approved and removed analysis-pending labels Sep 21, 2016

davidlange6 merged commit 64b8eb3 into cms-sw:CMSSW_8_1_X Sep 21, 2016

lgray mentioned this pull request Sep 21, 2016

Address residual code review for Timing PRs #15938

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vertex Reconstruction with Timing 810 #15710

Vertex Reconstruction with Timing 810 #15710

lgray commented Sep 2, 2016 •

edited

cmsbuild commented Sep 2, 2016

lgray commented Sep 2, 2016

cmsbuild commented Sep 2, 2016 •

edited

slava77 Sep 2, 2016

slava77 Sep 16, 2016

slava77 commented Sep 2, 2016

lgray commented Sep 2, 2016

lgray commented Sep 2, 2016

cmsbuild commented Sep 2, 2016

lgray commented Sep 2, 2016

lgray commented Sep 2, 2016

slava77 commented Sep 2, 2016

slava77 commented Sep 2, 2016

lgray commented Sep 3, 2016

cmsbuild commented Sep 20, 2016 •

edited

cmsbuild commented Sep 20, 2016

lgray commented Sep 20, 2016

Dr15Jones commented Sep 20, 2016

cmsbuild commented Sep 20, 2016

slava77 commented Sep 20, 2016

lgray commented Sep 20, 2016

lgray commented Sep 20, 2016

lgray commented Sep 20, 2016

monttj commented Sep 21, 2016

davidlange6 commented Sep 21, 2016

lgray commented Sep 21, 2016

lgray commented Sep 21, 2016

lgray commented Sep 21, 2016

		@@ -43,19 +43,27 @@ namespace reco {
		typedef math::Error<dimension>::type Error;

Vertex Reconstruction with Timing 810 #15710

Vertex Reconstruction with Timing 810 #15710

Conversation

lgray commented Sep 2, 2016 • edited

cmsbuild commented Sep 2, 2016

lgray commented Sep 2, 2016

cmsbuild commented Sep 2, 2016 • edited

slava77 Sep 2, 2016

Choose a reason for hiding this comment

slava77 Sep 16, 2016

Choose a reason for hiding this comment

slava77 commented Sep 2, 2016

lgray commented Sep 2, 2016

lgray commented Sep 2, 2016

cmsbuild commented Sep 2, 2016

lgray commented Sep 2, 2016

lgray commented Sep 2, 2016

slava77 commented Sep 2, 2016

slava77 commented Sep 2, 2016

lgray commented Sep 3, 2016

cmsbuild commented Sep 20, 2016 • edited

cmsbuild commented Sep 20, 2016

lgray commented Sep 20, 2016

Dr15Jones commented Sep 20, 2016

cmsbuild commented Sep 20, 2016

slava77 commented Sep 20, 2016

lgray commented Sep 20, 2016

lgray commented Sep 20, 2016

lgray commented Sep 20, 2016

monttj commented Sep 21, 2016

davidlange6 commented Sep 21, 2016

lgray commented Sep 21, 2016

lgray commented Sep 21, 2016

lgray commented Sep 21, 2016

lgray commented Sep 2, 2016 •

edited

cmsbuild commented Sep 2, 2016 •

edited

cmsbuild commented Sep 20, 2016 •

edited