[DD4hep] Ecal Barrel algorithm - use assemblies. Fix all overlaps in EBAR. #33339

ianna · 2021-04-06T13:00:20Z

PR description:

Two SubtractionSolids volumes: ESPM and EHAWR made of E_Air are replaced by Assemblies with identical names. One SubtractionSolid EFAW is left as a mother volume intentionally in the hierarchy. This helps to restrict the number of volumes for a ROOT overlap check and complete it within a reasonable time.

The main source of 36 overlaps for both DD and DD4hep-based EcalBarrel geometry was a rounding error. This is solved by adding 1 mm to a Z in EBAR envelope (in blue):

After this change there are no overlaps reported in Ecal barrel in DD4hep-based geometry:

@bsunanda - FYI

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

Before submitting your pull requests, make sure you followed this checklist:

verify that the PR is really intended for the chosen branch
verify that changes follow CMS Naming, Coding, And Style Rules
verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

cmsbuild · 2021-04-06T13:05:10Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33339/21911

This PR adds an extra 40KB to repository

cmsbuild · 2021-04-06T13:05:33Z

A new Pull Request was created by @ianna (Ianna Osborne) for master.

It involves the following packages:

Geometry/EcalCommonData

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please review it and eventually sign? Thanks.
@rchatter, @simonepigazzini, @fabiocos, @thomreis, @argiro 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

ianna · 2021-04-06T13:15:51Z

please test

cmsbuild · 2021-04-06T13:23:16Z

-1

Failed Tests: ClangBuild
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3936a8/14021/summary.html
COMMIT: 1f27570
CMSSW: CMSSW_11_3_X_2021-04-05-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33339/14021/install.sh to create a dev area with all the needed externals and cmssw changes.

Clang Build

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

civanch · 2021-04-06T13:49:02Z

Geometry/EcalCommonData/plugins/dd4hep/DDEcalBarrelNewAlgo.cc

@@ -2087,7 +2048,7 @@ static long algorithm(dd4hep::Detector& /* description */, cms::DDParsingContext
      Volume backPlateLog = Volume(back.plateName, backPlateSolid, ns.material(back.plateMat));

      const Position backPlateTra(
-          0.5 * back.sideHeight + backPlateParms[1], 0, backPlateParms[2] - 0.5 * back.sideLength);
+          0.5 * back.sideHeight + backPlateParms[1], 0 * dd4hep::mm, backPlateParms[2] - 0.5 * back.sideLength);


@ianna , here 0 is more correct then 0*mm

@civanch - the effect is the same. IMHO, adding units to a number explains better what the number is. There is no downside - the compiler will optimise it anyway. :-)

Geometry/EcalCommonData/plugins/dd4hep/DDEcalBarrelNewAlgo.cc

cvuosalo · 2021-04-06T18:49:26Z

Clang warnings need to be fixed:

Geometry/EcalCommonData/plugins/dd4hep/DDEcalBarrelNewAlgo.cc
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_3_X_2021-04-05-2300/src/Geometry/EcalCommonData/plugins/dd4hep/DDEcalBarrelNewAlgo.cc:965:19: warning: unused variable 'alltrot' [-Wunused-variable]
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_3_X_2021-04-05-2300/src/Geometry/EcalCommonData/plugins/dd4hep/DDEcalBarrelNewAlgo.cc:950:11: warning: unused variable 'temp2' [-Wunused-variable]
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_3_X_2021-04-05-2300/src/Geometry/EcalCommonData/plugins/dd4hep/DDEcalBarrelNewAlgo.cc:949:11: warning: unused variable 'temp1' [-Wunused-variable]

Geometry/EcalCommonData/plugins/dd4hep/DDEcalBarrelNewAlgo.cc

ianna · 2021-04-07T10:35:40Z

please test

cmsbuild · 2021-04-07T22:33:08Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3936a8/14076/summary.html
COMMIT: 9d48ceb
CMSSW: CMSSW_11_3_X_2021-04-06-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33339/14076/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-3936a8/11650.911_ZMM_14+2021_DD4hep+ZMM_14TeV_TuneCP5_GenSim+Digi+Reco+HARVEST+ALCA

Summary:

No significant changes to the logs found
Reco comparison results: 14117 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2640868
DQMHistoTests: Total failures: 198006
DQMHistoTests: Total nulls: 192
DQMHistoTests: Total successes: 2442648
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: found differences in 1 / 36 workflows

civanch · 2021-04-08T08:05:40Z

Minor changes in ECAL geometry provoke change in simulation history, so no regression in this PR.

cvuosalo · 2021-04-08T18:56:25Z

This PR still has a problem with the ECal barrel:

%MSG-w EcalGeom: OscarMTProducer:g4SimHits 07-Apr-2021 21:16:39 CEST Run: 1 Event: 1
ECalBarrelNumberingScheme::getUnitID(): Not enough levels found in EcalBaseNumber ( 11) Returning 0
%MSG

There are thousands of these messages in the workflow logs.

silviodonato · 2021-04-20T08:42:14Z

cc @cms-sw/ecal-dpg-l2

bsunanda · 2021-04-20T12:55:50Z

@ianna @silviodonato @qliphy This PR needs the new dd4hep version to work. The new dd4hep is indeed needed

civanch · 2021-04-20T13:43:23Z

@ianna , @bsunanda , @mrodozov , if special IB made recently by Mirco is working why not to make a new version of DD4Hep for 12_0_X? Or DD4Hep update is not yet tagged?

civanch · 2021-04-20T15:51:29Z

