Phase2-hgx291 Bug fix to address rotated layers in D86

[bsunanda]

PR description:

Bug fix to address rotated layers in D86. The changes will affect all workflows with HGCal. Mapping from position to DetId and vice versa are revised. This affects the public methods used in HGCalGeometry (getPosition, getClosestCell) and in HGCalDDDConstants (locateCell, locateCellHex, waferFromPosition, waferPosition). In the process the access method of getting position from ID is modified - earlier it was split between HGCalGeometry and HGCalDDConstant. Now it is done entirely in HGCalDDDConstants and this has affected the precision and hence there will be small changes in all earlier results.

PR validation:

Use the runTheMatrix test workflows

if this PR is a backport please specify the original PR and why you need to backport that PR:

Nothing special

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35563/25800

• This PR adds an extra 60KB to repository

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-35563/25800/code-format.patch
  e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35563/25800/code-format.patch | patch -p1
  You can also run scram build code-format to apply code format directly

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35563/25801

• This PR adds an extra 60KB to repository

[cmsbuild]

A new Pull Request was created by @bsunanda (Sunanda Banerjee) for master.

It involves the following packages:

• Geometry/HGCalCommonData (geometry, upgrade)
• Geometry/HGCalGeometry (geometry, upgrade)

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob can you please review it and eventually sign? Thanks.
@fabiocos 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

[bsunanda]

@cmsbuild Please test

[cmsbuild]

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e57e4/19448/summary.html
COMMIT: 43d8b8e
CMSSW: CMSSW_12_1_X_2021-10-06-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35563/19448/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

• 23234.023234.0_TTbar_14TeV+2026D49+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal/step2_TTbar_14TeV+2026D49+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal.log
• 28234.028234.0_TTbar_14TeV+2026D60+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal/step2_TTbar_14TeV+2026D60+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal.log
• 34634.034634.0_TTbar_14TeV+2026D76+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal/step2_TTbar_14TeV+2026D76+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal.log

Expand to see more relval errors ...

• 34834.999

[rovere]

FYI: @cseez @pfs

@bsunanda Could you please write a little more information about the why and the how of this PR? Your single line, 8 words explanation is worth Ungaretti's price (si sta come d'autunno sugli alberi le foglie) 😄

It is much more practical to have the explanation linked to the PR rather than searching for a plausible cause in long and private email threads.

Thanks.

[rovere]

Ciao @bsunanda,
you can very easily rebase your work on top of any recent IB: I doubt others had touched the packages modified in this PR.

Having a broken validation means I have no way of testing any PR involving geometry since I cannot trust the results of the comparisons.

Moreover, before any additional PR related to geometry is submitted, we need to integrate into the cms-bot a workflow based on D86 geometry, otherwise, we are simply blind to all the changes that will affect that scenario and to all side effects that regularly surface while touching the geometry.

[srimanob]

Hi @bsunanda @rovere

Thanks for introducing the fix, and reviewing the PR. I'm not sure if you can converge with the discussion in PR, or we should have a discussion verbally somewhere, i.e. HGCAL DPG meeting? Thanks.

[bsunanda]

@rovere I activated the debug switch in HGCalSimHitValidation and took out a few ID's from there and tried with the code of HGCalGeometryRotTest and I do get identical results when scale by a factor of 10 for cm->mm. Here are the results:
From SimHitValidation:
Detector HGCalHESiliconSensitive zside = 1 sector|wafer = -5:-10 type = 2 layer = 3 cell = 7:14 energy = 1.43931e-05 energyem = 0 energyhad = 1.43931e-05 time = 61.2611
983d55c7 global coordinate (69.3503,-1498.13,3805.45) time 61.2611:13.6438
----------------------- gx = 69.3503 gy = -1498.13 gz = 3805.45 phi = -1.52454 eta = 1.66104

From HGCalGeometryRotTest
Layer: 29 U -5 V -10 Position (0, -145.008) phi -90
HGCSiliconDetId::EE:HE= 0:1 type= 2 z= 1 layer= 3 wafer(u,v:x,y)= (-5,-10:0,-20) cell(u,v:x,y)= (7,14:20,-8) Position (6.93503,-149.813,380.545) phi -87.3496

