Make units dd4hep-dependent in PPS #32538
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ | |
| #include "DataFormats/CTPPSDetId/interface/TotemTimingDetId.h" | ||
| #include "DataFormats/CTPPSDetId/interface/CTPPSPixelDetId.h" | ||
| #include "DataFormats/CTPPSDetId/interface/CTPPSDiamondDetId.h" | ||
| #include <DD4hep/DD4hepUnits.h> | ||
| #include "FWCore/MessageLogger/interface/MessageLogger.h" | ||
|
|
||
| /* | ||
|
|
@@ -47,14 +47,14 @@ DetGeomDesc::DetGeomDesc(const cms::DDFilteredView& fv, const bool isRun2) | |
| : m_name(computeNameWithNoNamespace(fv.name())), | ||
| m_copy(fv.copyNum()), | ||
| m_isDD4hep(true), | ||
| m_trans(fv.translation() / dd4hep::mm), // converted from cm (DD4hep) to mm | ||
| m_rot(fv.rotation()), | ||
| m_params(computeParameters(fv)), // default unit from DD4hep (cm) | ||
| m_isABox(dd4hep::isA<dd4hep::Box>(fv.solid())), | ||
| m_diamondBoxParams(computeDiamondDimensions(m_isABox, m_isDD4hep, m_params)), // converted from cm (DD4hep) to mm | ||
| m_sensorType(computeSensorType(fv.name())), | ||
| m_geographicalID(computeDetIDFromDD4hep(m_name, fv.copyNos(), fv.copyNum(), isRun2)), | ||
| m_z(fv.translation().z() / dd4hep::mm) // converted from cm (DD4hep) to mm | ||
| {} | ||
|
|
||
| DetGeomDesc::DetGeomDesc(const DetGeomDesc& ref, CopyMode cm) { | ||
|
|
@@ -152,9 +152,7 @@ DiamondDimensions DetGeomDesc::computeDiamondDimensions(const bool isABox, | |
| boxShapeParameters = {params.at(0), params.at(1), params.at(2)}; | ||
| } else { | ||
| // convert cm (DD4hep) to mm (legacy expected by PPS reco software) | ||
|
||
| boxShapeParameters = {params.at(0) / dd4hep::mm, params.at(1) / dd4hep::mm, params.at(2) / dd4hep::mm}; | ||
| } | ||
| } | ||
| return boxShapeParameters; | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 would convert
m_paramsto mm right here (actually incomputeParameters). Then all members ofDetGeomDescwould be in Geant4 units.computeDiamondDimensionscould then be simplified because the input units would be always mm.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.
When old DD is removed, that case will disappear anyway.
In the meantime, I would not make old DD case depend on the DD4hep units, while the DD4hep case needs the
/dd4hep::mmto have the certainty of getting mm regardless of the DD4hep internals.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.
@ghugo83 I am saying the opposite. Make the interface consistent. For DetGeomDesc members and methods, all units should be mm. Right now, the
paramsmethod (which is never used, as far as I can tell) andm_paramsstore DD4hep units, while all the other members and methods use mm.If
m_paramsin the DD4hep version were converted to mm at initialization, thencomputeDiamondDimensionswould expect mm only, and it could be simplified by the removal of theif.Uh oh!
There was an error while loading. Please reload this page.
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 Ha yes, I looked at it more properly, sorry I see what you mean now.
params()is used (from legacy) in CondTools/Geometry/plugins/PGeometricDetBuilder.cc to storePGeometricDetinfo to DB.It is true that it would be easier to just convert all shape parameters to mm in a
computeParametersfunction. ThecomputeDiamondDimensionswould just be a particular case of this, in the event that the shape is a box.It is actually the version which I had at first.
But the thing is that it was implying a specific treatment for each possible shape, because we want to convert only the parameters which are lengths, and not all the parameters of the shape.
This was making the
computeParameters()function quite heavy.And in practice, as you noticed, only the box shape parameters are used anyway in the PPS legacy code.
Hence, after discussion with the PPS team, it was decided that the shape parameters are only treated in the case of a box, with the
computeDiamondDimensionsandgetDiamondDimensionsfunctions.The params() function instead, which provides info to DB, is kept untouched, and just provides the shape parameters in their native format (units, but also orders of parameters).
Shall we want to simplify this, we could just remove
params()function - but that would imply removing (useless) info stored in DB in legacy, hence changing the CondFormats objects, hence PPS team preferred keeping thatparams()function as-is, and just having a special treatment for boxes.Does this make more sense? :)