-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DD4hep] Fix last of preshower overlaps #33840
Conversation
urgent |
@qliphy We would like to get this PR into 12_0_0_pre2. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33840/22862
|
A new Pull Request was created by @cvuosalo (Carl Vuosalo) 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. cms-bot commands are listed here |
@cmsbuild please test workflow 11634.911 |
-1 Failed Tests: ClangBuild Clang BuildI found compilation warning while trying to compile with clang. Command used:
See details on the summary page. |
@cvuosalo thanks. You mentioned "A backport is probably not necessary.", but we may need 11_3_X for high statistics production, so why not back-porting to 11_3_X? |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33840/22863
|
Pull request #33840 was updated. @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please check and sign again. |
@cmsbuild please test workflow 11634.911 |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2ccfaf/15307/summary.html Comparison SummarySummary:
|
+1 |
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) |
+1 |
The ECal preshower showed overlaps of about 1 mm between volumes
SFAbsNPbLOutAl
andSFAbsNPbLinPb
. These volumes are created by the preshower algorithm. The algorithm was also creating a few unneeded solids and volumes.It turns out that the
SFAbsNPbLOutAl
volume was being built incorrectly. With old DD, this volume is heavily nested, while with DD4hep it was missing some components. This PR changes the code so that this volume gets the same structure in DD4hep as in the old DD. To add to the confusion, there were two different components both namedSFAbsNPbLOutAl
, and this PR changes the name of the volume toSFAbsNPbLOutAlVol
to distinguish it from the like-named Tube.The fix in this PR is complicated because the old DD code was using the capability of the old DD to create objects before they are fully defined. Old DD places the
SFAbsNPbLOutAl
volume before fully defining it and filling it with solids. That is not possible with DD4hep. To get around this problem, this PR adds a new loop where in the first iteration the necessary components are built and in the second iteration they are placed. The resulting code is even more complex than the original. It would be good to rewrite all this code to more clearly adapt it to the DD4hep paradigm where all components have to be defined before use, but we are trying to merge this PR for 12_0_0_pre2, and there is not time for such an extensive rewrite.This PR removes uneeded solids and volumes and re-names a few components in cases where two different components had identical names.
PR validation:
The overlap checker,
SimG4Core/PrintGeomInfo/test/python/g4OverlapCheck_dd4hep_cfg.py
, was run before and after this change to show that the overlaps were fixed. Also, all solids were compared between old DD and DD4hep and found to be the same for the preshower (ignoring the new assemblies).A backport is probably not necessary.