The issue cms-sw/cmsdist#6824 is created.

ianna · 2021-04-21T12:19:00Z

@civanch - I guess this one can go in now. It will need a PR from @bsunanda to update the numbering.

bsunanda · 2021-04-21T12:54:40Z

@ianna @civanch @silviodonato The PR for numbering is already integrated with the master. So with the new version of dd4hep, this PR can be integrated.

civanch · 2021-04-21T14:12:21Z

+1

cmsbuild · 2021-04-21T14:12:41Z

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)

silviodonato · 2021-04-21T15:24:16Z

+1

…ms-sw#33471)

Run3-gee71x Correct EB geometry and numbering scheme (backport #33339 and #33471)

ianna added 5 commits April 6, 2021 10:37

use assemblies

ea7bf47

clean up

6c43f82

code format

0c81ac4

fix extrusions in Ecal Barrel

4ce5e38

cleanup

1f27570

cmsbuild added this to the CMSSW_11_3_X milestone Apr 6, 2021

cmsbuild added code-checks-pending geometry-pending orp-pending pending-signatures tests-pending labels Apr 6, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 6, 2021

cmsbuild added tests-started tests-rejected and removed tests-pending tests-started labels Apr 6, 2021

civanch reviewed Apr 6, 2021

View reviewed changes

Geometry/EcalCommonData/plugins/dd4hep/DDEcalBarrelNewAlgo.cc Outdated Show resolved Hide resolved

cvuosalo reviewed Apr 6, 2021

View reviewed changes

Geometry/EcalCommonData/plugins/dd4hep/DDEcalBarrelNewAlgo.cc Outdated Show resolved Hide resolved

fix clang warnings and address comments

6fa446d

cmsbuild added code-checks-pending and removed code-checks-approved tests-rejected labels Apr 7, 2021

cmsbuild added tests-started and removed tests-approved labels Apr 7, 2021

cmsbuild added tests-approved and removed tests-started labels Apr 7, 2021

cmsbuild mentioned this pull request Apr 9, 2021

MultiCluster-CaloParticle associator #32941

Closed

cmsbuild modified the milestones: CMSSW_11_3_X, CMSSW_12_0_X Apr 15, 2021

civanch mentioned this pull request Apr 20, 2021

Update DD4Hep to the most recent version cms-sw/cmsdist#6824

Closed

cmsbuild added fully-signed geometry-approved and removed geometry-pending pending-signatures labels Apr 21, 2021

cmsbuild added orp-approved and removed orp-pending labels Apr 21, 2021

cmsbuild merged commit b218083 into cms-sw:master Apr 21, 2021

bsunanda pushed a commit to bsunanda/cmssw that referenced this pull request Apr 21, 2021

Correct EB geometry and numbering scheme (backport cms-sw#33339 and c…

32e8961

…ms-sw#33471)

bsunanda mentioned this pull request Apr 21, 2021

Run3-gee71x Correct EB geometry and numbering scheme (backport #33339 and #33471) #33493

Merged

cmsbuild added a commit that referenced this pull request Apr 23, 2021

Merge pull request #33493 from bsunanda/Run3-geex71x

eaadaf7

Run3-gee71x Correct EB geometry and numbering scheme (backport #33339 and #33471)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DD4hep] Ecal Barrel algorithm - use assemblies. Fix all overlaps in EBAR. #33339

[DD4hep] Ecal Barrel algorithm - use assemblies. Fix all overlaps in EBAR. #33339

ianna commented Apr 6, 2021

cmsbuild commented Apr 6, 2021

cmsbuild commented Apr 6, 2021

ianna commented Apr 6, 2021

cmsbuild commented Apr 6, 2021

civanch Apr 6, 2021

ianna Apr 7, 2021

cvuosalo commented Apr 6, 2021

ianna commented Apr 7, 2021

cmsbuild commented Apr 7, 2021

civanch commented Apr 8, 2021 •

edited

cvuosalo commented Apr 8, 2021

silviodonato commented Apr 20, 2021

bsunanda commented Apr 20, 2021

civanch commented Apr 20, 2021

civanch commented Apr 20, 2021

ianna commented Apr 21, 2021

bsunanda commented Apr 21, 2021

civanch commented Apr 21, 2021

cmsbuild commented Apr 21, 2021

silviodonato commented Apr 21, 2021

[DD4hep] Ecal Barrel algorithm - use assemblies. Fix all overlaps in EBAR. #33339

[DD4hep] Ecal Barrel algorithm - use assemblies. Fix all overlaps in EBAR. #33339

Conversation

ianna commented Apr 6, 2021

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Apr 6, 2021

cmsbuild commented Apr 6, 2021

ianna commented Apr 6, 2021

cmsbuild commented Apr 6, 2021

Clang Build

civanch Apr 6, 2021

Choose a reason for hiding this comment

ianna Apr 7, 2021

Choose a reason for hiding this comment

cvuosalo commented Apr 6, 2021

ianna commented Apr 7, 2021

cmsbuild commented Apr 7, 2021

Comparison Summary

civanch commented Apr 8, 2021 • edited

cvuosalo commented Apr 8, 2021

silviodonato commented Apr 20, 2021

bsunanda commented Apr 20, 2021

civanch commented Apr 20, 2021

civanch commented Apr 20, 2021

ianna commented Apr 21, 2021

bsunanda commented Apr 21, 2021

civanch commented Apr 21, 2021

cmsbuild commented Apr 21, 2021

silviodonato commented Apr 21, 2021

civanch commented Apr 8, 2021 •

edited