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

guitargeek · 2021-07-25T09:27:34Z

PR description:

Part of the series of PRs that merges .h files with the .cc files of the plugins in the PhysicsTools subsystem, continuing the work from #34594.

It makes maintaining the plugins much easier and also excludes the possibility to wrongly include plugin header files.

There were the steps taken to merge the files:

copy-paste
sort includes
removed duplicate and unnecessary includes

PR validation:

CMSSW compiles.

if this PR is a backport please specify the original PR and why you need to backport that PR:

No backport intended.

cmsbuild · 2021-07-25T09:56:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34615/24192

This PR adds an extra 28KB to repository

cmsbuild · 2021-07-25T09:56:38Z

A new Pull Request was created by @guitargeek (Jonas Rembser) for master.

It involves the following packages:

PhysicsTools/IsolationAlgos (reconstruction)
PhysicsTools/PatAlgos (reconstruction)
PhysicsTools/TagAndProbe (analysis)

@perrotta, @jpata, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @gouskos, @JyothsnaKomaragiri, @ahinzmann, @schoef, @rappoccio, @swozniewski, @jdamgov, @jdolen, @nhanvtran, @gkasieczka, @clelange, @emilbols, @hatakeyamak, @mbluj, @gpetruc, @andrzejnovak, @mariadalfonso, @seemasharmafnal this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy, @perrotta you are the release manager for this.

cms-bot commands are listed here

perrotta · 2021-07-25T10:57:42Z

please test

cmsbuild · 2021-07-25T16:04:27Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49923b/17178/summary.html
COMMIT: 0673c6e
CMSSW: CMSSW_12_0_X_2021-07-25-0000/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34615/17178/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 1 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49923b/17178/llvm-analysis/legacy-mod-sa.txt for details.

Comparison Summary

Summary:

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

jpata

Thanks for this refactoring! Please see inline for a few questions.

jpata · 2021-07-27T08:46:20Z

PhysicsTools/PatAlgos/plugins/TauJetCorrFactorsProducer.cc

  // get tau-jet collection from the event
-  edm::Handle<edm::View<reco::BaseTau> > tauJets;
-  evt.getByToken(srcToken_, tauJets);
+  auto tauJets = evt.getHandle(srcToken_);


since the Handle is not used later, it seems like this could work too?

Suggested change

auto tauJets = evt.getHandle(srcToken_);

const auto& tauJets = evt.get(srcToken_);

Okay right, but this commit is outdated now because the esConsumes migration of the TauJetCorrFactorsProducer was done in #34997.

jpata · 2021-07-27T08:59:49Z

PhysicsTools/PatAlgos/plugins/TauJetCorrFactorsProducer.cc

-      es.get<JetCorrectionsRecord>().get(payload, jecParameters);
-
-      correctorMapping[payload] = std::make_shared<FactorizedJetCorrector>(params(*jecParameters, levels_));
+    const int payloadTokenValue = payloadToken->index().value();


This is a nice refactoring to go from a string to an int in the map, but could you comment if it's guaranteed to give the same result? It's not immediate (to me at least) what index().value() corresponds to here.

Okay right, but this commit is outdated now because the esConsumes migration of the TauJetCorrFactorsProducer was done in #34997.

jpata · 2021-08-09T13:45:06Z

kind ping @guitargeek, just in case you are around

jpata · 2021-08-23T13:46:16Z

kind ping @guitargeek

jpata · 2021-08-30T13:45:18Z

kind ping @guitargeek
there's also a conflict.

cmsbuild · 2021-08-31T17:36:32Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34615/24979

This PR adds an extra 12KB to repository

cmsbuild · 2021-08-31T17:36:55Z

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

jpata · 2021-09-01T07:14:51Z

@cmsbuild please test

cmsbuild · 2021-09-01T10:35:29Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49923b/18188/summary.html
COMMIT: d363c3b
CMSSW: CMSSW_12_1_X_2021-08-31-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34615/18188/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 1 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49923b/18188/llvm-analysis/legacy-mod-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 7 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000404
DQMHistoTests: Total failures: 11
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3000370
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

