Phase2-hgx112 Correct Reco geometry of post TDR definition and interface to DIGI+RECO codes #23303

bsunanda · 2018-05-23T21:53:05Z

Simulation, Reconstruction geometry and numbering schemes for the new geometry are tested. Also changes are made to the DIGI and local RECO code to access data from new geometry

…CO codes

cmsbuild · 2018-05-23T21:53:25Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-05-23T21:55:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23303/4798

cmsbuild · 2018-05-23T21:55:44Z

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

It involves the following packages:

DataFormats/ForwardDetId
Geometry/CaloEventSetup
Geometry/CaloTopology
Geometry/HGCalCommonData
Geometry/HGCalGeometry
RecoLocalCalo/HGCalRecAlgos
RecoLocalCalo/HGCalRecProducers
SimCalorimetry/HGCalSimProducers

@perrotta, @civanch, @Dr15Jones, @ianna, @kpedro88, @cmsbuild, @mdhildreth, @slava77 can you please review it and eventually sign? Thanks.
@edjtscott, @vandreev11, @sethzenz, @felicepantaleo, @rovere, @argiro, @cseez, @pfs, @lgray, @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 · 2018-05-23T21:57:28Z

@cmsbuild Please test

cmsbuild · 2018-05-23T21:57:47Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28147/console Started: 2018/05/23 23:58

cmsbuild · 2018-05-24T01:21:17Z

+1
Tested at: 5a1ce38
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-23303/28147/summary.html

cmsbuild · 2018-05-24T01:21:20Z

Comparison job queued.

cmsbuild · 2018-05-24T03:22:03Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 2155 differences found in the comparisons
DQMHistoTests: Total files compared: 31
DQMHistoTests: Total histograms compared: 2901712
DQMHistoTests: Total failures: 117
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2901405
DQMHistoTests: Total skipped: 190
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
Checked 128 log files, 14 edm output root files, 31 DQM output files

kpedro88 · 2018-05-25T02:34:34Z

Geometry/HGCalCommonData/src/HGCalDDDConstants.cc

+  if ((mode_ == HGCalGeometryMode::Hexagon8) ||
+      (mode_ == HGCalGeometryMode::Hexagon8Full)) {
+    auto itr = hgpar_->typesInLayers_.find(HGCalWaferIndex::waferIndex(layer,waferU,waferV));
+    int type = ((itr == hgpar_->typesInLayers_.end() ? 2 : 


magic number 2 -> descriptive constant

kpedro88 · 2018-05-25T02:35:27Z

Geometry/HGCalCommonData/src/HGCalDDDConstants.cc

+  } else if ((mode_ == HGCalGeometryMode::Hexagon) ||
+	     (mode_ == HGCalGeometryMode::HexagonFull)) {
+    int type = (((waferU>=0)&&(waferU<(int)(hgpar_->waferTypeL_.size()))) ? 
+		hgpar_->waferTypeL_[waferU] : 0);


also for magic number 0

kpedro88 · 2018-05-25T02:36:53Z

Geometry/HGCalCommonData/src/HGCalGeomParameters.cc

-      mytrf.hr    = hr;
-      trforms.emplace_back(mytrf);
-      trformUse.emplace_back(false);
+      auto ktr = trforms.find(std::pair<int,int>(lay,zside));


use make_pair

kpedro88 · 2018-05-25T02:38:05Z

Geometry/HGCalCommonData/src/HGCalGeomParameters.cc

+	mytrf.subsec= 0;
+	mytrf.h3v   = h3v;
+	mytrf.hr    = hr;
+	trforms[std::pair<int,int>(lay,zside)] = mytrf;


use make_pair

kpedro88 · 2018-05-25T02:38:44Z

Geometry/HGCalCommonData/src/HGCalGeomParameters.cc

-      trforms.emplace_back(mytrf);
-      trformUse.emplace_back(false);
+      auto ktr = trforms.find(std::pair<int,int>(lay,zside));
+      if (ktr == trforms.end()) {


could avoid temporary ktr since it's only checked here

kpedro88 · 2018-05-25T02:53:02Z

RecoLocalCalo/HGCalRecAlgos/interface/HGCalRecHitSimpleAlgo.h

    } else {
      throw cms::Exception("InvalidRecHit")
 	<< "HGCalRecHitSimpleAlgo encountered a non-HGCal det id: " << baseid.det() << ' ' << baseid.subdetId() << ' ' << baseid.rawId();
    }

-    HGCalDetId hid(uncalibRH.id());
+    // HGCalDetId hid(uncalibRH.id());


delete commented-out code

kpedro88 · 2018-05-25T02:58:15Z

RecoLocalCalo/HGCalRecAlgos/src/RecHitTools.cc

@@ -53,6 +55,22 @@ namespace {
    return ddd;
  }

+  inline const HGCalDDDConstants* get_ddd(const CaloSubdetectorGeometry* geom,
+					  const HGCSiliconDetId& detid) {
+    const HGCalGeometry* hg = dynamic_cast<const HGCalGeometry*>(geom);


doing this dynamic_cast for every DetID seems suboptimal. Can the ddd constant pointers be kept as class members?

why cannot you just get a handle to the constants from an event setup?

it looks like the response to this comment on the dynamic_cast is "it was there before" for the other methods.
However, I agree that this can be improved.
A somewhat daring way is to use a reinterpret_cast

Please do not use a reinterpret_cast. A static_cast would be sufficient.

kpedro88 · 2018-05-25T02:59:55Z

RecoLocalCalo/HGCalRecAlgos/src/RecHitTools.cc

+  int layer;
+  switch (type) {
+  case(DetId::HGCalEE): {
+    auto geomEE = dynamic_cast<const HGCalGeometry*>(geom_->getSubdetectorGeometry(type,ForwardSubdetector::ForwardEmpty));


this function could also profit from the optimization suggested above

kpedro88 · 2018-05-25T03:03:28Z

SimCalorimetry/HGCalSimProducers/python/hgcalDigitizer_cfi.py

@@ -208,3 +211,13 @@ def HGCal_setEndOfLifeNoise(digitizer,process):
        values = cms.vdouble([x for x in endOfLifeNoises])
        )

+def HGCal_setPostTDRGeometry(digitizer,process):


we should introduce a modifier for this (I don't see a way around it)

kpedro88 · 2018-05-25T03:04:16Z

SimCalorimetry/HGCalSimProducers/src/HGCDigitizer.cc

      HGCalTestNumbering::unpackHexagonIndex(simId, subdet, zp, layer, sec, subsec, cell);
      //sec is wafer and subsec is celltyp
+      //skip this hit if after ganging it is not valid
+      std::pair<int,int> recoLayerCell=dddConst.simToReco(cell,layer,sec,topo.detectorType());


kpedro88 · 2018-05-25T03:07:46Z

@bsunanda is this PR expected to cause the changes observed in existing 2023 workflows?

ianna · 2018-05-25T08:06:47Z

RecoLocalCalo/HGCalRecAlgos/src/RecHitTools.cc

@@ -53,6 +55,22 @@ namespace {
    return ddd;
  }

+  inline const HGCalDDDConstants* get_ddd(const CaloSubdetectorGeometry* geom,
+					  const HGCSiliconDetId& detid) {
+    const HGCalGeometry* hg = dynamic_cast<const HGCalGeometry*>(geom);


why cannot you just get a handle to the constants from an event setup?

ianna · 2018-05-25T08:07:41Z

RecoLocalCalo/HGCalRecAlgos/src/RecHitTools.cc

-  unsigned int wmaxFH = 1 + (geomFH->topology().dddConstants()).waferMax();
+  unsigned int wmaxEE(0), wmaxFH(0);
+  if (mode_ == 0) {
+    auto geomEE = dynamic_cast<const HGCalGeometry*>(geom_->getSubdetectorGeometry(DetId::Forward,ForwardSubdetector::HGCEE));


get it from an event setup?

RecHitTools is Marco's code. I just added what changes to be made for the new geometry. I do not want to change the design of this. Later Marco can take care of the changes in the RecHitTools code

@rovere FYI

ianna · 2018-05-25T08:10:23Z

RecoLocalCalo/HGCalRecAlgos/src/RecHitTools.cc

+  } else if (id.det() == DetId::HGCalEE || id.det() == DetId::HGCalHSi) {
+    const HGCSiliconDetId hid(id);
+    auto ddd = get_ddd(geom,hid);
+    thick    = ddd->cellThick(hid.layer(),hid.waferU(),hid.waferV());


if getSiThickness is used very often, it's better to store its record in an event setup

Thickness depends on the wafer position. Again I am not changing the coding style there.

cmsbuild · 2018-05-30T10:55:23Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 31
DQMHistoTests: Total histograms compared: 2902471
DQMHistoTests: Total failures: 25
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2902256
DQMHistoTests: Total skipped: 190
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
Checked 128 log files, 14 edm output root files, 31 DQM output files

kpedro88 · 2018-05-30T13:11:00Z

+1
followup items:

use geometry info in HGCalRecHitSimpleAlgo (https://github.com/cms-sw/cmssw/pull/23303/files#r190779830)
reduce duplicated functions in digitizers (https://github.com/cms-sw/cmssw/pull/23303/files#r191300383)
simplify get_ddd() in RecHitTools (already partially addressed by use of static_cast, could probably be further improved) (Phase2-hgx112 Correct Reco geometry of post TDR definition and interface to DIGI+RECO codes #23303 (comment))
also look at how geometry is obtained in RecHitTools (Phase2-hgx112 Correct Reco geometry of post TDR definition and interface to DIGI+RECO codes #23303 (comment))
use phase2_hgcalV9 modifier in Era for forthcoming workflow w/ new HGCal

bsunanda · 2018-05-30T18:56:49Z

@civanch @ianna @slava77 @perrotta Please approve this PR

bsunanda · 2018-05-31T14:46:43Z

@slava77 @ianna @civanch @fabiocos Could you please approve & integrate this at the earliest. Many work is waiting which are waiting for this

slava77 · 2018-05-31T19:58:33Z

+1

for #23303 4758834

code changes are in line with the PR description and the follow up review; behavior of the code currently used in the standard [jenkins tested] workflows is not affected
jenkins tests pass and comparisons with the baseline show no differences

bsunanda · 2018-06-02T06:17:14Z

@ianna @civanch @fabiocos Please approve and merge this PR at the earliest

bsunanda · 2018-06-05T04:21:43Z

@ianna @civanch @fabiocos Please approve and merge this PR at the earliest. We are stuck of progressing - please try to do this

ianna · 2018-06-05T15:36:23Z

+1

civanch · 2018-06-05T15:42:02Z

+1

fabiocos · 2018-06-05T16:44:55Z

+operations

addition of a Modifier to manage a specific HGCal scenario configuration

fabiocos · 2018-06-05T16:45:11Z

+1

changes in comparison look unrelated to this PR

cmsbuild · 2018-06-05T16:45:16Z

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.

Correct Reco geometry of post TDR definition and interface to DIGI+RE…

5a1ce38

…CO codes

cmsbuild added this to the CMSSW_10_2_X milestone May 23, 2018

cmsbuild added code-checks-pending comparison-pending geometry-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending upgrade-pending labels May 23, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels May 23, 2018

cmsbuild added tests-started and removed tests-pending labels May 23, 2018

cmsbuild added tests-approved and removed tests-started labels May 24, 2018

cmsbuild added comparison-available and removed comparison-pending labels May 24, 2018

kpedro88 reviewed May 25, 2018

View reviewed changes

ianna reviewed May 25, 2018

View reviewed changes

cmsbuild added the tests-approved label May 30, 2018

cmsbuild added comparison-available and removed comparison-pending labels May 30, 2018

cmsbuild added upgrade-approved and removed upgrade-pending labels May 30, 2018

cmsbuild added reconstruction-approved and removed reconstruction-pending labels May 31, 2018

cmsbuild added geometry-approved and removed geometry-pending labels Jun 5, 2018

cmsbuild added simulation-approved and removed simulation-pending labels Jun 5, 2018

cmsbuild added fully-signed operations-approved orp-approved and removed operations-pending orp-pending pending-signatures labels Jun 5, 2018

cmsbuild merged commit 020159e into cms-sw:master Jun 5, 2018

Phase2-hgx112 Correct Reco geometry of post TDR definition and interface to DIGI+RECO codes #23303

Phase2-hgx112 Correct Reco geometry of post TDR definition and interface to DIGI+RECO codes #23303

Conversation

bsunanda commented May 23, 2018

cmsbuild commented May 23, 2018

cmsbuild commented May 23, 2018

cmsbuild commented May 23, 2018

bsunanda commented May 23, 2018

cmsbuild commented May 23, 2018 • edited Loading

cmsbuild commented May 24, 2018

cmsbuild commented May 24, 2018

cmsbuild commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpedro88 commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented May 30, 2018

kpedro88 commented May 30, 2018

bsunanda commented May 30, 2018

bsunanda commented May 31, 2018

slava77 commented May 31, 2018

bsunanda commented Jun 2, 2018

bsunanda commented Jun 5, 2018

ianna commented Jun 5, 2018

civanch commented Jun 5, 2018

fabiocos commented Jun 5, 2018

fabiocos commented Jun 5, 2018

cmsbuild commented Jun 5, 2018

cmsbuild commented May 23, 2018 •

edited

Loading