Phase-hgx323Again Further correction for ring number assignment of scintillator hits in HGCal geometry for versions beyond V13

[bsunanda]

PR description:

Further correction for ring number assignment of scintillator hits in HGCal geometry versions V14, V15, V16 and V17. This new PR was made because of merging #39164 and then reverting #39198. I wonder why?

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. If this PR will be backported please specify to which release cycle the backport is meant for:

This is a bug fix for all versions of HGCal geometry

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39206/31818

• This PR adds an extra 16KB to repository

[cmsbuild]

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

It involves the following packages:

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

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

cms-bot commands are listed here

[bsunanda]

@cmsbuild Please test

[perrotta]

Suggested change

	return ((ok && trapezoidFile()) ? tileExist(zside, layer, irad, iphi) : ok);
	return ((ok && trapezoidFile()) ? tileExist(zside, layer, irad, iphi) : true);

I understand that in practice ok && ok is identical to ok && true. But translating in C++ the sentence "irrespective of trapezoidFile() the earlier test is good for all types of scintillators" is exactly ok && true

[bsunanda]

It should return the results ok if it is false and if ok is true, then one should see if TrapezoidFile() is the case then should return a test on tileExist. That is the correct logic

[bsunanda]

@cmsbuild type bug-fix

[bsunanda]

type bug-fix

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-705c74/27105/summary.html
COMMIT: 2f54b1d
CMSSW: CMSSW_12_5_X_2022-08-25-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39206/27105/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially added 47163 lines to the logs
• Reco comparison results: 2815 differences found in the comparisons
• DQMHistoTests: Total files compared: 51
• DQMHistoTests: Total histograms compared: 3695708
• DQMHistoTests: Total failures: 38014
• DQMHistoTests: Total nulls: 5
• DQMHistoTests: Total successes: 3657667
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.004 KiB( 50 files compared)
• DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
• Checked 212 log files, 49 edm output root files, 51 DQM output files
• TriggerResults: no differences found

[srimanob]

+Upgrade

Change in D77 is expected (see comment #39164 (comment)). Change in D88 is unclear if it will be visible, this should be followed up with HGCAL. Propose to merge this PR to CMSSW_12_5_0_pre5, then follow up on HGCAL validation. FYI @cms-sw/hgcal-dpg-l2

Copy the comment from original PR #39164
This bug was there all the time. When the position at the border of 2 rings, it is usually referred to as the lower ring number. This is fine as long as there is no tile in the lower ring. We did not have a good valid test of a detid earlier. We have to introduce one now and that revealed the discrepancy. We do have detailed testing code in the repository and we do conduct them to look into details. The standard validation code can look into broad discrepancies so far and the detailed tests are better done by the experts of the geometry code.

[perrotta]

+1

• Let have this merged for next pre5 (@smuzaffar kindly agreed to postpone the 1100 IB for it)
• Since @cms-sw/geometry-l2 and @cms-sw/simulation-l2 already signed the identical Phase2-hgx323 Further correction for ring number assignment of scintillator hits in HGCal geometry versions beyond V13 #39164, I assume I can take their signature for granted also here
• Differences in D77 outpus are expected, see also Phase-hgx323Again Further correction for ring number assignment of scintillator hits in HGCal geometry for versions beyond V13 #39206 (comment): a more detailed comparison will be possible with the 12_5_0_pre5 phase2 relval campaigns
• Issues reported by the SA are independent from this PR, and they are anyhow dealt woth separately in Run3-hcx334 Take care of resulst from static analyser for Geometry/CaloTopology #39208 and Run3-sim126 Try to adress complaints from static analyser for SimG4CMS/Calo #39209

[perrotta]

merge

[bsunanda]

+geometry