Fix Walloc-size-larger-than warnings in SimG4CMS/Forward/*NumberingScheme #41942

iarspider · 2023-06-13T13:53:53Z

PR description:

Fix warnings reported here: #41795 . The changes are similar to what was done in #41826 .

PR validation:

No special validation

cmsbuild · 2023-06-13T14:00:18Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41942/35903

This PR adds an extra 16KB 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-41942/35903/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41942/35903/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

…ingScheme and ZDCNumberingScheme

cmsbuild · 2023-06-13T14:08:40Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41942/35904

This PR adds an extra 16KB to repository

cmsbuild · 2023-06-13T14:09:02Z

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

It involves the following packages:

SimG4CMS/Forward (simulation)

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@bsunanda, @rovere, @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

iarspider · 2023-06-13T14:09:47Z

please test for el8_amd64_gcc12

iarspider · 2023-06-13T14:09:51Z

please test

iarspider · 2023-06-13T14:10:38Z

please test for CMSSW_13_2_DBG_X

just in case - some code changes are conditionally compiled only in DBG_X

iarspider · 2023-06-13T14:13:56Z

please test for el8_amd64_gcc12

cmsbuild · 2023-06-13T19:01:22Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f710c1/33122/summary.html
COMMIT: 3c866ea
CMSSW: CMSSW_13_2_X_2023-06-13-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41942/33122/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 2 lines from the logs
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3190910
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3190888
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 207 log files, 159 edm output root files, 48 DQM output files
TriggerResults: no differences found

cmsbuild · 2023-06-13T20:22:52Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f710c1/33124/summary.html
COMMIT: 3c866ea
CMSSW: CMSSW_13_2_X_2023-06-12-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41942/33124/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 6071 lines to the logs
Reco comparison results: 16068 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3190910
DQMHistoTests: Total failures: 40312
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3150575
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 207 log files, 159 edm output root files, 48 DQM output files
TriggerResults: no differences found

cmsbuild · 2023-06-14T01:57:56Z

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f710c1/33123/summary.html
COMMIT: 3c866ea
CMSSW: CMSSW_13_2_DBG_X_2023-06-08-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/41942/33123/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f710c1/33123/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f710c1/33123/git-merge-result

RelVals

23234.0

----- Begin Fatal Exception 14-Jun-2023 02:57:09 CEST-----------------------
An exception of category 'Invalid DetId' occurred while
   [0] Calling method for module HGCalBackendLayer1Producer/'l1tHGCalBackEndLayer1Producer'
   [1] Prefetching for EventSetup module HGCalTriggerGeometryESProducer/'l1tHGCalTriggerGeometryESProducer'
   [2] Calling method for EventSetup module HGCalGeometryESProducer/'HFNoseGeometryESProducer'
Exception Message:
Cannot initialize HGCSiliconDetId from 6c8bd800
----- End Fatal Exception -------------------------------------------------

civanch · 2023-06-14T09:13:05Z

@iarspider , I fully agree that making temporary C-arrays in these classes is not necessary, in general, this PR is valid. However, these classes are used by sub-detectors, which in normal tests are not involved, so validation of results should be done with enough statistics or in the new code you should have another proof that the result is not changed. In a standard WF you likely will not see anything. A safe approach would be to do exactly the same thing as before but without C-arrays.

civanch · 2023-06-14T09:15:25Z

SimG4CMS/Forward/src/BHMNumberingScheme.cc

-    detectorLevel(aStep, level, copyno, name);
+  //Find number of levels
+  const G4VTouchable* touch = aStep->GetPreStepPoint()->GetTouchable();
+  int level = (touch) ? ((touch->GetHistoryDepth()) + 1) : 0;


I do not think that touch can be undefined.

The check was there in the original code, I can drop it if you think it's safe.

civanch · 2023-06-14T09:18:04Z

SimG4CMS/Forward/src/BHMNumberingScheme.cc

-    delete[] copyno;
-    delete[] name;
+  LogDebug("BHMSim") << "BHMNumberingScheme number of levels= " << level;
+  if (level > 3) {


In previous implementation if(level>3) the fix depth is used not depended on level of 'level'

Not sure what you mean. In previous implementation if level was less than or equal to 3, the arrays filled by ::detectorLevel were not used - unless the calls in that function had side-effects

civanch · 2023-06-14T09:23:21Z

SimG4CMS/Forward/src/BscNumberingScheme.cc

-    for (int ich = 0; ich < level; ich++)
-      LogDebug("BscSim") << " name = " << name[ich] << " copy = " << copyno[ich];
-
+    for (int ich = 0; ich < level; ich++) {


Why we need this loop? LogDebug may be moved inside previous loop after line 31

civanch · 2023-06-14T09:25:07Z

SimG4CMS/Forward/src/ZdcNumberingScheme.cc

-                                     << "  section " << section << " zside " << zside << " layer " << layer << " fiber "
-                                     << fiber << " channel " << channel << "packedIndex =" << intindex
-                                     << " detId raw: " << std::hex << index << std::dec;
+    for (int ich = 0; ich < level; ich++) {


This loop is also not needed, LogVerbatim may be moved after the line 45

iarspider · 2023-06-14T11:20:22Z

@iarspider , I fully agree that making temporary C-arrays in these classes is not necessary, in general, this PR is valid. However, these classes are used by sub-detectors, which in normal tests are not involved, so validation of results should be done with enough statistics or in the new code you should have another proof that the result is not changed. In a standard WF you likely will not see anything. A safe approach would be to do exactly the same thing as before but without C-arrays.

@civanch correct me if I'm wrong, but these classes are mostly just packing and unpacking integers. Or the tricky part is to somehow make sure aStep->GetPreStepPoint()->GetTouchable(); iterates over all parts?

civanch · 2023-06-15T07:15:04Z

@iarspider , any hit should have a correct detector ID, which defined by numbering schema. It is critical in this PR to have exactly the same detID numbers as was before.

civanch · 2023-06-15T23:11:29Z

@iarspider , to me not clear why testing of this PR face problems while #41979 is working, let us close this.

cmsbuild added this to the CMSSW_13_2_X milestone Jun 13, 2023

cmsbuild added simulation-pending pending-signatures tests-pending orp-pending code-checks-pending labels Jun 13, 2023

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jun 13, 2023

Fix Walloc-size-larger-than warnings in BHMNumberingScheme, BSCNumber…

3c866ea

…ingScheme and ZDCNumberingScheme

iarspider force-pushed the fix-simg4cms-warnings branch from fdecb97 to 3c866ea Compare June 13, 2023 14:02

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jun 13, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 13, 2023

cmsbuild added tests-started and removed tests-pending labels Jun 13, 2023

cmsbuild added tests-approved and removed tests-started labels Jun 13, 2023

civanch reviewed Jun 14, 2023

View reviewed changes

Dr15Jones mentioned this pull request Jun 15, 2023

Improved *NumberingScheme code for Forward detectors #41979

Merged

iarspider closed this Jun 16, 2023

iarspider deleted the fix-simg4cms-warnings branch June 16, 2023 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Walloc-size-larger-than warnings in SimG4CMS/Forward/*NumberingScheme #41942

Fix Walloc-size-larger-than warnings in SimG4CMS/Forward/*NumberingScheme #41942

iarspider commented Jun 13, 2023

cmsbuild commented Jun 13, 2023

cmsbuild commented Jun 13, 2023

cmsbuild commented Jun 13, 2023

iarspider commented Jun 13, 2023

iarspider commented Jun 13, 2023

iarspider commented Jun 13, 2023

iarspider commented Jun 13, 2023

cmsbuild commented Jun 13, 2023

cmsbuild commented Jun 13, 2023

cmsbuild commented Jun 14, 2023

civanch commented Jun 14, 2023

civanch Jun 14, 2023

iarspider Jun 14, 2023

civanch Jun 14, 2023

iarspider Jun 14, 2023

civanch Jun 14, 2023

iarspider Jun 14, 2023

civanch Jun 14, 2023

iarspider Jun 14, 2023

iarspider commented Jun 14, 2023

civanch commented Jun 15, 2023

civanch commented Jun 15, 2023

Fix Walloc-size-larger-than warnings in SimG4CMS/Forward/*NumberingScheme #41942

Fix Walloc-size-larger-than warnings in SimG4CMS/Forward/*NumberingScheme #41942

Conversation

iarspider commented Jun 13, 2023

PR description:

PR validation:

cmsbuild commented Jun 13, 2023

cmsbuild commented Jun 13, 2023

cmsbuild commented Jun 13, 2023

iarspider commented Jun 13, 2023

iarspider commented Jun 13, 2023

iarspider commented Jun 13, 2023

iarspider commented Jun 13, 2023

cmsbuild commented Jun 13, 2023

Comparison Summary

cmsbuild commented Jun 13, 2023

Comparison Summary

cmsbuild commented Jun 14, 2023

RelVals

civanch commented Jun 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iarspider commented Jun 14, 2023

civanch commented Jun 15, 2023

civanch commented Jun 15, 2023