Fix ECAL Endcap quadrants issue with DD4hep reflected volumes + Also fix ECAL Barrel and CSCs reflected active detectors #32218

ghugo83 · 2020-11-20T17:50:58Z

This PR fixes the handling of reflected volumes in CMSSW in DD4hep workflows.

The key idea is that the handling of reflected volumes is different in old DD and DD4hep, as there are 2 different files in CMSSW:

In old DD, there is a special treatment for reflected volumes, so that the volumes with _refl are added to the sensitive detector catalogue.
In DD4hep, the G4 reflected volumes were properly created, but their names with _refl were not added to the active detectors catalogue list.
This resulted, further downstream in https://github.com/cms-sw/cmssw/blob/master/SimG4Core/SensitiveDetector/src/SensitiveDetector.cc#L53, to have the reflected volumes not set as active detectors.
This issue did not appear with the TGeo trick, because the reflected volumes with TGeo, did not have the _refl names.
The reflected active volumes being not considered as sensitive detectors by G4, there were obviously no SimHit on them.

The proper list of active volumes is now fixed, and includes the active reflected volumes.

NB: The affected active volumes, which are now present on a DD4hep workflow, and which were NOT considered as active volumes until now, without the TGeo trick, are:

In ECAL Endcap:
reflectedG4LogicalVolumeName = EFRY_refl
-> This solves the ECAL Endcap quadrants issue.

In ECAL Barrel:
reflectedG4LogicalVolumeName = ebalgo:EBRY_01_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_01_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_01_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_02_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_02_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_02_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_03_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_03_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_03_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_07_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_04_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_04_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_04_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_05_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_05_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_05_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_06_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_06_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_06_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_07_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_07_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_08_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_08_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_08_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_09_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_09_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_09_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_10_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_10_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_10_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_11_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_11_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_11_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_12_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_12_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_12_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_13_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_13_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_13_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_14_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_14_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_14_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_15_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_15_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_15_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_16_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_16_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_16_refl
reflectedG4LogicalVolumeName = ebalgo:EBRY_17_refl
reflectedG4LogicalVolumeName = ebalgo:EATJ_17_refl
reflectedG4LogicalVolumeName = ebalgo:EAPD_17_refl

In CSC:
reflectedG4LogicalVolumeName = csc:ME1A_ActiveGasVol_refl
reflectedG4LogicalVolumeName = csc:ME11_ActiveGasVol_refl
reflectedG4LogicalVolumeName = csc:ME12_ActiveGasVol_refl
reflectedG4LogicalVolumeName = csc:ME13_ActiveGasVol_refl
reflectedG4LogicalVolumeName = csc:ME21_ActiveGasVol_refl
reflectedG4LogicalVolumeName = csc:ME22_ActiveGasVol_refl
reflectedG4LogicalVolumeName = csc:ME31_ActiveGasVol_refl
reflectedG4LogicalVolumeName = csc:ME32_ActiveGasVol_refl
reflectedG4LogicalVolumeName = csc:ME41_ActiveGasVol_refl
reflectedG4LogicalVolumeName = csc:ME42_ActiveGasVol_refl

NB 2: PR quickly done here, might push a few commits to clean it.

@ianna @civanch @bsunanda @cvuosalo

…p_DDG4Builder.cc

…reflected active volumes in CSCs and ECAL Barrel. The idea is that the G4 reflected volumes (with names _refl) were not added to the sensitive catalogue, hence the associated reflected G4 logical volumes were not activated as sensitive, hence no SimHits on these active volumes.

cmsbuild · 2020-11-20T17:57:55Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32218/19964

This PR adds an extra 12KB to repository

cmsbuild · 2020-11-20T17:58:14Z

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

It involves the following packages:

SimG4Core/Geometry

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@makortel, @rovere, @fabiocos, @slomeo this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

civanch · 2020-11-20T18:03:58Z

please test

cmsbuild · 2020-11-20T18:04:23Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-11-20-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/10910/console Started: 2020/11/20 19:06

ghugo83 · 2020-11-20T18:10:34Z

