PPS clean code for bad pot #38740

fabferro · 2022-07-14T14:28:07Z

PR description:

This PR is a follow-up of PR #38553 implemented the requested code cleaning. No changes expected.

PR validation:

Run3 data processing.

No backport needed.

@tvami @grzanka

cmsbuild · 2022-07-14T14:34:54Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38740/31055

This PR adds an extra 16KB to repository

cmsbuild · 2022-07-14T14:35:17Z

A new Pull Request was created by @fabferro (Fabrizio Ferro) for master.

It involves the following packages:

RecoPPS/Local (reconstruction)

@jpata, @cmsbuild, @clacaputo can you please review it and eventually sign? Thanks.
@forthommel, @grzanka this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

tvami · 2022-07-15T11:01:07Z

@cmsbuild , please test

cmsbuild · 2022-07-15T14:48:20Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-608883/26259/summary.html
COMMIT: c6e0b2a
CMSSW: CMSSW_12_5_X_2022-07-14-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38740/26259/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: 50
DQMHistoTests: Total histograms compared: 3653966
DQMHistoTests: Total failures: 8
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3653936
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 208 log files, 45 edm output root files, 50 DQM output files
TriggerResults: no differences found

clacaputo · 2022-07-15T16:04:54Z

+reconstruction

follow-up of PR PPS reco patch for bad pot #38553
no reco changes

cmsbuild · 2022-07-15T16:05:20Z

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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2022-07-15T16:18:50Z

RecoPPS/Local/plugins/CTPPSPixelLocalTrackProducer.cc

