Ecal Detailed Time Rechits in 810 #15613

lgray · 2016-08-25T23:34:46Z

Forward port and modernization of #2703.

Also includes new runTheMatrix workflows for tests, tested with 21224.0 and 22024.0.
New Eras 2023C1_timing and 2023C2_timing.

@pmeridian I checked this in TTbar and the distributions looked OK. Since you are the author of the code could you please take a look in CMSSW_8_1_X?

@bendavid @kpedro88

cmsbuild · 2016-08-25T23:35:10Z

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

It involves the following packages:

Configuration/Eras
Configuration/PyReleaseValidation
Configuration/StandardSequences
DataFormats/CaloRecHit
DataFormats/EcalDigi
DataFormats/EcalRecHit
RecoLocalCalo/Configuration
RecoLocalCalo/EcalRecProducers
SLHCUpgradeSimulations/Configuration
SimCalorimetry/Configuration
SimCalorimetry/EcalSimAlgos
SimCalorimetry/EcalSimProducers
SimG4CMS/Calo
SimG4Core/Configuration
SimGeneral/MixingModule

The following packages do not have a category, yet:

Configuration/Eras

@civanch, @cvuosalo, @franzoni, @mdhildreth, @fabozzi, @cmsbuild, @srimanob, @slava77, @hengne, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @wmtan, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @argiro, @dgulhan 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-08-25T23:47:18Z

Had to rebase, need to double check before launching matrix tests here.

lgray · 2016-08-26T01:58:08Z

@cmsbuild please test

cmsbuild · 2016-08-26T01:58:31Z

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

cmsbuild · 2016-08-26T04:18:13Z

-1

Tested at: 0b38b6a

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15613/14745/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:
4.22 step1

DAS Error

lgray · 2016-08-26T04:24:11Z

@cmsbuild please test

cmsbuild · 2016-08-26T04:24:34Z

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

cmsbuild · 2016-08-26T06:49:01Z

+1
Tested at: 0b38b6a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15613/14746/summary.html

makortel · 2016-08-26T06:53:28Z

Configuration/Eras/python/Era_Phase2C1_timing_cff.py

+from Configuration.Eras.Modifier_run3_GEM_cff import run3_GEM
+from Configuration.Eras.Modifier_phase2_timing_cff import phase2_timing
+
+Phase2C1_timing = cms.ModifierChain(run2_common, phase2_common, phase2_tracker, trackingPhase2PU140, phase2_muon, run3_GEM, phase2_timing)


If the point is to be Phase2C1 + phase2_timing always, how about Phase2C1 = cms.ModifierChain(Phase2C1, phase2_timing)? Then all future changes to Phase2C1 would be automatically included here.

(same comment for Phase2C2_timing below)

Indeed, and done. Running local tests first to make sure everything is OK.

cmsbuild · 2016-08-26T07:27:15Z

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

cmsbuild · 2016-09-14T11:26:25Z

Pull request #15613 was updated. @civanch, @cvuosalo, @franzoni, @mdhildreth, @fabozzi, @cmsbuild, @srimanob, @slava77, @hengne, @davidlange6 can you please check and sign again.

cmsbuild · 2016-09-14T14:53:57Z

Pull request #15613 was updated. @civanch, @cvuosalo, @franzoni, @mdhildreth, @fabozzi, @cmsbuild, @srimanob, @slava77, @hengne, @davidlange6 can you please check and sign again.

lgray · 2016-09-14T14:54:03Z

@cmsbuild please test

cmsbuild · 2016-09-14T14:54:23Z

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

lgray · 2016-09-14T14:54:51Z

@cvuosalo I'll put in the python pipework for these last commits in another PR once this is in an IB.

I am done adding things to this PR.

lgray · 2016-09-14T14:56:39Z

@cvuosalo Note: if you add #15835 on top of this PR the timing degradations from turning on the ECAL Detailed Time vanish. The additional memory usage stays.

cmsbuild · 2016-09-14T20:36:27Z

+1
Tested at: 369440a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-15613/15138/summary.html

cmsbuild · 2016-09-14T21:25:15Z

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

civanch · 2016-09-15T10:04:38Z

+1

cvuosalo · 2016-09-15T15:58:24Z

RecoParticleFlow/PFSimProducer/plugins/EcalBarrelClusterFastTimer.cc

+    std::vector<std::pair<float,DetId> > smeared_times;
+    resolutions.reserve(clusters.size());
+    smeared_times.reserve(clusters.size());
+    std::auto_ptr<std::vector< std::vector<float> > > outP( new std::vector< std::vector<float> > );


auto_ptr is deprecated. Please change to unique_ptr/shared_ptr. Same for line 73.

Would it be possible to address this in the next PR where this code is introduced into the reconstruction workflows? Unless there are other major comments.

Yes, these minor issues could be fixed in a later PR.