From SimHitValidation:
Detector HGCalHESiliconSensitive zside = 1 sector|wafer = 1:3 type = 0 layer = 1 cell = 4:10 energy = 3.2011e-05 energyem = 0 energyhad = 3.2011e-05 time = 12.4293
90118544 global coordinate (74.4737,378.969,3679.35) time 12.4293:12.3404
----------------------- gx = 74.4737 gy = 378.969 gz = 3679.35 phi = 1.37675 eta = 2.94998

From HGCalGeometryRotTest
Layer: 27 U 1 V 3 Position (8.37204, 43.5024) phi 79.1066
HGCSiliconDetId::EE:HE= 0:1 type= 0 z= 1 layer= 1 wafer(u,v:x,y)= (1,3:1,6) cell(u,v:x,y)= (4,10:-4,-14) Position (7.44737,37.8969,367.935) phi 78.8821

From SimHitValidation:
Detector HGCalHESiliconSensitive zside = 1 sector|wafer = -1:2 type = 0 layer = 2 cell = 6:1 energy = 6.69811e-05 energyem = 0 energyhad = 6.69811e-05 time = 12.6551

From HGCalGeometryRotTest
Layer: 28 U -1 V 2 Position (14.5008, 41.8602) phi 70.8934
HGCSiliconDetId::EE:HE= 0:1 type= 0 z= 1 layer= 2 wafer(u,v:x,y)= (-1,2:4,4) cell(u,v:x,y)= (6,1:-31,-1) Position (8.49488,37.9303,374.24) phi 77.3763

From SimHitValidation:
90214426 global coordinate (84.9488,379.303,3742.4) time 12.6551:12.5505
Detector HGCalHESiliconSensitive zside = 1 sector|wafer = 2:0 type = 0 layer = 2 cell = 10:8 energy = 3.66259e-05 energyem = 3.66259e-05 energyhad = 0 time = 12.6634
9020090a global coordinate (-310.036,-178.999,3742.4) time 12.6634:12.5403
----------------------- gx = -310.036 gy = -178.999 gz = 3742.4 phi = -2.61799 eta = 3.04238

From HGCalGeometryRotTest
Layer: 28 U 2 V 0 Position (-29.0016, -16.7441) phi -150
HGCSiliconDetId::EE:HE= 0:1 type= 0 z= 1 layer= 2 wafer(u,v:x,y)= (2,0:-4,0) cell(u,v:x,y)= (10,8:-10,0) Position (-31.0036,-17.8999,374.24) phi -150

SO I DONOT SEE any issue with HGCALSimHitValidation

[rovere]

Ciao @bsunanda
thanks. Sorry, I must have mixed geometries.

[rovere]

+1

[srimanob]

test parameters:

• workflow = 38634.0

[srimanob]

@cmsbuild please test

Final test with D86 workflow, just to make sure everything runs fine before signing. The last test was done 6 days ago without D86, which is the target on this PR.

[bsunanda]

@civanch Can you approve this PR? I wonder why Phat asked for a fresh test. All the testes Marco and I did made use of D86 geometry. I wonder what he will see in this test - only his way of including the test.

[civanch]

+1

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e57e4/19715/summary.html
COMMIT: e085563
CMSSW: CMSSW_12_1_X_2021-10-18-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35563/19715/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

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

• /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-8e57e4/38634.0_TTbar_14TeV+2026D86+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

• No significant changes to the logs found
• Reco comparison results: 4670 differences found in the comparisons
• DQMHistoTests: Total files compared: 40
• DQMHistoTests: Total histograms compared: 2751113
• DQMHistoTests: Total failures: 34865
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2716226
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 39 files compared)
• Checked 170 log files, 37 edm output root files, 40 DQM output files
• TriggerResults: no differences found

[srimanob]

+Upgrade

Note:

• If this PR targets D86, we should run the test with D86 workflow, apart from the private one. The way I do is the standard way of PR testing. The PR test with the short matrix you have done so far using "please test", which does not include D86 workflow.
• As the result of the PR, it affects all geometries, not only D86. This should be commented in the open validation campaign. Note to @cms-sw/pdmv-l2

[cmsbuild]

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)

[bsunanda]

@perrotta @qliphy Could you integrate this quickly. Waiting for some bug fixes which has to follow this integration

[perrotta]

+1

• Extensively discussed and validated in the thread
• Please notice that this PR will cause regression in all Phase2 workflows: this has to be taken into account if a backport in a production release is going to be requested