@@ -65,31 +65,31 @@ class CTPPSPixelLocalTrackProducer : public edm::stream::EDProducer<> {

  edm::InputTag inputTag_;


inputTag_ never used

perrotta · 2022-07-15T16:22:22Z

RecoPPS/Local/plugins/CTPPSPixelLocalTrackProducer.cc

@@ -65,31 +65,31 @@ class CTPPSPixelLocalTrackProducer : public edm::stream::EDProducer<> {

  edm::InputTag inputTag_;


inputTag_ never used

perrotta · 2022-07-15T16:23:31Z

RecoPPS/Local/plugins/CTPPSPixelLocalTrackProducer.cc

@@ -65,31 +65,31 @@ class CTPPSPixelLocalTrackProducer : public edm::stream::EDProducer<> {

  edm::InputTag inputTag_;
  edm::EDGetTokenT<edm::DetSetVector<CTPPSPixelRecHit>> tokenCTPPSPixelRecHit_;


why not to make const all other classs members initialized in the ctor initialization list (and never modified afterwards)?

perrotta · 2022-07-15T16:26:26Z

RecoPPS/Local/plugins/CTPPSPixelLocalTrackProducer.cc

  edm::ESWatcher<VeryForwardRealGeometryRecord> geometryWatcher_;

-  edm::ESGetToken<CTPPSPixelAnalysisMask, CTPPSPixelAnalysisMaskRcd> tokenCTPPSPixelAnalysisMask_;
+  const edm::ESGetToken<CTPPSPixelAnalysisMask, CTPPSPixelAnalysisMaskRcd> tokenCTPPSPixelAnalysisMask_;

  uint32_t numberOfPlanesPerPot_;
  std::vector<uint32_t> listOfAllPlanes_;


This is only used locally in the constructor: no need to make it a class member.
Moreover: why to reserve(6) for it? Are you hardcoding somehow the number of elements that should be passed with the numberOfPlanesPerPot_ parameter instead? In any case, if this variable becomes local in the constructor, there is no need to reserve its size

qliphy · 2022-07-20T01:06:22Z

@fabferro Please have a look at above comments. Moreover, this PR should be backport to 12_4_X

cmsbuild · 2022-07-20T11:33:06Z

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

fabferro · 2022-07-20T11:35:26Z

@perrotta : comments implemented. Backport PR opened #38797

perrotta · 2022-07-20T11:59:46Z

RecoPPS/Local/plugins/CTPPSPixelLocalTrackProducer.cc

  const edm::ESGetToken<CTPPSGeometry, VeryForwardRealGeometryRecord> tokenCTPPSGeometry_;
  edm::ESWatcher<VeryForwardRealGeometryRecord> geometryWatcher_;

  const edm::ESGetToken<CTPPSPixelAnalysisMask, CTPPSPixelAnalysisMaskRcd> tokenCTPPSPixelAnalysisMask_;

-  uint32_t numberOfPlanesPerPot_;
-  std::vector<uint32_t> listOfAllPlanes_;
+  const uint32_t numberOfPlanesPerPot_;


Thank you @fabferro
To be fully compliant with the CMS style, since this is not a class member any more, you should remove the trailing underscore from the name.
Moreover, I'd move this definition right before when it is used, i.e before L103

(I know, this is not fundamental... But since tests weren't started yet, I think you can finish the cleaning before we launch them)

perrotta · 2022-07-20T12:24:24Z

please test

cmsbuild · 2022-07-20T12:25:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38740/31154

This PR adds an extra 20KB to repository

cmsbuild · 2022-07-20T12:25:53Z

Pull request #38740 was updated. @jpata, @clacaputo can you please check and sign again.

cmsbuild · 2022-07-20T17:10:57Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-608883/26343/summary.html
COMMIT: 7414392
CMSSW: CMSSW_12_5_X_2022-07-19-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38740/26343/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: 50
DQMHistoTests: Total histograms compared: 3662417
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3662393
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 208 log files, 45 edm output root files, 50 DQM output files
TriggerResults: no differences found

perrotta · 2022-07-22T08:43:48Z

@cms-sw/reconstruction-l2 could you please check and (re)sign, if you agree with the latest changes?

perrotta · 2022-07-22T17:05:54Z

+1

perrotta · 2022-07-22T17:07:26Z

merge

@clacaputo already signed this once, and now also the pretty identical 12_4_X backport, see [12_4] PPS clean code for bad pot #38797 (comment)
I assume that the intention was to also re-sign this one in the master

jfernan2 · 2023-09-18T08:08:41Z

+1
(for the reords)

cmsbuild · 2023-09-18T17:44: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 be automatically merged.

fabferro added 2 commits July 14, 2022 16:18

cleanup

2d2da58

after checks

c6e0b2a

cmsbuild added this to the CMSSW_12_5_X milestone Jul 14, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jul 14, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 14, 2022

cmsbuild added tests-started and removed tests-pending labels Jul 15, 2022

cmsbuild added tests-approved and removed tests-started labels Jul 15, 2022

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Jul 15, 2022

perrotta reviewed Jul 15, 2022

View reviewed changes

add const and remove reserve

ad10e4b

cmsbuild removed reconstruction-approved fully-signed tests-approved labels Jul 20, 2022

fabferro mentioned this pull request Jul 20, 2022

[12_4] PPS clean code for bad pot #38797

Merged

perrotta reviewed Jul 20, 2022

View reviewed changes

more cleaning

7414392

cmsbuild added code-checks-pending and removed code-checks-approved labels Jul 20, 2022

cmsbuild added tests-started and removed tests-pending labels Jul 20, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 20, 2022

cmsbuild added tests-approved and removed tests-started labels Jul 20, 2022

cmsbuild added orp-approved and removed orp-pending labels Jul 22, 2022

cmsbuild merged commit d255b3f into cms-sw:master Jul 22, 2022

cmsbuild mentioned this pull request Jul 22, 2022

Add wfs to test Run3 skims #38830

Merged

cmsbuild added reconstruction-approved fully-signed and removed reconstruction-pending pending-signatures labels Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PPS clean code for bad pot #38740

PPS clean code for bad pot #38740

fabferro commented Jul 14, 2022

cmsbuild commented Jul 14, 2022

cmsbuild commented Jul 14, 2022

tvami commented Jul 15, 2022

cmsbuild commented Jul 15, 2022

clacaputo commented Jul 15, 2022

cmsbuild commented Jul 15, 2022

perrotta Jul 15, 2022

perrotta Jul 15, 2022

perrotta Jul 15, 2022

perrotta Jul 15, 2022

qliphy commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

fabferro commented Jul 20, 2022

perrotta Jul 20, 2022

perrotta commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

perrotta commented Jul 22, 2022

perrotta commented Jul 22, 2022

perrotta commented Jul 22, 2022

jfernan2 commented Sep 18, 2023 •

edited

cmsbuild commented Sep 18, 2023

		@@ -65,31 +65,31 @@ class CTPPSPixelLocalTrackProducer : public edm::stream::EDProducer<> {

		edm::InputTag inputTag_;

		@@ -65,31 +65,31 @@ class CTPPSPixelLocalTrackProducer : public edm::stream::EDProducer<> {

		edm::InputTag inputTag_;
		edm::EDGetTokenT<edm::DetSetVector<CTPPSPixelRecHit>> tokenCTPPSPixelRecHit_;

PPS clean code for bad pot #38740

PPS clean code for bad pot #38740

Conversation

fabferro commented Jul 14, 2022

PR description:

PR validation:

cmsbuild commented Jul 14, 2022

cmsbuild commented Jul 14, 2022

tvami commented Jul 15, 2022

cmsbuild commented Jul 15, 2022

Comparison Summary

clacaputo commented Jul 15, 2022

cmsbuild commented Jul 15, 2022

perrotta Jul 15, 2022

Choose a reason for hiding this comment

perrotta Jul 15, 2022

Choose a reason for hiding this comment

perrotta Jul 15, 2022

Choose a reason for hiding this comment

perrotta Jul 15, 2022

Choose a reason for hiding this comment

qliphy commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

fabferro commented Jul 20, 2022

perrotta Jul 20, 2022

Choose a reason for hiding this comment

perrotta commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

cmsbuild commented Jul 20, 2022

Comparison Summary

perrotta commented Jul 22, 2022

perrotta commented Jul 22, 2022

perrotta commented Jul 22, 2022

jfernan2 commented Sep 18, 2023 • edited

cmsbuild commented Sep 18, 2023

jfernan2 commented Sep 18, 2023 •

edited