Run3-hcx373 Correct the HcalTopology class for 2 new geometry modes #46308

bsunanda · 2024-10-08T18:11:06Z

PR description:

Correct the HcalTopology class for 2 new geometry modes. Geometry mode is used to specify the detector scenario. LHC was the fixed version of the legacy detector. New modes are defined to specify the current situation with new ZDC assignment with the RPD detector (Run3) and the Phase2 setup (Run4) with no HE detector. It will be expanded in future to mark when HF DetIds were enlarged (2016), HE started expansion (2017) and HB had several depths (post LS2). Some of this is reflected in this version of HcalTopology. The usage of the modes was not correctly interpreted in the earlier version of the HcalTopology code.

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:

Backport to 14_1_X using #46309

cmsbuild · 2024-10-08T18:11:34Z

cms-bot internal usage

cmsbuild · 2024-10-08T18:13:07Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46308/42132

There are other open Pull requests which might conflict with changes you have proposed:
- File Geometry/CaloTopology/src/HcalTopology.cc modified in PR(s): Fix uninitialized values in HcalTopology #46305

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

cmsbuild · 2024-10-08T18:21:22Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46308/42133

There are other open Pull requests which might conflict with changes you have proposed:
- File Geometry/CaloTopology/src/HcalTopology.cc modified in PR(s): Fix uninitialized values in HcalTopology #46305

cmsbuild · 2024-10-08T18:21:48Z

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

It involves the following packages:

CondTools/Geometry (db)
Geometry/CaloTopology (geometry)

@Dr15Jones, @atpathak, @bsunanda, @civanch, @cmsbuild, @consuegs, @francescobrivio, @kpedro88, @makortel, @mdhildreth, @perrotta can you please review it and eventually sign? Thanks.
@JanChyczynski, @PonIlya, @ReyerBand, @fabiocos, @martinamalberti, @mmusich, @rsreds, @thomreis, @wang0jin, @yuanchao this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

…s-sw#46308

bsunanda · 2024-10-08T18:28:53Z

@cmsbuild Please test

Dr15Jones · 2024-10-08T18:30:48Z

It would seem to me that instead of having if ( mode_ == .... || mode_ ....) all over the code, to use a different new variable phase_ which is either Phase1 or Phase2 based on the value of mode_.

bsunanda · 2024-10-10T06:25:07Z

+geometry

bsunanda · 2024-10-10T06:25:29Z

@perrotta Please approve this

perrotta

This PR includes some updates/fixes with respect to the HcalTopology currently in CMSSW: please add the description of the rationale of those changes/fixes in the PR description, so that it can also remain as a future documentation (besides helping now the poor reviewer of the PR).

As far as I understand, the fixes are the following ones:

(1) Avoid undefined behaviour at L75 for modes different from LHC and SLHC
(2) Remove Run3 from the Phase2 modes, as it is erroneously now the case in the release

Then I remain with my doubt about L198 that I expect you can explain here (or fix, if needed)

perrotta · 2024-10-10T09:55:20Z

Geometry/CaloTopology/src/HcalTopology.cc

