Skip to content
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] Minor size correction of 0.1 to 7 degrees in phi for tube shapes that are non-sensitive TIB pillars and ribs #34088

Merged
merged 5 commits into from Jun 14, 2021

Conversation

cvuosalo
Copy link
Contributor

@cvuosalo cvuosalo commented Jun 10, 2021

In the DD4hep TIB code to create support structures, tube shapes are used for the non-sensitive aluminum and carbon fiber pillars and ribs. These tube shapes had a slight error in their phi extent, being from 0.1 degrees to 7 degrees too large.
While developing the code to create the simulation geometry XML DB payload with DD4hep, I validated my code by checking each geometry component between old DD and DD4hep. These checks revealed this slight discrepancy in these non-sensitive volumes.

In DDTIBLayerAlgo.cc two solids had slightly incorrect sizes, which were corrected. The solid definitions are in loops, so actually about 30 solids are affected, being the pillars and ribs.

Additionally, and unrelated, the validation also showed there was a misnamed solid in DDEcalBarrelNewAlgo.cc , and it was fixed.

PR validation:

The full XML geometry descriptions for old DD and DD4hep were compared to check these fixes corrected the discrepancies between them. Also, the 11634.911 DD4hep workflow was run successfully.

These fixes could be backported to 11_3, but I'm not sure if that is really necessary.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34088/23254

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @cvuosalo (Carl Vuosalo) for master.

It involves the following packages:

Geometry/EcalCommonData
Geometry/TrackerCommonData

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

@cvuosalo
Copy link
Contributor Author

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e166cf/15892/summary.html
COMMIT: 821e3d4
CMSSW: CMSSW_12_0_X_2021-06-10-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34088/15892/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3091 differences found in the comparisons
  • DQMHistoTests: Total files compared: 38
  • DQMHistoTests: Total histograms compared: 2862520
  • DQMHistoTests: Total failures: 8820
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2853677
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 37 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 160 log files, 37 edm output root files, 38 DQM output files
  • TriggerResults: found differences in 1 / 37 workflows

Copy link
Contributor

@ianna ianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo - the changes are not bug fixes, but new features. IMHO, it is too late introducing them because they change geometry that has been validated. Besides, I do not see how extra name manipulation would help in creating geometry payloads. I have looked at your branch and strongly object to the approach where you introduce DD dependency on Geant4 that will propagate to Reco.

I’d suggest that you present your design at a Core meeting for discussion. As we have discussed, there are alternatives.

FYI @makortel

@@ -11,7 +11,7 @@
<!-- support ring solids - refer to outer disk comments -->
<Algorithm name="track:DDCutTubsFromPoints">
<rParent name="pixfwdDisks:PixelForwardDiskZminus"/>
<String name="SolidName" value="PixelForwardInnerDiskOuterRing"/>
<String name="SolidName" value="PixelForwardInnerDiskOuterRingZminus"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo - identical names are intentional. They are distinguished by a copy number.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue relates to namespaces. In old DD, all components have names qualified by namespaces, so these duplicated names were not a problem since they were in different namespaces. With DD4hep, namespaces are used most, but not all, of the time. These pixel names happen to lack namespaces.

With old DD, these solids are Unions with unique names:

<UnionSolid name="pixfwdInnerDiskZplus:PixelForwardInnerDiskOuterRing">
<UnionSolid name="pixfwdInnerDiskZminus:PixelForwardInnerDiskOuterRing">

With DD4hep, without my fix, these solids get the same names because the namespace is missing (they are CutTubs with DD4hep):

<CutTubs name="PixelForwardInnerDiskOuterRing" dz="10.2500*mm" rMin="114.8500*mm" rMax="117.3500*mm" startPhi="265.0503*deg" deltaPhi="7.9068*deg" lx="-0.0000" ly="-0.0000" lz="-1.0000" tx="0.3970" ty="-0.0069" tz="0.9178"/>
<CutTubs name="PixelForwardInnerDiskOuterRing" dz="10.2500*mm" rMin="114.8500*mm" rMax="117.3500*mm" startPhi="265.0503*deg" deltaPhi="7.9068*deg" lx="0.3970" ly="-0.0069" lz="-0.9178" tx="0.0000" ty="0.0000" tz="1.0000"/>

Note the two solids have the same names but slightly different parameters. With my fix, the definitions become:

<CutTubs name="PixelForwardInnerDiskOuterRingZplus" dz="10.2500*mm" rMin="114.8500*mm" rMax="117.3500*mm" startPhi="265.0503*deg" deltaPhi="7.9068*deg" lx="-0.0000" ly="-0.0000" lz="-1.0000" tx="0.3970" ty="-0.0069" tz="0.9178"/>
<CutTubs name="PixelForwardInnerDiskOuterRingZminus" dz="10.2500*mm" rMin="114.8500*mm" rMax="117.3500*mm" startPhi="265.0503*deg" deltaPhi="7.9068*deg" lx="0.3970" ly="-0.0069" lz="-0.9178" tx="0.0000" ty="0.0000" tz="1.0000"/>