jpata · 2021-09-01T11:49:43Z

+reconstruction

technical
reco differences in ParticleNet and DeepTau look like from Non-reproducibility in DeepTau in 1325.81 #32628 and are unrelated, nothing else observed

perrotta · 2021-09-01T14:08:36Z

+1

perrotta · 2021-09-01T14:09:33Z

merge

santocch · 2021-09-14T07:23:31Z

+1

cmsbuild · 2021-09-14T07:23:53Z

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 be automatically merged.

cmsbuild added this to the CMSSW_12_0_X milestone Jul 25, 2021

cmsbuild added analysis-pending code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jul 25, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 25, 2021

cmsbuild added tests-started and removed tests-pending labels Jul 25, 2021

cmsbuild added tests-approved and removed tests-started labels Jul 25, 2021

jpata reviewed Jul 27, 2021

View reviewed changes

smuzaffar modified the milestones: CMSSW_12_0_X, CMSSW_12_1_X Jul 30, 2021

cmsbuild mentioned this pull request Aug 24, 2021

use esConsumes in PhysicsTools/PatAlgos modules #34997

Merged

Merge .h and .cc files of some more PhysicsTools plugins

d363c3b

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

guitargeek changed the title ~~Merge .h and .cc files of plugins in several PhysicsTools packages and esConsumes migration of TauJetCorrFactorsProducer~~ Merge .h and .cc files of plugins in several PhysicsTools packages Aug 31, 2021

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

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

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

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

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

cmsbuild merged commit 729ff10 into cms-sw:master Sep 1, 2021

guitargeek deleted the PhysicsTools_2 branch September 1, 2021 14:13

cmsbuild added analysis-approved fully-signed and removed analysis-pending pending-signatures labels Sep 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

guitargeek commented Jul 25, 2021 •

edited

Loading

cmsbuild commented Jul 25, 2021

cmsbuild commented Jul 25, 2021

perrotta commented Jul 25, 2021

cmsbuild commented Jul 25, 2021

jpata left a comment

jpata Jul 27, 2021

guitargeek Aug 31, 2021

jpata Jul 27, 2021

guitargeek Aug 31, 2021

jpata commented Aug 9, 2021

jpata commented Aug 23, 2021

jpata commented Aug 30, 2021

cmsbuild commented Aug 31, 2021

cmsbuild commented Aug 31, 2021

jpata commented Sep 1, 2021

cmsbuild commented Sep 1, 2021

jpata commented Sep 1, 2021

perrotta commented Sep 1, 2021

perrotta commented Sep 1, 2021

santocch commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

	auto tauJets = evt.getHandle(srcToken_);
	const auto& tauJets = evt.get(srcToken_);

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

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

Conversation

guitargeek commented Jul 25, 2021 • edited Loading

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Jul 25, 2021

cmsbuild commented Jul 25, 2021

perrotta commented Jul 25, 2021

cmsbuild commented Jul 25, 2021

Comparison Summary

jpata left a comment

Choose a reason for hiding this comment

jpata Jul 27, 2021

Choose a reason for hiding this comment

guitargeek Aug 31, 2021

Choose a reason for hiding this comment

jpata Jul 27, 2021

Choose a reason for hiding this comment

guitargeek Aug 31, 2021

Choose a reason for hiding this comment

jpata commented Aug 9, 2021

jpata commented Aug 23, 2021

jpata commented Aug 30, 2021

cmsbuild commented Aug 31, 2021

cmsbuild commented Aug 31, 2021

jpata commented Sep 1, 2021

cmsbuild commented Sep 1, 2021

Comparison Summary

jpata commented Sep 1, 2021

perrotta commented Sep 1, 2021

perrotta commented Sep 1, 2021

santocch commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

guitargeek commented Jul 25, 2021 •

edited

Loading