use esConsumes in PhysicsTools/PatAlgos modules #34997

Dr15Jones · 2021-08-24T16:16:53Z

PR description:

Converted all modules to use missing esConsumes.

PR validation:

Code compiles.

cmsbuild · 2021-08-24T16:25:32Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34997/24852

This PR adds an extra 88KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File PhysicsTools/PatAlgos/plugins/PATIsolatedTrackProducer.cc modified in PR(s): Add track-based isolation, recHit energy sum to IsolatedTrack class #31399
- File PhysicsTools/PatAlgos/plugins/TauJetCorrFactorsProducer.cc modified in PR(s): Merge .h and .cc files of plugins in several PhysicsTools packages #34615

cmsbuild · 2021-08-24T16:25:54Z

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

It involves the following packages:

PhysicsTools/PatAlgos (reconstruction)
RecoEgamma/EgammaTools (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @jainshilpi, @schoef, @emilbols, @mbluj, @demuller, @varuns23, @seemasharmafnal, @mmarionncern, @ahinzmann, @lgray, @jdolen, @Sam-Harper, @jdamgov, @nhanvtran, @gkasieczka, @hatakeyamak, @andrzejnovak, @clelange, @AlexDeMoor, @swozniewski, @JyothsnaKomaragiri, @sobhatta, @afiqaize, @gpetruc, @mariadalfonso, @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-24T16:29:25Z

please test

makortel · 2021-08-24T19:04:31Z

JME::JetResolution::get

JME::JetResolutionScaleFactor::get

Given that these functions do not seem to provide anything beyond EventSetup::getData() (except hiding the Record type, but that is needed for the ESGetToken anyway)

cmssw/JetMETCorrections/Modules/src/JetResolution.cc

Lines 23 to 28 in 00ca4ca

    
           const JetResolution JetResolution::get(const edm::EventSetup& setup, const std::string& label) { 
        
             edm::ESHandle<JetResolutionObject> handle; 
        
             setup.get<JetResolutionRcd>().get(label, handle); 
        
             return *handle; 
        
           }

I have elsewhere just replaced their use with direct use of EventSetup::getData() in the calling code.

Dr15Jones · 2021-08-24T19:06:15Z

I have elsewhere just replaced their use with direct use of EventSetup::getData() in the calling code.

That is what I was thinking, but I figured I'd want to do it systematically and then remove the interface.

cmsbuild · 2021-08-24T19:44:05Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6dbb72/18002/summary.html
COMMIT: d4863ee
CMSSW: CMSSW_12_1_X_2021-08-24-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34997/18002/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: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000330
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-08-24T19:52:28Z

I should have looked closer, TrackDetectorAssociator was already updated for esConsumes.

Instead, just get data directly from the EventSetup using the proper esConsumes.

Dr15Jones · 2021-08-24T20:44:19Z

I went ahead and removed the JetResolution*::get calls.

cmsbuild · 2021-08-24T20:49:29Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34997/24859

This PR adds an extra 88KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File PhysicsTools/PatAlgos/plugins/PATIsolatedTrackProducer.cc modified in PR(s): Add track-based isolation, recHit energy sum to IsolatedTrack class #31399
- File PhysicsTools/PatAlgos/plugins/TauJetCorrFactorsProducer.cc modified in PR(s): Merge .h and .cc files of plugins in several PhysicsTools packages #34615

cmsbuild · 2021-08-24T20:49:51Z

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

Dr15Jones · 2021-08-24T20:50:38Z

please test

cmsbuild · 2021-08-24T23:57:27Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6dbb72/18009/summary.html
COMMIT: b7f0296
CMSSW: CMSSW_12_1_X_2021-08-24-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34997/18009/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: 5
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3000324
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 38 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-08-26T13:08:04Z

+reconstruction

for #34997 b7f0296

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

cmsbuild · 2021-08-26T13:08:30Z

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-08-27T14:25:42Z

PhysicsTools/PatAlgos/plugins/PATJetSlimmer.cc

 void pat::PATJetSlimmer::produce(edm::Event& iEvent, const edm::EventSetup& iSetup) {
  using namespace edm;
  using namespace std;

+  if (modifyJet_)
+    jetModifier_->setEventContent(iSetup);


It was evidently done on purpose, and therefore I can imagine the answer: but is it really convenient calling this method once per event, instead of once per LuminosityBlock

The transition for the get call is set at compile time in the call to esConsumes. Making helper classes (which can be used by multiple modules) have different consumes for different modules is pretty annoying and error prone. Plus, getting data from the EventSetup is VERY fast (and so we recommend always getting it in the transition it is used as long as no further big computation is done with it).

perrotta · 2021-08-27T14:33:22Z

+1

Dr15Jones added 2 commits August 24, 2021 11:11

Use ESGetToken with EGRegressionModifierV3

e21f703

Use esConsumes for PhysicsTools/PatAlgos modules

d4863ee

cmsbuild added this to the CMSSW_12_1_X milestone Aug 24, 2021

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

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

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

makortel mentioned this pull request Aug 24, 2021

Migrate PAT EDProducers to esConsumes cms-sw/framework-team#246

Closed

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

Avoid calling JetResolution*::get

b7f0296

Instead, just get data directly from the EventSetup using the proper esConsumes.

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

cmsbuild removed the code-checks-pending label Aug 24, 2021

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

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

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

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Aug 26, 2021

perrotta reviewed Aug 27, 2021

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Aug 27, 2021

cmsbuild merged commit b60b388 into cms-sw:master Aug 27, 2021

cmsbuild mentioned this pull request Aug 27, 2021

Removed undefined use of union in libminifloat #35054

Merged

guitargeek mentioned this pull request Aug 31, 2021

Merge .h and .cc files of plugins in several PhysicsTools packages #34615

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use esConsumes in PhysicsTools/PatAlgos modules #34997

use esConsumes in PhysicsTools/PatAlgos modules #34997

Dr15Jones commented Aug 24, 2021 •

edited

cmsbuild commented Aug 24, 2021

cmsbuild commented Aug 24, 2021

Dr15Jones commented Aug 24, 2021

makortel commented Aug 24, 2021 •

edited

Dr15Jones commented Aug 24, 2021

cmsbuild commented Aug 24, 2021

Dr15Jones commented Aug 24, 2021

Dr15Jones commented Aug 24, 2021

cmsbuild commented Aug 24, 2021

cmsbuild commented Aug 24, 2021

Dr15Jones commented Aug 24, 2021

cmsbuild commented Aug 24, 2021

slava77 commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

perrotta Aug 27, 2021

Dr15Jones Aug 27, 2021

perrotta commented Aug 27, 2021

use esConsumes in PhysicsTools/PatAlgos modules #34997

use esConsumes in PhysicsTools/PatAlgos modules #34997

Conversation

Dr15Jones commented Aug 24, 2021 • edited

PR description:

PR validation:

cmsbuild commented Aug 24, 2021

cmsbuild commented Aug 24, 2021

Dr15Jones commented Aug 24, 2021

makortel commented Aug 24, 2021 • edited

Dr15Jones commented Aug 24, 2021

cmsbuild commented Aug 24, 2021

Comparison Summary

Dr15Jones commented Aug 24, 2021

Dr15Jones commented Aug 24, 2021

cmsbuild commented Aug 24, 2021

cmsbuild commented Aug 24, 2021

Dr15Jones commented Aug 24, 2021

cmsbuild commented Aug 24, 2021

Comparison Summary

slava77 commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

perrotta Aug 27, 2021

Choose a reason for hiding this comment

Dr15Jones Aug 27, 2021

Choose a reason for hiding this comment

perrotta commented Aug 27, 2021

Dr15Jones commented Aug 24, 2021 •

edited

makortel commented Aug 24, 2021 •

edited