Modernize some EGamma Reco modules #35021

Dr15Jones · 2021-08-26T00:26:48Z

PR description:

added remaining esConsumes calls.
updated helper classes to use ConsumesCollector.
Applied some code modernization
reworked LowPtGsfElectronIDProducer to be more efficient

PR validation:

Code compiles. None of the changes are meant to change behavior.

Users now pass in the required data products to the constructor.

- used esConsumes - avoid doing large numbers of string constructions and comparisons.

This avoids having to keep all the model results in memory at one time.

Some code-formatting also snuck in.

Dr15Jones · 2021-08-26T00:28:13Z

RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronIDProducer.cc

-  const std::string version_;
+  const std::string versionName_;
+  enum class Version { V0, V1 };
+  Version version_;


This addition avoid doing a string comparison for each module*electron on the version name.

Dr15Jones · 2021-08-26T00:29:02Z

RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronIDProducer.cc

    std::ostringstream os;
    os << "Problem accessing rho collection for low-pT electrons" << std::endl;
    throw cms::Exception("InvalidHandle", os.str());
  }

+  double rho = *hRho;


Handles have condition logic when dereferencing so best not do dereference in tight loop.

cmsbuild · 2021-08-26T00:34:35Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35021/24881

This PR adds an extra 64KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoEcal/EgammaClusterProducers/src/EgammaSCCorrectionMaker.cc modified in PR(s): Migrate EgammaSCCorrectionMaker to esConsumes() #35019
- File RecoEcal/EgammaCoreTools/interface/EcalClusterFunctionFactory.h modified in PR(s): Migrate EgammaSCCorrectionMaker to esConsumes() #35019
- File RecoEcal/EgammaCoreTools/plugins/EcalClusterCrackCorrection.cc modified in PR(s): Migrate EgammaSCCorrectionMaker to esConsumes() #35019
- File RecoEcal/EgammaCoreTools/plugins/EcalClusterEnergyCorrection.cc modified in PR(s): Migrate EgammaSCCorrectionMaker to esConsumes() #35019
- File RecoEcal/EgammaCoreTools/plugins/EcalClusterEnergyCorrectionObjectSpecific.cc modified in PR(s): Migrate EgammaSCCorrectionMaker to esConsumes() #35019
- File RecoEcal/EgammaCoreTools/plugins/EcalClusterEnergyUncertainty.cc modified in PR(s): Migrate EgammaSCCorrectionMaker to esConsumes() #35019
- File RecoEcal/EgammaCoreTools/test/testEcalClusterFunctions.cc modified in PR(s): Migrate EgammaSCCorrectionMaker to esConsumes() #35019
- File RecoEgamma/EgammaElectronProducers/plugins/GsfElectronProducer.cc modified in PR(s): Migrate EgammaSCCorrectionMaker to esConsumes() #35019
- File RecoEgamma/EgammaPhotonAlgos/src/PhotonEnergyCorrector.cc modified in PR(s): Migrate EgammaSCCorrectionMaker to esConsumes() #35019

cmsbuild · 2021-08-26T00:34:57Z

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

It involves the following packages:

