Added esConsumes to PhysicsTools/PatUtils #35680

Dr15Jones · 2021-10-14T17:33:34Z

PR description:

added esConsumes calls to modules
switched to using std::unique_ptr
removed edm::Event and edm::EventSetup dependencies from LeptonVertexSignificance

PR validation:

Code compiles.

The constructor of the class would never be able to be called properly from cmsRun.

-Also switched to using std::unique_ptr

cmsbuild · 2021-10-14T17:45:57Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35680/25971

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File PhysicsTools/PatUtils/interface/LeptonVertexSignificance.h modified in PR(s): Remove LeptonVertexSignificance as obsolete #35612
- File PhysicsTools/PatUtils/src/LeptonVertexSignificance.cc modified in PR(s): Remove LeptonVertexSignificance as obsolete #35612

cmsbuild · 2021-10-14T17:46:17Z

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

It involves the following packages:

PhysicsTools/PatUtils (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@AlexDeMoor, @rappoccio, @gouskos, @JyothsnaKomaragiri, @ahinzmann, @schoef, @mmarionncern, @jdamgov, @jdolen, @nhanvtran, @gkasieczka, @clelange, @emilbols, @hatakeyamak, @gpetruc, @andrzejnovak, @demuller, @mariadalfonso, @seemasharmafnal 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-10-14T17:50:08Z

please test

cmsbuild · 2021-10-14T21:08:20Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-585fe9/19642/summary.html
COMMIT: f611518
CMSSW: CMSSW_12_1_X_2021-10-14-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35680/19642/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: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 2796107
DQMHistoTests: Total failures: 10
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2796074
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 39 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 169 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

slava77

all comments are for LeptonVertexSignificance which has an alternative solution in #35612

perhaps completing the migration is more satisfactory, but I don't have a particularly strong preference.

slava77 · 2021-10-14T22:14:05Z

PhysicsTools/PatUtils/interface/LeptonVertexSignificance.h

-    ~LeptonVertexSignificance();
+    LeptonVertexSignificance() = default;
+    ~LeptonVertexSignificance() = default;
+    ;


Suggested change

;

slava77 · 2021-10-14T22:19:33Z

PhysicsTools/PatUtils/src/LeptonVertexSignificance.cc

-  reco::Vertex theVertex = vertexHandle.product()->front();
+float LeptonVertexSignificance::calculate(const reco::Track& theTrack,
+                                          const reco::VertexCollection& vertex,
+                                          TransientTrackBuilder& builder) {


Suggested change

TransientTrackBuilder& builder) {

const TransientTrackBuilder& builder) {

it looks like the build (and apparently all) method is const

slava77 · 2021-10-14T22:21:04Z

PhysicsTools/PatUtils/src/LeptonVertexSignificance.cc

-    return 0;
-  reco::Vertex theVertex = vertexHandle.product()->front();
+float LeptonVertexSignificance::calculate(const reco::Track& theTrack,
+                                          const reco::VertexCollection& vertex,


Suggested change

const reco::VertexCollection& vertex,

const reco::VertexCollection& vertices,

would be less confusing (or vertexColl

slava77 · 2021-10-14T22:22:34Z

PhysicsTools/PatUtils/src/LeptonVertexSignificance.cc

+float LeptonVertexSignificance::calculate(const reco::Track& theTrack,
+                                          const reco::VertexCollection& vertex,
+                                          TransientTrackBuilder& builder) {
+  reco::Vertex const& theVertex = vertex.front();


I'm missing a check for vertexColl.empty()

Suggested change

reco::Vertex const& theVertex = vertex.front();

if (vertex.empty()) return 0;

reco::Vertex const& theVertex = vertex.front();

@slava77

Changes suggested by @slava77

cmsbuild · 2021-10-15T14:10:14Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35680/25989

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File PhysicsTools/PatUtils/interface/LeptonVertexSignificance.h modified in PR(s): Remove LeptonVertexSignificance as obsolete #35612
- File PhysicsTools/PatUtils/src/LeptonVertexSignificance.cc modified in PR(s): Remove LeptonVertexSignificance as obsolete #35612

cmsbuild · 2021-10-15T14:10:45Z

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

Dr15Jones · 2021-10-15T14:10:48Z

please test

slava77 · 2021-10-15T22:16:26Z

@cmsbuild please test

the last attempt apparently got stuck in the network interruption

cmsbuild · 2021-10-16T01:33:16Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-585fe9/19671/summary.html
COMMIT: 88a57b6
CMSSW: CMSSW_12_1_X_2021-10-15-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35680/19671/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: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 2751113
DQMHistoTests: Total failures: 10
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2751080
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 39 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 170 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

slava77 · 2021-10-16T21:36:38Z

+reconstruction

for #35680 88a57b6

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

cmsbuild · 2021-10-16T21:37:07Z

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)

qliphy · 2021-10-17T01:54:28Z

+1

Dr15Jones added 2 commits October 14, 2021 12:29

Made LeptonVertexSignificance independent of Event and EventSetup

e0e66ae

The constructor of the class would never be able to be called properly from cmsRun.

Add esConsumes calls to modules in PhysicsTools/PatUtils

f611518

-Also switched to using std::unique_ptr

cmsbuild added this to the CMSSW_12_1_X milestone Oct 14, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 14, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 14, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 14, 2021

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

slava77 reviewed Oct 14, 2021

View reviewed changes

Improvements to LeptonVertexSignificance

88a57b6

Changes suggested by @slava77

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Oct 15, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 15, 2021

cmsbuild removed the tests-pending label Oct 15, 2021

cmsbuild added the tests-started label Oct 15, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 16, 2021

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Oct 16, 2021

slava77 mentioned this pull request Oct 16, 2021

Remove LeptonVertexSignificance as obsolete #35612

Closed

cmsbuild added orp-approved and removed orp-pending labels Oct 17, 2021

cmsbuild merged commit ff55d32 into cms-sw:master Oct 17, 2021

cmsbuild mentioned this pull request Oct 18, 2021

rootmodule: Updated root to tip of branch master cms-sw/cmsdist#7393

Merged

Dr15Jones deleted the deprecatedPhysicsToolsPatUtils branch October 20, 2021 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added esConsumes to PhysicsTools/PatUtils #35680

Added esConsumes to PhysicsTools/PatUtils #35680

Dr15Jones commented Oct 14, 2021

cmsbuild commented Oct 14, 2021

cmsbuild commented Oct 14, 2021

Dr15Jones commented Oct 14, 2021

cmsbuild commented Oct 14, 2021

slava77 left a comment

slava77 Oct 14, 2021

slava77 Oct 14, 2021

slava77 Oct 14, 2021

slava77 Oct 14, 2021

cmsbuild commented Oct 15, 2021

cmsbuild commented Oct 15, 2021

Dr15Jones commented Oct 15, 2021

slava77 commented Oct 15, 2021

cmsbuild commented Oct 16, 2021

slava77 commented Oct 16, 2021

cmsbuild commented Oct 16, 2021

qliphy commented Oct 17, 2021

	TransientTrackBuilder& builder) {
	const TransientTrackBuilder& builder) {

	const reco::VertexCollection& vertex,
	const reco::VertexCollection& vertices,

	reco::Vertex const& theVertex = vertex.front();
	if (vertex.empty()) return 0;
	reco::Vertex const& theVertex = vertex.front();

Added esConsumes to PhysicsTools/PatUtils #35680

Added esConsumes to PhysicsTools/PatUtils #35680

Conversation

Dr15Jones commented Oct 14, 2021

PR description:

PR validation:

cmsbuild commented Oct 14, 2021

cmsbuild commented Oct 14, 2021

Dr15Jones commented Oct 14, 2021

cmsbuild commented Oct 14, 2021

Comparison Summary

slava77 left a comment

Choose a reason for hiding this comment

slava77 Oct 14, 2021

Choose a reason for hiding this comment

slava77 Oct 14, 2021

Choose a reason for hiding this comment

slava77 Oct 14, 2021

Choose a reason for hiding this comment

slava77 Oct 14, 2021

Choose a reason for hiding this comment

cmsbuild commented Oct 15, 2021

cmsbuild commented Oct 15, 2021

Dr15Jones commented Oct 15, 2021

slava77 commented Oct 15, 2021

cmsbuild commented Oct 16, 2021

Comparison Summary

slava77 commented Oct 16, 2021

cmsbuild commented Oct 16, 2021

qliphy commented Oct 17, 2021