MTD & Tracker DD4hep Units #32488
MTD & Tracker DD4hep Units #32488
Conversation
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32488/20395
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32488/20396
|
|
A new Pull Request was created by @vargasa (Andres Vargas) for master. It involves the following packages: Geometry/MTDNumberingBuilder @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| GeometricTimingDet::GeometricTimingDet(cms::DDFilteredView* fv, GeometricTimingEnumType type) | ||
| : trans_(fv->translation()), | ||
| : trans_(fv->translation() / dd4hep::mm), |
There was a problem hiding this comment.
Has this line been wrong up to now? Before your change, the translations were in cm. Was that a bug?
There was a problem hiding this comment.
No, before it was redefined a right after the initialization list
There was a problem hiding this comment.
I see it now. I missed it before. It looks correct.
There was a problem hiding this comment.
@vargasa thanks for taking care of this, according to the unit test (and my check also on the GeometryBuilder) everything looks unchanged and consistent
| @@ -5,7 +5,7 @@ | |||
| #include "CondFormats/GeometryObjects/interface/PGeometricTimingDet.h" | |||
|
|
|||
| #include "CLHEP/Units/GlobalSystemOfUnits.h" | |||
There was a problem hiding this comment.
Please delete this line 7. It should have been done long ago. Also requiring change is line 232:
Position pos(float(trans_.x() / cm), float(trans_.y() / cm), float(trans_.z() / cm));
It will become:
Position pos(float(trans_.x() / dd4hep::cm), float(trans_.y() / dd4hep::cm), float(trans_.z() / dd4hep::cm));
There was a problem hiding this comment.
Just to confirm: As of now cm is 10. and dd4hep::cm is 1. Therefore, applying the suggested change we would expect these quantities to increase by a factor of 10. and not remain the same. Is that correct?
There was a problem hiding this comment.
Sorry, I goofed. In CLHEP / cm means "convert from mm to cm", so the line should become:
Position pos(static_cast<float>(convertMmToCm(trans_.x()), static_cast<float>(convertMmToCm((trans_.y())), static_cast<float>(convertMmToCm((trans_.z())));
I don't think this line is related to DD4hep. It's confusing. The translation is initially converted to mm and then converted to cm for this position.
There was a problem hiding this comment.
Ok. I think is done now 👍
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32488/20403
|
|
Pull request #32488 was updated. @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please check and sign again. |
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-392a04/11651/summary.html Comparison SummarySummary:
|
|
+1 |
|
Not a problem Fabio :)
… |
|
+upgrade |
cmsbuild
added
fully-signed
upgrade-approved
and removed
pending-signatures
upgrade-pending
labels
|
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 |
PR description:
DD4hep will transition to use geant4 units
PR validation:
convertCmToMmcurrently multiplies its input by 10. and the current value ofdd4hep::mmis0.1thereforex / dd4hep::mm == convertCmToMm(x).FYI: @cvuosalo