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.

Sign up for GitHub

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

Jump to bottom

[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
Merged

[DD4hep] Minor size correction of 0.1 to 7 degrees in phi for tube shapes that are non-sensitive TIB pillars and ribs #34088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
60c9560
Bug fixes from geometry validation
cvuosalo Jun 10, 2021
2e36ed0
Revert "Bug fixes from geometry validation"
cvuosalo Jun 10, 2021
821e3d4
Bug fixes from geometry validation
cvuosalo Jun 10, 2021
d2b14d5
Removed unnecessary rotation naming
cvuosalo Jun 11, 2021
a5b80a1
Remove files with only name changes
cvuosalo Jun 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Geometry/EcalCommonData/plugins/dd4hep/DDEcalBarrelNewAlgo.cc
View file
Open in desktop
Expand Up @@ -1559,7 +1559,7 @@ static long algorithm(dd4hep::Detector& /* description */, cms::DDParsingContext
);

const string cryDDName(cry.name + sType);
Solid crySolid = mytrap(cry.name, trapCry);
Solid crySolid = mytrap(cryDDName, trapCry);
Volume cryLog = Volume(cryDDName, crySolid, ns.material(cry.mat));

//++++++++++++++++++++++++++++++++++ APD ++++++++++++++++++++++++++++++++++
Expand Down
4 changes: 2 additions & 2 deletions Geometry/TrackerCommonData/plugins/dd4hep/DDTIBLayerAlgo.cc
View file
Open in desktop
Expand Up @@ -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.

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;
Expand Down Expand Up @@ -490,7 +490,7 @@ static long algorithm(Detector& /* description */, cms::DDParsingContext& contex
break;
}

solid = ns.addSolidNS(name, Tube(pillarRin, pillarRout, pillarDz, -pillarDPhi, 2. * pillarDPhi));
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;
Expand Down