Phase2-hgx337B Try to solve the cassette shift issue and fix a bug in Geometry/HGCalCommonData

[bsunanda]

PR description:

Try to solve the cassette shift issue in Geometry/HGCalCommonData

PR validation:

Use the script in Validation/HGCalValidation

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Nothing special

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41380/35242

• This PR adds an extra 56KB 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-41380/35242/code-format.patch
  e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41380/35242/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-41380/35244

• This PR adds an extra 56KB to repository

[cmsbuild]

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

It involves the following packages:

• Geometry/HGCalCommonData (geometry, upgrade)
• SimG4CMS/Calo (simulation)

@civanch, @Dr15Jones, @bsunanda, @makortel, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob can you please review it and eventually sign? Thanks.
@felicepantaleo, @rovere, @thomreis, @wang0jin, @fabiocos, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

[bsunanda]

@cmsbuild Please test

[perrotta]

@bsunanda is this PR meant to fix #41376 ?
If so, could you please add a reference to the github issue in the PR description?

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41380/35245

• This PR adds an extra 56KB to repository

[srimanob]

@civanch
There is a warning in static check, not related to the PR,

/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_13_1_X_2023-04-19-2300/src/SimG4CMS/Calo/src/CaloSD.cc:186:13: warning: const_cast was used, this may result in thread-unsafe code [threadsafety.ConstCast]
            const_cast<G4Track*>(tk)->SetTrackStatus(fStopAndKill);
            ^~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dc3d0f/32071/llvm-analysis/runStaticChecks.log

Could you please have a look, and maybe provide the follow up PR? Thanks.

[srimanob]

test parameters:

• workflows = 25234.911
• relvals_opt = --what cleanedupgrade,standard,highstats,pileup,generator,extendedgen,production,identity,ged,machine,premix,nano,gpu,2017,2026

[srimanob]

@cmsbuild please test

Re-trigger the test with workflow that shows an issue.

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dc3d0f/32082/summary.html
COMMIT: 0e7ab3b
CMSSW: CMSSW_13_1_X_2023-04-20-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41380/32082/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially added 6 lines to the logs
• Reco comparison results: 11 differences found in the comparisons
• DQMHistoTests: Total files compared: 49
• DQMHistoTests: Total histograms compared: 3555367
• DQMHistoTests: Total failures: 9
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3555336
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
• Checked 212 log files, 163 edm output root files, 49 DQM output files
• TriggerResults: no differences found

[civanch]

@srimanob , this static analyzer warning is false positive. It has nothing to do with multi-threading but is status change of G4Track, which normally should not be done in sensitive detector class. However, this does not contradict to Geant4 tracking. I will have a look in this place without any guarantee that the warning may be removed.

[srimanob]

+Upgrade

Workflow 25234.911 is running fine now,
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dc3d0f/32082/runTheMatrix-results/25234.911_TTbar_14TeV+2026D99_DD4hep/

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

[perrotta]

Workflow 25234.911 is running fine now, https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dc3d0f/32082/runTheMatrix-results/25234.911_TTbar_14TeV+2026D99_DD4hep/

Well, the error does not reproduce alway even in the IBs... We'll verify only once this is merged whether it got fixed, or not.

@bsunanda , since this PR contains a bug fix, could you please add in the PR description, for documentation purposes:

• which was the bug causing the issue reported in Segfault in HGCalCassette::getShift() in workflow 25234.911 #41376, and which was the PR that introduced it
• how did you fix it here.

Since, as you write this PR contains something else than that fix, please also describe it in the PR description.

And yes, if the bug is there already in 13_0_X (not clear to me until you explain what was the bug and with which PR was it introduced), a backport is in order: thank you.

[bsunanda]

@perrotta Try to estimate phi before using it to find the cassette number of the scintillator

[bsunanda]

@perrotta Do you still have the segmentation violation? I believe that the phi is now computed correctly before using it in getting the cassette number. Please merge this PR and let me know if the bug-fix part should be back ported or not

[srimanob]

Hi @bsunanda @perrotta @rappoccio
Just a comment from the upgrade side, we have no plan for production in 13_0. Having this fix or not will not affect 13_0 IB nor production in future. However, I leave the decision to release manager and geometry.

[rappoccio]

+1

Let's see if the segfaults show up after merging this. We will still need the backport for 13_0 for the big fix.