These names are the basis for several solids and logical volumes, so without the fix, there are a set of different components that share the same names.
I suspect that this problem may be related to Gabrielle's PR #32843, where she talks about removing the namespace from Tracker names, but I don't see a direct cause in that PR.
I think it is important to avoid duplicate names for different solids.

xilyLog.placeVolume(ilyPTMLog,
ptmCopy,
Transform3D(myrot(ns,
std::string(ilyPTMLog.name()) + "_rot" + std::to_string(ptmCopy),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo - this is an unnecessary performance bomb. A new object is returned after each +. That means that a new buffer is used each time. You are doing a ton of extra + operations - it is not efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove these rotation names since they are not strictly necessary.

ilyFanOutLog,
fanOutCopy,
Transform3D(myrot(ns,
std::string(ilyFanOutLog.name()) + "_rot" + std::to_string(fanOutCopy),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

femCopy,
Transform3D(myrot(ns,
std::string(ilyFEMLog.name()) + "_rot" + std::to_string(femCopy),
Rotation3D(RotationZ(phi))),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Position(xx, yy, zz)));
Transform3D(
myrot(ns,
std::string(ilyPipeLog[type].name()) + "_rot" + std::to_string(ly),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@@ -409,6 +409,7 @@ static long algorithm(Detector& /* description */, cms::DDParsingContext& ctxt,
dy = 0.5 * pitchThick;
dz = 0.5 * pitchHeight;
solid = Box(dx, dy, dz);
name += "Box";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo - why? It’s a Box and you can always check it by type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This problem relates to namespaces. In the old DD, all component names are qualified by namespaces, so these definitions result:

<Box name="tecmodule0r:TECModule0PA" dx="43.9000*mm" dy="0.3325*mm" dz="6.5000*mm"/>
<Trapezoid name="tecmodule0s:TECModule0PA" dz="43.9000*mm" theta="2.8576*deg" phi="0.0000*deg" h1="0.3325*mm" bl1="9.3663*mm" tl1="9.3663*mm" alp1="0.0000*deg" h2="0.3325*mm" bl2="4.9837*mm" tl2="4.9837*mm" alp2="0.0000*deg"/>

but with DD4hep, there is no namespace for these names, so WITHOUT this fix these solid definitions result :

<Box name="TECModule0PA" dx="43.9000*mm" dy="0.3325*mm" dz="6.5000*mm"/>
<Trapezoid name="TECModule0PA" dz="43.9000*mm" theta="2.8576*deg" phi="0.0000*deg" h1="0.3325*mm" bl1="9.3663*mm" tl1="9.3663*mm" alp1="0.0000*deg" h2="0.3325*mm" bl2="4.9837*mm" tl2="4.9837*mm" alp2="0.0000*deg"/>

Here are two different solids with the same name. I don't see how that could work correctly. Running the code with the appended names gives:

<Box name="TECModule0PABox" dx="43.9000*mm" dy="0.3325*mm" dz="6.5000*mm"/>
<Trapezoid name="TECModule0PATrap" dz="43.9000*mm" theta="2.8576*deg" phi="0.0000*deg" h1="0.3325*mm" bl1="9.3663*mm" tl1="9.3663*mm" alp1="0.0000*deg" h2="0.3325*mm" bl2="4.9837*mm" tl2="4.9837*mm" alp2="0.0000*deg"/>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not accidental that one is a Box and the other a Trapezoid?
BTW: what the box is used for in reconstruction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why these two solids were defined as a Box and a Trapezoid. I can only say that this feature is in the old DD and has been there for a long time.

@@ -252,7 +252,7 @@ static long algorithm(Detector& /* description */, cms::DDParsingContext& contex
double dz = 0.5 * layerL - 2. * fillerDz;
double _rmi = std::min(rin + 0.5 * dd4hep::mm, rout - 0.5 * dd4hep::mm);
double _rma = std::max(rin + 0.5 * dd4hep::mm, rout - 0.5 * dd4hep::mm);
solid = ns.addSolidNS(name, Tube(_rmi, _rma, dz, -0.5 * width, width));
solid = ns.addSolidNS(name, Tube(_rmi, _rma, dz, -0.5 * width, 0.5 * width));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo - it is strange that this did not come out in validation. Could you, please, share a snapshot of the geometry fragment before and after? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will reproduce the bug and post the before and after. It is a very small difference, so that is maybe why it wasn't detected earlier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo, I suspect, that old variant of the Tube was correct. The signature of the Tube:
rmin, rmax, dz, phi1, phi2. Problem is in badly chosen name "width", it should be "phiWidth". It is correct to have "-0.5*phiwidth, phiwidth".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can compare old DD with DD4hep for these tubes. Here is old DD:

<Tubs name="tiblayer0:TIBLayer0Rib0" rMin="254.0000*mm" rMax="258.0000*mm" dz="696.0000*mm" startPhi="-3.9167*deg" deltaPhi="7.8334*deg"/>

Here is DD4hep WITHOUT the fix:

<Tubs name="tiblayer0:TIBLayer0Rib0" rMin="254.0000*mm" rMax="258.0000*mm" dz="696.0000*mm" startPhi="-3.9167*deg" deltaPhi="11.7501*deg"/>

The problem comes from the change in the Tube constructor between old DD and DD4hep. In old DD, the last parameter is deltaPhi:

tubs(const DDName& name, double zhalf, double rIn, double rOut, double startPhi, double deltaPhi)

and the corresponding line of code is:

tubs(DDName(name, idNameSpace), dz, rin + 0.5 * CLHEP::mm, rout -0.5* CLHEP::mm, -0.5* width, width);

but with DD4hep the last parameter is endPhi:

Tube (double rmin, double rmax, double dz, double startPhi, double endPhi)

so the last the last parameter with DD4hep needs to be endPhi = startPhi + deltaPhi. As long as startPhi is 0, the old and new parameters are the same, but in this case, startPhi is not 0.
With the fix, DD4hep gives the correct values:

<Tubs name="tiblayer0:TIBLayer0Rib0" rMin="254.0000*mm" rMax="258.0000*mm" dz="696.0000*mm" startPhi="-3.9167*deg" deltaPhi="7.8334*deg"/>

It is unfortunate that this bug was not found until now. I think it affects about 20 solids. I would think we would want to fix it now, rather than leaving it for all of Run 3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo , thank you for the explanation but in my view what DD4Hep did is a scandal - they change signature in a place which is very difficult to debug. We may have thousands similar places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@civanch I did a systematic comparison of all solids between old DD and DD4hep. That's how I discovered the minor discrepancies discussed in this PR. I don't think there are any more. Probably, in most cases for tubes, startPhi is 0, so there is no chance of problem, and in the other cases, endPhi is set correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo - it would be good to see a snapshot before and after the fix. I'd assume there would be an overlap detected with a double delta phi. Also, it is not obvious what DD4hep is doing when constructing the shape. Sometimes the arguments may differ.

@ianna
Copy link
Contributor

ianna commented Jun 11, 2021

-1

@cvuosalo
Copy link
Contributor Author

I will address the other comments soon. I need to run with and without the fixes to show the effects.

@makortel
Copy link
Contributor

I’d suggest that you present your design at a Core meeting for discussion. As we have discussed, there are alternatives.

@ianna Given the later discussion in this PR do you think a discussion in Core meeting would be useful? We might be able to squeeze a short discussion for next Tuesday, and the subsequent slot would be in July 6.

@ianna
Copy link
Contributor

ianna commented Jun 11, 2021

I’d suggest that you present your design at a Core meeting for discussion. As we have discussed, there are alternatives.

@ianna Given the later discussion in this PR do you think a discussion in Core meeting would be useful? We might be able to squeeze a short discussion for next Tuesday, and the subsequent slot would be in July 6.

@makortel - this PR does not include the code @cvuosalo is writing to produce SIM DB payloads. IMHO, his design (especially its dependency on Geant4) should be discussed. The goal of the DDD migration to DD4hep was not to write more code, but rather reduce the CMSSW codebase.

@civanch
Copy link
Contributor

civanch commented Jun 11, 2021

@cvuosalo , @ianna , let use discuss this Monday at Sim Chat. If it would be possible, please, upload some slides or other text.

@cvuosalo
Copy link
Contributor Author

@ianna I have removed the DD dependence on Geant4. Note that the branch you looked at is a work in progress, nothing final, and it has some outdated file versions. I am making a separate, cleaned-up branch for the PR.

@cvuosalo
Copy link
Contributor Author

I tried to address the issues raised. I think the problems mentioned in this PR are real bugs that should be fixed. The only change in geometry is the tube deltaPhi fix discussed above. The name changes shouldn't change geometry but will prevent name clashes and confusion.

@civanch
Copy link
Contributor

civanch commented Jun 12, 2021

I understand, that these fixes are correct but I do not understand why this problem do not affect results? Are all these cuted tubes absorbers?

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e166cf/15918/summary.html
COMMIT: a5b80a1
CMSSW: CMSSW_12_0_X_2021-06-11-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34088/15918/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3087 differences found in the comparisons
  • DQMHistoTests: Total files compared: 38
  • DQMHistoTests: Total histograms compared: 2862520
  • DQMHistoTests: Total failures: 8815
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2853683
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 37 files compared)
  • Checked 160 log files, 37 edm output root files, 38 DQM output files
  • TriggerResults: found differences in 1 / 37 workflows

@cvuosalo cvuosalo changed the title [DD4hep] Fix bugs revealed by geometry validation during DB payload development [DD4hep] Minor size correction of 0.1 to 7 degrees in phi for tube shapes that are non-sensitive TIB pillars and ribs Jun 12, 2021
@cmsbuild cmsbuild removed the bug-fix label Jun 12, 2021
@cvuosalo
Copy link
Contributor Author

The comparison differences in the DD4hep workflow are merely statistical fluctuations. This PR changes the geometry very slightly so it is no longer bit-wise identical, and that triggers a change in the random seeds, so the results are no longer bit-wise identical.

@cvuosalo
Copy link
Contributor Author

We can consider whether the correction in this PR is too minor to bother with.

Copy link
Contributor

@ianna ianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo - please, provide a snapshot from visualisation (cmsShow or G4). Thanks!

@@ -252,7 +252,7 @@ static long algorithm(Detector& /* description */, cms::DDParsingContext& contex
double dz = 0.5 * layerL - 2. * fillerDz;
double _rmi = std::min(rin + 0.5 * dd4hep::mm, rout - 0.5 * dd4hep::mm);
double _rma = std::max(rin + 0.5 * dd4hep::mm, rout - 0.5 * dd4hep::mm);
solid = ns.addSolidNS(name, Tube(_rmi, _rma, dz, -0.5 * width, width));
solid = ns.addSolidNS(name, Tube(_rmi, _rma, dz, -0.5 * width, 0.5 * width));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo - it would be good to see a snapshot before and after the fix. I'd assume there would be an overlap detected with a double delta phi. Also, it is not obvious what DD4hep is doing when constructing the shape. Sometimes the arguments may differ.

@civanch
Copy link
Contributor

civanch commented Jun 12, 2021

@ianna , @cvuosalo , it is possible to check overlaps using both visualisation and PrintGeomInfo.
According to Carl explanations, I think that now DD4Hep will agree with DDD for these volumes. Let us check by both methods. It is also possible to compare gdml files before and after this PR.

If DD4Hep is taken these Tube definition and making different interpretation of it, then we need check all Tubes and also other volumes.

@civanch
Copy link
Contributor

civanch commented Jun 12, 2021

By the way, in PrintGeomInfo it is possible to dump any volume by name. For these particular case, it is the fastest test without visualisation.

@civanch
Copy link
Contributor

civanch commented Jun 12, 2021

I just compared DD/DD4Hep for "Rib" volumes - deltaphi are different, startphi are the same if we take into account that in DDD startphi are usually negative, in DD4Hep - a bit less than 360 degrees, so effectively they are the same. I did not yet tried this PR.

@civanch
Copy link
Contributor

civanch commented Jun 13, 2021

This PR provide the same "Rib" volumes inside TIB detector as DDD. Previous DD4Hep description was larger but the bias does not make extra overlap.

There is no extra overlaps due to this PR. So, I confirm, that this PR is correct.

@civanch
Copy link
Contributor

civanch commented Jun 13, 2021

+1

@cmsbuild
Copy link
Contributor

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)

@qliphy
Copy link
Contributor

qliphy commented Jun 14, 2021

@ianna @civanch Just to double check, this PR is fine to go, right? Thanks!

@civanch
Copy link
Contributor

civanch commented Jun 14, 2021

@qliphy , yes, this fix is correct.

@qliphy
Copy link
Contributor

qliphy commented Jun 14, 2021

+1

@cmsbuild cmsbuild merged commit 7b66901 into cms-sw:master Jun 14, 2021
@makortel
Copy link
Contributor

I’d suggest that you present your design at a Core meeting for discussion. As we have discussed, there are alternatives.

@ianna Given the later discussion in this PR do you think a discussion in Core meeting would be useful? We might be able to squeeze a short discussion for next Tuesday, and the subsequent slot would be in July 6.

@makortel - this PR does not include the code @cvuosalo is writing to produce SIM DB payloads. IMHO, his design (especially its dependency on Geant4) should be discussed. The goal of the DDD migration to DD4hep was not to write more code, but rather reduce the CMSSW codebase.

@ianna @cvuosalo In light of #34109 and #34111 do you think further discussion in core meeting would be useful?

@cvuosalo
Copy link
Contributor Author

@makortel I think some of the concerns may have been addressed, and #34111 is available as a forum for further discussion. Maybe it is not necessary to take up a time slot at the core meeting, though I will attend in case the topic is discussed at the meeting.

@makortel
Copy link
Contributor

Thanks @cvuosalo. The practical challenge is that the agenda tomorrow is already rather full, and the next meeting slot is practically on July 6th.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants