Address residual code review for Timing PRs #15938

lgray · 2016-09-21T21:25:46Z

Follow up code review for #15710.

cmsbuild · 2016-09-21T21:26:22Z

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

It involves the following packages:

DataFormats/VertexReco
RecoVertex/Configuration
RecoVertex/PrimaryVertexProducer

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @dgulhan, @rovere, @VinInn 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-21T21:59:25Z

@cmsbuild please test

cmsbuild · 2016-09-21T22:00:24Z

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

makortel · 2016-09-22T07:15:13Z

What about the incorrect placement of

eras.phase2_timing.toModify( quickTrackAssociatorByHits, ...)
eras.phase2_timing.toModify( tpClusterProducer, ...)

in RecoVertex/Configuration/python/RecoVertex_cff.py? (and I'm tempted to argue that both should be phase2_tracker)

cmsbuild · 2016-09-22T08:04:05Z

+1
Tested at: 5276b9e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15938/15318/summary.html

cmsbuild · 2016-09-22T09:34:48Z

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

10424.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017NewFPix_GenSimFull+DigiFull_2017NewFPix+RecoFull_2017NewFPix+HARVESTFull_2017NewFPix

lgray · 2016-09-22T12:44:58Z

@makortel Actually, since that modification gets used in a few places in different eras that are notionally independent of each other, I think it might be a good idea to write a centralize function to make the changes and then have each era call that function.

i.e. what if someone wants to run timing era without phase 2 tracker? or hgcal without phase2 tracker?

makortel · 2016-09-22T12:55:33Z

@lgray For timing era with phase0/phase1 tracker, why the default parameters of quickTrackAssociatorByHits and tpClusterProducer would not work?

(anyway my main "problem" is with the wrong placement of the era customizations, they should be moved to corresponding _cfi's; ok, tpClusterProducer needs a bit more since the _cfi is auto-generated)

lgray · 2016-09-22T12:56:36Z

Uff, you are right. Too early here. Easy enough to make the changes.

lgray · 2016-09-22T13:02:40Z

@makortel Where's the code that generates the tpClusterProducer _cfi ?

makortel · 2016-09-22T13:05:18Z

https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/SimTracker/TrackerHitAssociation/plugins/ClusterTPAssociationProducer.cc#L73

lgray · 2016-09-22T13:07:06Z

Oh, fun, where would you prefer I put the era customization in that case?

On Thu, Sep 22, 2016 at 8:05 AM, Matti Kortelainen <notifications@github.com

wrote:

https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/SimTracker/
TrackerHitAssociation/plugins/ClusterTPAssociationProducer.cc#L73

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

makortel · 2016-09-22T13:07:13Z

By the way, if you're indeed making the tpClusterProducer customization with phase2_tracker, could you also remove the corresponding customizations from
https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/SLHCUpgradeSimulations/Configuration/python/phase2TkFlat.py#L78
https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/SLHCUpgradeSimulations/Configuration/python/phase2TkTilted.py#L77
since you're essentially doing the era migration of those parts? :)

FYI @ebrondol

makortel · 2016-09-22T13:09:32Z

@lgray Two options:

make tpClusterProducer_cff that imports the _cfi and makes the customizations, and migrate everything already importing the _cfi to import _cff
modify the .cc to generate e.g. tpClusterProducerDefault_cfi, and then make tpClusterProducer_cfi.py that imports the ..Default_cfi and makes the customization; no migration of importers needed

(nowadays my personal preference is on 2 since it's simpler to do)

makortel · 2016-09-22T13:10:51Z

There is also an open issue on the matter #13326 :)

cmsbuild · 2016-09-29T02:24:38Z

Pull request #15938 was updated. @civanch, @cvuosalo, @mdhildreth, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

lgray · 2016-09-29T02:25:28Z

@cmsbuild please test

cmsbuild · 2016-09-29T02:25:42Z

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

cmsbuild · 2016-09-29T04:49:14Z

+1
Tested at: 8bc21be
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15938/15428/summary.html

cmsbuild · 2016-09-29T06:10:32Z

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

slava77 · 2016-09-29T14:49:15Z

+1

for #15938 8bc21be

re-signoff after rebase, needed to adjust after Better era imports #16001
jenkins tests pass and comparisons show no differences, which include D3Timing wf 22424.0

civanch · 2016-10-03T08:33:07Z

+1

cmsbuild · 2016-10-03T08:33:25Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar

davidlange6 · 2016-10-03T14:16:36Z

+1

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

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 21, 2016

lgray force-pushed the followup_timing_codereview branch from 288a268 to 5276b9e Compare September 21, 2016 21:29

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

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

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

cmsbuild removed comparison-available tests-approved labels Sep 22, 2016

cmsbuild added the tests-rejected label Sep 29, 2016

lgray added 2 commits September 28, 2016 21:23

put tracker customizations in the right eras

259b53b

indents and copy/pasta

8bc21be

lgray force-pushed the followup_timing_codereview branch from 071a629 to 8bc21be Compare September 29, 2016 02:24

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

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

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

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

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

cmsbuild added fully-signed simulation-approved and removed pending-signatures simulation-pending labels Oct 3, 2016

cmsbuild added orp-approved and removed orp-pending labels Oct 3, 2016

cmsbuild merged commit 5ee2bb8 into cms-sw:CMSSW_8_1_X Oct 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address residual code review for Timing PRs #15938

Address residual code review for Timing PRs #15938

lgray commented Sep 21, 2016

cmsbuild commented Sep 21, 2016

lgray commented Sep 21, 2016

cmsbuild commented Sep 21, 2016 •

edited

makortel commented Sep 22, 2016

cmsbuild commented Sep 22, 2016

cmsbuild commented Sep 22, 2016

lgray commented Sep 22, 2016

makortel commented Sep 22, 2016

lgray commented Sep 22, 2016

lgray commented Sep 22, 2016

makortel commented Sep 22, 2016

lgray commented Sep 22, 2016

makortel commented Sep 22, 2016

makortel commented Sep 22, 2016

makortel commented Sep 22, 2016

cmsbuild commented Sep 29, 2016

lgray commented Sep 29, 2016

cmsbuild commented Sep 29, 2016 •

edited

cmsbuild commented Sep 29, 2016

cmsbuild commented Sep 29, 2016

slava77 commented Sep 29, 2016

civanch commented Oct 3, 2016

cmsbuild commented Oct 3, 2016

davidlange6 commented Oct 3, 2016

Address residual code review for Timing PRs #15938

Address residual code review for Timing PRs #15938

Conversation

lgray commented Sep 21, 2016

cmsbuild commented Sep 21, 2016

lgray commented Sep 21, 2016

cmsbuild commented Sep 21, 2016 • edited

makortel commented Sep 22, 2016

cmsbuild commented Sep 22, 2016

cmsbuild commented Sep 22, 2016

lgray commented Sep 22, 2016

makortel commented Sep 22, 2016

lgray commented Sep 22, 2016

lgray commented Sep 22, 2016

makortel commented Sep 22, 2016

lgray commented Sep 22, 2016

makortel commented Sep 22, 2016

makortel commented Sep 22, 2016

makortel commented Sep 22, 2016

cmsbuild commented Sep 29, 2016

lgray commented Sep 29, 2016

cmsbuild commented Sep 29, 2016 • edited

cmsbuild commented Sep 29, 2016

cmsbuild commented Sep 29, 2016

slava77 commented Sep 29, 2016

civanch commented Oct 3, 2016

cmsbuild commented Oct 3, 2016

davidlange6 commented Oct 3, 2016

cmsbuild commented Sep 21, 2016 •

edited

cmsbuild commented Sep 29, 2016 •

edited