-          ((mode == HcalTopologyMode::SLHC) || (mode_ == HcalTopologyMode::Run3) || (mode_ == HcalTopologyMode::Run4))
-              ? 500
-              : 87) {
+      numberOfShapes_((mode_ == HcalTopologyMode::LHC) ? 87 : 500) {


If I want to reproduce what in L82 I should write here:

Suggested change

numberOfShapes_((mode_ == HcalTopologyMode::LHC) ? 87 : 500) {

numberOfShapes_((mode_ == HcalTopologyMode::LHC) ? 87 : ((maxPhiHE_ > 72) ? 1200 : 500)) {

Could you please explain why they are different, if they need to be so? Or fix the wrong one, if they have to be the same?

perrotta · 2024-10-10T09:58:15Z

Geometry/CaloTopology/src/HcalTopology.cc

@@ -571,8 +567,7 @@ bool HcalTopology::validRaw(const HcalDetId& id) const {

  if (ok) {
    if (subdet == HcalBarrel) {
-      if ((mode_ == HcalTopologyMode::SLHC) || (mode_ == HcalTopologyMode::H2HE) || (mode_ == HcalTopologyMode::Run3) ||
-          (mode_ == HcalTopologyMode::Run4)) {
+      if (phase2() || (mode_ == HcalTopologyMode::H2HE)) {


Ok, I understand now that this is the fix: previously Run3 was erroneously counted together with the Phase2 modes.
Please, write it in the PR description, so that one can understand from there what you are fixing.

Dr15Jones · 2024-10-10T16:06:20Z

So I removed the constructor

cmssw/Geometry/CaloTopology/interface/HcalTopology.h

Lines 29 to 32 in 9668a4d

    
           HcalTopology(HcalTopologyMode::Mode mode, 
        
                        int maxDepthHB, 
        
                        int maxDepthHE, 
        
                        HcalTopologyMode::TriggerMode tmode = HcalTopologyMode::TriggerMode_2009);

from the class, did git cms-checkdeps -a and rebuilt all the relevant parts of CMSSW and everything compiled just fine. That means we should remove that constructor for the class to help with maintenance.

The other reason to remove that constructor is it can lead to an instance of HcalTopology which will crash on certain calls because large number of member data expect the member data hcons_ to be set, which does not happen with that constructor.

perrotta · 2024-10-10T16:58:43Z

So I removed the constructor

cmssw/Geometry/CaloTopology/interface/HcalTopology.h

Lines 29 to 32 in 9668a4d

HcalTopology(HcalTopologyMode::Mode mode,

int maxDepthHB,

int maxDepthHE,

HcalTopologyMode::TriggerMode tmode = HcalTopologyMode::TriggerMode_2009);

from the class, did git cms-checkdeps -a and rebuilt all the relevant parts of CMSSW and everything compiled just fine. That means we should remove that constructor for the class to help with maintenance.

The other reason to remove that constructor is it can lead to an instance of HcalTopology which will crash on certain calls because large number of member data expect the member data hcons_ to be set, which does not happen with that constructor.

Thank you @Dr15Jones , I second your proposal.
It would also remove the ambiguity I pointed out in #46308 (comment) : still I would like to hear from @bsunanda which is the correct formulation for numberOfShapes_

bsunanda · 2024-10-11T06:59:35Z

@perrotta Please sign this. I am preparing the correct implementation of HcalTopology given what I wrote earlier for different eras of HCAL. This version is adequate for CMSSW_14_1_X.

perrotta · 2024-10-11T07:15:29Z

@perrotta Please sign this. I am preparing the correct implementation of HcalTopology given what I wrote earlier for different eras of HCAL. This version is adequate for CMSSW_14_1_X.

@bsunanda could you please at least reply to my question in #46308 (comment) ? Since there are two different computations of the same quantity in the two different constructors I think it is mandatory to understand whether it is correct that they are such different, and if not the wrong one must be fixed (or even removed, as suggested by @Dr15Jones )

bsunanda · 2024-10-11T07:35:51Z

@perrotta At the moment the second constructor is not used and it won't create a mess. I shall surely remove it in the next version. It was left there because when we were changing the HcalTopology (some 10-12 years ago), some of the codes were still using HcalTopology without accessing Hcal..Const from EventSetup. That practice has disappeared as seen by Chris's test. So it is safe to remove it. If I remove this in this PR, this will trigger the tests and the whole process of integration
will be delayed.

perrotta · 2024-10-11T07:44:22Z

@perrotta At the moment the second constructor is not used and it won't create a mess. I shall surely remove it in the next version. It was left there because when we were changing the HcalTopology (some 10-12 years ago), some of the codes were still using HcalTopology without accessing Hcal..Const from EventSetup. That practice has disappeared as seen by Chris's test. So it is safe to remove it. If I remove this in this PR, this will trigger the tests and the whole process of integration will be delayed.

Fine.
But which is the correct definition of numberOfShapes_ (fourth time I ask here....)? The one in the constructor that is used, or the one in the unused constructor?
Only if the correct definition is the one in the used constructor I can accept:

signing and merging this PR in the master
make at once another PR in master that removes the unused (and bugged) constructor
backport in Run3-hcx373X Correct the HcalTopology class for 2 new geometry modes - backport #46308 #46309 the combination of the two PRs

bsunanda · 2024-10-11T07:45:49Z

@perrotta The first constructor is the right one. I shall remove the second constructor in my next PR

perrotta · 2024-10-11T07:51:32Z

+db

This PR implements some necessary fix to the HcalTopology class:
- Avoid undefined behaviour at L75 for modes different from LHC and SLHC
- Remove Run3 from the Phase2 modes, as it is erroneously now the case in the release
It is understood that another (minor) fix is needed to remove a bugged constructor: a fix will be also provided and combined in the backport PR

cmsbuild · 2024-10-11T07:51:57Z

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

perrotta · 2024-10-11T07:52:43Z

urgent
(@cms-sw/orp-l2 could you please merge in time for the 1100 IB, so that the necessary actions can be taken at once to implement further fixes and prepare a backport?)

mandrenguyen · 2024-10-11T07:58:28Z

+1

Correct the HcalTopology class for 2 new geometry modes

90941f6

cmsbuild added this to the CMSSW_14_2_X milestone Oct 8, 2024

cmsbuild added db-pending geometry-pending pending-signatures tests-pending orp-pending code-checks-pending labels Oct 8, 2024

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 8, 2024

Code check

f728e88

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 8, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 8, 2024

bsunanda pushed a commit to bsunanda/cmssw that referenced this pull request Oct 8, 2024

Correct the HcalTopology class for 2 new geometry modes - backport cm…

3745f30

…s-sw#46308

bsunanda mentioned this pull request Oct 8, 2024

Run3-hcx373X Correct the HcalTopology class for 2 new geometry modes - backport #46308 #46309

Closed

cmsbuild added tests-started and removed tests-pending labels Oct 8, 2024

Dr15Jones mentioned this pull request Oct 8, 2024

Fix uninitialized values in HcalTopology #46305

Closed

Suggestion from Chris

3e95ab9

cmsbuild added tests-pending and removed tests-started code-checks-approved labels Oct 8, 2024

cmsbuild added the tests-approved label Oct 9, 2024

cmsbuild added geometry-approved and removed geometry-pending labels Oct 10, 2024

bsunanda pushed a commit to bsunanda/cmssw that referenced this pull request Oct 10, 2024

Correct HcalTopology class - backport from cms-sw#46308 and cms-sw#46309

895a951

bsunanda mentioned this pull request Oct 10, 2024

Run3-hcx373Y Correct HcalTopology class - backport from #46308 and #46309 #46332

Closed

perrotta reviewed Oct 10, 2024

View reviewed changes

cmsbuild added fully-signed db-approved and removed db-pending pending-signatures labels Oct 11, 2024

cmsbuild added the urgent label Oct 11, 2024

cmsbuild added orp-approved and removed orp-pending labels Oct 11, 2024

cmsbuild merged commit 1e14eab into cms-sw:master Oct 11, 2024
11 checks passed

cmsbuild mentioned this pull request Oct 11, 2024

Runtime cxxmodules aarch64 cms-sw/root#212

Closed

perrotta mentioned this pull request Oct 11, 2024

Run3-hcx376 Get rid of the obsolete constructor of the HcalTopology class #46357

Merged

cmsbuild mentioned this pull request Oct 16, 2024

Update a few MC Gts in autoCond to the latest version available on 14/10/2024 #46379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run3-hcx373 Correct the HcalTopology class for 2 new geometry modes #46308

Run3-hcx373 Correct the HcalTopology class for 2 new geometry modes #46308

bsunanda commented Oct 8, 2024 •

edited

Loading

cmsbuild commented Oct 8, 2024 •

edited

Loading

cmsbuild commented Oct 8, 2024

cmsbuild commented Oct 8, 2024

cmsbuild commented Oct 8, 2024

bsunanda commented Oct 8, 2024

Dr15Jones commented Oct 8, 2024

bsunanda commented Oct 10, 2024

bsunanda commented Oct 10, 2024

perrotta left a comment •

edited

Loading

perrotta Oct 10, 2024

perrotta Oct 10, 2024

Dr15Jones commented Oct 10, 2024 •

edited

Loading

perrotta commented Oct 10, 2024

bsunanda commented Oct 11, 2024

perrotta commented Oct 11, 2024

bsunanda commented Oct 11, 2024

perrotta commented Oct 11, 2024

bsunanda commented Oct 11, 2024

perrotta commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

perrotta commented Oct 11, 2024

mandrenguyen commented Oct 11, 2024

	numberOfShapes_((mode_ == HcalTopologyMode::LHC) ? 87 : 500) {
	numberOfShapes_((mode_ == HcalTopologyMode::LHC) ? 87 : ((maxPhiHE_ > 72) ? 1200 : 500)) {

Run3-hcx373 Correct the HcalTopology class for 2 new geometry modes #46308

Run3-hcx373 Correct the HcalTopology class for 2 new geometry modes #46308

Conversation

bsunanda commented Oct 8, 2024 • edited Loading

PR description:

PR validation:

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:

cmsbuild commented Oct 8, 2024 • edited Loading

cmsbuild commented Oct 8, 2024

cmsbuild commented Oct 8, 2024

cmsbuild commented Oct 8, 2024

bsunanda commented Oct 8, 2024

Dr15Jones commented Oct 8, 2024

bsunanda commented Oct 10, 2024

bsunanda commented Oct 10, 2024

perrotta left a comment • edited Loading

Choose a reason for hiding this comment

perrotta Oct 10, 2024

Choose a reason for hiding this comment

perrotta Oct 10, 2024

Choose a reason for hiding this comment

Dr15Jones commented Oct 10, 2024 • edited Loading

perrotta commented Oct 10, 2024

bsunanda commented Oct 11, 2024

perrotta commented Oct 11, 2024

bsunanda commented Oct 11, 2024

perrotta commented Oct 11, 2024

bsunanda commented Oct 11, 2024

perrotta commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

perrotta commented Oct 11, 2024

mandrenguyen commented Oct 11, 2024

bsunanda commented Oct 8, 2024 •

edited

Loading

cmsbuild commented Oct 8, 2024 •

edited

Loading

perrotta left a comment •

edited

Loading

Dr15Jones commented Oct 10, 2024 •

edited

Loading