[DD4hep] Minor size correction of 0.1 to 7 degrees in phi for tube shapes that are non-sensitive TIB pillars and ribs #34088
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ | |
| <!-- support ring solids - refer to outer disk comments --> | ||
| <Algorithm name="track:DDCutTubsFromPoints"> | ||
| <rParent name="pixfwdDisks:PixelForwardDiskZminus"/> | ||
| <String name="SolidName" value="PixelForwardInnerDiskOuterRingZminus"/> | ||
|
There was a problem hiding this comment. @cvuosalo - identical names are intentional. They are distinguished by a copy number. There was a problem hiding this comment. 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: With DD4hep, without my fix, these solids get the same names because the namespace is missing (they are CutTubs with DD4hep): Note the two solids have the same names but slightly different parameters. With my fix, the definitions become: 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. |
||
| <String name="Material" value="pixfwdMaterials:C_C_InnerOuterRing"/> | ||
| <Numeric name="zPos" value="[OuterRingZ]"/> | ||
| <Numeric name="rMin" value="[pixfwdSupportRingParameters:InnerDiskOuterRingRMin]"/> | ||
|
|
@@ -22,7 +22,7 @@ | |
| </Algorithm> | ||
| <Algorithm name="track:DDCutTubsFromPoints"> | ||
| <rParent name="pixfwdDisks:PixelForwardDiskZminus"/> | ||
| <String name="SolidName" value="PixelForwardInnerDiskCFOuterRingZminus"/> | ||
| <String name="Material" value="pixfwdMaterials:FPix_CFSkin_InnerOuterRing"/> | ||
| <Numeric name="zPos" value="[OuterRingZ]"/> | ||
| <Numeric name="rMin" value="[pixfwdSupportRingParameters:InnerDiskOuterRingCFRMin]"/> | ||
|
|
@@ -33,7 +33,7 @@ | |
| </Algorithm> | ||
| <Algorithm name="track:DDCutTubsFromPoints"> | ||
| <rParent name="pixfwdDisks:PixelForwardDiskZminus"/> | ||
| <String name="SolidName" value="PixelForwardInnerDiskInnerRingZminus"/> | ||
| <String name="Material" value="pixfwdMaterials:C_C_InnerInnerRing"/> | ||
| <Numeric name="zPos" value="[InnerRingZ]"/> | ||
| <Numeric name="rMin" value="[pixfwdSupportRingParameters:InnerDiskInnerRingRMin]"/> | ||
|
|
@@ -44,7 +44,7 @@ | |
| </Algorithm> | ||
| <Algorithm name="track:DDCutTubsFromPoints"> | ||
| <rParent name="pixfwdDisks:PixelForwardDiskZminus"/> | ||
| <String name="SolidName" value="PixelForwardInnerDiskCFInnerRingZminus"/> | ||
| <String name="Material" value="pixfwdMaterials:FPix_CFSkin_InnerInnerRing"/> | ||
| <Numeric name="zPos" value="[InnerRingZ]"/> | ||
| <Numeric name="rMin" value="[pixfwdSupportRingParameters:InnerDiskInnerRingCFRMin]"/> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"; | ||
|
There was a problem hiding this comment. @cvuosalo - why? It’s a There was a problem hiding this comment. This problem relates to namespaces. In the old DD, all component names are qualified by namespaces, so these definitions result: but with DD4hep, there is no namespace for these names, so WITHOUT this fix these solid definitions result : 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: There was a problem hiding this comment. Is it not accidental that one is a Box and the other a Trapezoid? There was a problem hiding this comment. 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. |
||
| ns.addSolidNS(name, solid); | ||
| edm::LogVerbatim("TECGeom") << "Solid:\t" << name << " " << solid.name() << " Box made of " << pitchMat | ||
| << " of dimensions " << dx << ", " << dy << ", " << dz; | ||
|
|
@@ -419,6 +420,7 @@ static long algorithm(Detector& /* description */, cms::DDParsingContext& ctxt, | |
| bl2 = 0.5 * pitchHeight - 0.5 * dz * sin(detTilt); | ||
| thet = atan((bl1 - bl2) / (2. * dz)); | ||
| solid = Trap(dz, thet, 0, h1, bl1, bl1, 0, h1, bl2, bl2, 0); | ||
| name += "Trap"; | ||
| ns.addSolidNS(name, solid); | ||
| edm::LogVerbatim("TECGeom") << "Solid:\t" << name << " " << solid.name() << " Trap made of " << pitchMat | ||
| << " of dimensions " << dz << ", " << convertRadToDeg(thet) << ", 0, " << h1 << ", " | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, 0.5 * width)); | ||
|
There was a problem hiding this comment. @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. There was a problem hiding this comment. 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. There was a problem hiding this comment. @cvuosalo, I suspect, that old variant of the Tube was correct. The signature of the Tube: There was a problem hiding this comment. We can compare old DD with DD4hep for these tubes. Here is old DD: Here is DD4hep WITHOUT the fix: The problem comes from the change in the Tube constructor between old DD and DD4hep. In old DD, the last parameter is and the corresponding line of code is: but with DD4hep the last parameter is so the last the last parameter with DD4hep needs to be 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. There was a problem hiding this comment. @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. There was a problem hiding this comment. @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, There was a problem hiding this comment. @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. |
||
| LogDebug("TIBGeom") << solid.name() << " Tubs made of " << ribMat << " from " << -0.5 * convertRadToDeg(width) | ||
| << " to " << 0.5 * convertRadToDeg(width) << " with Rin " << rin + 0.5 * dd4hep::mm << " Rout " | ||
| << rout - 0.5 * dd4hep::mm << " ZHalf " << dz; | ||
|
|
@@ -490,7 +490,7 @@ static long algorithm(Detector& /* description */, cms::DDParsingContext& contex | |
| break; | ||
| } | ||
|
|
||
| solid = ns.addSolidNS(name, Tube(pillarRin, pillarRout, pillarDz, -pillarDPhi, pillarDPhi)); | ||
| Volume Pillar = ns.addVolumeNS(Volume(name, solid, ns.material(pillarMaterial))); | ||
| LogDebug("TIBGeom") << solid.name() << " Tubs made of " << pillarMaterial << " from " << -pillarDPhi << " to " | ||
| << pillarDPhi << " with Rin " << pillarRin << " Rout " << pillarRout << " ZHalf " << pillarDz; | ||
|
|
||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.