Ecal Endcap quadrants are back, and this time with no TGeo trick! This is using the DD4hep reflected volumes, on CMSSW_11_2_X_2020-11-17-2300.

cmsbuild · 2020-11-20T19:34:53Z

+1
Tested at: db38ed1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a5dc00/10910/summary.html
CMSSW: CMSSW_11_2_X_2020-11-20-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-11-20T19:34:55Z

Comparison job queued.

cvuosalo · 2020-11-20T19:50:46Z

SimG4Core/Geometry/src/DD4hep_DDG4Builder.cc

+    G4LogicalVolumeStore *theStore = G4LogicalVolumeStore::GetInstance();
+    for (const auto &lv : *theStore) {
+      if (lv->GetName().find(reflectedG4LogicalVolumeName) != std::string::npos) {
+        hasReflectedVolumeInStore = true;


Shouldn't you add after this line:

break;

yes need to clean style a bit, will slightly improve.

But for now I was focused on finding the issue

break is a good point, I will add it

@cvuosalo Yep I remember what happened now, actually I wanted to use a std::find, but I did not know what was the type of G4LogicalVolumeStore, so just made the easy loop, to first see whether the fix was working anyway.
Just looked it up, G4LogicalVolumeStore is a std::vector<G4LogicalVolume*>.
I have just used a std::find_if in #32241 , it is equivalent to a loop and break, as of course stops at first occurrence.

cvuosalo · 2020-11-20T20:03:22Z

@ghugo83 How many logical volumes are contained in the G4LogicalVolumeStore? Does searching through the store one-by-one for each reflected volume take a long time? Maybe make one pass through the store?

ghugo83 · 2020-11-20T20:59:43Z

@ghugo83 How many logical volumes are contained in the G4LogicalVolumeStore? Does searching through the store one-by-one for each reflected volume take a long time? Maybe make one pass through the store?

The overall complexity is O(number of active non-reflected logical volumes x number of logical volumes).
Please note that these are logical volumes, not placed volumes (way less).

This PR has no effect on the overall complexity of the sensitive catalogue initialization phase, because there is already an existing O(number of active non-reflected logical volumes x number of logical volumes) complexity from a loop just after anyway.

It is here: https://github.com/cms-sw/cmssw/blob/master/SimG4Core/SensitiveDetector/src/SensitiveDetector.cc#L49
AssignSD has complexity O(number of logical volumes), and is called O(number of active logical volumes) times from
https://github.com/cms-sw/cmssw/blob/master/SimG4Core/SensitiveDetector/src/SensitiveDetector.cc#L36

This PR has same complexity, hence no observable effect on complexity overall.

ghugo83 · 2020-11-20T21:18:09Z

The time spent at the creation of the SensitiveCatalogue is overall negligible anyway, it is not a bottleneck AND additionally, it is only called once :)
Way more time is spent elsewhere (huge numbers of orders of magnitude more) at initialization :)

cmsbuild · 2020-11-21T00:19:57Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a5dc00/10910/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4355 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2962300
DQMHistoTests: Total failures: 106736
DQMHistoTests: Total nulls: 2
DQMHistoTests: Total successes: 2855540
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 158 log files, 34 edm output root files, 37 DQM output files

civanch · 2020-11-21T15:09:51Z

Number of logical volumes is ~4000 in Run-2.

civanch · 2020-11-21T15:10:03Z

+1

cmsbuild · 2020-11-21T15:10:09Z

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

civanch · 2020-11-21T15:10:11Z

urgent

qliphy · 2020-11-21T15:30:09Z

+1

cvuosalo · 2020-11-21T20:11:49Z

Did we merge the version without break;?

civanch · 2020-11-21T22:33:45Z

@ghugo83 , @cvuosalo , sorry, I have signed it too fast - "break;" will speed-up the code. Let us add in the next PR.

ghugo83 added 3 commits November 20, 2020 12:14

Simple printouts of reflected voluems in SimG4Core/Geometry/src/DD4he…

c61d9a8

