Use esConsumes in CosmicSeedGenerator #35258

Dr15Jones · 2021-09-13T20:41:47Z

PR description:

Modified helpers used by CosmicSeedGenerator to either no longer depend no the EventSetup or to use esConsumes.

PR validation:

Code compiles.

Now pass in the necessary ES data product.

Now pass in the needed EventSetup data products.

Dr15Jones · 2021-09-13T20:41:58Z

@makortel FYI

cmsbuild · 2021-09-13T20:50:00Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35258/25241

This PR adds an extra 32KB to repository

cmsbuild · 2021-09-13T20:50:22Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

RecoPixelVertexing/PixelTriplets (reconstruction)
RecoTracker/SpecialSeedGenerators (reconstruction)
RecoTracker/TkHitPairs (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @ebrondol, @gpetruc, @mmusich, @mtosi, @dgulhan this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

makortel · 2021-09-13T20:56:28Z

@cmsbuild, please test

cmsbuild · 2021-09-14T00:33:03Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-aa7d62/18572/summary.html
COMMIT: f13a105
CMSSW: CMSSW_12_1_X_2021-09-13-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35258/18572/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 5 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-aa7d62/18572/llvm-analysis/esrget-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3001001
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000979
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-16T17:55:15Z

RecoTracker/SpecialSeedGenerators/src/SeedGeneratorForCRack.cc

+  edm::ESHandle<GeometricSearchTracker> track;
+  iSetup.get<TrackerRecoGeometryRecord>().get(track);
+  edm::ESHandle<TrackerTopology> httopo;
+  iSetup.get<TrackerTopologyRcd>().get(httopo);


is this and 4 more es::get left for another PR?

That class apparently isn't used in any of the RelVals so its conversion isn't pressing.

This is occasionally used by the SiStrip group to test reconstruction with the Cosmic Rack, I can have a look next week.

RecoTracker/TkHitPairs/src/CosmicHitPairGeneratorFromLayerPair.cc

slava77 · 2021-09-16T18:03:31Z

RecoTracker/TkHitPairs/src/CosmicHitPairGeneratorFromLayerPair.cc

+  /*
+  class CompareHitPairsY {
+  public:


is this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant
... here it looks like a comment like // uncomment if the sort operation commented out below needs to be called

I'm extending the multiline comment to include the sort part so it is obvious they belong together.

RecoTracker/TkHitPairs/interface/CosmicLayerPairs.h

cmsbuild · 2021-09-16T19:00:02Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35258/25330

This PR adds an extra 36KB to repository

cmsbuild · 2021-09-16T19:00:31Z

Pull request #35258 was updated. @jpata, @cmsbuild, @slava77 can you please check and sign again.

Dr15Jones · 2021-09-16T19:01:58Z

please tset

slava77 · 2021-09-17T13:20:06Z

@cmsbuild please test

cmsbuild · 2021-09-17T16:40:18Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-aa7d62/18706/summary.html
COMMIT: 069dc6c
CMSSW: CMSSW_12_1_X_2021-09-17-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35258/18706/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 5 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-aa7d62/18706/llvm-analysis/esrget-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000833
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000805
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-20T15:44:02Z

+reconstruction

for #35258 069dc6c

code changes are technical, in line with the PR description and the follow up review
- remaining es.get calls in SeedGeneratorForCRack.cc are not in the workflows tested in IBs and can be followed up later by the POG developers
jenkins tests pass and comparisons with the baseline show no (relevant) differences

cmsbuild · 2021-09-20T15:44:21Z

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 will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-09-20T22:18:12Z

+1

Dr15Jones added 5 commits September 13, 2021 13:45

Remove EventSetup dependency from CosmicLayerPairs

8a32284

Remove EventSetup use in CosmicHitPairGeneratorFromLayerPair

e9f86b5

Now pass in the necessary ES data product.

Removed EventSetup use in CosmicLayerTriplets

aa435c0

Now pass in the needed EventSetup data products.

Removed Eventsetup dependency from CosmicHitTripletGenerator

bd47432

Added esConsumes calls to SeedGeneratorForCosmics

f13a105

cmsbuild added this to the CMSSW_12_1_X milestone Sep 13, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 13, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 13, 2021

makortel mentioned this pull request Sep 13, 2021

Use esConsumes in CosmicSeedGenerator cms-sw/framework-team#267

Closed

cmsbuild added tests-started and removed tests-pending labels Sep 13, 2021

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

slava77 reviewed Sep 16, 2021

View reviewed changes

Better handling of commented out code

069dc6c

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Sep 16, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 16, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 17, 2021

cmsbuild added tests-approved and removed tests-started labels Sep 17, 2021

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Sep 20, 2021

cmsbuild added orp-approved and removed orp-pending labels Sep 20, 2021

cmsbuild merged commit ff2ea8e into cms-sw:master Sep 20, 2021

Dr15Jones deleted the esConsumesCosmicSeedGenerator branch September 21, 2021 18:44

mmusich mentioned this pull request Sep 22, 2021

finalize esConsumes for RecoTracker/SpecialSeedGenerators #35367

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use esConsumes in CosmicSeedGenerator #35258

Use esConsumes in CosmicSeedGenerator #35258

Dr15Jones commented Sep 13, 2021

Dr15Jones commented Sep 13, 2021

cmsbuild commented Sep 13, 2021

cmsbuild commented Sep 13, 2021

makortel commented Sep 13, 2021

cmsbuild commented Sep 14, 2021

slava77 Sep 16, 2021

Dr15Jones Sep 16, 2021

mmusich Sep 19, 2021

mmusich Sep 22, 2021

slava77 Sep 16, 2021

Dr15Jones Sep 16, 2021

cmsbuild commented Sep 16, 2021

cmsbuild commented Sep 16, 2021

Dr15Jones commented Sep 16, 2021

slava77 commented Sep 17, 2021

cmsbuild commented Sep 17, 2021

slava77 commented Sep 20, 2021

cmsbuild commented Sep 20, 2021

perrotta commented Sep 20, 2021

Use esConsumes in CosmicSeedGenerator #35258

Use esConsumes in CosmicSeedGenerator #35258

Conversation

Dr15Jones commented Sep 13, 2021

PR description:

PR validation:

Dr15Jones commented Sep 13, 2021

cmsbuild commented Sep 13, 2021

cmsbuild commented Sep 13, 2021

makortel commented Sep 13, 2021

cmsbuild commented Sep 14, 2021

Comparison Summary

slava77 Sep 16, 2021

Choose a reason for hiding this comment

Dr15Jones Sep 16, 2021

Choose a reason for hiding this comment

mmusich Sep 19, 2021

Choose a reason for hiding this comment

mmusich Sep 22, 2021

Choose a reason for hiding this comment

slava77 Sep 16, 2021

Choose a reason for hiding this comment

Dr15Jones Sep 16, 2021

Choose a reason for hiding this comment

cmsbuild commented Sep 16, 2021

cmsbuild commented Sep 16, 2021

Dr15Jones commented Sep 16, 2021

slava77 commented Sep 17, 2021

cmsbuild commented Sep 17, 2021

Comparison Summary

slava77 commented Sep 20, 2021

cmsbuild commented Sep 20, 2021

perrotta commented Sep 20, 2021