RecoEcal/EgammaClusterProducers (reconstruction)
RecoEcal/EgammaCoreTools (reconstruction)
RecoEgamma/EgammaElectronAlgos (reconstruction)
RecoEgamma/EgammaElectronProducers (reconstruction)
RecoEgamma/EgammaPhotonAlgos (reconstruction)
TrackingTools/GsfTracking (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @jainshilpi, @argiro, @thomreis, @varuns23, @JanFSchulte, @lgray, @simonepigazzini, @Sam-Harper, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @ebrondol, @mtosi, @dgulhan, @rchatter, @sobhatta, @lecriste, @afiqaize, @gpetruc, @mmusich, @ram1123 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

Dr15Jones · 2021-08-26T01:05:10Z

please test

cmsbuild · 2021-08-26T06:10:33Z

-1

Failed Tests: RelVals AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9ff3a4/18041/summary.html
COMMIT: 18f1955
CMSSW: CMSSW_12_1_X_2021-08-25-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35021/18041/install.sh to create a dev area with all the needed externals and cmssw changes.

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

RelVals

4.22

----- Begin Fatal Exception 26-Aug-2021 07:35:23 CEST-----------------------
An exception of category 'ESGetTokenWrongTransition' occurred while
   [0] Processing  stream begin Run run: 160960 stream: 0
   [1] Calling method for module GEDPhotonProducer/'photons'
Exception Message:
The transition ID stored in the ESGetToken does not match the
transition where the token is being used. The associated record
type is: EcalClusterEnergyCorrectionParametersRcd
For producers, filters and analyzers this transition ID is
set as a template parameter to the call to the esConsumes
function that creates the token. Event is the default transition.
Other possibilities are BeginRun, EndRun, BeginLuminosityBlock,
or EndLuminosityBlock. You may need multiple tokens if you want to
get the same data in multiple transitions. The transition ID has a
different meaning in ESProducers. For ESProducers, the transition
ID identifies the function that produces the EventSetup data (often
there is one function named produce but there can be multiple
functions with different names). For ESProducers, the ESGetToken
must be used in the function associated with the ESConsumesCollector
returned by the setWhatProduced function.
----- End Fatal Exception -------------------------------------------------

5.1

----- Begin Fatal Exception 26-Aug-2021 07:35:45 CEST-----------------------
An exception of category 'ESGetTokenWrongTransition' occurred while
   [0] Processing  stream begin Run run: 1 stream: 0
   [1] Calling method for module GEDPhotonProducer/'photons'
Exception Message:
The transition ID stored in the ESGetToken does not match the
transition where the token is being used. The associated record
type is: EcalClusterEnergyCorrectionParametersRcd
For producers, filters and analyzers this transition ID is
set as a template parameter to the call to the esConsumes
function that creates the token. Event is the default transition.
Other possibilities are BeginRun, EndRun, BeginLuminosityBlock,
or EndLuminosityBlock. You may need multiple tokens if you want to
get the same data in multiple transitions. The transition ID has a
different meaning in ESProducers. For ESProducers, the transition
ID identifies the function that produces the EventSetup data (often
there is one function named produce but there can be multiple
functions with different names). For ESProducers, the ESGetToken
must be used in the function associated with the ESConsumesCollector
returned by the setWhatProduced function.
----- End Fatal Exception -------------------------------------------------

135.4

----- Begin Fatal Exception 26-Aug-2021 07:35:46 CEST-----------------------
An exception of category 'ESGetTokenWrongTransition' occurred while
   [0] Processing  stream begin Run run: 1 stream: 0
   [1] Calling method for module GEDPhotonProducer/'photons'
Exception Message:
The transition ID stored in the ESGetToken does not match the
transition where the token is being used. The associated record
type is: EcalClusterEnergyCorrectionParametersRcd
For producers, filters and analyzers this transition ID is
set as a template parameter to the call to the esConsumes
function that creates the token. Event is the default transition.
Other possibilities are BeginRun, EndRun, BeginLuminosityBlock,
or EndLuminosityBlock. You may need multiple tokens if you want to
get the same data in multiple transitions. The transition ID has a
different meaning in ESProducers. For ESProducers, the transition
ID identifies the function that produces the EventSetup data (often
there is one function named produce but there can be multiple
functions with different names). For ESProducers, the ESGetToken
must be used in the function associated with the ESConsumesCollector
returned by the setWhatProduced function.
----- End Fatal Exception -------------------------------------------------

Expand to see more relval errors ...

AddOn Tests

fastsim

----- Begin Fatal Exception 26-Aug-2021 07:36:16 CEST-----------------------
An exception of category 'ESGetTokenWrongTransition' occurred while
   [0] Processing  stream begin Run run: 1 stream: 1
   [1] Calling method for module GEDPhotonProducer/'photons'
Exception Message:
The transition ID stored in the ESGetToken does not match the
transition where the token is being used. The associated record
type is: EcalClusterEnergyCorrectionParametersRcd
For producers, filters and analyzers this transition ID is
set as a template parameter to the call to the esConsumes
function that creates the token. Event is the default transition.
Other possibilities are BeginRun, EndRun, BeginLuminosityBlock,
or EndLuminosityBlock. You may need multiple tokens if you want to
get the same data in multiple transitions. The transition ID has a
different meaning in ESProducers. For ESProducers, the transition
ID identifies the function that produces the EventSetup data (often
there is one function named produce but there can be multiple
functions with different names). For ESProducers, the ESGetToken
must be used in the function associated with the ESConsumesCollector
returned by the setWhatProduced function.
----- End Fatal Exception -------------------------------------------------

fastsim1

----- Begin Fatal Exception 26-Aug-2021 07:36:14 CEST-----------------------
An exception of category 'ESGetTokenWrongTransition' occurred while
   [0] Processing  stream begin Run run: 1 stream: 1
   [1] Calling method for module GEDPhotonProducer/'photons'
Exception Message:
The transition ID stored in the ESGetToken does not match the
transition where the token is being used. The associated record
type is: EcalClusterEnergyCorrectionParametersRcd
For producers, filters and analyzers this transition ID is
set as a template parameter to the call to the esConsumes
function that creates the token. Event is the default transition.
Other possibilities are BeginRun, EndRun, BeginLuminosityBlock,
or EndLuminosityBlock. You may need multiple tokens if you want to
get the same data in multiple transitions. The transition ID has a
different meaning in ESProducers. For ESProducers, the transition
ID identifies the function that produces the EventSetup data (often
there is one function named produce but there can be multiple
functions with different names). For ESProducers, the ESGetToken
must be used in the function associated with the ESConsumesCollector
returned by the setWhatProduced function.
----- End Fatal Exception -------------------------------------------------

fastsim2

----- Begin Fatal Exception 26-Aug-2021 07:36:24 CEST-----------------------
An exception of category 'ESGetTokenWrongTransition' occurred while
   [0] Processing  stream begin Run run: 1 stream: 2
   [1] Calling method for module GEDPhotonProducer/'photons'
Exception Message:
The transition ID stored in the ESGetToken does not match the
transition where the token is being used. The associated record
type is: EcalClusterEnergyCorrectionParametersRcd
For producers, filters and analyzers this transition ID is
set as a template parameter to the call to the esConsumes
function that creates the token. Event is the default transition.
Other possibilities are BeginRun, EndRun, BeginLuminosityBlock,
or EndLuminosityBlock. You may need multiple tokens if you want to
get the same data in multiple transitions. The transition ID has a
different meaning in ESProducers. For ESProducers, the transition
ID identifies the function that produces the EventSetup data (often
there is one function named produce but there can be multiple
functions with different names). For ESProducers, the ESGetToken
must be used in the function associated with the ESConsumesCollector
returned by the setWhatProduced function.
----- End Fatal Exception -------------------------------------------------

Expand to see more addon errors ...

thomreis · 2021-08-26T08:43:59Z

Looks like there is some overlap with PR #35019

makortel · 2021-08-26T12:44:18Z

The test error could be fixed with de70d49

(after that I could close #35019 in favor of this one)

…ducer::beginRun() It is called unconditionally from GEDPhotonProducer::produce(), I can not see how the call from beginRun() could do anything additional except it would require different set ESGetTokens wihtin PhotonEnergyCorrector etc compared to other users.

cmsbuild · 2021-08-27T14:50:05Z

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

Dr15Jones · 2021-08-27T14:50:49Z

please test

slava77 · 2021-08-28T20:44:54Z

abort test

30hrs since the last request doesn't look healthy

slava77 · 2021-08-28T20:45:23Z

@cmsbuild please test

cmsbuild · 2021-08-29T00:04:03Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9ff3a4/18122/summary.html
COMMIT: d73e61c
CMSSW: CMSSW_12_1_X_2021-08-27-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35021/18122/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000318
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

Dr15Jones · 2021-09-01T14:00:44Z

Ping @cms-sw/reconstruction-l2

slava77 · 2021-09-01T15:35:58Z

+reconstruction

for #35021 d73e61c

code changes are technical, in line with the PR description and the follow up review
jenkins tests pass and comparisons with the baseline show no relevant differences

cmsbuild · 2021-09-01T15:36:17Z

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-01T15:49:26Z

+1

Dr15Jones added 8 commits August 25, 2021 14:50

GsfConstraintAtVertex no longer accesses the EventSetup

432a700

Users now pass in the required data products to the constructor.

EcalClusterFunctionBaseClass classes use esConsumes

9ceca49

Improved LowPtGsfElectronIDProducer

37d2c89

- used esConsumes - avoid doing large numbers of string constructions and comparisons.

Removed unnecessary use of edm::Ptr

50d6202

consolidate common code patterns into a template function

6284031

Changed loop order to model then electrons

4a3752c

This avoids having to keep all the model results in memory at one time.

Use EDPutTokenT to avoid making a temporary container on the heap

7d4a23c

Some code-formatting also snuck in.

Added missing esConsumes to LowPtGsfElectronSeedProducer

18f1955

cmsbuild added this to the CMSSW_12_1_X milestone Aug 26, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 26, 2021

Dr15Jones commented Aug 26, 2021

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 26, 2021

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

cmsbuild added tests-rejected and removed tests-started labels Aug 26, 2021

thomreis mentioned this pull request Aug 26, 2021

Migrate EgammaSCCorrectionMaker to esConsumes() #35019

Closed

cmsbuild added the code-checks-approved label Aug 27, 2021

cmsbuild added tests-started and removed tests-pending labels Aug 27, 2021

cmsbuild added tests-pending and removed tests-started labels Aug 28, 2021

cmsbuild added tests-started and removed tests-pending labels Aug 28, 2021

cmsbuild added tests-approved and removed tests-started labels Aug 29, 2021

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

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

cmsbuild merged commit 9dccb35 into cms-sw:master Sep 1, 2021

This was referenced Sep 3, 2021

[GCC11] Fix compilation warning for CommonTools #35144

Merged

[GCC11] Fix compilation warning for EcalMatacqDigi #35148

Merged

[GCC11] Fix compilation warning for FW/ToyIntProducers #35143

Merged

Dr15Jones deleted the esConsumesRecoEgamma branch September 9, 2021 17:48

makortel mentioned this pull request Sep 13, 2021

Migrate RecoEgamma/EgammaElectronProducers to esConsumes cms-sw/framework-team#249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize some EGamma Reco modules #35021

Modernize some EGamma Reco modules #35021

Dr15Jones commented Aug 26, 2021

Dr15Jones Aug 26, 2021

Dr15Jones Aug 26, 2021

cmsbuild commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

Dr15Jones commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

thomreis commented Aug 26, 2021

makortel commented Aug 26, 2021

cmsbuild commented Aug 27, 2021

Dr15Jones commented Aug 27, 2021

slava77 commented Aug 28, 2021

slava77 commented Aug 28, 2021

cmsbuild commented Aug 29, 2021

Dr15Jones commented Sep 1, 2021

slava77 commented Sep 1, 2021

cmsbuild commented Sep 1, 2021

perrotta commented Sep 1, 2021

Modernize some EGamma Reco modules #35021

Modernize some EGamma Reco modules #35021

Conversation

Dr15Jones commented Aug 26, 2021

PR description:

PR validation:

Dr15Jones Aug 26, 2021

Choose a reason for hiding this comment

Dr15Jones Aug 26, 2021

Choose a reason for hiding this comment

cmsbuild commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

Dr15Jones commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

RelVals

AddOn Tests

thomreis commented Aug 26, 2021

makortel commented Aug 26, 2021

cmsbuild commented Aug 27, 2021

Dr15Jones commented Aug 27, 2021

slava77 commented Aug 28, 2021

slava77 commented Aug 28, 2021

cmsbuild commented Aug 29, 2021

Comparison Summary

Dr15Jones commented Sep 1, 2021

slava77 commented Sep 1, 2021

cmsbuild commented Sep 1, 2021

perrotta commented Sep 1, 2021