…p_DDG4Builder.cc

Cosmetics

db38ed1

cmsbuild added this to the CMSSW_11_2_X milestone Nov 20, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures simulation-pending tests-pending labels Nov 20, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 20, 2020

cmsbuild added tests-started and removed tests-pending labels Nov 20, 2020

cmsbuild added tests-approved and removed tests-started labels Nov 20, 2020

cvuosalo reviewed Nov 20, 2020

View reviewed changes

cmsbuild added comparison-available and removed comparison-pending labels Nov 21, 2020

cmsbuild added fully-signed simulation-approved and removed pending-signatures simulation-pending labels Nov 21, 2020

cmsbuild added the urgent label Nov 21, 2020

cmsbuild added orp-approved and removed orp-pending labels Nov 21, 2020

cmsbuild merged commit d3024a8 into cms-sw:master Nov 21, 2020

ghugo83 mentioned this pull request Nov 23, 2020

Mini-PR: Minor style cleaning #32241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ECAL Endcap quadrants issue with DD4hep reflected volumes + Also fix ECAL Barrel and CSCs reflected active detectors #32218

Fix ECAL Endcap quadrants issue with DD4hep reflected volumes + Also fix ECAL Barrel and CSCs reflected active detectors #32218

ghugo83 commented Nov 20, 2020 •

edited

Loading

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

civanch commented Nov 20, 2020

cmsbuild commented Nov 20, 2020 •

edited

Loading

ghugo83 commented Nov 20, 2020 •

edited

Loading

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

cvuosalo Nov 20, 2020

ghugo83 Nov 20, 2020

ghugo83 Nov 20, 2020 •

edited

Loading

ghugo83 Nov 20, 2020

ghugo83 Nov 23, 2020 •

edited

Loading

cvuosalo commented Nov 20, 2020

ghugo83 commented Nov 20, 2020 •

edited

Loading

ghugo83 commented Nov 20, 2020

cmsbuild commented Nov 21, 2020

civanch commented Nov 21, 2020

civanch commented Nov 21, 2020

cmsbuild commented Nov 21, 2020

civanch commented Nov 21, 2020

qliphy commented Nov 21, 2020

cvuosalo commented Nov 21, 2020

civanch commented Nov 21, 2020

Fix ECAL Endcap quadrants issue with DD4hep reflected volumes + Also fix ECAL Barrel and CSCs reflected active detectors #32218

Fix ECAL Endcap quadrants issue with DD4hep reflected volumes + Also fix ECAL Barrel and CSCs reflected active detectors #32218

Conversation

ghugo83 commented Nov 20, 2020 • edited Loading

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

civanch commented Nov 20, 2020

cmsbuild commented Nov 20, 2020 • edited Loading

ghugo83 commented Nov 20, 2020 • edited Loading

cmsbuild commented Nov 20, 2020

cmsbuild commented Nov 20, 2020

cvuosalo Nov 20, 2020

Choose a reason for hiding this comment

ghugo83 Nov 20, 2020

Choose a reason for hiding this comment

ghugo83 Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

ghugo83 Nov 20, 2020

Choose a reason for hiding this comment

ghugo83 Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

cvuosalo commented Nov 20, 2020

ghugo83 commented Nov 20, 2020 • edited Loading

ghugo83 commented Nov 20, 2020

cmsbuild commented Nov 21, 2020

civanch commented Nov 21, 2020

civanch commented Nov 21, 2020

cmsbuild commented Nov 21, 2020

civanch commented Nov 21, 2020

qliphy commented Nov 21, 2020

cvuosalo commented Nov 21, 2020

civanch commented Nov 21, 2020

ghugo83 commented Nov 20, 2020 •

edited

Loading

cmsbuild commented Nov 20, 2020 •

edited

Loading

ghugo83 commented Nov 20, 2020 •

edited

Loading

ghugo83 Nov 20, 2020 •

edited

Loading

ghugo83 Nov 23, 2020 •

edited

Loading

ghugo83 commented Nov 20, 2020 •

edited

Loading