Phase2-hgx207 Have uniform wafer type from HGCalDDDConst and reduce usage of magic numbers #27585

bsunanda · 2019-07-22T22:05:07Z

PR description:

Have uniform wafer type definition from HGCalDDDConst and reduce usage of magic numbers

PR validation:

Use standard matrix for testing this

if this PR is a backport please specify the original PR:

Nothing special

…numbers

cmsbuild · 2019-07-22T22:05:39Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-07-22T22:11:18Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27585/10995

This PR adds an extra 36KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File SimCalorimetry/HGCalSimProducers/interface/HGCDigitizerBase.h modified in PR(s): Improvements to the HGC digitizer and implementation of a end-of-life scenario for the HGC silicon sector #27582

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27585/10995/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27585/10995/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2019-07-22T22:14:19Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-07-22T22:20:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27585/10996

This PR adds an extra 36KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File SimCalorimetry/HGCalSimProducers/interface/HGCDigitizerBase.h modified in PR(s): Improvements to the HGC digitizer and implementation of a end-of-life scenario for the HGC silicon sector #27582

cmsbuild · 2019-07-22T22:20:43Z

A new Pull Request was created by @bsunanda for master.

It involves the following packages:

Geometry/HGCalCommonData
RecoLocalCalo/HGCalRecAlgos
SimCalorimetry/HGCalSimProducers

@perrotta, @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks.
@edjtscott, @vandreev11, @sethzenz, @makortel, @felicepantaleo, @rovere, @lgray, @cseez, @pfs, @deguio, @argiro, @kpedro88 this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

bsunanda · 2019-07-22T23:50:44Z

@cmsbuild Please test

cmsbuild · 2019-07-22T23:51:14Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/1589/console Started: 2019/07/23 01:52

cmsbuild · 2019-07-23T01:30:58Z

+1
Tested at: 3f17c16
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5848a9/1589/summary.html

cmsbuild · 2019-07-23T01:31:03Z

Comparison job queued.

cmsbuild · 2019-07-23T17:24:42Z

+1
Tested at: 20e66a7
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5848a9/1597/summary.html

cmsbuild · 2019-07-23T17:24:46Z

Comparison job queued.

cmsbuild · 2019-07-23T19:24:18Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5848a9/1597/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 2628546
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2628228
DQMHistoTests: Total skipped: 317
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 133 log files, 14 edm output root files, 32 DQM output files

kpedro88 · 2019-07-23T20:08:28Z

+upgrade

perrotta · 2019-07-24T09:38:50Z

Geometry/HGCalCommonData/src/HGCalDDDConstants.cc

