[DD4hep] Name clarification: Restore namespace to several names to enable reading sim geo DB payload #34109
Conversation
| edm::LogVerbatim("TrackerGeom").log([&](auto& log) { | ||
| std::string segname(solidOutput + "_seg_" + std::to_string(segment)); |
There was a problem hiding this comment.
In the code before this PR, the segname variable is defined and used in a debug message to describe the solids being created, but then it is NOT actually used. That would appear to me to be an oversight. Comparing to the old DD code, it seems clear this name is intended to be used as the name for the Cut Tubes. See below.
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34109/23298
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
| @@ -159,7 +160,7 @@ static long algorithm(dd4hep::Detector& /* description */, cms::DDParsingContext | |||
| n_y_t /= norm; | |||
| n_z_t /= norm; | |||
|
|
|||
| auto seg = dd4hep::CutTube(r_min, r_max, dz, phi1, phi2, n_x_l, n_y_l, n_z_l, n_x_t, n_y_t, n_z_t); | |||
| auto seg = dd4hep::CutTube(segname, r_min, r_max, dz, phi1, phi2, n_x_l, n_y_l, n_z_l, n_x_t, n_y_t, n_z_t); | |||
There was a problem hiding this comment.
In the old DD version of this file, the corresponding line is:
DDSolid seg = DDSolidFactory::cuttubs(segname, dz, r_min, r_max, phi1, dphi, n_x_l, n_y_l, n_z_l, n_x_t, n_y_t, n_z_t);
It seems clear segname is intended to be used for the name of this Cut Tube. Making this change creates an exact match between old DD and DD4hep for these solids.
Geometry/TrackerCommonData/plugins/dd4hep/DDCutTubsFromPoints.cc
Outdated
Show resolved
Hide resolved
| solid = dd4hep::UnionSolid(solidOutput + "_uni_" + std::to_string(i + 1), | ||
| solid, | ||
| segments[i], | ||
| dd4hep::Position(0., 0., offsets[i] - shift)); |
There was a problem hiding this comment.
The corresponding line in the old DD version is:
DDName unionname(solidOutput.name() + "_uni_" + std::to_string(i + 1), solidOutput.ns());
Adding the _ allows the names to match exactly between old DD and DD4hep. (Sorry, the automatic code formatting changed the format of the line, but the only change is the addition of _ after uni.)
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34109/23299
|
|
A new Pull Request was created by @cvuosalo (Carl Vuosalo) for master. It involves the following packages: Geometry/TrackerCommonData @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 |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-387473/15929/summary.html Comparison SummarySummary:
|
|
+1 @cvuosalo - thanks for detailed explanation! |
|
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 |
In the old DD, all names of geometry components (other than SpecPars) included the namespace. That feature promoted uniqueness of names. With DD4hep, most names still contain the namespace, but a small minority do not. Nonetheless, most of these names still are unique. Without this PR, there are a handful of names which show duplication. Internally, this duplication is not a problem because ROOT implicitly appends a hex address to all names to make them unique, but the human-readable portion of the name might show duplication.
A PR that will be submitted very soon will propose a tool for creating the simulation geometry DB payload with DD4hep. This payload will not be readable if it contains duplicate names for different components (e.g., two different solids with the same name). To prevent this duplication, the namespace needs to be restored to a handful of component names, making them match exactly their names in the old DD and preventing duplication.
In addition, a small typo was discovered in
Geometry/TrackerCommonData/plugins/dd4hep/DDCutTubsFromPoints.cc. Fixing the typo restores the code to match the old DD and also restores the names of the solids affected to exactly match the old DD. See the doc below and my note on the file page for more details.This Google doc shows the names in detail and how they change with this PR:
https://docs.google.com/document/d/1kNMv9FWSRcfM_FUjzrFzNH5igZhW62ZwjYNIj98zaSM/edit?usp=sharing
Note: This PR clarifies several names but does not actually change the geometry.
PR validation:
The solids affected by this PR were checked to see that their parameters were exactly the same before and after the PR, and also that, after this PR, the DD4hep solids exactly match those in the old DD.
Code searches were performed to see if the names affected by this PR are referred to anywhere else in the code. They are not. Also, the clarifications in this PR should be safe because the names are being restored to match the old DD.
if this PR is a backport please specify the original PR and why you need to backport that PR:
A backport is probably not necessary since this PR only clarifies several names.