@cvuosalo These are fixed by @wmtan. Thanks! Consider this comment addressed.

cvuosalo · 2016-09-15T16:02:58Z

RecoParticleFlow/PFSimProducer/plugins/EcalBarrelClusterFastTimer.cc

+  float correctTimeToVertex(const float intime, const DetId& timeDet, const reco::Vertex& vtx, 
+                            const CaloSubdetectorGeometry* ecalGeom) const;
+  // RNG
+  CLHEP::HepRandomEngine* _rng_engine;


Leading underbar is not allowed in a variable name: _rng_engine.

Umm this is not the version of the file in that's in git and tested, you are looking at a previous revision. Please double check.

OK, sorry. It's already fixed.

cvuosalo · 2016-09-15T16:05:07Z

@lgray: Does the PR description need revision? I don't think 11624.0 and 12024.0 are 2023 workflows anymore.

lgray · 2016-09-15T16:11:13Z

workflow numbers are updated!

cvuosalo · 2016-09-15T19:09:36Z

+1

For #15613 369440a

For Phase 2, adding a detailed simulation of a timing layer in the ECAL.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_1_X_2016-09-14-1100 show no significant differences. Extensive timing tests are discussed at length above, with the conclusion that fixes in other current or upcoming PRs will address the issues.

lgray · 2016-09-15T19:53:05Z

@davidlange6 If possible it would be nice to get this in tonight's IB. I'd like to rebase the vertex timing PR to this one without recompiling 500 packages.

boudoul · 2016-09-16T10:07:47Z

Configuration/PyReleaseValidation/python/upgradeWorkflowComponents.py

+
+#standard PU sequences
+upgradeProperties[2023]['2017PU'] = deepcopy(upgradeProperties[2017]['2017'])
+upgradeProperties[2023]['2017PU']['ScenToRun'] = ['GenSimFull','DigiFullPU','RecoFullPU','HARVESTFullPU']


I belive there is a typo here (2013 instead of 2017 ) @kpedro88 @lgray

Yeah, this looks like a rebase mistake - the real 2017PU workflows are defined further up. @lgray, can you fix this in your followup PR? (or @boudoul, if you're rebasing your PR onto this anyway, feel free to fix it there...)

Indeed, I'll patch it in #15710

Actually, making a separate patch PR.

Fixed in #15870

cmsbuild added this to the Next CMSSW_8_1_X milestone Aug 25, 2016

cmsbuild added reconstruction-pending simulation-pending operations-pending pending-signatures tests-pending orp-pending new-package-pending comparison-pending pdmv-pending labels Aug 25, 2016

cmsbuild added tests-started and removed tests-pending labels Aug 26, 2016

cmsbuild added tests-pending tests-started and removed tests-started tests-pending labels Aug 26, 2016

makortel reviewed Aug 26, 2016
View reviewed changes

cmsbuild added comparison-available and removed comparison-pending comparison-available tests-started labels Aug 26, 2016

lgray added 2 commits September 14, 2016 13:24

fix up matrix

ebeb3f7

bring in ecal cluster time assignment

9dac168

add in code for ecal cluster time assigner

369440a

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

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

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

cmsbuild added simulation-approved and removed simulation-pending labels Sep 15, 2016

cvuosalo reviewed Sep 15, 2016

View reviewed changes

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

davidlange6 merged commit 86088d6 into cms-sw:CMSSW_8_1_X Sep 15, 2016

boudoul reviewed Sep 16, 2016

View reviewed changes

Ecal Detailed Time Rechits in 810 #15613

Ecal Detailed Time Rechits in 810 #15613

Conversation

lgray commented Aug 25, 2016 • edited

cmsbuild commented Aug 25, 2016

lgray commented Aug 25, 2016

lgray commented Aug 26, 2016

cmsbuild commented Aug 26, 2016 • edited

cmsbuild commented Aug 26, 2016

lgray commented Aug 26, 2016

cmsbuild commented Aug 26, 2016 • edited

cmsbuild commented Aug 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Aug 26, 2016

cmsbuild commented Sep 14, 2016

cmsbuild commented Sep 14, 2016

lgray commented Sep 14, 2016

cmsbuild commented Sep 14, 2016 • edited

lgray commented Sep 14, 2016 • edited

lgray commented Sep 14, 2016

cmsbuild commented Sep 14, 2016

cmsbuild commented Sep 14, 2016

civanch commented Sep 15, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvuosalo commented Sep 15, 2016

lgray commented Sep 15, 2016

cvuosalo commented Sep 15, 2016

lgray commented Sep 15, 2016

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

lgray commented Aug 25, 2016 •

edited

cmsbuild commented Aug 26, 2016 •

edited

cmsbuild commented Aug 26, 2016 •

edited

cmsbuild commented Sep 14, 2016 •

edited

lgray commented Sep 14, 2016 •

edited