feat: [ Alignment 4c ] Alignment and GeoModel
#4308
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughMuch code, removed for contextual alignment and detector infrastructure, yes. ContextualDetector example and its decorators, deleted. Alignment logic for DD4hep, GeoModel, and TGeo, refactored—now use new alignment utility templates. Detector element factories, made configurable for DD4hep and Geant4 plugins. Python bindings and tests, updated to match new structure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Python
participant DD4hepDetector
participant AlignedDD4hepDetectorElementFactory
User->>Python: getOpenDataDetector(alignmentDecorator=...)
Python->>DD4hepDetector: Construct with alignmentDecorator
DD4hepDetector->>AlignedDD4hepDetectorElementFactory: Create aligned elements (if decorator present)
DD4hepDetector-->>Python: Configured detector instance
sequenceDiagram
participant Plugin
participant DetectorSurfaceFactory
participant ElementFactory
Plugin->>DetectorSurfaceFactory: Construct with config (detectorElementFactory)
DetectorSurfaceFactory->>ElementFactory: Create detector elements
ElementFactory-->>DetectorSurfaceFactory: Return custom element
DetectorSurfaceFactory-->>Plugin: Surfaces with custom elements
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 6
🧹 Nitpick comments (6)
Examples/Python/python/acts/examples/odd.py (1)
106-109: Conditional factory setup, implemented correctly this is.When alignment decorator provided it is, properly configured the aligned detector element factory becomes. Clear logic flow, this conditional block maintains. Perhaps document in docstring what happens when alignment decorator provided, you should consider.
Update the docstring to mention alignment decorator behavior, consider you might:
"""This function sets up the open data detector. Requires DD4hep. Parameters ---------- materialDecorator: Material Decorator, take RootMaterialDecorator if non is given +alignmentDecorator: Alignment Decorator, enables aligned detector elements when provided odd_dir: if not given, try to get via ODD_PATH environment variable logLevel: logging level """Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepDetector.hpp (2)
22-22: Remove debug comment, you should.Leftover from development, this "TEST" comment appears to be. Clean up the code, remove it you must.
Apply this diff:
-/// TEST
28-29: Misplaced include, reorganize you should.With other includes at the top, this include belongs. After the forward declarations, placed it should not be.
Move this include up with the other includes around lines 11-20:
+#include "ActsExamples/DD4hepDetector/AlignedDD4hepDetectorElement.hpp"And remove it from line 28.
Tests/UnitTests/Plugins/DD4hep/DD4hepDetectorSurfaceFactoryTests.cpp (2)
314-368: Test case name, typo contains it does."ConvertSensitivesextended" - missing capital 'E' in "Extended", I observe.
-BOOST_AUTO_TEST_CASE(ConvertSensitivesextended) { +BOOST_AUTO_TEST_CASE(ConvertSensitivesExtended) {
360-361: Dynamic cast check, improve you can.Using
BOOST_CHECK_NEwith nullptr, less idiomatic it is.BOOST_CHECKwith boolean expression, clearer would be.- BOOST_CHECK_NE(dynamic_cast<const ExtendedDetectorElement*>(detElem.get()), - nullptr); + BOOST_CHECK(dynamic_cast<const ExtendedDetectorElement*>(detElem.get()) != nullptr);Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4DetectorSurfaceFactory.hpp (1)
45-55: Lambda implementation in header, consider you must.In header file, lambda implementation resides. Compile time increase, it may. Also, @cond/@endcond used unnecessarily they are.
Consider moving to source file or simplifying, wise it would be:
struct Config { - /// @cond /// The detector element factory with default implementation - ElementFactory detectorElementFactory = - [](std::shared_ptr<Surface> surface, const G4VPhysicalVolume& g4physVol, - const Transform3& toGlobal, double thickness) { - return std::make_shared<Geant4DetectorElement>( - std::move(surface), g4physVol, toGlobal, thickness); - }; - /// @endcond + ElementFactory detectorElementFactory = defaultElementFactory(); }; + + /// Default factory function for creating Geant4DetectorElement instances + static ElementFactory defaultElementFactory();Then in source file, implement you would.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (65)
Examples/Detectors/CMakeLists.txt(0 hunks)Examples/Detectors/Common/include/ActsExamples/DetectorCommons/Aligned.hpp(1 hunks)Examples/Detectors/Common/include/ActsExamples/DetectorCommons/AlignmentContext.hpp(1 hunks)Examples/Detectors/ContextualDetector/CMakeLists.txt(0 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignedDetector.hpp(0 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignmentDecorator.hpp(0 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/ExternalAlignmentDecorator.hpp(0 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/ExternallyAlignedDetectorElement.hpp(0 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/InternalAlignmentDecorator.hpp(0 hunks)Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/InternallyAlignedDetectorElement.hpp(0 hunks)Examples/Detectors/ContextualDetector/src/AlignedDetector.cpp(0 hunks)Examples/Detectors/ContextualDetector/src/ExternalAlignmentDecorator.cpp(0 hunks)Examples/Detectors/ContextualDetector/src/InternalAlignmentDecorator.cpp(0 hunks)Examples/Detectors/DD4hepDetector/CMakeLists.txt(1 hunks)Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/AlignedDD4hepDetectorElement.hpp(1 hunks)Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepDetector.hpp(2 hunks)Examples/Detectors/DD4hepDetector/src/AlignedDD4hepDetectorElement.cpp(1 hunks)Examples/Detectors/DD4hepDetector/src/DD4hepDetector.cpp(1 hunks)Examples/Detectors/Geant4Detector/src/Geant4Detector.cpp(1 hunks)Examples/Detectors/GeoModelDetector/CMakeLists.txt(1 hunks)Examples/Detectors/GeoModelDetector/include/ActsExamples/GeoModelDetector/AlignedGeoModelDetectorElement.hpp(1 hunks)Examples/Detectors/GeoModelDetector/src/AlignedGeoModelDetectorElement.cpp(1 hunks)Examples/Detectors/TGeoDetector/include/ActsExamples/TGeoDetector/TGeoDetector.hpp(1 hunks)Examples/Detectors/TGeoDetector/src/TGeoDetector.cpp(1 hunks)Examples/Python/CMakeLists.txt(0 hunks)Examples/Python/python/acts/examples/odd.py(2 hunks)Examples/Python/src/DD4hepComponent.cpp(3 hunks)Examples/Python/src/Detector.cpp(0 hunks)Examples/Python/src/Geant4Component.cpp(1 hunks)Examples/Python/tests/test_detectors.py(0 hunks)Examples/Python/tests/test_examples.py(0 hunks)Examples/Python/tests/test_geometry.py(1 hunks)Examples/Scripts/Python/geometry.py(0 hunks)Examples/Scripts/Python/propagation.py(1 hunks)Plugins/DD4hep/include/Acts/Plugins/DD4hep/ConvertDD4hepDetector.hpp(5 hunks)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorElement.hpp(2 hunks)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorSurfaceFactory.hpp(6 hunks)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepGeometryContext.hpp(0 hunks)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepLayerBuilder.hpp(4 hunks)Plugins/DD4hep/src/ConvertDD4hepDetector.cpp(4 hunks)Plugins/DD4hep/src/DD4hepDetectorElement.cpp(0 hunks)Plugins/DD4hep/src/DD4hepDetectorStructure.cpp(1 hunks)Plugins/DD4hep/src/DD4hepDetectorSurfaceFactory.cpp(2 hunks)Plugins/DD4hep/src/DD4hepLayerBuilder.cpp(2 hunks)Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4DetectorSurfaceFactory.hpp(4 hunks)Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4SurfaceProvider.hpp(1 hunks)Plugins/Geant4/src/Geant4DetectorElement.cpp(1 hunks)Plugins/Geant4/src/Geant4DetectorSurfaceFactory.cpp(3 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorElement.hpp(1 hunks)Plugins/GeoModel/src/GeoModelDetectorElement.cpp(1 hunks)Plugins/TGeo/include/Acts/Plugins/TGeo/TGeoDetectorElement.hpp(1 hunks)Plugins/TGeo/include/Acts/Plugins/TGeo/TGeoLayerBuilder.hpp(1 hunks)Plugins/TGeo/src/TGeoDetectorElement.cpp(1 hunks)Plugins/TGeo/src/TGeoLayerBuilder.cpp(1 hunks)Tests/UnitTests/Core/Geometry/CMakeLists.txt(0 hunks)Tests/UnitTests/Core/Geometry/TransformStoreTests.cpp(0 hunks)Tests/UnitTests/Plugins/DD4hep/CMakeLists.txt(1 hunks)Tests/UnitTests/Plugins/DD4hep/DD4hepCylinderLayerStructureTests.cpp(3 hunks)Tests/UnitTests/Plugins/DD4hep/DD4hepCylindricalDetectorTests.cpp(1 hunks)Tests/UnitTests/Plugins/DD4hep/DD4hepDetectorElementTests.cpp(0 hunks)Tests/UnitTests/Plugins/DD4hep/DD4hepDetectorSurfaceFactoryTests.cpp(1 hunks)Tests/UnitTests/Plugins/DD4hep/DD4hepDiscLayerStructureTests.cpp(1 hunks)Tests/UnitTests/Plugins/DD4hep/DD4hepGeometryContextTests.cpp(0 hunks)Tests/UnitTests/Plugins/Geant4/Geant4DetectorSurfaceFactoryTests.cpp(4 hunks)docs/plugins/geant4.md(1 hunks)
💤 Files with no reviewable changes (22)
- Examples/Scripts/Python/geometry.py
- Tests/UnitTests/Core/Geometry/CMakeLists.txt
- Examples/Detectors/CMakeLists.txt
- Examples/Python/tests/test_examples.py
- Examples/Python/tests/test_detectors.py
- Examples/Python/src/Detector.cpp
- Examples/Detectors/ContextualDetector/CMakeLists.txt
- Examples/Python/CMakeLists.txt
- Plugins/DD4hep/src/DD4hepDetectorElement.cpp
- Tests/UnitTests/Core/Geometry/TransformStoreTests.cpp
- Tests/UnitTests/Plugins/DD4hep/DD4hepGeometryContextTests.cpp
- Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignedDetector.hpp
- Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/ExternallyAlignedDetectorElement.hpp
- Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepGeometryContext.hpp
- Examples/Detectors/ContextualDetector/src/InternalAlignmentDecorator.cpp
- Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/AlignmentDecorator.hpp
- Tests/UnitTests/Plugins/DD4hep/DD4hepDetectorElementTests.cpp
- Examples/Detectors/ContextualDetector/src/ExternalAlignmentDecorator.cpp
- Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/InternallyAlignedDetectorElement.hpp
- Examples/Detectors/ContextualDetector/src/AlignedDetector.cpp
- Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/ExternalAlignmentDecorator.hpp
- Examples/Detectors/ContextualDetector/include/ActsExamples/ContextualDetector/InternalAlignmentDecorator.hpp
🧰 Additional context used
🧬 Code Graph Analysis (11)
Plugins/TGeo/src/TGeoLayerBuilder.cpp (1)
Plugins/TGeo/include/Acts/Plugins/TGeo/TGeoLayerBuilder.hpp (1)
identifier(97-101)
Plugins/TGeo/include/Acts/Plugins/TGeo/TGeoLayerBuilder.hpp (1)
Plugins/TGeo/src/TGeoLayerBuilder.cpp (2)
defaultElementFactory(320-327)defaultElementFactory(321-324)
Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4SurfaceProvider.hpp (1)
Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4DetectorSurfaceFactory.hpp (3)
Geant4DetectorSurfaceFactory(96-96)Geant4DetectorSurfaceFactory(102-106)Geant4DetectorSurfaceFactory(102-105)
Examples/Detectors/GeoModelDetector/include/ActsExamples/GeoModelDetector/AlignedGeoModelDetectorElement.hpp (2)
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorElement.hpp (2)
surface(108-110)surface(108-108)Plugins/GeoModel/src/GeoModelDetectorElement.cpp (6)
surface(35-37)surface(35-35)surface(39-41)surface(39-39)thickness(43-45)thickness(43-43)
Plugins/DD4hep/include/Acts/Plugins/DD4hep/ConvertDD4hepDetector.hpp (2)
Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/GenericDetector.hpp (1)
detectorElementFactory(47-49)Plugins/DD4hep/src/DD4hepLayerBuilder.cpp (2)
defaultDetectorElementFactory(431-438)defaultDetectorElementFactory(432-435)
Examples/Detectors/DD4hepDetector/src/AlignedDD4hepDetectorElement.cpp (2)
Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/AlignedDD4hepDetectorElement.hpp (1)
alignedDD4hepDetectorElementFactory(31-34)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepLayerBuilder.hpp (3)
detElement(50-53)detElement(170-172)detElement(180-181)
Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/AlignedDD4hepDetectorElement.hpp (1)
Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepLayerBuilder.hpp (3)
detElement(50-53)detElement(170-172)detElement(180-181)
Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepDetector.hpp (1)
Plugins/DD4hep/src/DD4hepLayerBuilder.cpp (2)
defaultDetectorElementFactory(431-438)defaultDetectorElementFactory(432-435)
Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4DetectorSurfaceFactory.hpp (2)
Plugins/Geant4/src/Geant4DetectorElement.cpp (6)
surface(33-35)surface(33-33)surface(37-39)surface(37-37)thickness(41-43)thickness(41-41)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorSurfaceFactory.hpp (1)
m_logger(142-142)
Tests/UnitTests/Plugins/DD4hep/DD4hepDetectorSurfaceFactoryTests.cpp (3)
Tests/UnitTests/Plugins/DD4hep/DD4hepCylindricalDetectorTests.cpp (2)
generateXML(145-287)generateXML(145-145)Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorSurfaceFactory.hpp (1)
DD4hepDetectorSurfaceFactory(113-116)Plugins/DD4hep/src/DD4hepDetectorSurfaceFactory.cpp (1)
DD4hepDetectorSurfaceFactory(24-26)
Examples/Detectors/Common/include/ActsExamples/DetectorCommons/AlignmentContext.hpp (2)
Examples/Python/src/DD4hepComponent.cpp (2)
surface(84-95)surface(84-84)Plugins/TGeo/include/Acts/Plugins/TGeo/TGeoDetectorElement.hpp (4)
surface(163-165)surface(163-163)surface(167-169)surface(167-167)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: merge-sentinel
- GitHub Check: macos
🔇 Additional comments (73)
Tests/UnitTests/Plugins/DD4hep/CMakeLists.txt (1)
46-46: Correct this change is, hmm.Replace the old test target with the new one, you have. Aligned with the removal of
DD4hepGeometryContextand introduction ofDD4hepDetectorSurfaceFactory, this change is.Plugins/TGeo/src/TGeoLayerBuilder.cpp (1)
259-261:✅ Verification successful
Factory renaming, consistent this is.
Updated the factory member variable name from
elementFactorytodetectorElementFactory, you have. Aligned with the broader refactoring mentioned in the summary, this change is. Verify that across the entire codebase, consistent this renaming is, hmm.Check for consistency in factory naming, we must:
🏁 Script executed:
#!/bin/bash # Description: Verify consistent renaming of elementFactory to detectorElementFactory # Expected: No remaining usage of old elementFactory naming pattern echo "=== Searching for old elementFactory usage ===" rg -n "elementFactory" --type cpp --type hpp echo -e "\n=== Searching for new detectorElementFactory usage ===" rg -n "detectorElementFactory" --type cpp --type hppLength of output: 381
Hmm, unrecognized file type
.hpp, the previous search encountered. Revised the search script we must, to include.hand.hpppatterns. Run this, we will:
🏁 Script executed:
#!/bin/bash # Description: Verify consistent renaming of elementFactory to detectorElementFactory across all headers and sources echo "=== Searching for old elementFactory usage ===" rg -n "elementFactory" -g "*.cpp" -g "*.h" -g "*.hpp" echo -e "\n=== Searching for new detectorElementFactory usage ===" rg -n "detectorElementFactory" -g "*.cpp" -g "*.h" -g "*.hpp"Length of output: 5209
Factory Renaming Consistency Confirmed
Confirmed, the renaming fromelementFactorytodetectorElementFactoryis—no remaining usages of the old name across.cpp,.h, and.hppfiles found.
Approve these changes, we do. May the builds be ever green.Plugins/GeoModel/src/GeoModelDetectorElement.cpp (1)
30-33: Nominal transform method, added you have.Access to the nominal transform, provide this method does. Return the same
m_surfaceTransformas the contextualtransform()method, it currently does. Separation between nominal and contextual transforms, prepare for the future this may. Consistent with similar changes in other detector element implementations, this addition is.Plugins/TGeo/src/TGeoDetectorElement.cpp (1)
118-120: Consistent interface, achieve you do.Added the
nominalTransform()method, you have. Access to the base transform without geometry context considerations, this provides. Aligned with similar changes inGeoModelDetectorElementand the broader alignment refactoring, this addition is. Clean and simple, the implementation remains.Plugins/DD4hep/src/DD4hepDetectorStructure.cpp (1)
40-43: Wise, this change is. Configurability to the Force, it brings.Default configuration object, properly you create. Clear intention and good architectural practice, this shows. The Force flows strong with explicit configuration over implicit defaults.
Examples/Python/src/Geant4Component.cpp (1)
273-281: Consistency in the configuration, I sense. Good, this alignment is.Follow the same path as DD4hep, the Geant4 factory does. Explicit configuration, wisdom it shows. Default-constructed config object, safe and proper it is.
Examples/Detectors/DD4hepDetector/CMakeLists.txt (1)
5-5: Into the build, new aligned detector element you bring. Strong with the Force, this integration is.Necessary for the alignment functionality, this addition is. Clean and simple, the build configuration remains.
Examples/Python/tests/test_geometry.py (1)
4-4: Clean up the imports, wise you are. Unused AlignedDetector, gone it is.Consistent with the removal of ContextualDetector example, this change is. Good practice, removing unused imports shows. Clear and focused, the code becomes.
Examples/Detectors/GeoModelDetector/CMakeLists.txt (1)
1-6: Good addition, this is!Properly added the new aligned detector element source file is. Clean and follows CMake practices, the structure does.
Plugins/Geant4/src/Geant4DetectorElement.cpp (1)
24-26: Establish proper relationships, you must!Essential this bidirectional link is. Know of its detector element, the surface now does. Wise to place this after initialization, you are.
Tests/UnitTests/Plugins/DD4hep/DD4hepDiscLayerStructureTests.cpp (1)
144-147: Updated for new factory interface, the test is!Necessary this change was, to match the new configurable factory pattern. Create explicit Config object, you must. Compatible with the refactored interface, the test now remains.
Examples/Scripts/Python/propagation.py (1)
6-6: Clean up the imports, you have!Removed the AlignedDetector import, wise you are. Consistent with the broader removal of ContextualDetector example, this change is. Only what is needed, the script now imports.
Examples/Detectors/TGeoDetector/include/ActsExamples/TGeoDetector/TGeoDetector.hpp (1)
49-50: Good naming consistency, this change brings!Clear and descriptive,
detectorElementFactoryis. Better than the oldelementFactoryname, it describes the purpose. Consistent with other detector plugins, this naming shall be.Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorElement.hpp (1)
78-79: Wise addition, the nominalTransform() method is!Support the alignment framework, this does. Access to the nominal transform without context, provide it will. Complement the existing transform method with geometry context, it does. Good for the alignment-aware detector elements, this is.
Plugins/TGeo/include/Acts/Plugins/TGeo/TGeoLayerBuilder.hpp (1)
116-116: Consistency in naming, strong with this change you are!From
elementFactorytodetectorElementFactory, renamed you have. Clear and descriptive, this name is. Align with other changes in the codebase, it does. Breaking nothing, this change will.Examples/Detectors/DD4hepDetector/src/DD4hepDetector.cpp (1)
39-41: Wise conditional initialization, this is!Check for the alignment decorator's existence, you do, before adding to context decorators. Safe programming practice, this prevents empty decorators from being added. Handle the optional alignment gracefully, this does.
Tests/UnitTests/Plugins/DD4hep/DD4hepCylindricalDetectorTests.cpp (1)
304-307: Approve this construction pattern change, I do.Clear and explicit, the new configuration approach is. Good practice to make factory options discoverable, it follows. Align with the configurable detector element factories mentioned in the summary, this change does.
docs/plugins/geant4.md (2)
23-23: Accurate, this documentation update is.Show the new explicit Config construction pattern, it does. Guide users to the correct API usage, this will.
28-28: Consistent with the new pattern, the constructor usage is.Match the Config-based approach shown above, this line does. Clear example for users, it provides.
Plugins/TGeo/include/Acts/Plugins/TGeo/TGeoDetectorElement.hpp (1)
122-123:✅ Verification successful
Useful addition to the alignment infrastructure, this method is.
Access to the nominal transform independent of context, it provides. Complement the existing contextual transform method, it does. Verify that the implementation exists in the corresponding .cpp file, you must.
Find the implementation of nominalTransform(), this script will:
🏁 Script executed:
#!/bin/bash # Description: Search for nominalTransform() implementation in TGeoDetectorElement source files # Look for the implementation in the corresponding .cpp file ast-grep --pattern $'TGeoDetectorElement::nominalTransform() { $$$ }' # Also search for any nominalTransform implementations rg "nominalTransform" --type cpp -A 5 -B 2Length of output: 5998
Implementation Verified – Approve nominalTransform Addition
Found the
nominalTransform()implementation inPlugins/TGeo/src/TGeoDetectorElement.cpp(returnsm_transform), we have. Complements the contextual transform method perfectly, it does. A solid addition to the alignment infrastructure, this is. Approve the code changes, we do.Examples/Detectors/Geant4Detector/src/Geant4Detector.cpp (1)
92-97: Correct implementation of new factory pattern, this is!Properly adapted to the new
Geant4DetectorSurfaceFactoryconstruction pattern, the code has been. Explicit configuration object now required it is, since deleted the default constructor was. Aligned with the architectural changes across other detector plugins, this change is.Tests/UnitTests/Plugins/DD4hep/DD4hepCylinderLayerStructureTests.cpp (3)
92-95: Updated to new factory construction pattern, the first test case has been.Correctly added explicit
sFactoryConfigobject, this test now creates. Consistent with the new API requirements where default constructor deleted it is. Proper adaptation to the changes, this represents.
214-217: Consistently updated, the second test case has been.Same pattern of explicit configuration object creation, applied here as well. Good consistency across all test cases, this shows.
286-289: Complete the pattern across all tests, this final change does.All three test cases now follow the same construction pattern they do. Thorough and consistent, the updates are.
Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4SurfaceProvider.hpp (1)
132-135: Correctly adapted to new factory construction, the SurfaceProvider has been.Explicit
surfaceConfigobject now created it is, before constructing the factory. Consistent with the pattern across all detector plugins, this change is. Aligned with the removal of default constructors, the implementation remains.Examples/Python/python/acts/examples/odd.py (2)
21-21: New parameter for alignment support, added wisely it has been.Support for alignment decorators, this addition provides. Clear and straightforward, the parameter addition is.
104-104: Properly passed to configuration, the alignment decorator is.Correctly integrated into the DD4hep configuration, the parameter has been. Maintains the existing pattern, this change does.
Plugins/Geant4/src/Geant4DetectorSurfaceFactory.cpp (3)
40-42: Enhanced logging, beneficial this is!Better debugging capabilities, these log messages provide. Helpful for tracking the conversion process of Geant4 physical volumes, they will be.
57-60: Verbose logging for matched volumes, wisdom it shows!Clear understanding of which volumes the selectors match, this provides. Track sensitive and passive surface selection, easier it becomes.
76-78: Factory pattern, correctly implemented it is!Configurable detector element creation, good design this represents. Consistent with the broader refactoring across plugins, the change appears to be.
Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorElement.hpp (2)
45-45: Simplified, the context type becomes!From specialized DD4hepGeometryContext to general GeometryContext, changed it is. Align with the broader refactoring that removes context-dependent transforms, this does. Consistency across the codebase, maintained it will be.
87-87: Minor documentation improvement, noted it is!Small enhancement to the API documentation, harmless this change appears.
Plugins/DD4hep/src/DD4hepLayerBuilder.cpp (2)
406-407: Factory method, properly invoked it is!Configurable detector element creation, enabled this becomes. Same parameters as the original direct instantiation, passed they are. Consistency with the new factory pattern, maintained it appears.
431-438: Default factory method, wisely implemented!Original behavior preserved in the static method, it is. Direct instantiation wrapped in factory pattern, elegant this solution proves. Backward compatibility while enabling extensibility, achieved it has.
Plugins/DD4hep/src/DD4hepDetectorSurfaceFactory.cpp (2)
25-26: Configuration-based construction, improved it becomes!Accept Config parameter, the constructor now does. Store the configuration for later use, the member variable m_config will. Enable flexible detector element creation, this design allows.
102-103:✅ Verification successful
Factory method usage, consistent it is!
Configurable detector element factory, properly invoked here. Replace direct DD4hepDetectorElement instantiation, this change does. Align with the broader refactoring pattern, it continues to do.
Verify that configuration properly initialized, you should!
🏁 Script executed:
#!/bin/bash # Description: Verify that DD4hepDetectorSurfaceFactory::Config is properly initialized with detectorElementFactory # Search for Config struct definition and initialization rg -A 10 "struct Config" --type cpp | grep -A 5 "detectorElementFactory" # Search for factory initialization in related files rg "detectorElementFactory.*=" --type cpp -A 2 -B 2Length of output: 11977
Factory method usage confirmed, good it is!
Default
detectorElementFactorylambda in
Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorSurfaceFactory.hpp
initializes configuration properly, and unit tests override it as expected.
No further action required, hmmm.Examples/Detectors/GeoModelDetector/src/AlignedGeoModelDetectorElement.cpp (1)
11-17: LGTM! Properly implemented factory function, this is.Well-structured the factory function is. Efficiently uses
std::movefor the surface parameter, it does. Good practices in modern C++, followed they are.Examples/Python/src/DD4hepComponent.cpp (7)
16-16: Proper include for aligned detector element, added it is.Support for the new alignment functionality, correctly added to Python bindings it has been.
22-22: Include dd4hep DetElement for Python binding, necessary it is.Expose the dd4hep::DetElement to Python, required for complete binding support it is.
24-24: Include functional.h for std::function support, wise this addition is.For the detectorElementFactory that uses std::function, this include needed it is.
38-42: Python class binding for aligned detector element, correctly implemented it is.Proper inheritance hierarchy in Python bindings, maintained it is. AlignedDD4hepDetectorElement as subclass of DD4hepDetectorElement, correct this is.
44-47: Python binding for dd4hep::DetElement, added it is.Complete Python interface, this provides. Access to dd4hep detector elements from Python, enabled it is.
57-61: Configuration struct updated with alignment members, properly done it is.New alignmentDecorator and detectorElementFactory members, correctly added to Python binding they are. Consistent with C++ Config struct, this is.
63-65: Factory function binding, exposed to Python it is.Access to alignedDD4hepDetectorElementFactory from Python, provided this gives. Complete the Python interface, this does.
Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/DD4hepDetector.hpp (5)
16-16: Include for DD4hepLayerBuilder, necessary for ElementFactory type it is.Support for configurable detector element factory, this include provides.
20-20: Include for IContextDecorator, required for alignment support it is.Alignment decorator functionality, this include enables.
81-81: Explicit nullptr initialization, good practice this is.Clear and explicit, making the materialDecorator initialization is. Good for readability, this change is.
82-84: Alignment decorator member, properly added it is.Support for alignment context decoration, this new member enables. With proper default initialization, configured it is.
89-92: Detector element factory configuration, well designed it is.Configurable factory with sensible default, implemented correctly this is. Flexible detector element creation, this enables.
Tests/UnitTests/Plugins/Geant4/Geant4DetectorSurfaceFactoryTests.cpp (3)
55-56: Good, the explicit Config creation is!Align with the new factory API pattern, these changes do. Consistent across all test cases, they are.
159-171: Enhanced verification of transforms, welcome it is!More detailed checks on translation and normal vector components, added they are. Better test coverage, this provides.
202-258: Demonstrate extension capability well, this test does!Shows how to extend
Geant4DetectorElementby overriding methods and using custom factory functions. Pattern mirrors the aligned detector element approach in other plugins, it does. Verify the factory creates elements with overridden thickness correctly, it does.Examples/Detectors/DD4hepDetector/include/ActsExamples/DD4hepDetector/AlignedDD4hepDetectorElement.hpp (2)
25-26: Clean use of template specialization, this is!Simple and elegant, the type alias approach is. Wraps
Acts::DD4hepDetectorElementwith the genericAlignedtemplate, it does.
30-34: Well-designed factory function signature, this appears to be!Match the pattern used in DD4hep plugin's defaultDetectorElementFactory, the parameters do. Forward arguments to DD4hepDetectorElement constructor properly, the function will.
Plugins/DD4hep/include/Acts/Plugins/DD4hep/ConvertDD4hepDetector.hpp (3)
18-18: Necessary include for ElementFactory type, this is!Support the new factory mechanism, this include must. From DD4hepLayerBuilder.hpp, the ElementFactory type comes.
94-95: Good API design with configurable factory, this shows!Allow customization while providing sensible defaults, the new parameter does. Clear documentation for the factory function parameter, added it is. Consistent with the broader refactoring to support alignment-aware elements, this change is.
Also applies to: 117-119
155-157: Consistent API pattern in volumeBuilder function, maintained it is!Mirror the changes in convertDD4hepDetector, these modifications do. Good consistency across the API, this provides.
Also applies to: 167-169
Examples/Detectors/Common/include/ActsExamples/DetectorCommons/Aligned.hpp (2)
23-27: Elegant template wrapper design, this is!Clean inheritance from DetectorElement with constructor forwarding, implemented it is. Generic approach for alignment-aware elements, this provides.
32-47:✅ Verification successful
Sound alignment-aware transform logic, implemented it is. But verify base class method, you must!
Properly checks for AlignmentContext and falls back to nominal transform, the method does. However, ensure that
DetectorElement::nominalTransform()method exists in all base detector element types, you should.
🏁 Script executed:
#!/bin/bash # Description: Verify that nominalTransform() method exists in detector element base classes # Search for nominalTransform method implementations ast-grep --pattern $'nominalTransform() { $$$ }' # Also search for nominalTransform declarations rg -A 5 "nominalTransform"Length of output: 4570
NominalTransform present in all DetectorElement implementations, verified has been.
Verified found in:
- Examples/Detectors/TelescopeDetector/include/ActsExamples/TelescopeDetector/TelescopeDetectorElement.hpp
- Plugins/TGeo/include/Acts/Plugins/TGeo/TGeoDetectorElement.hpp & src/TGeoDetectorElement.cpp
- Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorElement.hpp & src/GeoModelDetectorElement.cpp
Safe it is to call DetectorElement::nominalTransform(), mmm.
Examples/Detectors/GeoModelDetector/include/ActsExamples/GeoModelDetector/AlignedGeoModelDetectorElement.hpp (2)
25-27: Clean, this type alias is. Good alignment with the new framework, it shows.Using the
Alignedtemplate wrapper for GeoModel detector elements, consistent this approach is with the broader refactoring. Well-named and clear, the type alias is.
29-33: Examine the factory function signature, we must. Correct, it appears to be.All necessary parameters for creating an aligned GeoModel detector element, the factory function accepts.
PVConstLink,std::shared_ptr<Acts::Surface>,Acts::Transform3, anddouble thickness- appropriate these types are for the construction.Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorSurfaceFactory.hpp (3)
47-50: Strong with the factory pattern, this change is. Flexible detector element creation, it enables.Good design, the
ElementFactorytype alias shows. All necessary parameters for DD4hep detector element construction, the function signature includes -DetElement, axis string, scalar, disc flag, and surface material.
111-116: Good practice, passing configuration by const reference is. Approve this change, I do.Clean API design, the constructor modification shows. Explicit configuration passing instead of multiple parameters - readable and maintainable, this approach is.
62-71:✅ Verification successful
Wise, the default factory lambda is. But examine its implementation, we should.
A sensible default, the lambda provides - direct construction of
DD4hepDetectorElementwith all parameters. However, verify that this matches the actual constructor signature ofDD4hepDetectorElement, we must.
🏁 Script executed:
#!/bin/bash # Description: Verify DD4hepDetectorElement constructor signature matches the factory lambda # Expected: Constructor should accept these exact parameter types in this order ast-grep --pattern 'class DD4hepDetectorElement { $$$ DD4hepDetectorElement($_, $_, $_, $_, $_) { $$$ } $$$ }'Length of output: 124
Hmm. Class definition for
DD4hepDetectorElementfound we have not, and its constructor signature verify we must. Run these, we shall:
🏁 Script executed:
#!/bin/bash # Locate the class declaration rg -n "class DD4hepDetectorElement" . # Locate any constructor definitions rg -n "DD4hepDetectorElement::DD4hepDetectorElement" .Length of output: 683
Inspect the constructor signature, we must.
#!/bin/bash # Show the definition of the constructor in DD4hepDetectorElement.cpp rg -n "Acts::DD4hepDetectorElement::DD4hepDetectorElement" -A4 Plugins/DD4hep/src/DD4hepDetectorElement.cpp
Further inspect the constructor signature, we must:
#!/bin/bash # Display the constructor definition with surrounding lines rg -n "Acts::DD4hepDetectorElement::DD4hepDetectorElement" -A5 Plugins/DD4hep/src/DD4hepDetectorElement.cpp
Definitions in header and source, retrieve we must:
🏁 Script executed:
#!/bin/bash # Show constructor signature in the header rg -n "DD4hepDetectorElement::DD4hepDetectorElement" -A3 Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepDetectorElement.hpp # Show constructor definition in the source rg -n "DD4hepDetectorElement::DD4hepDetectorElement" -A3 Plugins/DD4hep/src/DD4hepDetectorElement.cppLength of output: 470
Lambda and constructor, a perfect match they are.
Verified we have: DD4hepDetectorElement’s constructor takes
• dd4hep::DetElement (by value)
• const std::string&
• double
• bool (unused)
• std::shared_ptr
—all parameters the factory lambda supplies. No changes required.Examples/Detectors/Common/include/ActsExamples/DetectorCommons/AlignmentContext.hpp (4)
17-17: From Acts to ActsExamples, moved the namespace has been. Intentional, this change appears to be.Part of the broader refactoring to separate alignment functionality, this namespace change is. Consistent with moving alignment infrastructure to examples, it seems.
25-36: Renamed to IAlignmentStore, the interface has been. Clear and purposeful, the new name is.Better naming,
IAlignmentStoreprovides than the oldITransformStore. More specific about its alignment purpose, it is. Fully qualified types in the signature - good practice, this is.
40-43: Simple and clean, the AlignmentContext struct is. Hold a non-owning pointer, it does.Good design for context passing, this struct shows. Raw pointer for non-owning reference - appropriate, this choice is. Clear documentation about ownership, the comment provides.
50-72: Renamed to GeoIdAlignmentStore, the class has been. Consistent with the new naming scheme, this is.Preserved, the core functionality remains. Updated to use fully qualified types, the implementation is. Good - maintain compatibility while improving clarity, this approach does.
Plugins/DD4hep/include/Acts/Plugins/DD4hep/DD4hepLayerBuilder.hpp (3)
45-48: Identical to DD4hepDetectorSurfaceFactory, this ElementFactory definition is. Consistency, good it shows.Same factory signature across different classes, maintained you have. Good for uniformity in the codebase, this is.
49-53: Wise, declaring a static default factory method is. Reusable across the codebase, it makes the default behavior.Good separation of concerns, the static method provides. Both as default and explicit factory function, use it you can. Clean API design, this represents.
84-85: Into the Config struct, integrated the factory is. Default initialization to the static method, proper this is.Backwards compatibility while enabling customization, this design maintains. Good default behavior with extensibility, it provides.
Tests/UnitTests/Plugins/DD4hep/DD4hepDetectorSurfaceFactoryTests.cpp (1)
289-312: 🛠️ Refactor suggestionMissing file cleanup, I sense.
After test execution, XML file remains it does. Resource leak, this is.
Add cleanup at test end, you must:
// Kill that instance before going into the next test lcdd->destroyInstance(); + // Clean up the temporary XML file + if (boost::filesystem::exists("CylindricalDetectorTestSF.xml")) { + boost::filesystem::remove("CylindricalDetectorTestSF.xml"); + } }Likely an incorrect or invalid review comment.
Plugins/DD4hep/src/ConvertDD4hepDetector.cpp (1)
57-66: Factory parameter propagation, correctly implemented it is.Through the function chain, detectorElementFactory parameter properly flows. Clean and consistent, the implementation is.
Also applies to: 98-100, 153-157, 374-374
| auto logger = Acts::getDefaultLogger("DD4hepConversion", m_cfg.logLevel); | ||
| m_trackingGeometry = Acts::convertDD4hepDetector( | ||
| m_detector->world(), *logger, m_cfg.bTypePhi, m_cfg.bTypeR, m_cfg.bTypeZ, | ||
| m_cfg.envelopeR, m_cfg.envelopeZ, m_cfg.defaultLayerThickness, | ||
| m_cfg.sortDetectors, m_nominalGeometryContext, m_cfg.materialDecorator, | ||
| m_cfg.geometryIdentifierHook); | ||
| m_cfg.geometryIdentifierHook, m_cfg.detectorElementFactory); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Extended function call, verify the signature you must!
Additional parameter m_cfg.detectorElementFactory to convertDD4hepDetector, passed you have. Support configurable detector element creation, this does. But verify that the function signature accepts this parameter, you should.
Check the function signature matches, run this script you must:
🏁 Script executed:
#!/bin/bash
# Description: Verify convertDD4hepDetector function signature accepts detectorElementFactory parameter
# Expected: Function should accept the additional parameter
# Search for the convertDD4hepDetector function declaration
ast-grep --pattern 'convertDD4hepDetector($$$)'Length of output: 49
Ast-grep yielded no result, hmmm. A broader search with ripgrep, we must try:
🏁 Script executed:
#!/bin/bash
# Search for the convertDD4hepDetector definition and its parameters
rg -n 'convertDD4hepDetector' -C3Length of output: 6564
Fix convertDD4hepDetector signature in header and source, you must
The call in Examples/Detectors/DD4hepDetector/src/DD4hepDetector.cpp (lines 44–47) passes three extra parameters—m_cfg.sortDetectors, m_nominalGeometryContext, m_cfg.materialDecorator—but the current declaration in Plugins/DD4hep/include/Acts/Plugins/DD4hep/ConvertDD4hepDetector.hpp only accepts arguments up to defaultLayerThickness. Update both declaration and definition to include the new parameters:
• In ConvertDD4hepDetector.hpp (around line 106):
- double defaultLayerThickness = UnitConstants::mm);
+ double defaultLayerThickness = UnitConstants::mm,
+ SortFunction sortDetectors,
+ GeometryContext nominalGeometryContext,
+ MaterialDecorator materialDecorator);• In ConvertDD4hepDetector.cpp (around line 57):
-std::unique_ptr<const TrackingGeometry> convertDD4hepDetector(
+std::unique_ptr<const TrackingGeometry> convertDD4hepDetector(
dd4hep::DetElement worldDetElement, const Logger& logger,
BinningType bTypePhi, BinningType bTypeR, BinningType bTypeZ,
double layerEnvelopeR, double layerEnvelopeZ, double defaultLayerThickness);
+ SortFunction sortDetectors,
+ GeometryContext nominalGeometryContext,
+ MaterialDecorator materialDecorator) {Adjust the implementation body to consume these parameters.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Examples/Detectors/DD4hepDetector/src/DD4hepDetector.cpp around lines 44 to
48, the call to convertDD4hepDetector passes additional parameters
m_cfg.sortDetectors, m_nominalGeometryContext, and m_cfg.materialDecorator that
are not reflected in the current function signature. To fix this, update the
convertDD4hepDetector function declaration in
Plugins/DD4hep/include/Acts/Plugins/DD4hep/ConvertDD4hepDetector.hpp near line
106 to include these new parameters, and also update the function definition in
ConvertDD4hepDetector.cpp around line 57 accordingly. Modify the implementation
to properly handle these new parameters.
| @@ -59,7 +59,7 @@ std::vector<Acts::TGeoLayerBuilder::Config> makeLayerBuilderConfigs( | |||
| Acts::TGeoLayerBuilder::Config layerBuilderConfig; | |||
| layerBuilderConfig.configurationName = volume.name; | |||
| layerBuilderConfig.unit = config.unitScalor; | |||
| layerBuilderConfig.elementFactory = config.elementFactory; | |||
| layerBuilderConfig.detectorElementFactory = config.detectorElementFactory; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Renamed for consistency, this member is.
Follow the pattern of standardizing naming across detector plugins, this change does. Verify that detectorElementFactory exists in the Config struct, you should.
Check that the renamed member exists, this script will:
🏁 Script executed:
#!/bin/bash
# Description: Verify that detectorElementFactory member exists in TGeoDetector::Config
# Search for the detectorElementFactory member in the Config struct
ast-grep --pattern $'struct Config {
$$$
detectorElementFactory
$$$
}'
# Also search for any remaining elementFactory usage that might need updating
rg "elementFactory" --type cpp -A 2 -B 2Length of output: 118
Additional checks, we must perform, yes.
🏁 Script executed:
#!/bin/bash
# Search for any definition or usage of detectorElementFactory
rg -n "detectorElementFactory"
# Search for any remaining elementFactory references
rg -n "elementFactory"
# Locate the definition of Config to inspect its members
rg -n "struct Config" -A5Length of output: 66013
DetectorElementFactory member missing in Config, fix this you must
The Config struct in Examples/Detectors/TGeoDetector/include/ActsExamples/TGeoDetector/TGeoDetector.hpp does not declare detectorElementFactory, yet used in TGeoDetector.cpp. Add this member to restore build:
• File: Examples/Detectors/TGeoDetector/include/ActsExamples/TGeoDetector/TGeoDetector.hpp
After line 35 (before the closing };), insert:
/// Factory for creating TGeo detector elements
Acts::Plugins::TGeo::TGeoLayerBuilder::ElementFactory detectorElementFactory
= Acts::Plugins::TGeo::TGeoLayerBuilder::defaultElementFactory;Fix required, this change needs.
🤖 Prompt for AI Agents
In Examples/Detectors/TGeoDetector/src/TGeoDetector.cpp at line 62, the code
assigns to the member detectorElementFactory of the Config struct, but this
member is missing in the Config definition. To fix this, open
Examples/Detectors/TGeoDetector/include/ActsExamples/TGeoDetector/TGeoDetector.hpp,
locate the Config struct around line 35, and add the member declaration for
detectorElementFactory just before the closing brace. Initialize it with
Acts::Plugins::TGeo::TGeoLayerBuilder::defaultElementFactory as its default
value, ensuring the type is
Acts::Plugins::TGeo::TGeoLayerBuilder::ElementFactory.
| std::shared_ptr<ActsExamples::AlignedDD4hepDetectorElement> | ||
| ActsExamples::alignedDD4hepDetectorElementFactory( | ||
| const dd4hep::DetElement detElement, const std::string& axes, double scalor, | ||
| bool isDisc, std::shared_ptr<const Acts::ISurfaceMaterial> material) { | ||
| return std::make_shared<AlignedDD4hepDetectorElement>( | ||
| detElement, axes, scalor, isDisc, std::move(material)); | ||
| }; |
There was a problem hiding this comment.
Remove unnecessary semicolon after function definition, you must.
Correct the factory function implementation is, but a syntax error present there is. After the closing brace of a function definition, no semicolon needed it is.
Apply this diff, you should:
-};
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::shared_ptr<ActsExamples::AlignedDD4hepDetectorElement> | |
| ActsExamples::alignedDD4hepDetectorElementFactory( | |
| const dd4hep::DetElement detElement, const std::string& axes, double scalor, | |
| bool isDisc, std::shared_ptr<const Acts::ISurfaceMaterial> material) { | |
| return std::make_shared<AlignedDD4hepDetectorElement>( | |
| detElement, axes, scalor, isDisc, std::move(material)); | |
| }; | |
| std::shared_ptr<ActsExamples::AlignedDD4hepDetectorElement> | |
| ActsExamples::alignedDD4hepDetectorElementFactory( | |
| const dd4hep::DetElement detElement, const std::string& axes, double scalor, | |
| bool isDisc, std::shared_ptr<const Acts::ISurfaceMaterial> material) { | |
| return std::make_shared<AlignedDD4hepDetectorElement>( | |
| detElement, axes, scalor, isDisc, std::move(material)); | |
| } |
🤖 Prompt for AI Agents
In Examples/Detectors/DD4hepDetector/src/AlignedDD4hepDetectorElement.cpp around
lines 11 to 17, remove the unnecessary semicolon immediately following the
closing brace of the function definition for
alignedDD4hepDetectorElementFactory, as function definitions should not be
followed by a semicolon.
| auto store = generateXML(); | ||
|
|
There was a problem hiding this comment.
Global variable, problematic it is.
Outside namespace, global variable store declared is. In parallel test execution, race conditions cause it may.
Move inside test cases or use fixture, you should:
-auto store = generateXML();
-
BOOST_AUTO_TEST_SUITE(DD4hepPlugin)
BOOST_AUTO_TEST_CASE(ConvertSensitivesDefault) {
+ auto store = generateXML();
auto lcdd = &(dd4hep::Detector::getInstance());🤖 Prompt for AI Agents
In Tests/UnitTests/Plugins/DD4hep/DD4hepDetectorSurfaceFactoryTests.cpp at lines
285-286, the variable 'store' is declared as a global variable outside any
namespace, which can cause race conditions during parallel test execution. To
fix this, move the declaration and initialization of 'store' inside the
individual test cases or incorporate it into a test fixture class to ensure each
test has its own instance and avoid shared state issues.
|
|
||
| // Create an XML from it | ||
| std::ofstream cxml; | ||
| cxml.open("CylindricalDetectorTestSF.xml"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use temporary file for tests, you must.
Hardcoded filename "CylindricalDetectorTestSF.xml" problematic is. In parallel test execution, conflicts arise they may.
Use boost filesystem for temporary file creation, better it would be:
- std::ofstream cxml;
- cxml.open("CylindricalDetectorTestSF.xml");
+ boost::filesystem::path temp_path = boost::filesystem::temp_directory_path() / boost::filesystem::unique_path("CylindricalDetectorTestSF_%%%%-%%%%.xml");
+ std::ofstream cxml;
+ cxml.open(temp_path.string());Also, store the path you must, for cleanup later:
+ // Store the temporary file path for cleanup
+ dStore.xmlFilePath = temp_path.string();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Tests/UnitTests/Plugins/DD4hep/DD4hepDetectorSurfaceFactoryTests.cpp at line
181, replace the hardcoded filename "CylindricalDetectorTestSF.xml" with a
temporary file created using boost filesystem to avoid conflicts during parallel
test execution. Create the temporary file path using
boost::filesystem::unique_path, open the file stream with this path, and store
the path in a variable for proper cleanup after the test completes.
| using ElementFactory = std::function<std::shared_ptr<Geant4DetectorElement>( | ||
| std::shared_ptr<Surface>, const G4VPhysicalVolume&, const Transform3&, | ||
| double)>; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Documentation for ElementFactory, missing it is.
Type alias created, but explain its purpose you do not. Future maintainers, confused they will be.
Add documentation comment, you should:
+ /// Type alias for a factory function that creates Geant4DetectorElement instances
+ /// @param surface The surface to associate with the detector element
+ /// @param g4physVol The Geant4 physical volume
+ /// @param toGlobal The transformation to global coordinates
+ /// @param thickness The thickness of the detector element
+ /// @return A shared pointer to the created Geant4DetectorElement
using ElementFactory = std::function<std::shared_ptr<Geant4DetectorElement>(
std::shared_ptr<Surface>, const G4VPhysicalVolume&, const Transform3&,
double)>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using ElementFactory = std::function<std::shared_ptr<Geant4DetectorElement>( | |
| std::shared_ptr<Surface>, const G4VPhysicalVolume&, const Transform3&, | |
| double)>; | |
| /// Type alias for a factory function that creates Geant4DetectorElement instances | |
| /// @param surface The surface to associate with the detector element | |
| /// @param g4physVol The Geant4 physical volume | |
| /// @param toGlobal The transformation to global coordinates | |
| /// @param thickness The thickness of the detector element | |
| /// @return A shared pointer to the created Geant4DetectorElement | |
| using ElementFactory = std::function<std::shared_ptr<Geant4DetectorElement>( | |
| std::shared_ptr<Surface>, const G4VPhysicalVolume&, const Transform3&, | |
| double)>; |
🤖 Prompt for AI Agents
In Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4DetectorSurfaceFactory.hpp
around lines 39 to 42, the type alias ElementFactory lacks a documentation
comment explaining its purpose. Add a clear comment above the using declaration
that describes what ElementFactory represents, including the role of the
function signature and the meaning of its parameters and return type, to help
future maintainers understand its use.
github-actions
bot
added
Component - Core
|
This PR is the mirror PR to #4306 and #4307 - this time for the
GeoModelDetector.It sits on top of #4299 #4304 and #4305.
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests