Account for layer offset when using waferZ in new geometry #27503

clelange · 2019-07-11T18:13:29Z

PR description:

There is a bug in RecHitTools::getPositionLayer(int layer, bool nose): when getting the z position of the wafer, one needs to account for the FH (HGCalSi) offset, starting the layer number at 1 instead of continuing to count up beyond EE.

PR validation:

Ran in D41 (geometryType_ == 1) and instead of getting z = 0 for layers > 28, I actually obtained increasing z values:

layer 1: z = 322.103
layer 2: z = 323.047
layer 3: z = 325.073
layer 4: z = 326.017
layer 5: z = 328.043
layer 6: z = 328.987
layer 7: z = 331.013
layer 8: z = 331.957
layer 9: z = 333.983
layer 10: z = 334.927
layer 11: z = 336.953
layer 12: z = 337.897
layer 13: z = 339.923
layer 14: z = 340.867
layer 15: z = 342.893
layer 16: z = 343.837
layer 17: z = 345.863
layer 18: z = 346.807
layer 19: z = 348.833
layer 20: z = 349.777
layer 21: z = 351.803
layer 22: z = 352.747
layer 23: z = 354.773
layer 24: z = 355.717
layer 25: z = 357.743
layer 26: z = 358.687
layer 27: z = 360.713
layer 28: z = 361.657
layer 29: z = 367.699
layer 30: z = 373.149
layer 31: z = 378.599
layer 32: z = 384.049
layer 33: z = 389.499
layer 34: z = 394.949
layer 35: z = 400.399
layer 36: z = 405.849
layer 37: z = 411.299
layer 38: z = 416.749
layer 39: z = 422.199
layer 40: z = 427.649
layer 41: z = 436.199
layer 42: z = 444.749
layer 43: z = 453.299
layer 44: z = 461.849
layer 45: z = 470.399
layer 46: z = 478.949
layer 47: z = 487.499
layer 48: z = 496.049
layer 49: z = 504.599
layer 50: z = 513.149

FYI @bsunanda

cmsbuild · 2019-07-11T18:13:55Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-07-11T18:21:35Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27503/10846

This PR adds an extra 12KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoLocalCalo/HGCalRecAlgos/src/RecHitTools.cc modified in PR(s): Layer Clustering for HGCal Scintillator #27485

cmsbuild · 2019-07-11T18:22:07Z

A new Pull Request was created by @clelange (Clemens Lange) for master.

It involves the following packages:

RecoLocalCalo/HGCalRecAlgos

@perrotta, @cmsbuild, @kpedro88, @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

kpedro88 · 2019-07-11T18:26:48Z

please test

cmsbuild · 2019-07-11T18:27:16Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/1435/console Started: 2019/07/11 20:28

cmsbuild · 2019-07-11T19:53:14Z

+1
Tested at: 97f6dd9
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5a8e84/1435/summary.html

cmsbuild · 2019-07-11T19:53:17Z

Comparison job queued.

cmsbuild · 2019-07-11T22:36:45Z

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-5a8e84/20034.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D35_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D35+RecoFullGlobal_2023D35+HARVESTFullGlobal_2023D35
/build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-5a8e84/20434.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D41_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D41+RecoFullGlobal_2023D41+HARVESTFullGlobal_2023D41
/build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-5a8e84/21234.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D44_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D44+RecoFullGlobal_2023D44+HARVESTFullGlobal_2023D44

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 1 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3081858
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3081535
DQMHistoTests: Total skipped: 322
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-13T01:37:14Z

+upgrade

perrotta · 2019-07-14T12:19:08Z

RecoLocalCalo/HGCalRecAlgos/src/RecHitTools.cc