@@ -1241,15 +1246,15 @@ std::pair<double, double> HGCalDDDConstants::waferPosition(int waferU, int wafer
 int HGCalDDDConstants::waferType(DetId const& id) const {


The return value of this function is the previous one minus 1. This is correctly propagated to the HGCalDigitizer, the only users of this method as far as I could tell: therefore the net effect is that no changes are expected. (OK!)

This function has not changed at all except it uses enum values instead of numbers. The o/p is indeed unchanged

perrotta · 2019-07-24T09:42:42Z

Geometry/HGCalCommonData/src/HGCalDDDConstants.cc

  }
  return type;
 }

 int HGCalDDDConstants::waferType(int layer, int waferU, int waferV) const {
-  int type(-1);
+  int type(HGCSiliconDetId::HGCalCoarseThick);


Here your are changing the default value from "-1" to "2". Is that default value ever accessed? Was anything else modified in the rest of the code to take into account this change? (I couldn't easily find it...)

-1 does not mean anything - it was used earlier so that all variables are initialized to some value. That default value is never accessed

Ok, thank you Sunanda. I just wanted to be sure that such a modified initialization default is never accessed, which is what I expected but not obvious from the code itself.

perrotta · 2019-07-24T15:03:26Z

+1

Centralized enum values replace previous hardcoded numbers, with the same meaning
Jenkins tests pass and show no difference with respect to the baseline, as it should

cvuosalo · 2019-07-24T16:44:15Z

+1

civanch · 2019-07-24T21:10:18Z

+1

cmsbuild · 2019-07-24T21:10:47Z

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

fabiocos · 2019-07-25T08:48:48Z

+1

Have uniform wafer type from HGCalDDDConst and reduce usage of magic …

0ac8d17

…numbers

cmsbuild added this to the CMSSW_11_0_X milestone Jul 22, 2019

cmsbuild added code-checks-pending comparison-pending geometry-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending upgrade-pending labels Jul 22, 2019

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jul 22, 2019

Take care of code check

3f17c16

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jul 22, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 22, 2019

cmsbuild added tests-started and removed tests-pending labels Jul 22, 2019

cmsbuild removed the tests-started label Jul 23, 2019

cmsbuild added the tests-started label Jul 23, 2019

cmsbuild added tests-approved and removed tests-started labels Jul 23, 2019

cmsbuild added comparison-available and removed comparison-pending labels Jul 23, 2019

cmsbuild added upgrade-approved and removed upgrade-pending labels Jul 23, 2019

perrotta reviewed Jul 24, 2019

View reviewed changes

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jul 24, 2019

cmsbuild added geometry-approved and removed geometry-pending labels Jul 24, 2019

cmsbuild mentioned this pull request Jul 24, 2019

Improvements to the HGC digitizer and implementation of a end-of-life scenario for the HGC silicon sector #27582

Merged

cmsbuild added fully-signed simulation-approved and removed pending-signatures simulation-pending labels Jul 24, 2019

cmsbuild added orp-approved and removed orp-pending labels Jul 25, 2019

cmsbuild merged commit 4d18927 into cms-sw:master Jul 25, 2019

cmsbuild mentioned this pull request Jul 25, 2019

[BugFix] DDG4ProductionCuts Debug Printout #27597

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phase2-hgx207 Have uniform wafer type from HGCalDDDConst and reduce usage of magic numbers #27585

Phase2-hgx207 Have uniform wafer type from HGCalDDDConst and reduce usage of magic numbers #27585

bsunanda commented Jul 22, 2019

cmsbuild commented Jul 22, 2019

cmsbuild commented Jul 22, 2019

cmsbuild commented Jul 22, 2019

cmsbuild commented Jul 22, 2019

cmsbuild commented Jul 22, 2019

bsunanda commented Jul 22, 2019

cmsbuild commented Jul 22, 2019 •

edited

cmsbuild commented Jul 23, 2019

cmsbuild commented Jul 23, 2019

cmsbuild commented Jul 23, 2019

cmsbuild commented Jul 23, 2019

cmsbuild commented Jul 23, 2019

kpedro88 commented Jul 23, 2019

perrotta Jul 24, 2019

bsunanda Jul 24, 2019

perrotta Jul 24, 2019

bsunanda Jul 24, 2019

perrotta Jul 24, 2019

perrotta commented Jul 24, 2019

cvuosalo commented Jul 24, 2019

civanch commented Jul 24, 2019

cmsbuild commented Jul 24, 2019

fabiocos commented Jul 25, 2019

		@@ -1241,15 +1246,15 @@ std::pair<double, double> HGCalDDDConstants::waferPosition(int waferU, int wafer
		int HGCalDDDConstants::waferType(DetId const& id) const {

Phase2-hgx207 Have uniform wafer type from HGCalDDDConst and reduce usage of magic numbers #27585

Phase2-hgx207 Have uniform wafer type from HGCalDDDConst and reduce usage of magic numbers #27585

Conversation

bsunanda commented Jul 22, 2019

PR description:

PR validation:

if this PR is a backport please specify the original PR:

cmsbuild commented Jul 22, 2019

cmsbuild commented Jul 22, 2019

cmsbuild commented Jul 22, 2019

cmsbuild commented Jul 22, 2019

cmsbuild commented Jul 22, 2019

bsunanda commented Jul 22, 2019

cmsbuild commented Jul 22, 2019 • edited

cmsbuild commented Jul 23, 2019

cmsbuild commented Jul 23, 2019

cmsbuild commented Jul 23, 2019

cmsbuild commented Jul 23, 2019

cmsbuild commented Jul 23, 2019

kpedro88 commented Jul 23, 2019

perrotta Jul 24, 2019

Choose a reason for hiding this comment

bsunanda Jul 24, 2019

Choose a reason for hiding this comment

perrotta Jul 24, 2019

Choose a reason for hiding this comment

bsunanda Jul 24, 2019

Choose a reason for hiding this comment

perrotta Jul 24, 2019

Choose a reason for hiding this comment

perrotta commented Jul 24, 2019

cvuosalo commented Jul 24, 2019

civanch commented Jul 24, 2019

cmsbuild commented Jul 24, 2019

fabiocos commented Jul 25, 2019

cmsbuild commented Jul 22, 2019 •

edited