add misalignment decoration for DD4hep detector #3891
Conversation
WalkthroughIn this pull request, a comprehensive enhancement to the DD4hep detector alignment framework has been introduced. The changes involve adding new classes and methods to support more flexible geometry transformations and misalignment handling. A new Changes
Sequence DiagramsequenceDiagram
participant AD as AlignmentDecorator
participant GC as GeometryContext
participant DE as DetectorElement
AD->>GC: Create Context
AD->>AD: Parse Geometry
AD->>GC: Set Alignment Store
DE->>GC: Request Transform
GC-->>DE: Return Contextual Transform
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Examples
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
Examples/Detectors/DD4hepDetector/src/DD4hepAlignmentDecorator.cpp (2)
23-29: Helpful it is, to handle unexpected geometry states gracefully!
If no valid tracking geometry is found, consider a warning or fallback path, you should. In robust code, foreseen such cases must be.
38-53: Account for missing nominal transforms, you must.
If the alignment store fails to find a matching identifier, fallback or logging, wise it would be. Currently, quietly it proceeds with no action.Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepAlignmentDecorator.hpp (2)
30-37: Defaulting to nominal geometry, wise approach but caution needed.
Document well, you should, that false means real misalignment used. Perhaps clarifying the naming of “nominal” or the field’s doc, would help future maintainers.
60-64: Check file paths for cross-platform reliability, you must.
Absolute vs. relative paths in misJson lead to confusion. Provide a clearer doc or fallback search path, recommended it is.Plugins/DD4hep/CMakeLists.txt (1)
12-12: Mind the library ordering, you must.
Depending on ‘ActsPluginDD4hep’ is, but if new dependencies appear, update them in target_link_libraries, you must.Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepGeometryContext.hpp (2)
20-30: Enhance documentation, young padawan must.Documentation clarity improve we should. Add these details to class documentation:
- Thread safety guarantees
- Lifetime management of stored transformations
- Example usage with JSON configuration
50-51: Thread safety considerations, address we must.Concurrent access to alignment store, protected it must be. Consider making
setAlignmentStoreand internal store thread-safe, or document thread safety requirements clearly.Plugins/DD4hep/src/DD4hepGeometryContext.cpp (1)
22-22: Modern string formatting, use you should.Replace
Form("%lld", dElement.identifier())withstd::to_string(dElement.identifier()), more modern and safer it is.Plugins/DD4hep/src/DD4hepDetectorElement.cpp (2)
30-38: Validation and performance, improve we must.Two suggestions have I:
- Add validation for nullptr detector element
- Consider caching context lookup results for performance
const Acts::Transform3& Acts::DD4hepDetectorElement::transform( const GeometryContext& gctx) const { + // Cache the context lookup result + static thread_local const Acts::DD4hepGeometryContext* lastContext = nullptr; + static thread_local const GeometryContext* lastGctx = nullptr; + + if (lastGctx != &gctx) { + lastGctx = &gctx; + lastContext = gctx.maybeGet<DD4hepGeometryContext>(); + } + - const Acts::DD4hepGeometryContext* dd4hepGtx = - gctx.maybeGet<DD4hepGeometryContext>(); - if (dd4hepGtx != nullptr && !dd4hepGtx->isNominal()) { - return dd4hepGtx->contextualTransform(*this); + if (lastContext != nullptr && !lastContext->isNominal()) { + return lastContext->contextualTransform(*this); } return TGeoDetectorElement::transform(gctx); }
11-12: Include order, maintain we should.Group related includes together, improve readability it does:
-#include "Acts/Plugins/DD4hep/DD4hepGeometryContext.hpp" - -#include <iostream> +#include "Acts/Plugins/DD4hep/DD4hepGeometryContext.hpp" +#include <iostream>Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepDetector.hpp (1)
94-95: Document the purpose of m_nominal, young padawan must.Hmmmm. Clear documentation for this flag, essential it is. Its purpose and impact on detector alignment, explain you should.
Add documentation like this, you must:
+ /// @brief Controls whether to use nominal geometry (false) or misaligned geometry (true) + /// @note When false, misalignment parameters from JSON will be applied bool m_nominal = false;Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorElement.hpp (2)
91-94: Document the transformation methods, crucial it is.Hmmmm. Understanding of these methods, future maintainers must have. Document their purpose and relationship, we should.
+ /// @brief Get the contextual transform for this detector element + /// @param gctx The geometry context containing alignment information + /// @return The aligned transform if context contains alignment, or nominal transform otherwise const Transform3& transform(const GeometryContext& gctx) const override; + + /// @brief Get the nominal (non-aligned) transform for this detector element + /// @param gctx The geometry context (unused in nominal transform) + /// @return The nominal transform from the base TGeoDetectorElement Transform3 nominalTransform(const GeometryContext& gctx) const { return TGeoDetectorElement::transform(gctx); }
12-12: Forward declaration, consider you must.Hmmmm. Include DD4hepGeometryContext.hpp, heavy it might be. Forward declaration, lighter path it is.
-#include "Acts/Plugins/DD4hep/DD4hepGeometryContext.hpp" +class DD4hepGeometryContext;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Examples/Detectors/DD4hepDetector/CMakeLists.txt(1 hunks)Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepAlignmentDecorator.hpp(1 hunks)Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepDetector.hpp(1 hunks)Examples/Detectors/DD4hepDetector/src/DD4hepAlignmentDecorator.cpp(1 hunks)Examples/Detectors/DD4hepDetector/src/DD4hepDetector.cpp(2 hunks)Plugins/DD4hep/CMakeLists.txt(1 hunks)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorElement.hpp(2 hunks)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepGeometryContext.hpp(1 hunks)Plugins/DD4hep/src/DD4hepDetectorElement.cpp(2 hunks)Plugins/DD4hep/src/DD4hepGeometryContext.cpp(1 hunks)
🔇 Additional comments (5)
Examples/Detectors/DD4hepDetector/src/DD4hepAlignmentDecorator.cpp (2)
34-38: Check the logic, you must, for missing misalignment data edge case.
If the JSON path is invalid or no alignment data is found, gracefully handle it you should. The code now assumes data is present, but error states handle it must.
44-49: Check matrix composition carefully, you must.
Rotation followed by translation, correct the transform creation is. Mistakes in ordering create confusion. Confirm with test, you should.
Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepAlignmentDecorator.hpp (2)
24-24: Interface usage, correct it seems.
Implements IContextDecorator, well-structured it is. Good job, you have done.
67-83: Verify parameter ordering, you should.
Rotations applied in x, y, z order they are. If the specification demands a different sequence, ensure correctness you must.
Examples/Detectors/DD4hepDetector/CMakeLists.txt (1)
6-6: Add the new source file you have, yes.
Remember to ensure consistent naming or usage across the build, for clarity and discoverability.
| void ActsExamples::DD4hep::DD4hepAlignmentDecorator::parseGeometry( | ||
| const Acts::TrackingGeometry& tGeometry) { | ||
| // Double-visit - first count | ||
| std::size_t nTransforms = 0; | ||
| tGeometry.visitSurfaces([&nTransforms](const auto*) { ++nTransforms; }); | ||
| std::unordered_map<std::string, Acts::Transform3> aStore; | ||
| Acts::GeometryContext nominalCtx = {}; | ||
| // Collect the surfacas into the nominal store | ||
| auto fillTransforms = [&aStore, &nominalCtx](const auto* surface) -> void { | ||
| if (surface == nullptr) { | ||
| throw std::invalid_argument("Surface is nullptr."); | ||
| } | ||
| auto alignableElement = dynamic_cast<const Acts::DD4hepDetectorElement*>( | ||
| surface->associatedDetectorElement()); | ||
| if (alignableElement == nullptr) { | ||
| throw std::invalid_argument("Surface is not alignable"); | ||
| } | ||
| unsigned long long id = alignableElement->identifier(); | ||
| aStore[Form("%lld", id)] = surface->transform(nominalCtx); | ||
| }; | ||
| tGeometry.visitSurfaces(fillTransforms); | ||
| m_nominalStore = std::move(aStore); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exception throwing you do, but more detail helpful it may be.
When surfaces are null or not alignable, an invalid_argument is thrown. Additional context about which surface triggers the error, wise it would be to include.
| const Acts::Transform3& Acts::DD4hepGeometryContext::contextualTransform( | ||
| const DD4hepDetectorElement& dElement) const { | ||
| if (!this->isNominal()) { | ||
| auto it = m_alignmentStore.find(Form("%lld", dElement.identifier())); | ||
| if (it != m_alignmentStore.end()) { | ||
| return it->second; | ||
| } else { | ||
| return dElement.nominalTransform(DD4hepGeometryContext()); | ||
| } | ||
| } else { | ||
| return dElement.nominalTransform(DD4hepGeometryContext()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling wisdom, share I must.
Consider adding error handling for invalid transformations:
const Acts::Transform3& Acts::DD4hepGeometryContext::contextualTransform(
const DD4hepDetectorElement& dElement) const {
if (!this->isNominal()) {
auto it = m_alignmentStore.find(Form("%lld", dElement.identifier()));
if (it != m_alignmentStore.end()) {
+ // Validate transformation matrix
+ if (!it->second.matrix().allFinite()) {
+ throw std::runtime_error("Invalid transformation matrix for detector element " +
+ std::to_string(dElement.identifier()));
+ }
return it->second;
} else {
return dElement.nominalTransform(DD4hepGeometryContext());
}
} else {
return dElement.nominalTransform(DD4hepGeometryContext());
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const Acts::Transform3& Acts::DD4hepGeometryContext::contextualTransform( | |
| const DD4hepDetectorElement& dElement) const { | |
| if (!this->isNominal()) { | |
| auto it = m_alignmentStore.find(Form("%lld", dElement.identifier())); | |
| if (it != m_alignmentStore.end()) { | |
| return it->second; | |
| } else { | |
| return dElement.nominalTransform(DD4hepGeometryContext()); | |
| } | |
| } else { | |
| return dElement.nominalTransform(DD4hepGeometryContext()); | |
| } | |
| } | |
| const Acts::Transform3& Acts::DD4hepGeometryContext::contextualTransform( | |
| const DD4hepDetectorElement& dElement) const { | |
| if (!this->isNominal()) { | |
| auto it = m_alignmentStore.find(Form("%lld", dElement.identifier())); | |
| if (it != m_alignmentStore.end()) { | |
| // Validate transformation matrix | |
| if (!it->second.matrix().allFinite()) { | |
| throw std::runtime_error("Invalid transformation matrix for detector element " + | |
| std::to_string(dElement.identifier())); | |
| } | |
| return it->second; | |
| } else { | |
| return dElement.nominalTransform(DD4hepGeometryContext()); | |
| } | |
| } else { | |
| return dElement.nominalTransform(DD4hepGeometryContext()); | |
| } | |
| } |
| dd4ContextDecorators.push_back(std::make_shared<DD4hepAlignmentDecorator>( | ||
| std::move(dd4hepAcfg), | ||
| Acts::getDefaultLogger("AlignmentDecorator", Acts::Logging::INFO))); | ||
| std::cout << "After dd4ContextDecorators push back" << std::endl; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper logging mechanism, you must. std::cout, a path to the dark side it is.
Replace direct console output with proper logging framework, we should.
- std::cout << "After dd4ContextDecorators push back" << std::endl;
+ ACTS_DEBUG("Alignment decorator successfully initialized");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::cout << "After dd4ContextDecorators push back" << std::endl; | |
| ACTS_DEBUG("Alignment decorator successfully initialized"); |
| DD4hepAlignmentDecorator::Config dd4hepAcfg; | ||
| dd4hepAcfg.nominal = m_nominal; | ||
| dd4hepAcfg.trackingGeometry = dd4tGeometry; | ||
| ContextDecorators dd4ContextDecorators = {}; | ||
| dd4ContextDecorators.push_back(std::make_shared<DD4hepAlignmentDecorator>( | ||
| std::move(dd4hepAcfg), | ||
| Acts::getDefaultLogger("AlignmentDecorator", Acts::Logging::INFO))); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle decorator initialization failures, wise one must.
Hmmmm. Protect against initialization failures of alignment decorator, we should. Validate configuration before proceeding, essential it is.
DD4hepAlignmentDecorator::Config dd4hepAcfg;
dd4hepAcfg.nominal = m_nominal;
dd4hepAcfg.trackingGeometry = dd4tGeometry;
+ if (!dd4hepAcfg.trackingGeometry) {
+ throw std::runtime_error{"Invalid tracking geometry in alignment decorator configuration"};
+ }
ContextDecorators dd4ContextDecorators = {};
dd4ContextDecorators.push_back(std::make_shared<DD4hepAlignmentDecorator>(
std::move(dd4hepAcfg),
Acts::getDefaultLogger("AlignmentDecorator", Acts::Logging::INFO)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DD4hepAlignmentDecorator::Config dd4hepAcfg; | |
| dd4hepAcfg.nominal = m_nominal; | |
| dd4hepAcfg.trackingGeometry = dd4tGeometry; | |
| ContextDecorators dd4ContextDecorators = {}; | |
| dd4ContextDecorators.push_back(std::make_shared<DD4hepAlignmentDecorator>( | |
| std::move(dd4hepAcfg), | |
| Acts::getDefaultLogger("AlignmentDecorator", Acts::Logging::INFO))); | |
| DD4hepAlignmentDecorator::Config dd4hepAcfg; | |
| dd4hepAcfg.nominal = m_nominal; | |
| dd4hepAcfg.trackingGeometry = dd4tGeometry; | |
| if (!dd4hepAcfg.trackingGeometry) { | |
| throw std::runtime_error{"Invalid tracking geometry in alignment decorator configuration"}; | |
| } | |
| ContextDecorators dd4ContextDecorators = {}; | |
| dd4ContextDecorators.push_back(std::make_shared<DD4hepAlignmentDecorator>( | |
| std::move(dd4hepAcfg), | |
| Acts::getDefaultLogger("AlignmentDecorator", Acts::Logging::INFO))); |
odd-misalignment-matrix.jsonis required. The json file includes the identifiers of the misaligned DD4hepDetectorElements, as well as six parameters: the translation parameters in the x, y, and z directions (unit: cm), and the rotation angles along the x, y, and z axes (unit: degree). In the decorator,DD4hepGeometryContextis used to store the misaligned transform matrix.Summary by CodeRabbit
New Features
DD4hepAlignmentDecoratorclass for managing detector alignment.DD4hepGeometryContextclass to handle alignment functionalities in the DD4hep framework.DD4hepDetectorElementandDD4hepGeometryContextclasses.Bug Fixes
Documentation