@@ -150,6 +150,10 @@ GlobalPoint RecHitTools::getPositionLayer(int layer, bool nose) const {
    }
  } else {
    const HGCalDDDConstants* ddd = get_ddd(geom_, geometryType_, fhOffset_, lay);
+    if (geometryType_ == 1) {
+      if (lay > fhOffset_)


Shouldn't this be ">="? (Just guessing...)

No, the fhOffset_ is the last layer of EE (=28), and only for the first layer of the hadronic calorimeter (29) the offset correction (29-28=1) be used, since the layer numbering starts with 1.

perrotta · 2019-07-15T07:38:31Z

Please test
(Phase2 reco comparisons should be back and available now)

cmsbuild · 2019-07-15T07:39:04Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/1468/console Started: 2019/07/15 09:40

cmsbuild · 2019-07-15T08:56:17Z

+1
Tested at: 97f6dd9
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5a8e84/1468/summary.html

cmsbuild · 2019-07-15T08:56:20Z

Comparison job queued.

cmsbuild · 2019-07-15T10:35:22Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3081858
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3081534
DQMHistoTests: Total skipped: 322
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

perrotta · 2019-07-15T10:47:24Z

@clelange , automatic tests show no difference in the D41 workflow 20434.0
Shouldn't we notice any effect of the bug fix? There are a few methods in the HGCal software that rely on RecHitTools::getPositionLayer...

clelange · 2019-07-15T11:07:33Z

@perrotta , I wouldn't expect any differences: there is only one method that's using this within CMSSW at the moment (see https://cmssdt.cern.ch/lxr/ident?_i=getPositionLayer) and that only uses the first layer of EE (https://github.com/cms-sw/cmssw/blob/master/RecoEgamma/EgammaTools/src/EgammaPCAHelper.cc#L330). However, for code that's developed outside the CMSSW repository, this shows (the expected) differences.

perrotta · 2019-07-15T11:30:58Z

+1

The fix has been confirmed and validated by the relevant experts
Jenkins tests pass and show no differences, as the fix is effective only for code not yet in cmssw, see Account for layer offset when using waferZ in new geometry #27503 (comment)

cmsbuild · 2019-07-15T11:31:22Z

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-15T16:13:35Z

as far as I can see there should not be real conflict with #27485, so it looks ok to integrate

fabiocos · 2019-07-15T17:31:03Z

+1

Account for layer offset when using waferZ in new geometry

97f6dd9

cmsbuild added this to the CMSSW_11_0_X milestone Jul 11, 2019

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

clelange mentioned this pull request Jul 11, 2019

Use RecHitTools for layer navigation CMS-HGCAL/reco-ntuples#77

Merged

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

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

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

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

slava77 mentioned this pull request Jul 11, 2019

deepAK8 reproducibility problems after switching to openblas #27504

Closed

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

cmsbuild mentioned this pull request Jul 14, 2019

Layer Clustering for HGCal Scintillator #27485

Merged

perrotta reviewed Jul 14, 2019

View reviewed changes

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-approved labels Jul 15, 2019

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

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

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

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

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

cmsbuild merged commit 21acc72 into cms-sw:master Jul 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for layer offset when using waferZ in new geometry #27503

Account for layer offset when using waferZ in new geometry #27503

clelange commented Jul 11, 2019 •

edited

cmsbuild commented Jul 11, 2019

cmsbuild commented Jul 11, 2019

cmsbuild commented Jul 11, 2019

kpedro88 commented Jul 11, 2019

cmsbuild commented Jul 11, 2019 •

edited

cmsbuild commented Jul 11, 2019

cmsbuild commented Jul 11, 2019

cmsbuild commented Jul 11, 2019

kpedro88 commented Jul 13, 2019

perrotta Jul 14, 2019

clelange Jul 14, 2019

perrotta commented Jul 15, 2019

cmsbuild commented Jul 15, 2019 •

edited

cmsbuild commented Jul 15, 2019

cmsbuild commented Jul 15, 2019

cmsbuild commented Jul 15, 2019

perrotta commented Jul 15, 2019

clelange commented Jul 15, 2019

perrotta commented Jul 15, 2019

cmsbuild commented Jul 15, 2019

fabiocos commented Jul 15, 2019

fabiocos commented Jul 15, 2019

Account for layer offset when using waferZ in new geometry #27503

Account for layer offset when using waferZ in new geometry #27503

Conversation

clelange commented Jul 11, 2019 • edited

PR description:

PR validation:

cmsbuild commented Jul 11, 2019

cmsbuild commented Jul 11, 2019

cmsbuild commented Jul 11, 2019

kpedro88 commented Jul 11, 2019

cmsbuild commented Jul 11, 2019 • edited

cmsbuild commented Jul 11, 2019

cmsbuild commented Jul 11, 2019

cmsbuild commented Jul 11, 2019

kpedro88 commented Jul 13, 2019

perrotta Jul 14, 2019

Choose a reason for hiding this comment

clelange Jul 14, 2019

Choose a reason for hiding this comment

perrotta commented Jul 15, 2019

cmsbuild commented Jul 15, 2019 • edited

cmsbuild commented Jul 15, 2019

cmsbuild commented Jul 15, 2019

cmsbuild commented Jul 15, 2019

perrotta commented Jul 15, 2019

clelange commented Jul 15, 2019

perrotta commented Jul 15, 2019

cmsbuild commented Jul 15, 2019

fabiocos commented Jul 15, 2019

fabiocos commented Jul 15, 2019

clelange commented Jul 11, 2019 •

edited

cmsbuild commented Jul 11, 2019 •

edited

cmsbuild commented Jul 15, 2019 •

edited