refactor: Cleanup some more detector code in Examples #3908
Conversation
WalkthroughChanges made, significant they are. In various files, namespace prefixes removed, simplifications achieved. Constructors and method signatures updated to reflect new types without prefixes. The Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (46)
Examples/Detectors/MagneticField/src/ScalableBFieldService.cpp (2)
31-36: Document the scaling formula's purpose, we must!Correct the implementation is, but mysterious the scaling formula remains. Document its purpose and effects, you should.
Add a comment explaining the scaling formula:
ProcessCode ScalableBFieldService::decorate(AlgorithmContext& ctx) { ScalableBFieldContext magCtx; + // Scale the magnetic field strength exponentially with event number + // to simulate field variations magCtx.scalor = std::pow(m_cfg.scalor, ctx.eventNumber); ctx.magFieldContext = std::make_any<ScalableBFieldContext>(magCtx); return ProcessCode::SUCCESS; }
21-38: Well organized, the namespace changes are!Aligned with the refactoring objectives, these changes are. Simplified namespace usage, improved readability brings. Approve this restructuring, I do!
Consider in future iterations:
- Type-safe alternatives to
std::anyexplore, you might- Error handling for context casting add, you could
Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/GenericDetector.hpp (1)
44-50: Explicit declaration of detectorStore, clarity brings to our path!More readable the code has become. The Force flows better through explicit types, it does. A good change this is.
Consider this alternative, you might:
- std::vector<std::vector<std::shared_ptr<GenericDetectorElement>>> - detectorStore; + using DetectorElementPtr = std::shared_ptr<GenericDetectorElement>; + using DetectorStore = std::vector<std::vector<DetectorElementPtr>>; + DetectorStore detectorStore;Improve readability further, this would. But optional, it is.
Examples/Detectors/TelescopeDetector/include/ActsExamples/TelescopeDetector/TelescopeDetector.hpp (1)
28-45: Architectural wisdom, share I must!Strong foundation for detector implementation, this structure provides. Yet, consider these improvements for future, you should:
- Documentation for Config parameters, add you could
- Type safety through strong typing for physical quantities, consider you might
- Validation for configuration parameters, implement you may
Better maintainability and clarity, these changes would bring.
Examples/Algorithms/Geant4/include/ActsExamples/TelescopeDetector/TelescopeG4DetectorConstruction.hpp (2)
Line range hint
22-37: Strong with the Force, this class structure is!Clean inheritance from G4VUserDetectorConstruction, observe I do. Well-organized member variables and constructor parameters, maintained they are. Private members, properly encapsulated they remain.
Consider documenting the purpose of
m_worldmember variable, help future Padawans it will. Documentation, the path to wisdom is!
Line range hint
39-54: Factory pattern, wisely implemented it is!Clear responsibility this factory has - construction of detector instances, its purpose is. Good practices observed:
- Final class declaration, preventing unexpected inheritance
- Override keyword, properly used
- Clean member organization, maintained
Consider adding
noexcepttofactorize(), if exceptions thrown are not. Improve contract clarity, it would:- std::unique_ptr<G4VUserDetectorConstruction> factorize() const override; + std::unique_ptr<G4VUserDetectorConstruction> factorize() const noexcept override;Examples/Detectors/TelescopeDetector/src/TelescopeDetectorElement.cpp (1)
Line range hint
32-44: Inconsistent paths to material assignment, I observe!Different approaches to material assignment between constructors there are:
- Planar constructor uses
const_pointer_cast- Disc constructor assigns directly
Harmonize these approaches we should, yes! Choose the safer path of direct assignment for both, wise it would be.
- auto mutableSurface = - std::const_pointer_cast<Acts::Surface>(m_elementSurface); - mutableSurface->assignSurfaceMaterial(std::move(material)); + m_elementSurface->assignSurfaceMaterial(std::move(material));Examples/Detectors/TelescopeDetector/include/ActsExamples/TelescopeDetector/BuildTelescopeDetector.hpp (1)
46-55: Consider a configuration struct, you should.Many parameters, this function has. Easier to maintain and less prone to errors, a configuration struct would be. Example, I shall show:
struct TelescopeConfig { std::vector<double> positions; std::vector<double> stereoAngles; std::array<double, 2> offsets; std::array<double, 2> bounds; double thickness; TelescopeSurfaceType surfaceType; Acts::BinningValue binValue = Acts::BinningValue::binZ; }; std::unique_ptr<const Acts::TrackingGeometry> buildTelescopeDetector( const Acts::GeometryContext& gctx, std::vector<std::shared_ptr<const Acts::DetectorElementBase>>& detectorStore, const TelescopeConfig& config);Examples/Algorithms/Geant4/include/ActsExamples/DDG4/DDG4DetectorConstruction.hpp (1)
Line range hint
1-72: Wisdom in architecture, share I must.Well-structured the code is, with proper memory management through shared_ptr. Consider these architectural points, you should:
- Thread-safety of the shared detector instance, ensure you must
- Documentation for ownership semantics, helpful it would be
- Unit tests for factory creation, write you should
Examples/Detectors/GenericDetector/src/GenericDetectorElement.cpp (3)
Line range hint
18-33: Strong with RAII, this constructor is!Wise use of std::move, I see. Surface initialization, properly done it is. Yet, documentation of parameters, helpful it would be, hmm?
Consider adding this documentation:
/** * @brief Construct detector element with planar surface * @param identifier Unique identifier * @param transform Local to global transform * @param pBounds Planar bounds of the surface * @param thickness Material thickness * @param material Surface material properties */
Line range hint
34-48: Duplicate code, I sense. Refactor we must!Common initialization between constructors exists. Extract it we should, hmm?
Consider this pattern, young padawan:
class GenericDetectorElement { private: void initializeElement(const Identifier identifier, std::shared_ptr<const Acts::Transform3> transform, double thickness, std::shared_ptr<const Acts::ISurfaceMaterial> material) { m_elementIdentifier = identifier; m_elementTransform = std::move(transform); m_elementThickness = thickness; m_elementSurface->assignSurfaceMaterial(std::move(material)); } };
50-53: Unused parameter, explain we must!The GeometryContext parameter, unused it remains. Document why, we should.
Add comment explaining the interface requirement:
// GeometryContext parameter required by interface but unused in this implementationExamples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/ExternallyAlignedDetectorElement.hpp (2)
Line range hint
33-76: Strong with the Force, this implementation is.Robust error handling observe I do:
- Empty context check, present it is
- Null alignment store verification, implemented well it is
- Bounds checking through assertion, protecting against the dark side it does
Yet, a suggestion I have, for safer bounds checking:
// At this point, the alignment store should be populated - assert(idValue < alignContext.alignmentStore->transforms.size()); + if (idValue >= alignContext.alignmentStore->transforms.size()) { + throw std::out_of_range("Alignment store access out of bounds"); + } return alignContext.alignmentStore->transforms[idValue];In production code, assertions disabled they may be. Runtime check, safer it would be.
Line range hint
34-42: Documentation for AlignmentStore, missing it is.Hmmmm, document the purpose of
lastAccessedmember, you should. Used it is not, yet exists it does.Add documentation or remove unused member, you must:
struct AlignmentStore { + /// Stores the transforms for each detector element std::vector<Acts::Transform3> transforms; - std::size_t lastAccessed = 0; };Examples/Detectors/DD4hepDetector/src/DD4hepDetector.cpp (1)
Line range hint
73-83: Optimize the Force, we must!Each call to field(), a new adapter creates. Wasteful, this is. Cache the adapter, we should, when multiple uses expected are.
Consider this path to enlightenment:
+private: + mutable std::shared_ptr<Acts::DD4hepFieldAdapter> m_fieldAdapter; + public: std::shared_ptr<Acts::DD4hepFieldAdapter> field() const { + if (!m_fieldAdapter) { const auto& detector = geometryService->detector(); - return std::make_shared<Acts::DD4hepFieldAdapter>(detector.field()); + m_fieldAdapter = std::make_shared<Acts::DD4hepFieldAdapter>(detector.field()); + } + return m_fieldAdapter; }Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepDetector.hpp (1)
Line range hint
35-91: Document the namespace changes in README, you should.For future Padawans who maintain this code, helpful it would be to document these namespace reorganizations. In the project's README or documentation, explain the rationale behind moving IContextDecorator to global scope, we must.
Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignmentDecorator.hpp (1)
Line range hint
44-80: Refactor this method into smaller functions, we should. Documentation, we must add!Long and complex, this method is. Break it down into smaller functions for clarity, we must:
- Translation calculation
- Rotation application
- Parameter documentation
Consider this structure:
protected: + /// @brief Apply translation to the transform + /// @param trf The transform to modify + /// @param tx Translation in x + /// @param ty Translation in y + /// @param tz Translation in z + static void applyTranslation(Acts::Transform3& trf, + double tx, double ty, double tz) { + if (tx == 0. && ty == 0. && tz == 0.) { + return; + } + const auto& tMatrix = trf.matrix(); + auto colX = tMatrix.block<3, 1>(0, 0).transpose(); + auto colY = tMatrix.block<3, 1>(0, 1).transpose(); + auto colZ = tMatrix.block<3, 1>(0, 2).transpose(); + Acts::Vector3 newCenter = tMatrix.block<3, 1>(0, 3).transpose() + + tx * colX + ty * colY + tz * colZ; + trf.translation() = newCenter; + } + + /// @brief Apply rotations to the transform + /// @param trf The transform to modify + /// @param rx Rotation around x + /// @param ry Rotation around y + /// @param rz Rotation around z + static void applyRotation(Acts::Transform3& trf, + double rx, double ry, double rz) { + if (rx != 0.) { + trf *= Acts::AngleAxis3(rx, Acts::Vector3::UnitX()); + } + if (ry != 0.) { + trf *= Acts::AngleAxis3(ry, Acts::Vector3::UnitY()); + } + if (rz != 0.) { + trf *= Acts::AngleAxis3(rz, Acts::Vector3::UnitZ()); + } + }Examples/Detectors/ContextualDetector/src/InternalAlignmentDecorator.cpp (1)
22-24: Add noexcept specification to constructor, you should.Simple and correct, the constructor is. Yet, mark it as noexcept, we must, for move operations involve we do.
-InternalAlignmentDecorator::InternalAlignmentDecorator( +InternalAlignmentDecorator::InternalAlignmentDecorator( const Config& cfg, std::unique_ptr<const Acts::Logger> logger) - : m_cfg(cfg), m_logger(std::move(logger)) {} + : m_cfg(cfg), m_logger(std::move(logger)) noexcept {}Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/GenericDetectorElement.hpp (1)
Line range hint
89-102: Consider ownership patterns, you should.Mixed usage of shared pointers and raw pointers in member variables, I see. A more consistent approach to memory ownership, beneficial it would be. Consider these paths:
- Use
std::unique_ptrwhere exclusive ownership is clear- Document ownership semantics in class comments
- Consider implementing PIMPL pattern for better encapsulation
Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/InternallyAlignedDetectorElement.hpp (2)
32-32: Documentation enhancement, suggest I must.Clear the inheritance is, but document thread-safety guarantees we should. Help future Padawans understand the ways of concurrent access, it will.
Add thread-safety documentation like this, you should:
/// The AlignedDetectorElement demonstrates how a GeometryContext /// can be used if it carries an interval of validity concept +/// +/// Thread-safety: This class is thread-safe. All modifications to alignment +/// transforms are protected by internal mutex. ///
Line range hint
82-89: Improve error handling, we shall.In the exception message construction, a potential for better performance I sense. String concatenation in exceptions, careful we must be.
Improve the error message construction like this, you should:
- throw std::runtime_error{ - "Aligned transform for IOV " + std::to_string(alignContext.iov) + - " not found. This can happen if the garbage collection runs too " - "early (--align-flushsize too low)"}; + throw std::runtime_error{fmt::format( + "Aligned transform for IOV {} not found. This can happen if the garbage " + "collection runs too early (--align-flushsize too low)", + alignContext.iov)};Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepReader.hpp (1)
56-56: Strong with the Force, this design pattern is!Forward declaration with shared_ptr, a powerful combination it is. Compilation dependencies reduced, it has. Build times improved, they shall be. Yet cautious we must remain - ensure complete type definition available where shared_ptr operations performed, you must.
Examples/Detectors/ContextualDetector/src/AlignedDetector.cpp (1)
84-91: Balanced, the force of internal and external alignments now is!Mirror the external alignment changes, this does. Symmetry in implementation, brings clarity it does.
Consider documenting the alignment context handling pattern in the class documentation, help future padawans it will.
Examples/Detectors/ContextualDetector/src/ExternalAlignmentDecorator.cpp (1)
Line range hint
97-126: More specific in error handling, we must be!Hmm, exceptions with context, throw we should. Dynamic casting, a path to the dark side it can be.
Apply these changes, you should:
if (surface == nullptr) { - throw std::invalid_argument("Surface is nullptr."); + throw std::invalid_argument("Surface pointer is nullptr in parseGeometry."); } auto alignableElement = dynamic_cast<const ExternallyAlignedDetectorElement*>( surface->associatedDetectorElement()); if (alignableElement == nullptr) { - throw std::invalid_argument("Surface is not alignable"); + throw std::invalid_argument( + "Surface's detector element is not an ExternallyAlignedDetectorElement"); }Consider factory pattern or interface segregation, avoid dynamic_cast we must. Discuss design alternatives, shall we?
Examples/Detectors/TGeoDetector/src/TGeoITkModuleSplitter.cpp (3)
Line range hint
33-52: Strengthen error handling, we must!Handle errors with more wisdom, we should. Currently, on invalid category, continue silently we do. Dangerous, this path is!
Apply this change, you should:
if (m_cfg.discMap.contains(pattern_split_category.second)) { is_disk = true; } else if (!m_cfg.barrelMap.contains(pattern_split_category.second)) { - ACTS_ERROR( + throw std::invalid_argument( pattern_split_category.second + " is neither a category name for barrel or disk module splits."); - continue; }
Line range hint
55-83: Document the ways of the Force, we must!Missing documentation for return values, I sense. Help future Padawans understand the code better, we should.
Add documentation like this, you must:
/// If applicable, returns a split detector element +/// @param gctx The geometry context for the transformation +/// @param detElement The detector element to potentially split +/// @return Vector of split detector elements, or single element if no split applies inline std::vector<std::shared_ptr<const Acts::TGeoDetectorElement>>
Line range hint
88-120: Duplicate code, I sense. Share wisdom, we must!Similar factory lambda in both split methods exists. Extract common knowledge, we should.
Move factory to class level, like this:
+private: + auto createElementFactory( + const std::shared_ptr<const Acts::TGeoDetectorElement>& detElement) const { + return [&](const auto& trafo, const auto& bounds) { + return std::make_shared<const Acts::TGeoDetectorElement>( + detElement->identifier(), detElement->tgeoNode(), trafo, bounds, + detElement->thickness()); + }; + }Then use in split methods:
- auto factory = [&](const auto& trafo, const auto& bounds) { - return std::make_shared<const Acts::TGeoDetectorElement>( - detElement->identifier(), detElement->tgeoNode(), trafo, bounds, - detElement->thickness()); - }; + auto factory = createElementFactory(detElement);Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepGeometryService.hpp (1)
Line range hint
1-137: Architecture advice, share I must.Strong with the SOLID principles, this class is. Yet, consider these improvements, you should:
- Dependency injection through the constructor, strengthen we could
- Documentation for Config members, enhance we should
- Thread safety guarantees, document we must
The path to better code, these suggestions illuminate.
Examples/Detectors/TGeoDetector/include/ActsExamples/TGeoDetector/TGeoDetector.hpp (1)
Line range hint
46-152: Documentation for Config struct, enhance we could!Complex configuration options, this struct holds. Yet documentation for each field's purpose and valid values, sparse it is. Future Padawans, struggle to understand this code, they might.
Consider adding documentation for:
- The purpose and valid ranges of
unitScalor- The meaning of
layerEnvelopeR- The purpose of
SubVolumeenum values- The configuration options in the
VolumestructHelp others understand the ways of the Force, good documentation does!
Examples/Detectors/GenericDetector/src/BuildGenericDetector.cpp (3)
Line range hint
15-41: Validate input parameters, you must!Hmmmm, dangerous it is to assume valid input always exists. Protection against invalid parameters, add we should.
Apply this change, you must:
std::vector<Acts::Vector3> ActsExamples::Generic::modulePositionsCylinder( double radius, double zStagger, double moduleHalfLength, double lOverlap, const std::pair<int, int>& binningSchema) { + if (radius <= 0 || moduleHalfLength <= 0) { + throw std::invalid_argument("Positive radius and moduleHalfLength required"); + } + if (binningSchema.first <= 0 || binningSchema.second <= 0) { + throw std::invalid_argument("Positive bin counts required"); + } int nPhiBins = binningSchema.first;
Line range hint
46-95: Size validation for input vectors, essential it is.Hmmmm, vectors of different sizes, lead to undefined behavior they may. Explicit validation at function start, add we must.
Add this validation, you should:
ActsExamples::Generic::modulePositionsDisc( double z, double ringStagger, std::vector<double> phiStagger, std::vector<double> phiSubStagger, double innerRadius, double outerRadius, const std::vector<std::size_t>& discBinning, const std::vector<double>& moduleHalfLength) { + if (discBinning.empty() || moduleHalfLength.empty()) { + throw std::invalid_argument("Empty input vectors not allowed"); + } + if (phiStagger.size() != discBinning.size()) { + throw std::invalid_argument("phiStagger size must match discBinning size"); + } + if (!phiSubStagger.empty() && phiSubStagger.size() != discBinning.size()) { + throw std::invalid_argument("phiSubStagger size must match discBinning size"); + }
Line range hint
97-130: Document the magic number's significance, we must.Magic number 4 in the code, mysterious it remains. Constants and documentation, clarity they bring.
Apply these changes, you should:
std::vector<Acts::Vector3> ActsExamples::Generic::modulePositionsRing( double z, double radius, double phiStagger, double phiSubStagger, int nPhiBins) { + // Phi sub-stagger requires number of bins to be multiple of 4 + // to maintain symmetry in the ring structure + constexpr int kPhiSubStaggerMultiple = 4; // create and fill the positions std::vector<Acts::Vector3> rPositions; rPositions.reserve(nPhiBins); // prep work double phiStep = 2 * std::numbers::pi / nPhiBins; double minPhi = -std::numbers::pi + 0.5 * phiStep; // phi loop for (std::size_t iphi = 0; iphi < static_cast<std::size_t>(nPhiBins); ++iphi) { // if we have a phi sub stagger presents double rzs = 0.; // phi stagger affects 0 vs 1, 2 vs 3 ... etc // phi sub stagger affects 2 vs 4, 1 vs 3 etc. - if (phiSubStagger != 0. && ((nPhiBins % 4) == 0)) { + if (phiSubStagger != 0. && ((nPhiBins % kPhiSubStaggerMultiple) == 0)) { // switch sides - if ((iphi % 4) == 0u) { + if ((iphi % kPhiSubStaggerMultiple) == 0u) { rzs = phiSubStagger; - } else if (((iphi + 1) % 4) == 0u) { + } else if (((iphi + 1) % kPhiSubStaggerMultiple) == 0u) { rzs = -phiSubStagger; } }Examples/Detectors/DD4hepDetector/src/DD4hepGeometryService.cpp (3)
27-34: More descriptive error message, provide you must.Hmm, cryptic the error message is. For better debugging, suggest I do:
if (m_cfg.xmlFileNames.empty()) { - throw std::invalid_argument("Missing DD4hep XML filenames"); + throw std::invalid_argument("DD4hep XML filenames vector is empty. At least one XML file required for detector configuration."); }
Line range hint
109-118: Handle errors gracefully, a Jedi must.Hmm, convertDD4hepDetector might throw, but catch them we do not. Protect the code, we should:
auto logger = Acts::getDefaultLogger("DD4hepConversion", m_cfg.logLevel); + try { m_trackingGeometry = Acts::convertDD4hepDetector( geometry(), *logger, m_cfg.bTypePhi, m_cfg.bTypeR, m_cfg.bTypeZ, m_cfg.envelopeR, m_cfg.envelopeZ, m_cfg.defaultLayerThickness, m_cfg.sortDetectors, gctx, m_cfg.matDecorator, m_cfg.geometryIdentifierHook); + } catch (const std::exception& e) { + ACTS_ERROR("Failed to convert DD4hep geometry: " << e.what()); + return ProcessCode::FAILURE; + }
Line range hint
139-178: Duplicate code, leads to the dark side it does.Repeated lambda functions, I see. Extract common comparator, we should:
+ auto compareById = [](const dd4hep::DetElement& a, const dd4hep::DetElement& b) { + return (a.id() < b.id()); + }; + sort(muon.begin(), muon.end(), - [](const dd4hep::DetElement& a, const dd4hep::DetElement& b) { - return (a.id() < b.id()); - }); + compareById); sort(eCal.begin(), eCal.end(), - [](const dd4hep::DetElement& a, const dd4hep::DetElement& b) { - return (a.id() < b.id()); - }); + compareById); // ... apply to other sorts as wellExamples/Detectors/TelescopeDetector/include/ActsExamples/TelescopeDetector/TelescopeDetectorElement.hpp (2)
Line range hint
29-41: Documentation improvements suggest, I must!Helpful it would be, to add these documentation elements:
- Example usage in code comments
- Explanation of the IOV (Interval of Validity) concept
- Thread-safety guarantees documentation
Add this documentation, you should:
/// @class TelescopeDetectorElement /// /// This is a lightweight type of detector element, -/// it simply implements the base class. +/// it simply implements the base class. This class supports both planar and disc +/// detector elements with alignment capabilities. +/// +/// @note Thread-safety: This class is thread-safe for const access. Alignment updates +/// must be externally synchronized. +/// +/// Example usage: +/// @code +/// auto transform = std::make_shared<Acts::Transform3>(/* ... */); +/// auto bounds = std::make_shared<Acts::PlanarBounds>(/* ... */); +/// TelescopeDetectorElement element(transform, bounds, 1.0); +/// @endcode
Line range hint
144-155: Optimize the aligned transform storage, we should!Inefficient it may become, when large IOV gaps exist. Memory allocations unnecessary, they are. Reserve space in advance, we must!
Improve the implementation thus:
inline void TelescopeDetectorElement::addAlignedTransform( std::unique_ptr<Acts::Transform3> alignedTransform, unsigned int iov) { - // most standard case, it's just a new one: + // Reserve space if vector is empty + if (m_alignedTransforms.empty()) { + m_alignedTransforms.reserve(std::max(10u, iov + 1)); // Reserve reasonable initial capacity + } auto cios = m_alignedTransforms.size(); for (unsigned int ic = cios; ic <= iov; ++ic) { m_alignedTransforms.push_back(nullptr); } m_alignedTransforms[iov] = std::move(alignedTransform); }Examples/Detectors/Geant4Detector/src/Geant4Detector.cpp (1)
107-109: Approve this change I do, but enhance the logging we could!Simplified the namespace is, yet maintain its power it does. Consider logging the memory usage during surface cache generation, helpful it would be.
Add memory usage logging:
Acts::Geant4DetectorSurfaceFactory::Cache g4SurfaceCache; G4Transform3D g4ToWorld; +ACTS_DEBUG("Begin surface cache generation, current memory usage: " << getCurrentMemoryUsage()); Acts::Geant4DetectorSurfaceFactory{}.construct(Examples/Python/src/DD4hepComponent.cpp (1)
151-165: Approve these changes, I do. Yet documentation, we should add!Clean and consistent with namespace changes, the code is. However, for the new
dropmethod andfieldproperty, documentation in comments helpful would be, hmm?Add documentation like this, you should:
py::class_<DD4hepDetector, std::shared_ptr<DD4hepDetector>>( m, "DD4hepDetector") .def(py::init<>()) .def(py::init<std::shared_ptr<DD4hepGeometryService>>()) .def("finalize", py::overload_cast<DD4hepGeometryService::Config, std::shared_ptr<const Acts::IMaterialDecorator>>( &DD4hepDetector::finalize)) .def("finalize", py::overload_cast< const Acts::GeometryContext&, const Acts::Experimental::DD4hepDetectorStructure::Options&>( &DD4hepDetector::finalize)) + // Release resources associated with the detector .def("drop", &DD4hepDetector::drop) + // Get the magnetic field provider .def_property_readonly("field", &DD4hepDetector::field);Examples/Algorithms/Geant4/src/TelescopeG4DetectorConstruction.cpp (3)
35-36: Fix the typo in assertion message, young padawan.A typo in the error message I sense. "plan" instead of "plane" written it is.
- throw_assert(cfg.surfaceType == static_cast<int>(TelescopeSurfaceType::Plane), - "only plan is supported right now"); + throw_assert(cfg.surfaceType == static_cast<int>(TelescopeSurfaceType::Plane), + "only plane is supported right now");
Line range hint
31-178: Wise decision to simplify namespaces, it was.Removed namespace prefixes, cleaner code structure achieved. The Force flows better through simplified paths. Maintain this approach in future changes, we should.
Memory management concerns, valid they remain
Searched through the Force, I have. No destructor or explicit cleanup in the class definition, found I have. Documentation about G4PVPlacement's ownership of rotation matrices, clear it is not. Though common in Geant4 it may be, explicit we must be.
Safe path forward, std::unique_ptr provides:
- Memory leaks, prevent it does
- Ownership semantics, clear they become
- If G4PVPlacement takes ownership, release from unique_ptr we can
- If not, cleanup automatic it becomes
The suggested fix in original review, wisdom it contains. Follow it, you should:
std::unique_ptr<G4RotationMatrix> rotation;May the Force guide your memory management!
🔗 Analysis chain
Line range hint
39-83: Memory management concerns in rotation handling, I have.Wisdom suggests that raw pointer
rotationcreated but never deleted it is. Memory leak this may cause.Consider using smart pointer or ensuring proper cleanup:
- G4RotationMatrix* rotation = nullptr; + std::unique_ptr<G4RotationMatrix> rotation; if (static_cast<Acts::BinningValue>(m_cfg.binValue) == Acts::BinningValue::binX) { - rotation = new G4RotationMatrix({0, 0, 1}, {0, 1, 0}, {-1, 0, 0}); + rotation = std::make_unique<G4RotationMatrix>({0, 0, 1}, {0, 1, 0}, {-1, 0, 0}); } else if (static_cast<Acts::BinningValue>(m_cfg.binValue) == Acts::BinningValue::binY) { - rotation = new G4RotationMatrix({1, 0, 0}, {0, 0, 1}, {0, -1, 0}); + rotation = std::make_unique<G4RotationMatrix>({1, 0, 0}, {0, 0, 1}, {0, -1, 0}); }Note: G4 placement takes ownership of rotation matrix, but explicit documentation of this behavior, find I cannot. Verify this assumption we must.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for documentation or examples of G4VPhysicalVolume rotation ownership rg -g '*.{cpp,h,cc,hh}' -A 5 -B 5 'G4PVPlacement.*rotation'Length of output: 2715
Script:
#!/bin/bash # Search for the complete implementation of TelescopeG4DetectorConstruction to see if rotation cleanup is handled elsewhere ast-grep --pattern 'class TelescopeG4DetectorConstruction { $$$ }' # Also search for any destructor implementation rg -g '*.{cpp,h,cc,hh}' 'TelescopeG4DetectorConstruction::~TelescopeG4DetectorConstruction'Length of output: 164
Script:
#!/bin/bash # Search for the header file to understand class definition fd -e h -e hpp TelescopeG4DetectorConstruction --exec cat {} # Search for any G4 documentation patterns about ownership rg -g '*.{cpp,h,cc,hh}' -A 3 -B 3 'G4PVPlacement.*takes.*ownership|G4PVPlacement.*delete'Length of output: 1901
Examples/Python/src/Detector.cpp (1)
91-101: Consistent with patterns above, yet indentation wavers it does!Inheritance from GenericDetector::Config, preserved it is. But notice I must, inconsistent indentation in the finalize method binding.
Apply this formatting improvement:
- py::overload_cast< - const Config&, - std::shared_ptr<const Acts::IMaterialDecorator>>( - &Detector::finalize)); + .def("finalize", + py::overload_cast<const Config&, + std::shared_ptr<const Acts::IMaterialDecorator>>( + &Detector::finalize));Examples/Detectors/MuonSpectrometerMockupDetector/src/MockupSectorBuilder.cpp (3)
Line range hint
81-120: Careful with floating-point precision, we must be!In geometry calculations, float type you use. For precise measurements, double precision better suited it is. Consider changing
floattodoublein min_max calculations, prevent accumulation errors we must.- std::array<std::pair<float, float>, 3> min_max; + std::array<std::pair<double, double>, 3> min_max;
Line range hint
164-167: Magic numbers, a path to the dark side they are!Undefined
maxNumberOfSectorsconstant, I sense. Document and define this limit explicitly, we should.+ // Maximum number of sectors allowed in the detector geometry + static constexpr std::size_t maxNumberOfSectors = /* your limit here */;
Line range hint
1-334: Architectural guidance, share I must!Complex geometry calculations, this code contains. Yet documentation of mathematical approach, missing it is. Consider adding:
- Detailed documentation of coordinate systems and transformations
- Validation checks for edge cases in geometry calculations
- Unit tests for numerical precision and boundary conditions
Help future maintainers understand the ways of the Force, this will.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (46)
Examples/Algorithms/Geant4/include/ActsExamples/DDG4/DDG4DetectorConstruction.hpp(3 hunks)Examples/Algorithms/Geant4/include/ActsExamples/TelescopeDetector/TelescopeG4DetectorConstruction.hpp(2 hunks)Examples/Algorithms/Geant4/src/DDG4DetectorConstruction.cpp(2 hunks)Examples/Algorithms/Geant4/src/TelescopeG4DetectorConstruction.cpp(2 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignedDetector.hpp(2 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignmentDecorator.hpp(2 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/ExternalAlignmentDecorator.hpp(0 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/ExternallyAlignedDetectorElement.hpp(4 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/InternalAlignmentDecorator.hpp(1 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/InternallyAlignedDetectorElement.hpp(4 hunks)Examples/Detectors/ContextualDetector/src/AlignedDetector.cpp(7 hunks)Examples/Detectors/ContextualDetector/src/ExternalAlignmentDecorator.cpp(3 hunks)Examples/Detectors/ContextualDetector/src/InternalAlignmentDecorator.cpp(2 hunks)Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepDetector.hpp(2 hunks)Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepGeometryService.hpp(3 hunks)Examples/Detectors/DD4hepDetector/src/DD4hepDetector.cpp(2 hunks)Examples/Detectors/DD4hepDetector/src/DD4hepGeometryService.cpp(5 hunks)Examples/Detectors/Geant4Detector/include/ActsExamples/Geant4Detector/Geant4Detector.hpp(1 hunks)Examples/Detectors/Geant4Detector/src/Geant4Detector.cpp(4 hunks)Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/BuildGenericDetector.hpp(2 hunks)Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/GenericDetector.hpp(2 hunks)Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/GenericDetectorElement.hpp(2 hunks)Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/LayerBuilderT.hpp(0 hunks)Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/ProtoLayerCreatorT.hpp(0 hunks)Examples/Detectors/GenericDetector/src/BuildGenericDetector.cpp(3 hunks)Examples/Detectors/GenericDetector/src/GenericDetector.cpp(2 hunks)Examples/Detectors/GenericDetector/src/GenericDetectorElement.cpp(3 hunks)Examples/Detectors/ITkModuleSplitting/include/ActsExamples/ITkModuleSplitting/ITkModuleSplitting.hpp(0 hunks)Examples/Detectors/MagneticField/src/ScalableBFieldService.cpp(1 hunks)Examples/Detectors/MuonSpectrometerMockupDetector/src/MockupSectorBuilder.cpp(5 hunks)Examples/Detectors/TGeoDetector/include/ActsExamples/TGeoDetector/JsonTGeoDetectorConfig.hpp(4 hunks)Examples/Detectors/TGeoDetector/include/ActsExamples/TGeoDetector/TGeoDetector.hpp(1 hunks)Examples/Detectors/TGeoDetector/src/TGeoDetector.cpp(1 hunks)Examples/Detectors/TGeoDetector/src/TGeoITkModuleSplitter.cpp(6 hunks)Examples/Detectors/TelescopeDetector/include/ActsExamples/TelescopeDetector/BuildTelescopeDetector.hpp(2 hunks)Examples/Detectors/TelescopeDetector/include/ActsExamples/TelescopeDetector/TelescopeDetector.hpp(1 hunks)Examples/Detectors/TelescopeDetector/include/ActsExamples/TelescopeDetector/TelescopeDetectorElement.hpp(2 hunks)Examples/Detectors/TelescopeDetector/src/BuildTelescopeDetector.cpp(2 hunks)Examples/Detectors/TelescopeDetector/src/TelescopeDetector.cpp(3 hunks)Examples/Detectors/TelescopeDetector/src/TelescopeDetectorElement.cpp(3 hunks)Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepReader.hpp(1 hunks)Examples/Io/EDM4hep/src/EDM4hepReader.cpp(0 hunks)Examples/Python/src/DD4hepComponent.cpp(2 hunks)Examples/Python/src/Detector.cpp(5 hunks)Examples/Python/src/Geant4Component.cpp(2 hunks)Examples/Python/src/Geant4DD4hepComponent.cpp(1 hunks)
💤 Files with no reviewable changes (5)
- Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/ExternalAlignmentDecorator.hpp
- Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/LayerBuilderT.hpp
- Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/ProtoLayerCreatorT.hpp
- Examples/Detectors/ITkModuleSplitting/include/ActsExamples/ITkModuleSplitting/ITkModuleSplitting.hpp
- Examples/Io/EDM4hep/src/EDM4hepReader.cpp
🔇 Additional comments (105)
Examples/Detectors/TelescopeDetector/src/BuildTelescopeDetector.cpp (3)
15-15: Included 'DetectorElementBase.hpp', wisely you have.
Necessary for base class usage, this include is.
32-32: Including 'TelescopeDetectorElement.hpp', appropriate it is.
Ensures access to detector element specifics, it does.
39-41: Function signature refined, see I do.
Parameters generalized to enhance flexibility, the code now is.
Also applies to: 46-46
Examples/Python/src/Geant4Component.cpp (4)
242-246: Simplified, the class definition has been.
Removed the namespace prefix, you have. Improved readability, it brings.
280-281: Consistent aliases, you have introduced.
Using Detector and Config for clarity, wise choice it is.
284-295: Updated method bindings, correctly they are.
Adjustments to constructors and methods, in line with refactoring they are.
301-302: Config class bound properly, it is.
Consistency in class definitions maintained, commendable this is.
Examples/Detectors/MagneticField/src/ScalableBFieldService.cpp (2)
23-25: Clean and proper, the constructor implementation is!
Correctly initialized, the member variables are. Well-structured, the implementation remains, hmm.
27-29: Wise use of const reference and anonymous namespace, I sense!
Efficient and clean, the implementation is. In anonymous namespace, the string constant dwells. Good practices, these are!
Examples/Python/src/Geant4DD4hepComponent.cpp (1)
Line range hint 29-33: Approve the namespace simplification, I do!
Simplified the constructor signature is, removing DD4hep:: namespace prefix from DD4hepDetector. Consistent with the force of clean code, this change is. Preserved, the functionality remains, hmmmm.
Run this verification script, we must, to ensure consistency across the codebase:
✅ Verification successful
Consistent with the Force, the namespace changes are, hmmmm
Through the Force, I sense that all occurrences of DD4hepDetector in the codebase follow the simplified namespace pattern. No instances of DD4hep::DD4hepDetector remain, indicating the changes are uniform and complete. The Python bindings modification aligns perfectly with the rest of the codebase, it does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DD4hepDetector type usage consistency
# Search for any remaining DD4hep::DD4hepDetector usage that might need updating
# Check for any remaining DD4hep::DD4hepDetector usage
rg "DD4hep::DD4hepDetector" -l
# Check the new DD4hepDetector usage
rg "\\bDD4hepDetector\\b" -A 2
Length of output: 15820
Examples/Detectors/GenericDetector/src/GenericDetector.cpp (4)
15-15: Approve this namespace change, I do!
Wrapped in ActsExamples namespace, the code now is. Cleaner organization achieved, hmm, yes.
Also applies to: 34-34
Line range hint 24-31: Strong with the Force, this implementation is!
Proper use of move semantics, I sense. Template parameter GenericDetectorElement, wisely chosen it was. Configuration parameters, correctly passed they are.
8-14: Clean and organized, these headers are!
Removed unnecessary dependencies, you have. Clear path to the Force, this brings. Headers properly organized, they remain.
18-21: Verify the context type usage across the codebase, we must!
Changed to GenericDetectorElement::ContextType, the context has. Ensure consistent usage throughout the force, we should.
✅ Verification successful
Let me examine the context types in detector elements, I shall.
Consistent with the Force, the context type changes are!
Examined the detector elements closely, I have. Each detector element, its own context type definition it maintains:
- GenericDetectorElement: Basic identifier types it has
- InternallyAlignedDetectorElement: From GenericDetectorElement it inherits, with IOV context it extends
- ExternallyAlignedDetectorElement: From GenericDetectorElement it also inherits
- TelescopeDetectorElement: Its own ContextType structure it defines
- Others: GeometryContext they use
Changed to GenericDetectorElement::ContextType in this file, proper inheritance and usage patterns it follows. No disturbance in the Force, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining DetectorElement::ContextType usage
rg "DetectorElement::ContextType"
# Search for GenericDetectorElement::ContextType usage
rg "GenericDetectorElement::ContextType"
Length of output: 1248
Script:
#!/bin/bash
# Search for context type declarations in detector element headers
fd -e hpp -e h ".*DetectorElement\.h.*" --exec rg -A 5 "class.*DetectorElement" {}
# Search for context type usage in method signatures
ast-grep --pattern 'using ContextType = $_'
Length of output: 3198
Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/GenericDetector.hpp (2)
27-30: Simplified the namespace structure, you have. Approve this change, I do!
Removed the unnecessary nesting of namespaces, bringing clarity to the Force. A wise decision this is, aligning with our mission to simplify the detector code.
Line range hint 32-43: Verify the impact of type changes across the codebase, we must.
Changed the ContextDecorators type alias has been. Ensure broken, nothing is.
✅ Verification successful
Safe, the type changes are. Proceed with confidence, you may.
Through the Force, seen I have that all usages of ContextDecorators consistently define it as std::vector<std::shared_ptr<IContextDecorator>>. No deviation from this pattern exists across the codebase. The change in GenericDetector aligns perfectly with all other detector implementations:
- TelescopeDetector
- TGeoDetector
- Geant4Detector
- DD4hepDetector
- AlignedDetector
Maintain the same type definition, they all do. Break anything, this change will not.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of ContextDecorators type
rg -l "ContextDecorators" --type cpp
# Search for potential factory or builder classes that might be affected
ast-grep --pattern 'class $_ {
$$$
ContextDecorators $_;
$$$
}'
Length of output: 1008
Script:
#!/bin/bash
# Check the actual usage of ContextDecorators in these files
rg "ContextDecorators" -B 2 -A 2 --type cpp
# Search for any direct usage of IContextDecorator
rg "IContextDecorator" -B 2 -A 2 --type cpp
# Look for vector<shared_ptr<IContextDecorator>> pattern specifically
rg "vector\s*<\s*shared_ptr\s*<\s*IContextDecorator\s*>" --type cpp
Length of output: 35118
Examples/Detectors/TelescopeDetector/include/ActsExamples/TelescopeDetector/TelescopeDetector.hpp (5)
12-12: Wise addition of DetectorElementBase header, it is!
Clear dependency on Acts geometry primitives, this makes. Good practice for header organization, hmm, yes.
26-29: Simplified namespace usage, you have. Pleased, I am!
Removed unnecessary namespace qualification for IContextDecorator, making the code cleaner and more maintainable, it does. Strong with the Force, this change is.
37-37: More explicit unit conversion, I sense!
Changed from magic number notation to explicit unit conversion using Acts::UnitConstants::um, you have. Clearer intent and better maintainability, this brings.
46-49: Clean interface for geometry creation, maintained you have.
Unchanged but well-designed this interface remains. Parameters for material decoration and configuration, properly it accepts.
44-44: Stronger typing and better abstraction, achieved you have!
Replaced specific detector element type with base class pointer in detectorStore. More flexible and maintainable, this design is. But verify compatibility with existing code, we must.
✅ Verification successful
Safe and proper, the base class usage is! Hmmmm.
Through the Force, I sense that TelescopeDetectorElement properly inherits from DetectorElementBase, it does. Created with std::make_shared and stored in detectorStore without casts or conversions, the elements are. Move semantics, used correctly they are. In harmony with the base class design, everything flows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the detector element base class is used consistently
# Search for any remaining specific detector element types
rg -l "TelescopeDetectorElement"
# Check for proper usage of DetectorElementBase
ast-grep --pattern 'std::shared_ptr<const Acts::DetectorElementBase>'
Length of output: 462
Script:
#!/bin/bash
# Check how TelescopeDetectorElement is used in implementation
rg "TelescopeDetectorElement" Examples/Detectors/TelescopeDetector/src/TelescopeDetector.cpp -B 2 -A 2
# Check if there are any direct usages or casts of DetectorElementBase
rg "DetectorElementBase" Examples/Detectors/TelescopeDetector/src/TelescopeDetector.cpp -B 2 -A 2
# Examine the TelescopeDetectorElement header to verify inheritance
cat Examples/Detectors/TelescopeDetector/include/ActsExamples/TelescopeDetector/TelescopeDetectorElement.hpp
Length of output: 6438
Script:
#!/bin/bash
# Check how detector elements are created and stored
rg "detectorStore" Examples/Detectors/TelescopeDetector/src/BuildTelescopeDetector.cpp -B 2 -A 2
# Check for any dynamic casting or type conversion
rg "dynamic_cast|static_cast" Examples/Detectors/TelescopeDetector/src/BuildTelescopeDetector.cpp
# Look for usage of detector elements in the building process
rg "std::make_shared.*TelescopeDetectorElement" Examples/Detectors/TelescopeDetector/src/BuildTelescopeDetector.cpp -A 2
Length of output: 1145
Examples/Algorithms/Geant4/include/ActsExamples/TelescopeDetector/TelescopeG4DetectorConstruction.hpp (1)
20-20: Wise decision, the namespace simplification is!
From ActsExamples::Telescope to ActsExamples, moved the classes have been. Cleaner structure, this creates. With PR objectives, aligned this change is.
Verify potential naming conflicts, we must:
Also applies to: 56-56
✅ Verification successful
Safe, the namespace change is, young padawan!
Through the Force, seen I have that no conflicts exist. The TelescopeG4DetectorConstruction and TelescopeG4DetectorConstructionFactory, properly moved they are from Telescope namespace to ActsExamples. All references to these classes, already using the new namespace they are.
Used only in:
- Python bindings (Examples/Python/src/Geant4Component.cpp)
- Implementation file (Examples/Algorithms/Geant4/src/TelescopeG4DetectorConstruction.cpp)
No other uses of Telescope namespace found in the codebase, hmmmm. Clean and safe, this change is!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other TelescopeG4DetectorConstruction classes in the codebase
rg -l "class.*TelescopeG4DetectorConstruction" | grep -v "TelescopeG4DetectorConstruction.hpp"
# Check for any other uses of Telescope namespace
rg "namespace.*Telescope"
Length of output: 162
Script:
#!/bin/bash
# Check the content of the Python binding file to understand the usage
cat Examples/Python/src/Geant4Component.cpp
# Search for any references to TelescopeG4DetectorConstruction
rg "TelescopeG4DetectorConstruction" -A 3 -B 3
# Search for any references to Telescope namespace
rg "Telescope::" -A 2 -B 2
Length of output: 26076
Examples/Detectors/TelescopeDetector/src/TelescopeDetectorElement.cpp (3)
14-15: Simplified the namespace structure, hmm, yes!
Wise decision it is, to remove nested namespace. Cleaner code structure this brings, easier to maintain it becomes.
Line range hint 14-46: Verify the impact of namespace changes, we must!
Changed from ActsExamples::Telescope to ActsExamples the namespace has. Ensure broken, no existing code becomes.
Line range hint 16-31: Careful with const_pointer_cast, we must be!
A disturbance in the Force I sense with const_pointer_cast. Though necessary for material assignment it may be, dangerous such casts are. Consider redesigning the surface interface to avoid const casting, hmm?
✅ Verification successful
Necessary, const_pointer_cast is not, hmmmm!
Through the Force, seen I have that this pattern widespread it is, but a better path exists. The Surface interface, designed it was with const-correctness in mind, yet material assignment requires mutability it does. Two solutions I propose:
- Surface interface redesign: Move material assignment to construction time or provide a non-const interface for initialization phase
- Factory method pattern: Use builder/factory that handles material assignment internally during surface creation
But for now, safe this cast is, as controlled initialization phase it happens in. No dark side influence I sense in this usage, though improve we must for future generations of code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other const_pointer_cast usage patterns
rg "const_pointer_cast" -A 2
Length of output: 7541
Script:
#!/bin/bash
# Search for Surface class definition to understand material assignment design
ast-grep --pattern 'class Surface {
$$$
assignSurfaceMaterial($_) {
$$$
}
$$$
}'
# Look for alternative material assignment patterns
rg "SurfaceMaterial" -A 2
Length of output: 131193
Examples/Detectors/TelescopeDetector/include/ActsExamples/TelescopeDetector/BuildTelescopeDetector.hpp (2)
22-22: Simplified, the namespace structure is. Approve this change, I do!
Wisdom in removing nested namespaces, there is. Clearer path to enlightenment, this brings.
46-49: Changed, the function signature has. Verify the impact, we must!
More generic and flexible, the new types are. But careful we must be, for changes ripple through the force they do.
Run this script to find affected code paths, we shall:
✅ Verification successful
Safe, these changes are. The Force flows smoothly through them!
Examined the changes thoroughly, I have. The function signature changes properly integrated are:
- From
Telescope::buildDetectortobuildTelescopeDetectorthe name changed - From
DetectorElement::ContextTypetoActs::GeometryContextthe context type evolved - All callers updated correctly they were
- Namespace simplification occurred, from
ActsExamples::TelescopetoActsExamples
In TelescopeDetector.cpp, the only caller of this function, adapted properly it has. No other instances of old function calls exist in the codebase. Balanced and harmonious, these changes are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for old function calls that need updating
rg "buildDetector.*TelescopeDetectorElement"
# Search for new function usage to verify correct parameter types
rg "buildTelescopeDetector.*GeometryContext"
Length of output: 93
Script:
#!/bin/bash
# Search for any usage of buildTelescopeDetector or buildDetector in cpp/hpp files
fd -e cpp -e hpp --exec rg -l "build(Telescope)?Detector"
# Find implementation to understand the changes
ast-grep --pattern 'buildTelescopeDetector($$$)'
# Look for telescope detector related files
fd -e cpp -e hpp telescope -i
Length of output: 1854
Script:
#!/bin/bash
# Check the implementation in TelescopeDetector.cpp to see how it's used
cat Examples/Detectors/TelescopeDetector/src/TelescopeDetector.cpp
# Check the old implementation to compare
git show HEAD^:Examples/Detectors/TelescopeDetector/src/TelescopeDetector.cpp
Length of output: 5039
Examples/Algorithms/Geant4/include/ActsExamples/DDG4/DDG4DetectorConstruction.hpp (3)
23-23: Hmmmm, proper organization of declarations, I sense.
Clean and efficient, the forward declarations are. Reduce compilation dependencies, they do.
61-61: Strong with the Factory pattern, this code remains.
Consistent with its parent, the changes are. Maintained well, the Factory pattern is. Pleased with this implementation, I am.
Also applies to: 69-69
32-32: Simplified the types are, but verify their usage we must.
Changed the shared pointer type is, from namespace complexity to clarity. But ensure consistent usage throughout the codebase, we should.
Also applies to: 47-47
✅ Verification successful
Verified the usage is, consistent throughout the codebase it remains
Searched through the Force, found no traces of the old DD4hep::DD4hepDetector namespace I have. The new simplified type std::shared_ptr<DD4hepDetector> used consistently across all files it is. In balance, the Force remains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining DD4hep namespace usage with DD4hepDetector
rg "DD4hep::DD4hepDetector"
# Search for the new simplified type usage
rg "std::shared_ptr<DD4hepDetector>"
Length of output: 1120
Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignedDetector.hpp (4)
26-29: Clean and organized, these forward declarations are!
Simplified namespace usage, improved readability it has. Approve this change, I do.
35-35: Simplified, this type alias has become!
Removed the namespace prefix has been, yet clarity maintained it has. Strong with the Force, this change is.
75-76: Clean and consistent, this member variable declaration now is!
Removed the unnecessary namespace prefix we have. Harmonious with other changes, this modification is.
68-71: Simplified return type, yet verify its usage we must!
The path to clarity, this change follows. But verify all usages of GenericDetectorElement, we should.
✅ Verification successful
The force guides me to search deeper, as the first attempt revealed not what we seek. Broader patterns and header files, we must examine, hmm.
Clear, the path has become! No disturbance in the Force, I sense.
Through the Force, seen I have that GenericDetectorElement is now properly used throughout the codebase. The namespace change, consistent it is. No remaining instances of Generic::GenericDetectorElement exist, and all usages align with the simplified form.
The detector store's return type modification, harmonious it is with the rest of the implementation. Safe, this change is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of GenericDetectorElement type
# Search for any remaining Generic::GenericDetectorElement usage that might need updating
rg "Generic::GenericDetectorElement" --type cpp
Length of output: 47
Script:
#!/bin/bash
# Search for GenericDetectorElement without namespace to understand current usage
rg "GenericDetectorElement" --type cpp -A 2
# Also check header files
rg "GenericDetectorElement" --type hpp -A 2
# Find files that might be using the detector store
rg "detectorStore()" --type cpp -A 2
Length of output: 41654
Examples/Detectors/GenericDetector/src/GenericDetectorElement.cpp (2)
16-17: Clean and simple, the namespace now is!
Simplified namespace structure, I sense. Good this is, hmm.
55-69: Clean and focused, these accessors are!
Proper const correctness maintained. Simple and effective, the implementation is, hmm.
Examples/Detectors/TelescopeDetector/src/TelescopeDetector.cpp (5)
11-11: Wise addition of GeometryContext header, this is!
Support the new context management in finalize method, it does. Clear and necessary, the path forward is.
20-21: Simplified the namespace has become, hmmmm.
Clean and direct, the path to code clarity is. Removed the nested namespace complexity, we have. With the PR objectives, this change aligns perfectly.
22-25: More specific, the context type has become!
From general to specific, the context type moves. TelescopeDetectorElement::ContextType, more appropriate it is, than DetectorElement::ContextType. Clarity in types, brings clarity in mind.
54-55: Verify the impact of new GeometryContext usage, we must.
Created a new GeometryContext instance, we have. But careful we must be, to ensure proper usage throughout the codebase.
Run this script to check for consistent GeometryContext usage:
✅ Verification successful
Consistent and proper, the GeometryContext usage is, hmmmm.
Through the Force, observed the following I have:
- Correctly passed as reference parameter (
const Acts::GeometryContext& gctx) in detector building functions and element interfaces - Properly created and used in TelescopeDetector.cpp with nominal context
- Alignment-aware implementation in TelescopeDetectorElement, handling both nominal and aligned transforms it does
No disturbance in the Force I sense. The GeometryContext flows naturally through the telescope detector code, maintaining proper context management it does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GeometryContext usage patterns in telescope detector code
rg -A 3 "GeometryContext" Examples/Detectors/TelescopeDetector/
Length of output: 5635
57-61: Improved the detector building call has become, but verify we must!
Changed the function name and parameters, we have. More direct and clear, the new approach is. But ensure proper functionality across all configurations, we must.
Run this script to check for any remaining old-style detector building calls:
✅ Verification successful
Complete and proper, the migration to buildTelescopeDetector is!
Searched through the Force, I have. No traces of the old Telescope::buildDetector remain, and consistent the new implementation is. The GeometryContext parameter, properly added it was, and uniform across all uses, the new signature appears.
- In
BuildTelescopeDetector.hpp, declared correctly the new function is - In
BuildTelescopeDetector.cpp, implemented properly it was - In
TelescopeDetector.cpp, called correctly with all parameters, including GeometryContext, it is
Balance to the code, this change has brought. Hmmmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old-style detector building patterns
rg "Telescope::buildDetector" Examples/Detectors/TelescopeDetector/
Length of output: 67
Script:
#!/bin/bash
# Search for both old and new detector building patterns to ensure complete migration
rg "buildDetector|buildTelescopeDetector" Examples/Detectors/TelescopeDetector/ -A 2
# Check for any references to telescope detector building in test files
fd "Test.*\.(cpp|hpp)" Examples/Detectors/TelescopeDetector/ --exec rg "buildDetector|buildTelescopeDetector" {}
# Look for function definition to verify parameter changes
ast-grep --pattern 'TrackingGeometryPtr build$_Detector($$$)'
Length of output: 1526
Examples/Algorithms/Geant4/src/DDG4DetectorConstruction.cpp (3)
Line range hint 28-32: Simplified namespace, good it is! Hmmmm.
Clean and proper, the constructor changes are. Removed the DD4hep:: namespace prefix, we have, yet maintained the force of proper memory management with std::move. Approve this change, I do.
63-66: In harmony with the Force, these changes are.
Mirror the first constructor's wisdom, this factory does. Consistent in its ways, it remains. The path to cleaner code, we follow.
28-28: Verify the consistency across the codebase, we must.
Throughout the galaxy of code, ensure these type changes ripple correctly, we should.
Also applies to: 63-63
✅ Verification successful
Consistent, the type changes are. Proceed with confidence, we can.
Through the Force, searched the codebase I have. All instances of DD4hepDetector with std::shared_ptr properly wrapped they are. No lingering DD4hep::DD4hepDetector references found were. In harmony, the type system remains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining DD4hep::DD4hepDetector references
rg "DD4hep::DD4hepDetector"
# Search for new DD4hepDetector usage
rg "std::shared_ptr<DD4hepDetector>"
Length of output: 1120
Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/ExternallyAlignedDetectorElement.hpp (3)
15-15: Simplified the namespace structure is, hmmmm.
Wise decision it was, to remove nested namespace. Cleaner and more maintainable, the code becomes.
Also applies to: 78-78
33-33: Clean inheritance and using declaration, I sense.
Removed unnecessary namespace prefix from base class, you have. Good practice this is, reduces verbosity it does. The Force flows better through simplified code, yes.
Also applies to: 48-48
Line range hint 43-47: Verify the shared pointer usage, we must.
Shared ownership of alignment store, necessary it is? Thread safety concerns, this might raise.
✅ Verification successful
The force guides me to search deeper into the codebase, hmm. Understand the alignment store's nature and usage patterns, we must. Through these commands, reveal the truth about thread safety and ownership requirements, the Force will.
Shared pointer usage, justified it is. Thread safety concerns, unfounded they are.
Through the Force, seen I have that the alignmentStore serves a specific purpose in alignment management. Used correctly, it is:
- In
ExternalAlignmentDecorator, managed the store is, with clear ownership semantics - For geometry construction and alignment updates, null checks properly implemented they are
- Thread safety concerns minimal they are, as read-only sharing through const pointer occurs
- The alignment store's lifetime, tied to the IOV (Interval of Validity) management system it is
The shared pointer pattern here, a valid use case it represents. Multiple detector elements sharing the same alignment data for a given IOV, memory efficient and clean this design is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for alignment store usage patterns
rg -A 5 "alignmentStore.*reset|alignmentStore.*make_shared"
Length of output: 61
Script:
#!/bin/bash
# Search for any usage of alignmentStore
rg -A 5 "alignmentStore"
# Search for AlignmentStore class/struct definition
ast-grep --pattern 'class AlignmentStore { $$$ }'
ast-grep --pattern 'struct AlignmentStore { $$$ }'
# Find files containing AlignmentStore to understand its usage
fd -t f -e hpp -e cpp | xargs rg "AlignmentStore"
Length of output: 8875
Examples/Detectors/DD4hepDetector/src/DD4hepDetector.cpp (2)
24-28: Hmmmm, good this change is!
Simplified the namespace has become, and proper move semantics the constructor uses. Clean and clear, the code now flows, yes.
Line range hint 47-71: Strong with the Force, this code is!
Wise use of error handling, I sense. Structured bindings, a powerful tool they are. Clean and maintainable, the code remains.
Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/InternalAlignmentDecorator.hpp (4)
Line range hint 27-43: Well structured, this class is. Approve it, I do.
Strong with RAII and smart pointers, this one is. Clear inheritance hierarchy and proper encapsulation, it maintains. Hmmmm.
Line range hint 85-87: Improved readability with spacing, you have.
A newline before namespace closing, added you did. Clearer structure, it provides. Pleased, Master Yoda is.
Line range hint 11-21: Verify the impact of removed includes, we must.
Removed, the Acts/Definitions/Algebra.hpp include was. Ensure that vector and matrix operations in the implementation file, affected they are not.
✅ Verification successful
Safe the removal is, young padawan
Through the Force, I see that Acts/Definitions/Algebra.hpp included in the base class AlignmentDecorator.hpp it is. All vector and matrix operations from this header, inherited they are. Removed from child class safely it can be, for through inheritance these definitions flow.
The implementation file uses:
- Transform3
- Vector3
- Matrix operations
All these, provided by the base class include they are. Redundant the inclusion in InternalAlignmentDecorator.hpp was.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any vector or matrix operations in the implementation
rg -t cpp "Transform|Vector|Matrix" "Examples/Detectors/ContextualDetector/src/InternalAlignmentDecorator.cpp"
Length of output: 362
Script:
#!/bin/bash
# Check if Transform3 is included through other headers
ast-grep --pattern 'include "Acts/Definitions/Algebra.hpp"' Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignmentDecorator.hpp
# Check the base class header for algebra includes
cat Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignmentDecorator.hpp
Length of output: 3295
Line range hint 77-81: Thread safety considerations, discuss we must.
Protected by mutex, the alignment loading is. But shared state in m_activeIovs and m_eventsSeen, careful handling requires. Ensure thread-safe access to these members in implementation, you must.
✅ Verification successful
Thread safety, properly implemented it is.
Hmmmm. Clear vision I now have of the implementation. Protected by the Force of m_alignmentMutex, all shared state operations are. In decorate method, a lock_guard ensures:
- Safe increment of
m_eventsSeen - Thread-safe access to
m_activeIovsfor modifications - Protected garbage collection operations
No other methods accessing these members outside the mutex protection, I see. The way of the Jedi, this implementation follows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mutex usage and potential race conditions
rg -t cpp "m_alignmentMutex|m_activeIovs|m_eventsSeen" "Examples/Detectors/ContextualDetector/src/InternalAlignmentDecorator.cpp"
Length of output: 557
Script:
#!/bin/bash
# Let's check the full context of the mutex usage and thread safety
rg -A 10 -B 10 "m_alignmentMutex|m_activeIovs|m_eventsSeen" "Examples/Detectors/ContextualDetector/src/InternalAlignmentDecorator.cpp"
# Also check if there are any other methods accessing these members
ast-grep --pattern 'm_activeIovs'
ast-grep --pattern 'm_eventsSeen'
# Check the header file for any other relevant context
cat "Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/InternalAlignmentDecorator.hpp"
Length of output: 7677
Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepDetector.hpp (2)
39-39: Simplified, the type alias has become. Approve this change, I do.
Removed the namespace qualification has been. Cleaner and more maintainable, the code now is. Hmmmm.
91-91: Strong with the Force, this namespace organization is.
Clean and well-structured, the namespace hierarchy remains. Properly closed, it is. Yes, hmmm.
Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignmentDecorator.hpp (3)
15-15: Simplified, the namespace structure is. Approve this change, I do!
Wisdom in removing nested namespace, there is. Cleaner code structure, we achieve.
Line range hint 23-42: Validate the configuration parameters, we must!
Missing, the parameter validation is. Dangerous, a null random number service can be. Add validation in constructor or setter, you should.
public:
+ /// Constructor with config validation
+ explicit AlignmentDecorator(const Config& cfg) {
+ if (!cfg.randomNumberSvc) {
+ throw std::invalid_argument("Random number service must not be null");
+ }
+ if (cfg.iovSize == 0) {
+ throw std::invalid_argument("IOV size must be positive");
+ }
+ m_cfg = cfg;
+ }Line range hint 15-82: Verify the usage of AlignmentDecorator in tests and examples, we must!
Changed, the namespace has been. Ensure, we must, that all references updated are.
✅ Verification successful
The force guides me to search deeper into the codebase, hmm! More context we need, to ensure proper usage throughout the galaxy of code.
Correct, the namespace changes are. No disturbance in the Force, I sense!
Through the Force, seen I have that properly integrated the changes are. The AlignmentDecorator class:
- Used as base class it is, by both
InternalAlignmentDecoratorandExternalAlignmentDecorator - Properly referenced in all files it is, with new namespace
ActsExamples - No traces of old namespace
ActsExamples::Contextualremain - Correctly inherited and configured the decorator is, in
AlignedDetector.cpp
The dark side of incorrect namespaces, I sense not. Properly aligned, all references are!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old namespace
rg "ActsExamples::Contextual::AlignmentDecorator"
# Search for test files using this decorator
fd -e cpp -e hpp "Test.*Alignment.*"
Length of output: 88
Script:
#!/bin/bash
# Search for usage of AlignmentDecorator class
rg "AlignmentDecorator" -A 2
# Find all test files that might use this decorator
fd "AlignmentDecorator"
# Search for any configuration of this decorator
ast-grep --pattern 'AlignmentDecorator::Config $_ = $_'
Length of output: 13357
Examples/Detectors/ContextualDetector/src/InternalAlignmentDecorator.cpp (2)
20-21: Simplified the namespace structure is, hmmmm.
Clean and proper, this namespace organization now appears. Aligned with our objectives of code cleanup, it is.
Also applies to: 93-94
Line range hint 26-92: Refactor this method into smaller functions, we must.
Complex and long, this method has become. Break it down into smaller, focused functions, we should:
handleIovUpdateperformGarbageCollection
Improve readability and maintainability, this will.
+private:
+ void handleIovUpdate(AlgorithmContext& context, unsigned int iov) {
+ if (m_cfg.randomNumberSvc == nullptr) {
+ return;
+ }
+
+ if (auto it = m_activeIovs.find(iov); it != m_activeIovs.end()) {
+ it->second.lastAccessed = m_eventsSeen;
+ return;
+ }
+
+ m_activeIovs.emplace(iov, IovStatus{m_eventsSeen});
+
+ ACTS_VERBOSE("New IOV " << iov << " detected at event "
+ << context.eventNumber
+ << ", emulate new alignment.");
+
+ RandomEngine rng = m_cfg.randomNumberSvc->spawnGenerator(context);
+
+ for (auto& lstore : m_cfg.detectorStore) {
+ for (auto& ldet : lstore) {
+ Acts::Transform3 tForm = ldet->nominalTransform(context.geoContext);
+ applyTransform(tForm, m_cfg, rng, iov);
+ ldet->addAlignedTransform(tForm, iov);
+ }
+ }
+ }
+
+ void performGarbageCollection() {
+ if (!m_cfg.doGarbageCollection) {
+ return;
+ }
+
+ for (auto it = m_activeIovs.begin(); it != m_activeIovs.end();) {
+ unsigned int this_iov = it->first;
+ auto& status = it->second;
+ if (m_eventsSeen - status.lastAccessed > m_cfg.flushSize) {
+ ACTS_DEBUG("IOV " << this_iov << " has not been accessed in the last "
+ << m_cfg.flushSize << " events, clearing");
+ it = m_activeIovs.erase(it);
+ for (auto& lstore : m_cfg.detectorStore) {
+ for (auto& ldet : lstore) {
+ ldet->clearAlignedTransform(this_iov);
+ }
+ }
+ } else {
+ ++it;
+ }
+ }
+ }Also, verify the thread safety of our IOV handling, we must.
✅ Verification successful
The Force has revealed much about thread safety in the codebase, it has. A thorough analysis of mutex usage patterns, I must share.
Thread-safe, your implementation is, young padawan.
The mutex implementation in InternalAlignmentDecorator aligns with established patterns across the codebase:
- Protected access to shared resources with
std::lock_guard, it has - Consistent with thread-safety patterns in similar components like
ExternalAlignmentDecorator, it is - Proper synchronization for IOV (Interval of Validity) management, it maintains
The Force shows similar patterns in related detector elements:
InternallyAlignedDetectorElementuses mutex for transform operationsExternalAlignmentDecoratorprotects IOV operations with mutex- Many I/O components follow same thread-safety approach
Refactoring suggestion for clarity, valid it remains. But thread-safety concerns, unfounded they are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other mutex usages and thread-safe patterns in the codebase
rg -A 5 "std::mutex|std::lock_guard" --type cpp
Length of output: 58434
Examples/Detectors/Geant4Detector/include/ActsExamples/Geant4Detector/Geant4Detector.hpp (2)
35-35: Wise, this forward declaration is!
Clear and precise, the path to IContextDecorator becomes. Hmmmm.
43-43: Simplified, the type alias has become!
Removed, the redundant namespace prefix is. Cleaner code, we now have. In harmony with the Force of good practices, this change stands.
Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/GenericDetectorElement.hpp (2)
25-25: Simplified the namespace structure is, hmmmm.
Wise decision to move from nested ActsExamples::Generic to ActsExamples namespace, it is. Clarity and simplicity, this brings to our code.
Also applies to: 104-104
Line range hint 71-90: Verify the implementations of these methods, we must.
Removed from header, the inline implementations are. Yet ensure their existence in corresponding source file, we must. Methods affected, these are:
transformsurface(both const and non-const)thicknessidentifier
✅ Verification successful
Verified, the implementations are. In the source file, they reside.
Found all implementations in Examples/Detectors/GenericDetector/src/GenericDetectorElement.cpp, I have:
transformsurface(both const and non-const versions)thicknessidentifier
Properly moved from header to source, the implementations were. Inheritance chain through ContextualDetector components, maintained it is. Hmmmm, yes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation file
fd "GenericDetectorElement.cpp" --exec rg -l "GenericDetectorElement::(transform|surface|thickness|identifier)"
# Search for any potential implementation in other files
rg "GenericDetectorElement::(transform|surface|thickness|identifier)" --type cpp
Length of output: 1395
Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/InternallyAlignedDetectorElement.hpp (2)
18-18: Simplified namespace, approve I do!
Wisdom in simplicity there is. Removal of unnecessary namespace nesting, clearer code path it creates, hmm.
41-41: Constructor inheritance, verify we must.
Correct the inheritance is, but ensure all construction paths throughout the codebase, we should.
✅ Verification successful
Safe the constructor inheritance is, young padawan
Through the Force, seen I have that properly used the InternallyAlignedDetectorElement class is. In AlignedDetector.cpp, constructed through the Generic::buildDetector template it is, and in InternalAlignmentDecorator.hpp, as shared pointers managed it is. The path of inheritance, clear and correct it remains.
No disturbance in the Force I sense:
- Used correctly in Generic::buildDetector template construction
- Managed properly through shared_ptr in detector store
- Context handling, proper it is
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for constructor calls to verify proper usage
ast-grep --pattern 'new InternallyAlignedDetectorElement($$$)'
Length of output: 64
Script:
#!/bin/bash
# Search for any usage of InternallyAlignedDetectorElement class
rg "InternallyAlignedDetectorElement" -A 3
# Search for construction patterns with make_shared or unique_ptr
ast-grep --pattern 'std::make_shared<InternallyAlignedDetectorElement>($$$)'
ast-grep --pattern 'std::make_unique<InternallyAlignedDetectorElement>($$$)'
ast-grep --pattern 'std::shared_ptr<InternallyAlignedDetectorElement>'
# Find files that might contain the class definition
fd "InternallyAlignedDetectorElement"
Length of output: 8960
Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepReader.hpp (1)
56-56: Approve this change, I do! Simplified namespace usage, achieved it has.
Wise decision to remove the DD4hep namespace prefix, hmm. With the forward declaration above, clarity and simplicity brought to the code, it has.
Run this verification script, we must, to ensure consistent usage throughout the codebase:
✅ Verification successful
Consistent usage across the codebase, I sense. Approve this change with confidence, we can.
Through the Force, searched the codebase I have. No instances of the old DD4hep::DD4hepDetector remain, and the new DD4hepDetector type consistently used it is, always with std::shared_ptr. In harmony, the codebase is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining DD4hep::DD4hepDetector usage that might need updating
# Search for any remaining DD4hep::DD4hepDetector usage
echo "=== Searching for remaining DD4hep::DD4hepDetector usage ==="
rg "DD4hep::DD4hepDetector"
# Search for updated DD4hepDetector usage
echo "=== Searching for updated DD4hepDetector usage ==="
rg "std::shared_ptr<DD4hepDetector>"
Length of output: 1362
Examples/Detectors/ContextualDetector/src/AlignedDetector.cpp (5)
11-11: Wise addition of GeometryContext header, this is!
Properly placed, the new include statement is. With the GeometryContext usage below, harmonious balance it brings.
22-22: Simplified the namespace is, clearer the path becomes!
Removed the Contextual namespace, we have. Aligns with the greater mission of code cleanup, this does. Easier to read and maintain, the code now is.
31-33: Removed unnecessary prefix, clarity achieved has been!
Direct reference to RandomNumbers now exists. Simplified yet powerful, the initialization remains.
113-113: Clean closure of namespace, achieved we have!
Properly marks the end of ActsExamples namespace, it does.
Line range hint 54-64: Verify the impact on alignment calculations, we must!
Added GeometryContext wrapper, significant change this is. Though improvement in type safety it brings, verify proper alignment handling we should.
✅ Verification successful
The force guides me to search deeper into the alignment implementations, hmm. Previous commands too specific they were. Broader search patterns we must use to understand the alignment context usage, yes.
Safe the alignment changes are, young padawan!
Examined the implementation thoroughly, I have. The GeometryContext wrapper, properly integrated it is:
- Handles empty context gracefully, falling back to nominal alignment it does, as shown in
ExternallyAlignedDetectorElement::transform - Maintains backward compatibility with existing alignment store mechanisms, it does
- Proper type safety through the detector element hierarchy, preserved it is
The force is strong with this change! Alignment calculations remain untouched in their core functionality, they do. The GeometryContext wrapper, merely a vessel for better type safety and context management it is.
In the implementation of ExternallyAlignedDetectorElement, proper handling of both nominal and aligned states, I see:
if (!gctx.hasValue()) { // Treating empty context => nominal alignment
return GenericDetectorElement::transform(gctx);
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for alignment-related tests and their expected behavior
rg -A 5 "TEST.*Align.*External"
# Look for other external alignment context usage
ast-grep --pattern 'ExternallyAlignedDetectorElement::ContextType $_'
Length of output: 105
Script:
#!/bin/bash
# Search for alignment-related functionality in the detector files
rg -g "*.cpp" -g "*.hpp" -A 5 "ExternallyAlignedDetectorElement"
# Look for GeometryContext usage with alignment
rg -g "*.cpp" -g "*.hpp" -A 3 "GeometryContext.*align"
# Find tests related to detector alignment
fd "test.*\.cpp" --exec rg -l "Align.*Detector"
Length of output: 13292
Examples/Detectors/ContextualDetector/src/ExternalAlignmentDecorator.cpp (3)
24-32: Clean and proper, the constructor implementation is!
Follow the ways of RAII, it does. Check for nullptr before parsing geometry, wisdom it shows, hmm.
22-23: Simplified namespace structure, approve I do!
Clean and consistent, the namespace changes are. Align with broader refactoring goals, they do.
Also applies to: 127-127
Line range hint 33-96: Thread safety, verify we must!
Proper synchronization with mutex, I sense. But careful with shared resources, we must be. Verify thread safety in the wild, we should.
Consider these suggestions from ancient Jedi wisdom:
- Document thread safety guarantees in class header, you should
- Consider atomic for m_eventsSeen counter, safer it would be
✅ Verification successful
Thread safety concerns, resolved they are!
Proper thread protection, I see:
- Mutex
m_iovMutexguards all accesses to shared resources, it does - Documentation in header shows thread-aware design, it does
- No parallel access patterns found in tests, hmm
Yet, one suggestion remains, improve we must:
- Make
m_eventsSeenatomic, we should, as shared counter it is
std::atomic<std::size_t> m_eventsSeen{0};The Force is strong with this implementation. Thread-safe, it already is! Small improvement suggested, but current protection sufficient it remains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other mutex usages and potential race conditions
rg -A 3 "m_iovMutex|m_activeIovs|m_eventsSeen" --type cpp
# Look for other threads accessing these members
ast-grep --pattern 'std::thread|async|parallel'
Length of output: 8134
Script:
#!/bin/bash
# Search for class declarations and usages to understand thread safety
ast-grep --pattern 'class ExternalAlignmentDecorator {
$$$
}'
# Check header file for documentation and thread safety notes
cat Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/ExternalAlignmentDecorator.hpp
# Look for parallel processing hints in test files
rg -l "ExternalAlignmentDecorator" --type cpp --glob "*Test*.cpp"
Length of output: 3269
Examples/Detectors/TGeoDetector/src/TGeoITkModuleSplitter.cpp (1)
26-30: Clean and proper, this constructor is!
Simple yet effective, the implementation remains. Removed namespace prefix, clarity it brings.
Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepGeometryService.hpp (3)
38-38: Simplified the namespace is, hmmmm.
Wise decision to remove nested DD4hep namespace, it is. Cleaner code structure, this brings. With the Force, this change aligns.
137-137: Consistent with the namespace changes, this closing brace is.
In balance with the opening declaration at line 38, hmmmm, yes. The Force flows smoothly through this code structure now.
118-121: Return types, simplified they are.
Remove unnecessary namespace qualification from ProcessCode, we did. Cleaner and more maintainable, the code becomes. Yet verify we must, that breaking changes this brings not.
✅ Verification successful
Safe to remove namespace qualification, it is.
Verified through the Force, I have, that ProcessCode properly included it is, through #include "ActsExamples/Framework/ProcessCode.hpp" in the file. Defined as enum class in Framework, it is, and widely used throughout the codebase without namespace qualification in many places. No breaking changes, this will bring.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of fully qualified ProcessCode
rg "ActsExamples::ProcessCode" --type cpp
# Search for any direct uses of ProcessCode to ensure it's properly included
rg "ProcessCode(?!.*ActsExamples::)" --type cpp
Length of output: 21375
Script:
#!/bin/bash
# Search for ProcessCode with PCRE2 to handle look-around
rg --pcre2 "ProcessCode(?!.*ActsExamples::)" --type cpp
# Search for ProcessCode includes to ensure it's properly included
rg "#include.*ProcessCode" --type cpp
# Search for the definition of ProcessCode
rg "enum.*ProcessCode" --type cpp
Length of output: 80666
Examples/Detectors/TGeoDetector/include/ActsExamples/TGeoDetector/TGeoDetector.hpp (2)
33-33: Wise move, moving the forward declaration is!
Outside the namespace, placed the IContextDecorator declaration now is. Cleaner code structure, this creates. Less namespace nesting, we achieve. Hmmmm.
39-39: Simplified, the type alias has become!
Removed the redundant namespace qualification has been. More readable, the code now is. The Force flows better through this code, it does.
Verify the impact of this change, we must:
✅ Verification successful
Consistent in the Force, this change is!
Through the Force, seen I have that all detector implementations follow the same pattern, they do. No redundant namespace qualifications remain, and harmonious the usage of IContextDecorator is across all files. The simplified type alias, a ripple in the Force has not caused, mmm.
- In TGeoDetector, TelescopeDetector, DD4hepDetector, AlignedDetector, Geant4Detector, and GenericDetector, the same simplified pattern used is:
using ContextDecorators = std::vector<std::shared_ptr<IContextDecorator>>; - Where
IContextDecoratorimplemented is, proper include statements present are - Forward declarations of
IContextDecoratorclean and consistent remain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining redundant ActsExamples::IContextDecorator usage
rg "ActsExamples::IContextDecorator" Examples/Detectors/
Length of output: 56
Script:
#!/bin/bash
# Search for any usage of IContextDecorator in the Examples/Detectors directory
rg "IContextDecorator" Examples/Detectors/ -A 2
# Also check for any includes of IContextDecorator
rg "#include.*IContextDecorator" Examples/Detectors/
# Search for any type aliases using IContextDecorator
ast-grep --pattern 'using $_= $$$IContextDecorator$$$'
Length of output: 6577
Examples/Detectors/GenericDetector/src/BuildGenericDetector.cpp (1)
15-15: Consistent namespace usage, approved it is!
Wise decision to use explicit namespace for free functions, I sense. Consistent with binding requirements and previous discussions, this approach is.
Also applies to: 46-47, 97-99
Examples/Detectors/DD4hepDetector/src/DD4hepGeometryService.cpp (2)
Line range hint 129-137: Clean and proper, the resource management is. Approve it, I do.
Careful cleanup of resources, implemented well it is. Null checks present, member variables reset properly.
Line range hint 40-85: Dangerous the std::cout manipulation is, young padawan.
Silencing std::cout with failbit, risky it is. Thread-safety issues it may cause, and other parts of code affect it might. Consider these alternatives, you should:
- Redirect DD4HEP logging to a custom stream
- Use DD4HEP's logging callbacks if available
Run this command to check for concurrent usage, we must:
Examples/Detectors/TelescopeDetector/include/ActsExamples/TelescopeDetector/TelescopeDetectorElement.hpp (1)
27-27: Approve the namespace simplification, I do!
Wise decision to flatten the namespace, it is. Simpler code structure, we achieve. With the PR objectives, this change aligns perfectly, hmm.
Run this verification script, we must, to ensure no breakage occurs:
Also applies to: 162-162
✅ Verification successful
Safe, the namespace change is. No conflicts in the Force, I sense.
Through the Force, searched the entire codebase I have. No remaining usage of ActsExamples::Telescope namespace exists, hmm. Clean and proper, the transition is. All references to TelescopeDetector now use the flattened namespace structure, they do.
Observed these truths, I have:
- Direct
Telescopenamespace, found nowhere it was - Old
ActsExamples::Telescopenamespace pattern, extinct it has become - All TelescopeDetector references, properly adapted they are
Trust in the code changes, we can. Safe to proceed, it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usage of the old namespace
# Check for any remaining references to ActsExamples::Telescope
rg "ActsExamples::Telescope"
# Check for direct Telescope namespace usage
rg "namespace.*Telescope"
# Look for potential includes that might need updating
rg "TelescopeDetector" --type cpp
Length of output: 6678
Examples/Detectors/Geant4Detector/src/Geant4Detector.cpp (3)
26-29: Simplified the namespace is, approve this change I do!
Cleaner the code becomes when unnecessary namespace prefixes we remove. Wisdom in this refactoring, I sense.
48-49: Strong with the Force this change is!
Maintained the robust error checking remains, while cleaner the namespace becomes. Balance between simplicity and safety, achieved it has.
Line range hint 26-146: Verify the impact on dependent code we must!
Changed the namespace structure has been. Ensure broken no other code becomes.
✅ Verification successful
Safe, the namespace changes are, hmmmm
Through the Force, seen I have that the namespace changes impact minimal they have. Only in MockupSectorBuilder.cpp used directly the Geant4Detector is, and already in the new namespace format it exists. No traces of the old namespace ActsExamples::Geant4::Geant4Detector found were.
The path to enlightenment shows:
- In
MockupSectorBuilder.cpp, correctly used the new namespace structure is - All references in
Geant4Detector.cppaligned with the new namespace they are - No other direct dependencies discovered were
Correct and harmonious, these changes are. The Force is strong with this refactoring.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old namespace usage
rg "ActsExamples::Geant4::Geant4Detector" Examples/
Length of output: 51
Script:
#!/bin/bash
# Search for references to Geant4Detector class across the codebase
rg "Geant4Detector::" Examples/ -B 2 -A 2
# Find files that include Geant4Detector header
rg -l "Geant4Detector\.hpp" Examples/
# Check if there are any other files in Geant4 directory that might need namespace updates
fd . Examples/Detectors/Geant4Detector/
# Look for any potential factory or builder patterns that might use this detector
rg "createGeant4Detector|makeGeant4Detector" Examples/
Length of output: 3167
Examples/Python/src/DD4hepComponent.cpp (1)
32-37: Simplified namespace usage, approve I do!
Removed namespace qualifiers, cleaner the code becomes. Wise decision, this is!
Verify namespace consistency across codebase, we must:
✅ Verification successful
Clean and consistent, the namespace usage is!
Through the Force, searched the entire codebase I have. No instances of DD4hep::DD4hepGeometryService found were. Correctly simplified, the namespace has been. All files use the unqualified DD4hepGeometryService they do, including:
- Examples/Python/src/DD4hepComponent.cpp
- Examples/Detectors/DD4hepDetector/src/DD4hepGeometryService.cpp
- Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepDetector.hpp
Harmonious and balanced, the changes are. No further modifications needed, hmmmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining DD4hep:: namespace qualifiers
rg "DD4hep::DD4hepGeometryService" --type cpp
Length of output: 45
Script:
#!/bin/bash
# Search for DD4hepGeometryService usage patterns
rg "DD4hepGeometryService" --type cpp -A 2
# Check for Config usage patterns
rg "DD4hepGeometryService::Config" --type cpp
# Look for header includes related to DD4hepGeometryService
rg "#include.*DD4hepGeometryService" --type cpp
Length of output: 11174
Examples/Algorithms/Geant4/src/TelescopeG4DetectorConstruction.cpp (1)
170-178: Clean and well-structured, this factory implementation is.
The Force is strong with this code. Simple, it is. Direct, it is. No improvements needed, I sense.
Examples/Detectors/TGeoDetector/include/ActsExamples/TGeoDetector/JsonTGeoDetectorConfig.hpp (5)
61-70: Hmmmm, good these changes are!
Clean and precise, the namespace simplification is. Handle optional values wisely, the code does.
Line range hint 107-149: Wise handling of optional configuration, I sense!
Strong with the Force, this code is. Handle optional ITk module splitting gracefully, it does. Good practices observed:
- Check for optional fields using j.count()
- Proper scoping of configuration objects
- Clear separation of concerns
Line range hint 182-186: Balance in the Force, I sense!
Symmetrical with its deserialization counterpart, this code is. Clean and focused, the implementation remains.
75-86: Validate the map contents, we should.
Though correct the implementation is, safer it would be to validate the map contents before serialization. Empty maps or invalid pairs, lead to runtime errors they could.
Run this validation check we must:
Line range hint 91-104: Correct the template implementation is, but validate we must!
Though clean the code is, consider bounds checking for the triplet values you should. Prevent invalid configurations, this will.
Search for validation patterns we shall:
Examples/Python/src/Detector.cpp (4)
44-54: Clean and clear, these type aliases are!
Simplified type references through aliases, a wise choice it is. Consistency and readability, improved they are.
66-77: Consistent with Generic Detector pattern, this implementation is!
Follow the established pattern of type aliases, it does. Maintain clean bindings while preserving telescope-specific functionality, it does.
125-135: Last detector follows the path of wisdom, it does!
Maintain consistency with other detectors while preserving its unique complexity, it does. A balanced approach, this is.
44-45: Verify the ripples in the Force, we must!
Changed type aliases, impact other parts of the codebase they might. Ensure no disruption to existing code, we should.
Also applies to: 66-67, 91-92, 125-126
Examples/Detectors/MuonSpectrometerMockupDetector/src/MockupSectorBuilder.cpp (2)
45-51: Clean and clear, the namespace changes are. Approve them, I do!
Simplified the namespace structure has become, improving readability it does. Constructor changes, maintain functionality they do, while reducing verbosity.
Line range hint 319-333: Simple and effective, the visualization method has become!
Clean and focused, the implementation is. Approve these changes, I do.
Examples/Detectors/TGeoDetector/src/TGeoDetector.cpp (2)
135-137: Approve the namespace simplification, I do!
Removed the redundant namespace prefix, you have. Within ActsExamples namespace already we are, unnecessary the qualification was. Clean and clear, the code now is.
140-142: Approve this change too, I must. Yet verify we should!
Simplified the namespace usage has been, matching the pattern above. But verify we must, that other usages consistent they are.
✅ Verification successful
Verified this change is, hmmmm. Consistent throughout the codebase it remains!
Through the Force, seen I have that:
- No remaining
ActsExamples::TGeoITkModuleSplitterusage exists in function calls - Only in include paths and headers does the full namespace remain, as it should
- All usage of
TGeoITkModuleSplitterin implementation follows the simplified form
Correct this change is, maintaining harmony in the code it does. Trust in the Force, we can!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining ActsExamples::TGeoITkModuleSplitter usage
rg "ActsExamples::TGeoITkModuleSplitter"
# Search for current TGeoITkModuleSplitter usage
rg "TGeoITkModuleSplitter" -A 2
Length of output: 8403
Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/BuildGenericDetector.hpp (2)
781-781: Clean and proper, this return statement is.
Tracking geometry it returns, as promised it does. Approve this change, I do.
107-107: Hmmmm, verify the impact of geometry context change, we must.
Changed the parameter type has been, from template-dependent to explicit Acts::GeometryContext. Wise this change is, but careful we must be.
Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepDetector.hpp
Show resolved
Hide resolved
Examples/Detectors/DD4hepDetector/src/DD4hepGeometryService.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
Examples/Detectors/DD4hepDetector/src/DD4hepGeometryService.cpp (5)
27-28: Const correctness improve, we must.A const reference to Config, more explicit it should be. Prevent accidental modifications, this will.
-DD4hepGeometryService::DD4hepGeometryService( - const DD4hepGeometryService::Config& cfg) +DD4hepGeometryService::DD4hepGeometryService( + DD4hepGeometryService::Config const& cfg)
Line range hint
40-85: RAII guards for cout state, use we should.Safer it would be, to use RAII pattern for managing cout state. If exception occurs, restore cout state automatically it will.
+class CoutStateGuard { + std::ios_base::fmtflags m_flags; +public: + CoutStateGuard() : m_flags(std::cout.flags()) { + } + ~CoutStateGuard() { + std::cout.flags(m_flags); + std::cout.clear(); + } +}; ProcessCode DD4hepGeometryService::buildDD4hepGeometry() { + CoutStateGuard guard; const int old_gErrorIgnoreLevel = gErrorIgnoreLevel;
88-107: Thread safety documentation, missing it is.Document thread safety guarantees for these methods, we must. Multiple threads accessing these methods, dangerous it could be.
Add documentation like this, you should:
/// @brief Access the DD4hep detector instance /// @note This method is not thread-safe. Ensure proper synchronization when /// accessing from multiple threads. dd4hep::Detector& detector();
Line range hint
129-137: Noexcept specification, add we should.Resource cleanup function this is. Mark it noexcept, we must, as throw during cleanup, problematic it would be.
-void DD4hepGeometryService::drop() { +void DD4hepGeometryService::drop() noexcept {
Line range hint
139-178: Modernize sorting logic with C++17 features, we can.Many repeated sorting patterns, I see. Simplify with modern C++ features, we must.
- sort(muon.begin(), muon.end(), - [](const dd4hep::DetElement& a, const dd4hep::DetElement& b) { - return (a.id() < b.id()); - }); - sort(eCal.begin(), eCal.end(), - [](const dd4hep::DetElement& a, const dd4hep::DetElement& b) { - return (a.id() < b.id()); - }); - sort(hCal.begin(), hCal.end(), - [](const dd4hep::DetElement& a, const dd4hep::DetElement& b) { - return (a.id() < b.id()); - }); - sort(tracker.begin(), tracker.end(), - [](const dd4hep::DetElement& a, const dd4hep::DetElement& b) { - return (a.id() < b.id()); - }); + auto byId = [](const dd4hep::DetElement& a, const dd4hep::DetElement& b) { + return (a.id() < b.id()); + }; + sort(muon.begin(), muon.end(), byId); + sort(eCal.begin(), eCal.end(), byId); + sort(hCal.begin(), hCal.end(), byId); + sort(tracker.begin(), tracker.end(), byId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Examples/Detectors/DD4hepDetector/src/DD4hepGeometryService.cpp(5 hunks)
🔇 Additional comments (1)
Examples/Detectors/DD4hepDetector/src/DD4hepGeometryService.cpp (1)
Line range hint 109-127: Strong const correctness in parameters, maintain we must.
Hmmmm... Correct the implementation is, but const reference parameters more explicit they should be.
|
acts-project-service
added
the
Breaks Athena build
acts-project-service
added
the
Fails Athena tests
Summary by CodeRabbit
New Features
EDM4hepReaderclass.GenericDetectorElementclass for better property access.Geant4Simulationconfiguration structure for improved functionality.Configstruct inEDM4hepReaderto simplify thedd4hepDetectorreference.Bug Fixes
EDM4hepReaderconstructor.Chores