-
Notifications
You must be signed in to change notification settings - Fork 173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Introduce the usage of the geomodel converters for the Muon System #3348
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a first round of comments, nothing very critical here.
I like the changes, especially it seems more logically to get the converters from the shape type and not pass them in from python.
In the future we might want to generalize the subvolume search a bit (the else
statement which currently allows either to convert the top level volume or the subvolumes).
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelConverters.hpp
Outdated
Show resolved
Hide resolved
Thanks Benjamin for the comments! Yes I agree with you with the else statement I am also not happy. As we discussed also offline, for the moment I had it like that because for the Muons the top-level physical volume is actually a bounding box, which is the multilayer and has the shape of std::Trd. So from the converters it will just pick the trapezoid converter and it will be turned into a plane surface which does not make sense. I am thinking about also converting the bounding box to a volume and then the else statement could be gone actually. |
Co-authored-by: Benjamin Huth <37871400+benjaminhuth@users.noreply.github.com>
Co-authored-by: Benjamin Huth <37871400+benjaminhuth@users.noreply.github.com>
📊: Physics performance monitoring for eaffacdphysmon summary
|
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelConverters.hpp
Outdated
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorElement.hpp
Outdated
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorElement.hpp
Outdated
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorSurfaceFactory.hpp
Outdated
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorSurfaceFactory.hpp
Outdated
Show resolved
Hide resolved
…ers.hpp Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com>
Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com>
…rSurfaceFactory.hpp Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com>
I would not put the ATLAS database file into the repository. |
Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoTubeConverter.hpp
Outdated
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoUnionDoubleTrdConverter.hpp
Outdated
Show resolved
Hide resolved
…l.hpp it will come from matt's pr
it will come from matt's pr
Quality Gate passedIssues Measures |
WalkthroughChange significant, yes. Updated the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (31)
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelTree.hpp (1)
18-23
: Documentation for this structure, add you should.Hmmmm... Important structure this is, yet documentation it lacks. Help future Padawans understand its purpose, we must.
Add documentation like this, you should:
namespace Acts { +/** @brief Tree structure for GeoModel geometry + * + * Contains the necessary components to read and access + * GeoModel geometry, including the world volume and reader. + */ struct GeoModelTree { std::shared_ptr<GeoModelIO::ReadGeoModel> geoReader = nullptr; PVConstLink worldVolume{nullptr}; std::string worldVolumeName = "World"; };Plugins/GeoModel/CMakeLists.txt (1)
Line range hint
1-1
: Document the version requirements, you should!For the Muon System integration, significant changes these are. Recommend adding a comment block at the top of CMakeLists.txt, I do, explaining:
- Minimum GeoModel version required (6.2.0)
- Dependency on GeoModelCore::GeoModelHelpers
- Special requirements for Muon System database compatibility
+# Required GeoModel version: 6.2.0+ +# Note: This version is required for Muon System database compatibility +# Dependencies: +# - GeoModelCore::GeoModelHelpers: Required for subvolume handling +# - GeoModelIO::GeoModelDBManager: Required for database operations + include(FetchContent)Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoIntersectionAnnulusConverter.hpp (1)
33-34
: Update the parameter documentation, you must!Outdated, the parameter description '@param geoFPV' is. Match the new parameter name 'geoPV', it should.
Apply this change, you should:
/// @brief Convert a GeoBox to a detector element and surface /// - /// @param geoFPV The full physical volume to convert (contains shape) + /// @param geoPV The physical volume to convert (contains shape) /// @param geoIntersection The GeoIntersection to convertPlugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoShiftConverter.hpp (1)
32-35
: Update documentation to reflect new parameter name, you must!Hmmmm. Documentation speaks of 'geoFPV', yet 'geoPV' the parameter is called. Consistency in documentation, maintain we should.
- /// @param geoFPV The full physical volume to convert (contains shape) + /// @param geoPV The physical volume to convert (contains shape)Plugins/GeoModel/src/GeoModelDetectorElement.cpp (1)
16-18
: Hmmmm, wise changes to constructor, I sense.Changed to PVConstLink, the parameter type has. Proper memory management and type safety, this brings. Clean initialization of member variable m_geoPhysVol, I observe.
Consider documenting in comments, you should, why PVConstLink over GeoFullPhysVol chosen was. Help future Padawans understand the ways of the Force, it will.
Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoTrdConverter.hpp (1)
Line range hint
29-32
: Update the documentation, you must!In the comments above, still speaks of 'geoFPV' the documentation does, while 'geoPV' the parameter has become. Harmony between code and documentation, maintain we must.
/// @brief Convert a GeoTrd to a detector element and surface /// - /// @param geoFPV The full physical volume to convert (contains shape) + /// @param geoPV The physical volume to convert (contains shape) /// @param geoTrd The GeoTrd to convertPlugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoTubeConverter.hpp (2)
38-41
: Update documentation to match parameter name, hmm.Correct the documentation must be, young padawan. Parameter name in comments still refers to
geoFPV
, while in signaturegeoPV
it has become. Consistency in documentation, maintain we must.Apply this change, you should:
/// @brief Convert a GeoTube to a detector element and surface /// - /// @param geoFPV The full physical volume to convert (contains shape) + /// @param geoPV The physical volume to convert (contains shape) /// @param geoTube The GeoTube shape to convert
38-41
: Wise choice, using PVConstLink is.Strong with the Force, this design change is. Smart pointer
PVConstLink
for volume handling, better memory management and ownership semantics it provides. Aligns with the path to subvolume extraction, this does.A suggestion for future path, I have:
- Consider documenting lifetime guarantees that
PVConstLink
provides- Thread safety implications, document you should
- Clear in documentation be, about when volumes may be null
Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoUnionDoubleTrdConverter.hpp (1)
Line range hint
33-36
: Update the documentation, you must!In the comments, still speaks of
geoFPV
the parameter does, thoughgeoPV
it has become. Align the documentation with reality, we should.Apply this change:
/// @brief Convert a GeoTube to a detector element and surface /// - /// @param geoFPV The full physical volume to convert (contains shape) + /// @param geoPV The physical volume to convert (contains shape) /// @param geoTube The GeoTube shape to convertPlugins/GeoModel/include/Acts/Plugins/GeoModel/IGeoShapeConverter.hpp (1)
38-38
: Hmmmm, wise changes these are, but documentation update you need!Changed the method signatures have been, separating transformation from volume handling we now do. Clear improvements these are, yes, but documentation above methods, updated it must be! Match the new parameters
geoPV
andtransform
, it should.Apply these changes to documentation, you must:
/// @brief Convert a GeoShape to a detector element and surface /// - /// @param geoFPV The full physical volume to convert (contains shape) + /// @param geoPV The physical volume to convert (contains shape) + /// @param transform The transformation to be applied /// /// @return The detector element and surfaceAlso applies to: 46-46
Tests/UnitTests/Plugins/GeoModel/GeoModelDetectorElementTests.cpp (1)
34-36
: Additional test cases, consider you must.Strong in the Force this test is, but incomplete still. Additional scenarios test we should:
- Null pointer handling
- Invalid geometry configurations
- Transform variations
Add these test cases, you should:
BOOST_AUTO_TEST_CASE(GeoModelDetectorElementEdgeCases) { // Test null pointer handling BOOST_CHECK_THROW({ PVConstLink nullPhys{nullptr}; Acts::GeoModelDetectorElement::createDetectorElement<Acts::PlaneSurface>( nullPhys, rBounds, Acts::Transform3::Identity(), 2.0); }, std::runtime_error); // Test with different transforms auto rotatedTransform = Acts::Transform3::Identity() * Acts::Transform3::RotationX(M_PI/4); auto elementRotated = Acts::GeoModelDetectorElement::createDetectorElement<Acts::PlaneSurface>( physXY, rBounds, rotatedTransform, 2.0); BOOST_CHECK(elementRotated != nullptr); }Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GenericGeoShapeConverter.hpp (2)
22-29
: Hmmmm, good changes these are, approve them I do!Wise decision it is, to use PVConstLink and explicit transform. Improved memory safety and control, this brings. Clear separation of concerns, we now have.
Consider in future iterations:
- Documentation for transform parameter add, you should
- Thread safety implications document, you must
39-48
: Strong with the Force, this implementation is!Consistent with toSensitiveSurface, these changes are. Robust error handling, you maintain. But a small suggestion, I have.
More concise this could be:
- auto res = Converter{}(geoPV, *concreteShape, transform, false); - if (!res.ok()) { - return res.error(); - } + ACTS_TRY_RESULT(auto [el, surface], + (Converter{}(geoPV, *concreteShape, transform, false)));Cleaner error propagation, this brings. Optional, but elegant, it is.
Plugins/GeoModel/src/detail/GeoShiftConverter.cpp (2)
62-65
: Consistent implementation across shape types, there is.The force is strong with this code. Three shape types it handles:
- TrapezoidBounds with GeoTrd
- RectangleBounds with GeoBox
- LineBounds with GeoTube
Yet, a suggestion I have - document the fallback order, we should. Important for maintainers to understand the conversion strategy, it is.
Add this documentation above the operator():
/// Attempts to convert the shape in the following order: /// 1. GeoTrd to TrapezoidBounds /// 2. GeoBox to RectangleBounds /// 3. GeoTube to LineBounds (straw by default) /// @returns GeoModelConversionError::WrongShapeForConverter if no conversion succeedsAlso applies to: 72-72, 80-80
Line range hint
1-89
: Architecture wisdom, share I must.For future consideration, extensible this converter system should be. Template-based registration of shape converters, consider you might. Easier to add new shape types, it would make.
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelConverters.hpp (2)
72-73
: Missing documentation, I sense. Clarify the function's purpose, we must.Add detailed documentation for the function, including:
- Parameter description
- Return value semantics
- Behavior for unknown shape IDs
/// @brief The map that maps the converters with the shapes +/// @brief Returns a converter for the given geometric shape ID +/// @param geoShapeId The ID of the geometric shape to convert +/// @return A shared pointer to the converter if found, nullptr otherwise +/// @note The converters are stored in a static map for efficiency
74-90
: Improvements to the Force, suggest I must.Several enhancements, consider you should:
- Make parameter const for clarity of intent
- Separate static map initialization for better readability
- Add debug logging when shape ID unknown is
-std::shared_ptr<const IGeoShapeConverter> GeoShapesConverters(int geoShapeId) { +std::shared_ptr<const IGeoShapeConverter> GeoShapesConverters(const int geoShapeId) { static const std::unordered_map<int, std::shared_ptr<const IGeoShapeConverter>> - converters{ - {GeoBox::getClassTypeID(), std::make_shared<GeoBoxConverter>()}, - {GeoShapeIntersection::getClassTypeID(), - std::make_shared<GeoIntersectionAnnulusConverter>()}, - {GeoShapeShift::getClassTypeID(), - std::make_shared<GeoShiftConverter>()}, - {GeoTrd::getClassTypeID(), std::make_shared<GeoTrdConverter>()}, - {GeoTube::getClassTypeID(), std::make_shared<GeoTubeConverter>()}, - {GeoShapeUnion::getClassTypeID(), - std::make_shared<GeoUnionDoubleTrdConverter>()}}; + converters = createConvertersMap(); auto itr = converters.find(geoShapeId); + if (itr == converters.end()) { + ACTS_DEBUG("No converter found for shape ID: " << geoShapeId); + } return itr != converters.end() ? itr->second : nullptr; }; + +namespace { +static std::unordered_map<int, std::shared_ptr<const IGeoShapeConverter>> +createConvertersMap() { + return { + {GeoBox::getClassTypeID(), std::make_shared<GeoBoxConverter>()}, + {GeoShapeIntersection::getClassTypeID(), + std::make_shared<GeoIntersectionAnnulusConverter>()}, + {GeoShapeShift::getClassTypeID(), + std::make_shared<GeoShiftConverter>()}, + {GeoTrd::getClassTypeID(), std::make_shared<GeoTrdConverter>()}, + {GeoTube::getClassTypeID(), std::make_shared<GeoTubeConverter>()}, + {GeoShapeUnion::getClassTypeID(), + std::make_shared<GeoUnionDoubleTrdConverter>()}}; +} +} // namespacePlugins/GeoModel/src/detail/GeoTubeConverter.cpp (1)
84-87
: For CylinderSurface creation, a suggestion I have.Empty line 84, unnecessary it is. Remove it we should, for cleaner code flow, yes.
auto surface = Surface::makeShared<CylinderSurface>(transform, cylinderBounds); return std::make_tuple(nullptr, surface); } // Create the element and the surface - auto detectorElement =
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorElement.hpp (1)
18-20
: Redundant forward declaration, I sense.Hmmmm. Include GeoFullPhysVol.h you do, yet forward declare GeoVPhysVol you still attempt. Unnecessary this is, when the full header present already is.
#include <GeoModelKernel/GeoFullPhysVol.h> -class GeoVPhysVol;Plugins/GeoModel/src/detail/GeoTrdConverter.cpp (2)
94-101
: Consider documenting thickness parameter's significance, we should.The thickness parameter in detector element creation, mysterious it remains. Documentation for future Padawans, helpful it would be.
Add this documentation above line 98:
+ // Thickness represents the material depth of the detector element
Line range hint
1-101
: A suggestion for future improvements, I have.Consider refactoring the transformation logic into a separate method, we should. Improve readability and testability, it would.
+ private: + // Helper method to compute the transformation matrix + Transform3 computeTransform(const Transform3& absTransform, + bool isXTrapezoidal, + int swapZ) const { + Transform3 transform = Transform3::Identity(); + transform.translation() = unitLength * absTransform.translation(); + RotationMatrix3 trotation = absTransform.rotation(); + + if (isXTrapezoidal) { + trotation.col(1) = swapZ * absTransform.rotation().col(2); + trotation.col(2) = -swapZ * absTransform.rotation().col(1); + } else { + trotation.col(0) = absTransform.rotation().col(1); + trotation.col(1) = swapZ * absTransform.rotation().col(2); + trotation.col(2) = swapZ * absTransform.rotation().col(0); + } + transform.linear() = trotation; + return transform; + }Tests/UnitTests/Plugins/GeoModel/GeoBoxConverterTests.cpp (2)
64-70
: Duplicate code patterns, I observe.Similar to XY plane test, this YZ plane setup is. Extract common setup into helper function, we should. DRY principles, follow we must.
Consider this pattern, you should:
+namespace { +PVConstLink createTestVolume(const std::string& name, double x, double y, double z, + GeoMaterial* material) { + auto box = make_intrusive<GeoBox>(x, y, z); + auto log = make_intrusive<GeoLogVol>(name, box, material); + return PVConstLink{make_intrusive<GeoFullPhysVol>(log)}; +} +} BOOST_AUTO_TEST_CASE(GeoBoxToSensitiveConversion) { auto material = make_intrusive<GeoMaterial>("Material", 1.0); - auto boxYZ = make_intrusive<GeoBox>(2, 200, 300); - auto logYZ = make_intrusive<GeoLogVol>("LogVolumeYZ", boxYZ, material); - PVConstLink physYZ{make_intrusive<GeoFullPhysVol>(logYZ)}; + auto physYZ = createTestVolume("LogVolumeYZ", 2, 200, 300, material.get());
Line range hint
1-121
: Missing test cases, I sense in the Force.Coverage gaps exist:
- Negative dimensions, handle we must
- Zero dimensions, test we should
- Error cases for invalid transforms, verify we need
Add these test cases, you should:
BOOST_AUTO_TEST_CASE(GeoBoxInvalidInputs) { auto material = make_intrusive<GeoMaterial>("Material", 1.0); // Test negative dimensions auto boxNeg = make_intrusive<GeoBox>(-100, 200, 2); auto logNeg = make_intrusive<GeoLogVol>("LogVolumeNeg", boxNeg, material); PVConstLink physNeg{make_intrusive<GeoFullPhysVol>(logNeg)}; auto converted = Acts::GeoBoxConverter{}.toSensitiveSurface( physNeg, Acts::Transform3::Identity()); BOOST_CHECK(!converted.ok()); // Test zero dimensions auto boxZero = make_intrusive<GeoBox>(0, 0, 0); // ... continue with similar pattern }Plugins/GeoModel/src/detail/GeoUnionDoubleTrdConverter.cpp (1)
146-149
: Remove commented code, we must!Hmmmm... Commented code, a path to the dark side it is. Clean and maintainable codebase, we seek.
Apply this change, you should:
- // PVConstLink pVolume{&geoFPV}; auto detectorElement = GeoModelDetectorElement::createDetectorElement<PlaneSurface>( geoPV, trapezoidBounds, transform, elA->thickness());
Tests/UnitTests/Plugins/GeoModel/GeoTrdConverterTests.cpp (1)
180-188
: Documentation, improve you should!Hmm, test the error case you do, but explain why invalid this geometry is, you do not. Add comments to share your wisdom with future Padawans, you must.
Add this comment before the test case:
+ // Test invalid geometry where both X and Y dimensions are trapezoidal + // This is not supported as it cannot be represented as a single Acts surfaceExamples/Python/src/GeoModel.cpp (1)
117-118
: Documentation improvements, suggest I must.Help future Padawans understand this power, we should. Add Python docstring for convertSubVolumes configuration option, wise it would be.
Apply this enhancement, you should:
.def_readwrite( "convertSubVolumes", - &Acts::GeoModelDetectorSurfaceFactory::Config::convertSubVolumes) + &Acts::GeoModelDetectorSurfaceFactory::Config::convertSubVolumes, + "Enable conversion of subvolumes within the GeoModel hierarchy")Examples/Scripts/Python/geomodel.py (2)
32-48
: Enhance the help text for clarity, young padawan.More specific, the help text must be. For the name and material lists, examples and format specifications, provide you should.
- help="List of Name List for the Surface Factory", + help="List of names for the Surface Factory (e.g., 'Chamber1,Chamber2')", - help="List of Material List for the Surface Factory", + help="List of materials for the Surface Factory (e.g., 'Silicon,Beryllium')",
148-214
: Refactor this large block into smaller functions, we should.Too long this function has grown. Split it into smaller functions, we must. Easier to maintain and understand, it will be.
Consider extracting these functions:
create_blueprint(args, gmSurfaces, gmTree, logLevel)
create_detector(args, gmBlueprint, materialDecorator, logLevel)
create_svg_visualization(args, detector, gContext)
Example for one such function:
def create_svg_visualization(args, detector, gContext): if not args.output_svg: return surfaceStyle = acts.svg.Style() surfaceStyle.fillColor = [5, 150, 245] surfaceStyle.fillOpacity = 0.5 surfaceOptions = acts.svg.SurfaceOptions() surfaceOptions.style = surfaceStyle volumeOptions = acts.svg.DetectorVolumeOptions() volumeOptions.surfaceOptions = surfaceOptions xyRange = acts.Extent([[acts.Binning.z, [-50, 50]]]) zrRange = acts.Extent([[acts.Binning.phi, [-0.8, 0.8]]]) acts.svg.viewDetector( gContext, detector, args.top_node, [[ivol, volumeOptions] for ivol in range(detector.numberVolumes())], [ ["xy", ["sensitives", "portals"], xyRange], ["zr", ["sensitives", "portals", "materials"], zrRange], ], args.output + "_detector", )🧰 Tools
🪛 Ruff (0.8.0)
196-196: Local variable
viewRange
is assigned to but never usedRemove assignment to unused variable
viewRange
(F841)
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorSurfaceFactory.hpp (2)
43-43
: Extra slashes in comment, there are.Redundant slashes remove, you should.
Apply this diff:
- // /// List for names to match + /// List for names to match
111-111
: Pass strings by const reference, efficient it is.In 'matchFunc', pass 'const std::string&' instead of 'std::string' you should, to avoid unnecessary copies.
Apply this diff:
- std::function<bool(std::string, PVConstLink)> matchFunc); + std::function<bool(const std::string&, PVConstLink)> matchFunc);Plugins/GeoModel/src/GeoModelDetectorSurfaceFactory.cpp (1)
9-14
: Elaborate, you must, on whyGeoModelTree.hpp
included first is.To aid future maintainers, a detailed comment explaining the necessity of this inclusion, add you should.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (30)
CMakeLists.txt
(1 hunks)Examples/Python/src/GeoModel.cpp
(1 hunks)Examples/Scripts/Python/geomodel.py
(5 hunks)Plugins/GeoModel/CMakeLists.txt
(1 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelConverters.hpp
(2 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorElement.hpp
(5 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorSurfaceFactory.hpp
(4 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelTree.hpp
(1 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/IGeoShapeConverter.hpp
(1 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GenericGeoShapeConverter.hpp
(2 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoBoxConverter.hpp
(1 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoIntersectionAnnulusConverter.hpp
(1 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoShiftConverter.hpp
(1 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoTrdConverter.hpp
(1 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoTubeConverter.hpp
(1 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoUnionDoubleTrdConverter.hpp
(1 hunks)Plugins/GeoModel/src/GeoModelBlueprintCreater.cpp
(1 hunks)Plugins/GeoModel/src/GeoModelDetectorElement.cpp
(2 hunks)Plugins/GeoModel/src/GeoModelDetectorSurfaceFactory.cpp
(4 hunks)Plugins/GeoModel/src/detail/GeoBoxConverter.cpp
(2 hunks)Plugins/GeoModel/src/detail/GeoIntersectionAnnulusConverter.cpp
(2 hunks)Plugins/GeoModel/src/detail/GeoShiftConverter.cpp
(3 hunks)Plugins/GeoModel/src/detail/GeoTrdConverter.cpp
(2 hunks)Plugins/GeoModel/src/detail/GeoTubeConverter.cpp
(4 hunks)Plugins/GeoModel/src/detail/GeoUnionDoubleTrdConverter.cpp
(3 hunks)Tests/UnitTests/Plugins/GeoModel/GeoBoxConverterTests.cpp
(3 hunks)Tests/UnitTests/Plugins/GeoModel/GeoModelDetectorElementTests.cpp
(1 hunks)Tests/UnitTests/Plugins/GeoModel/GeoTrdConverterTests.cpp
(5 hunks)cmake/ActsConfig.cmake.in
(1 hunks)cmake/ActsExternSources.cmake
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CMakeLists.txt
🧰 Additional context used
🪛 Ruff (0.8.0)
Examples/Scripts/Python/geomodel.py
196-196: Local variable viewRange
is assigned to but never used
Remove assignment to unused variable viewRange
(F841)
🔇 Additional comments (53)
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelTree.hpp (1)
21-21
: Missing include for PVConstLink, I sense. Fix this, we must.
Wise choice it is, to use PVConstLink
instead of raw pointer. Safer memory management, this brings. But include directive for PVConstLink
, missing it is.
Run this command to find the proper include path, we shall:
Plugins/GeoModel/CMakeLists.txt (2)
19-24
: Proper path configuration, I sense. Approve this change, I do!
Correctly structured, the include paths are. Both build-time and install-time configurations, handled properly they are.
25-26
: Verify the GeoModel version compatibility, we must!
Changed, the dependencies have. Added GeoModelCore::GeoModelHelpers has been. But verify the version compatibility across the project, we should.
✅ Verification successful
Verified, the GeoModel version compatibility is! Safe, this change appears to be.
Found the version requirements, I have. Set to 6.2.0, the GeoModel version is, and consistently used across the project, it is. The new dependency GeoModelCore::GeoModelHelpers aligns with the same version constraints, hmm. Proper version handling through CMake's find_package mechanism, I observe.
- In root CMakeLists.txt, version variable defined:
set(_acts_geomodel_version 6.2.0)
- Used consistently for both GeoModelCore and GeoModelIO packages
- Proper version checks in package configuration, implemented they are
- New dependency follows same version constraints, yes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GeoModel version requirements across the project
echo "Searching for GeoModel version requirements..."
rg -g 'CMakeLists.txt' -g '*.cmake' "find_package.*GeoModel" -A 5
rg -g 'CMakeLists.txt' -g '*.cmake' "GeoModel.*VERSION" -A 5
Length of output: 574
Script:
#!/bin/bash
# Search for GeoModel version variable definition
echo "Searching for GeoModel version variable..."
rg -g 'CMakeLists.txt' -g '*.cmake' "_acts_geomodel_version"
# Check if there are any version constraints in the package configuration
echo "Checking package configuration..."
fd "Config.*cmake" --exec cat {}
# Look for any GeoModel dependencies in the main CMake files
echo "Checking GeoModel dependencies..."
fd "CMakeLists.txt" --exec grep -A 5 "GeoModel" {}
Length of output: 10865
Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoIntersectionAnnulusConverter.hpp (1)
33-34
: Breaking change in operator() signature, approve I do!
Changed the parameter type has been, from GeoFullPhysVol
to PVConstLink
. Wise this change is, aligning with the broader updates across the codebase, hmm.
Search for other affected converters, we must:
✅ Verification successful
Consistent with broader changes, this parameter type modification is, hmm!
Verified I have, that all GeoModel converters consistently use PVConstLink parameter type they do. A systematic change across the codebase this is, not an isolated modification. In the GeoModel plugin, similar parameter signatures in other converter implementations found I have:
- GeoBoxConverter
- GeoTrdConverter
- GeoTubeConverter
- GeoShiftConverter
- GeoUnionDoubleTrdConverter
All these converters, the same parameter type pattern follow they do. A deliberate design choice this appears to be, for consistency and type safety across the GeoModel integration, yes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all converter implementations that might need similar updates
ast-grep --pattern 'struct Geo$_Converter {
$$$
operator()($$$) {
$$$
}
}'
Length of output: 86
Script:
#!/bin/bash
# Search for files containing GeoConverter in their names
fd "Geo.*Converter" --type f
# Search for operator() implementations in converter files
rg "operator\(\)" --type cpp -A 5
Length of output: 132890
Script:
#!/bin/bash
# Search for PVConstLink usage
rg "PVConstLink" --type cpp -A 2
Length of output: 14481
Script:
#!/bin/bash
# Search for GeoFullPhysVol usage
rg "GeoFullPhysVol" --type cpp -A 2
Length of output: 9181
Script:
#!/bin/bash
# Search specifically in the GeoModel plugin directory
fd . "Plugins/GeoModel" --type f --exec rg -l "operator\(\)"
Length of output: 852
Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoShiftConverter.hpp (1)
32-35
: Wise change to PVConstLink, this is!
Strong with the Force, this change is. Aligns with the PR objectives of accepting PVConstLink volumes, it does. Maintains const correctness and backward compatibility, it also does. Approve this change, I do.
Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoBoxConverter.hpp (2)
32-35
: Wise choice, the signature change is! Hmmmm.
Smart pointer PVConstLink
use, better memory management it brings. Consistent with other converters, this change is. More flexible handling of volumes, it allows.
32-35
: Verify the impact on existing code, we must.
Breaking change this is. All callers of GeoBoxConverter
, updated they must be.
Plugins/GeoModel/src/GeoModelDetectorElement.cpp (1)
40-41
: Simple and elegant, this implementation is.
Return type to PVConstLink changed, it has been. Direct return of m_geoPhysVol, cleaner code it makes. With the constructor changes, consistency maintained it is.
Verify the impact of these changes, we must:
✅ Verification successful
Let us deeper into the code look, hmm? More context we need, to understand the changes' impact, yes.
Correct and consistent, the changes are. No disturbance in the Force, I sense.
Through the Force, seen I have that PVConstLink
consistently used throughout the codebase it is. The change from GeoFullPhysVol
to PVConstLink
in the return type, aligned with the existing pattern it is. In constructor and member variable, same type already used it was.
A natural evolution of the interface this is, bringing harmony to the code. No breaking changes introduced, as PVConstLink
already the established type throughout the codebase it was.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct GeoFullPhysVol usage
rg -l "GeoFullPhysVol[^C]" --type cpp
# Find all files that might need similar updates
ast-grep --pattern 'const GeoFullPhysVol& $_'
Length of output: 1360
Script:
#!/bin/bash
# Search for actual usage patterns of GeoFullPhysVol in implementation files
rg "GeoFullPhysVol" -B2 -A2 --type cpp Plugins/GeoModel/src/
# Check header file for the type definition changes
cat Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorElement.hpp
# Look for PVConstLink usage patterns
rg "PVConstLink" -B2 -A2 --type cpp
Length of output: 29407
Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoTrdConverter.hpp (1)
37-40
: Hmmmm, wise change this is, approve I do!
Aligned with the Force, this signature change is. From GeoFullPhysVol to PVConstLink, a natural evolution I sense.
Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GeoUnionDoubleTrdConverter.hpp (1)
39-42
: Hmmmm, wise change this is, verify its usage we must!
Changed parameter type from GeoFullPhysVol
to PVConstLink
, align with the Force of smart pointers it does. Consistent with broader changes across the codebase, this modification is.
Run this verification script, we shall:
✅ Verification successful
Strong in the Force, this change is. Verified it has been!
Through the Force, seen I have that PVConstLink
parameter type consistently used it is across all shape converters in the codebase:
- In
GeoBoxConverter
,GeoTrdConverter
,GeoTubeConverter
and others, same parameter signature they share - Test files demonstrate proper usage with
make_intrusive<GeoFullPhysVol>
they do - Factory and detector element classes, aligned with this pattern they are
Harmonious balance in the codebase, this change maintains. No disturbance in the Force, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other converters to ensure consistent parameter type usage
# Search for other converter implementations
ast-grep --pattern 'Result<$_> operator()($_, $_)'
# Search for PVConstLink usage patterns
rg "PVConstLink" -A 2
Length of output: 14523
Plugins/GeoModel/include/Acts/Plugins/GeoModel/IGeoShapeConverter.hpp (1)
38-38
: Strong with the Force, this architectural change is!
Decoupled the transformation from physical volume representation, we have. Smart pointer PVConstLink
and explicit transform parameter, better flexibility they provide. Align with the ways of handling subvolumes in Muon System, this does.
Verify the impact across the codebase, we must:
Also applies to: 46-46
✅ Verification successful
Successful, the architectural change is! Properly implemented across the codebase, it has been.
Through the Force, seen I have that all shape converters correctly handle the decoupled transform parameter. In harmony with the new architecture, they are:
- In
GenericGeoShapeConverter
, properly forwarded the transform is - In concrete converters like
GeoBoxConverter
,GeoTrdConverter
, andGeoTubeConverter
, correctly used the transform parameter they have - In test files only, the
GeoFullPhysVol
references remain, as expected this is
No disturbance in the Force, I sense. Clean and consistent, the implementation is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations of IGeoShapeConverter interface
ast-grep --pattern 'class $_ : public IGeoShapeConverter {
$$$
}'
# Check for any remaining uses of GeoFullPhysVol in converter implementations
rg "GeoFullPhysVol" --type cpp
Length of output: 3521
Script:
#!/bin/bash
# Search for implementations of IGeoShapeConverter to see how they handle the transform
ast-grep --pattern 'class $_ : public IGeoShapeConverter {
$$$
convertImpl($_, $_, const Transform3& transform) {
$$$
}
}'
# Check for any direct transform access in the old code that might be affected
rg "getTransform|transform()" --type cpp -A 2 Plugins/GeoModel/
Length of output: 15493
Tests/UnitTests/Plugins/GeoModel/GeoModelDetectorElementTests.cpp (1)
32-33
: Wise choice, the use of PVConstLink is! Hmmmm.
Safer memory handling, this change brings. Through const-correctness and proper ownership semantics, protected our resources are.
Plugins/GeoModel/include/Acts/Plugins/GeoModel/detail/GenericGeoShapeConverter.hpp (1)
Line range hint 22-48
: Verify usage patterns across the codebase, we must!
Changed signatures ripple through the codebase, they do. Ensure consistent usage, we should.
✅ Verification successful
In harmony with the Force, these changes are!
Through the Force, seen I have that the interface IGeoShapeConverter
and its implementations maintain consistency they do. The parameter types PVConstLink
and Transform3
across the codebase aligned they are. In the test files only, the GeoFullPhysVol
usage appears, where appropriate it is.
No disturbance in the Force I sense. The changes, properly implemented they are!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential inconsistent usage patterns
ast-grep --pattern 'toSensitiveSurface($$$)'
ast-grep --pattern 'toPassiveSurface($$$)'
# Check for any remaining GeoFullPhysVol usage
rg "GeoFullPhysVol[^.]" --type cpp
Length of output: 2234
Script:
#!/bin/bash
# Search for implementations of toSensitiveSurface and toPassiveSurface
ast-grep --pattern 'toSensitiveSurface($$$) {
$$$
}'
ast-grep --pattern 'toPassiveSurface($$$) {
$$$
}'
# Check interface definition
rg "class IGeoShapeConverter" -A 20 --type cpp
Length of output: 2707
Plugins/GeoModel/src/detail/GeoBoxConverter.cpp (2)
67-68
: Correct usage of PVConstLink in detector element creation, I sense.
Properly adapted the detector element creation has been. Maintained the essential functionality while embracing the new parameter type, it has. Approve this change, I do.
25-28
: Approve the parameter type change, but verify consistency, hmmmm.
Changed from GeoFullPhysVol to PVConstLink, the parameter type has. Wise this change is, aligning with the PR's objectives it does. Yet verify its consistency across the codebase, we must.
✅ Verification successful
Let me search the codebase with a different approach, using ripgrep for broader context and file discovery, hmm. The previous ast-grep pattern might have been too specific, yes. A wider search, we need.
Approve this change, we must. Consistent across the codebase, it is.
Through the Force, seen I have that all GeoModel converters now use PVConstLink consistently. The old GeoFullPhysVol parameter appears only in test files and headers, where it is properly converted to PVConstLink. A harmonious transition, this is, hmmmm.
- In GeoBoxConverterTests.cpp and GeoTrdConverterTests.cpp, GeoFullPhysVol creates PVConstLink objects
- In production code, all converters use PVConstLink parameter type
- No lingering GeoFullPhysVol parameters in converter implementations remain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining GeoFullPhysVol parameters in converter classes
ast-grep --pattern 'class $_ {
$$$
operator()($$$GeoFullPhysVol$$$) {
$$$
}
}'
# Search for PVConstLink usage in other converters
ast-grep --pattern 'class $_ {
$$$
operator()($$$PVConstLink$$$) {
$$$
}
}'
Length of output: 178
Script:
#!/bin/bash
# Search for GeoFullPhysVol in all files
rg "GeoFullPhysVol" -A 2 -B 2
# Search for PVConstLink in all files
rg "PVConstLink" -A 2 -B 2
# Find all converter files
fd "Converter.*\.(cpp|hpp)"
Length of output: 44157
cmake/ActsExternSources.cmake (1)
34-34
: Hmmmm, verify the stability of GeoModel 6.2.0, we must!
A significant version leap this is, from 4.6.0 to 6.2.0. Careful consideration, such changes require. Breaking changes, they may bring.
Run this script to investigate the version's stability, we shall:
Consider pinning to specific commit hash instead of version tag, more stability it brings. Like this, you should format it:
- "GIT_REPOSITORY;https://gitlab.cern.ch/GeoModelDev/GeoModel;GIT_TAG;6.2.0;" CACHE STRING "Source to take GeoModel from")
+ "GIT_REPOSITORY;https://gitlab.cern.ch/GeoModelDev/GeoModel;GIT_TAG;<commit-hash>;" CACHE STRING "Source to take GeoModel from")
Plugins/GeoModel/src/detail/GeoShiftConverter.cpp (2)
23-24
: Wise change in parameter type, I sense.
From GeoFullPhysVol
to PVConstLink
, the parameter type has changed. Aligned with the PR objectives, this modification is, as it supports the new approach to volume handling, hmmmm.
37-37
: Careful with error handling, we must be.
The conversion result handling, robust it is. Yet, verify we must that all shape converters properly handle the new PVConstLink
parameter.
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelConverters.hpp (2)
26-26
: Wise choice, the unordered_map header is, hmmmm.
Correctly placed among the STL includes, it is. Previous suggestion, followed you have.
74-90
: Verify the completeness of our converter collection, we must.
Search for any shape types that might be missing from our converter map, we should.
✅ Verification successful
Let me search deeper into the Force to find all shape classes and their converters, I must.
Complete and balanced, the converter collection is, hmmmm.
Through the Force, I sense all shape types are properly accounted for, yes. The converters map contains:
- GeoBox with GeoBoxConverter
- GeoShapeIntersection with GeoIntersectionAnnulusConverter
- GeoShapeShift with GeoShiftConverter
- GeoTrd with GeoTrdConverter
- GeoTube with GeoTubeConverter
- GeoShapeUnion with GeoUnionDoubleTrdConverter
Match perfectly with the shape classes found in our codebase, these converters do. No missing converters, there are. In harmony with the Force, this implementation is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for GeoShape derived classes that might need converters
# Look for classes deriving from GeoShape
ast-grep --pattern 'class $_ : public GeoShape'
# Look for getClassTypeID usage to find shape types
rg 'getClassTypeID\(\)'
Length of output: 861
Script:
#!/bin/bash
# Search for all GeoShape-related classes and their inheritance
rg -B2 -A2 'class.*Geo.*Shape|class.*Geo.*(Box|Trd|Tube)'
# Search for shape-related converter implementations
rg -l 'class.*Converter'
Length of output: 6252
Plugins/GeoModel/src/detail/GeoTubeConverter.cpp (4)
Line range hint 29-33
: Approve parameter type change to PVConstLink, hmm yes.
Wise choice, the switch to PVConstLink is. Safer memory handling and consistent reference semantics, it provides. With the Force of modern C++, aligned this change is.
57-59
: For StrawSurface creation, proper the implementation is.
Handle detector element creation with grace, this code does. Proper surface binding and transformation, I sense. Strong with the Force, this implementation is.
72-74
: For DiscSurface creation, balanced the approach is.
Mirror the pattern of StrawSurface, this implementation does. Consistency in the code, brings harmony it does. Approve this way of the Force, I do.
Line range hint 29-90
: Verify the surface creation paths, we must.
Test all three surface types, essential it is. Ensure proper behavior with the new PVConstLink parameter, we should.
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorElement.hpp (4)
53-54
: Wise choice, the PVConstLink path is.
Strong with the Force, this change is. GeoModel's smart pointer semantics, it respects. Safe memory management, it provides.
69-71
: Balanced and harmonious, the constructor signature now is.
Consistency in the Force, I sense. The PVConstLink reference, well-aligned with factory method it is. Good practices, these changes follow.
99-99
: Complete, the transition to PVConstLink now is.
Member variable type, aligned with the whole it is. Balanced and complete, the changes are.
88-88
: Const-correctness maintained, it is. Yet verify usage we must.
Return type change, proper it is. But throughout the codebase, its impact we must verify.
✅ Verification successful
Verified, the change is. Safe to proceed, we are.
Through the Force, searched the codebase I have. Only two usages found:
- Implementation in
GeoModelDetectorElement.cpp
, correctly returnsm_geoPhysVol
- Usage in
GeoShiftConverter.cpp
, compatible withPVConstLink
return type it is
Hmmmm, yes. Clear it is that no breaking changes this modification brings. In harmony with the codebase, it remains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to physicalVolume() to verify compatibility
rg "physicalVolume\(\)" --type cpp -A 2 -B 2
Length of output: 1362
Plugins/GeoModel/src/detail/GeoTrdConverter.cpp (3)
27-30
: Approve the parameter type change, I do.
Wise decision it is, to use PVConstLink instead of GeoFullPhysVol. Aligns with the broader changes in the codebase, this does. Consistent with the PR objectives for the Muon System conversion, hmmmm.
87-92
: Handle the non-sensitive case with grace, you do.
Clean and clear, the separation of concerns is. Return nullptr for non-sensitive elements, wisdom shows.
Line range hint 31-86
: Robust and well-structured, the geometry transformation logic is.
Strong with the Force, this implementation is. Proper handling of unit conversions and coordinate transformations, it has. Particularly wise, the trapezoidal ambiguity check is.
Yet, verify the rotation matrix calculations, we must.
✅ Verification successful
Correct and well-implemented, the rotation matrix transformations are.
Verified through tests, the implementation is. Match perfectly with test expectations, the rotation matrix calculations do:
-
For diffX > diffY case (x,z,y transformation):
- Acyclic rotation with sign change, correctly implemented it is
- Proven by test cases in GeoTrdConverterTests.cpp, hmm yes
-
For diffX < diffY case (y,z,x transformation):
- Cyclic rotation without sign change, properly handled it is
- Validated by test assertions for both normal and swapped cases
The Force is strong with this code, as thorough test coverage shows all rotation cases working as intended, including edge cases and sign handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar rotation matrix transformations in the codebase
ast-grep --pattern 'trotation.col($_) = $_ * absTransform.rotation().col($_)'
Length of output: 79
Script:
#!/bin/bash
# Search for rotation matrix transformations in a broader context
rg -U "trotation.*rotation" -A 5 -B 5
# Also search for similar transformations with different variable names
rg -U "\.col\([0-2]\).*rotation\.col\([0-2]\)" -A 5 -B 5
# Look for other GeoTrd converters or similar geometry transformations
fd -e cpp -e h "Trd" --exec grep -l "rotation"
Length of output: 2122
Script:
#!/bin/bash
# Examine the test files to verify rotation matrix transformations
cat Tests/UnitTests/Plugins/GeoModel/GeoTrdConverterTests.cpp
# Check TGeo implementations for comparison
cat Tests/UnitTests/Plugins/TGeo/TGeoTrd1ConversionTests.cpp
cat Tests/UnitTests/Plugins/TGeo/TGeoTrd2ConversionTests.cpp
Length of output: 18319
Plugins/GeoModel/src/detail/GeoIntersectionAnnulusConverter.cpp (3)
29-29
: Wise change in parameter type, I sense.
From GeoFullPhysVol to PVConstLink, the path leads. Consistent with the broader changes in the codebase, this modification is. Smart pointer semantics, it brings.
Line range hint 1-98
: Verify the geometric calculations, we must.
Complex geometric operations, this converter handles. Ensure correctness of unit conversions and transformations, we should.
✅ Verification successful
Verified the geometric transformations are, hmmmm.
In balance with the Force, this code is. Observe I do:
- Consistent unit conversions across all geometric converters, maintained they are
- Unique transformation pattern for annulus vertices, properly implemented it is
- Careful handling of coordinate transformations, present it shows
Through the Force, seen I have that this implementation follows the sacred patterns of other geometric converters. The way of the Jedi, this code follows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other unit conversion patterns to ensure consistency
rg -A 2 'UnitConstants::mm.*GeoModelKernelUnits'
# Look for similar geometric transformation patterns
ast-grep --pattern 'Vector3{$_}'
Length of output: 1130
Script:
#!/bin/bash
# Search for similar geometric transformations in other converters
rg -A 5 'Vector3.*shift \*'
# Look for similar vertex transformation patterns
rg -A 10 'faceVertices3D.push_back'
# Check for similar sorting patterns
rg -A 5 'std::sort.*VectorHelpers::phi'
Length of output: 1200
86-89
: Remove the commented code, you must.
Dead code, a path to confusion leads. Clean and clear, your code should be.
Apply this change, you should:
- // PVConstLink pVolume{&geoFPV};
auto detectorElement =
GeoModelDetectorElement::createDetectorElement<DiscSurface>(
geoPV, annulusBounds, annulusTransform, thickness);
Verify the detector element creation changes across the codebase, we must.
✅ Verification successful
Remove the commented code, you must. Safe, this change is.
Searched through the codebase, I have. The pattern of detector element creation, consistent it remains across many files. The commented PVConstLink
line, obsolete it has become, and no impact its removal shall have. The new creation pattern with geoPV
, annulusBounds
, annulusTransform
, and thickness
, aligned with the Force it is, matching the usage in other converters:
GeoTrdConverter.cpp
GeoBoxConverter.cpp
GeoTubeConverter.cpp
- And others, hmmmm
Clean code leads to clarity, clarity leads to understanding, understanding leads to maintainability. Remove the commented line, you should.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns of detector element creation
ast-grep --pattern 'GeoModelDetectorElement::createDetectorElement<$_>($_)'
Length of output: 77
Script:
#!/bin/bash
# Search for detector element creation patterns using ripgrep
rg -A 2 "createDetectorElement" --type cpp
# Also search for any remaining commented code in the file
rg -A 2 "^[[:space:]]*//.*PVConstLink" "Plugins/GeoModel/src/detail/GeoIntersectionAnnulusConverter.cpp"
# Check for similar patterns with different syntax
rg -A 2 "GeoModelDetectorElement::" --type cpp
Length of output: 8653
cmake/ActsConfig.cmake.in (1)
Line range hint 82-85
: Compatible with ATLAS database ATLAS-R3-MUONTEST_v3.db, these versions must be.
Hmmmm. Verify we must that GeoModel 6.2.0 compatible it is with the ATLAS database mentioned in objectives. Critical for the Muon System conversion, this compatibility is.
Execute this script, we shall:
Consider documenting database compatibility requirements in README or configuration files, you should. Help future Padawans, this will.
Tests/UnitTests/Plugins/GeoModel/GeoBoxConverterTests.cpp (3)
34-34
: Hmmmm, wise choice of smart pointers, I sense.
Memory management improved with make_intrusive
, it has. Safe from the dark side of memory leaks, we now are.
92-98
: Pattern continues in XZ plane, it does.
Third occurrence of setup pattern, I see. More evidence for refactoring need, this is. Consistent with other tests, the implementation remains.
38-41
: Strong with the Force, this test pattern is.
Clean and consistent, the XY plane test setup appears. Identity transform used wisely, it is. But verify the cleanup of resources, we must.
Also applies to: 43-44
✅ Verification successful
Safe from resource leaks, your code is, young Padawan
Wisdom in your ways, I see. Smart pointers consistently you use - make_intrusive
for all allocations, and PVConstLink
for ownership transfer. Automatic cleanup, the Force of RAII provides. No manual resource management needed, hmm.
make_intrusive
for GeoMaterial, GeoBox, GeoLogVol - proper memory management this ensuresPVConstLink
wrapper safely manages GeoFullPhysVol ownership- No raw pointers or manual cleanup required, as the Force of smart pointers strong it is
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential resource leaks in test teardown
rg -A 5 "BOOST_AUTO_TEST_CASE\(" | rg -B 5 "BOOST_AUTO_TEST_SUITE_END"
Length of output: 526
Script:
#!/bin/bash
# Search for test fixture setup/teardown in the specific test file
rg -B 5 -A 10 "BOOST_AUTO_TEST_SUITE\(" "Tests/UnitTests/Plugins/GeoModel/GeoBoxConverterTests.cpp"
# Look for any cleanup or destructor patterns
ast-grep --pattern 'class $_ {
$$$
~$_() {
$$$
}
$$$
}'
# Check if smart pointers are consistently used
rg "make_intrusive|std::unique_ptr|std::shared_ptr" "Tests/UnitTests/Plugins/GeoModel/GeoBoxConverterTests.cpp"
Length of output: 1450
Plugins/GeoModel/src/detail/GeoUnionDoubleTrdConverter.cpp (3)
50-52
: Approve parameter type change, I do!
Changed from GeoFullPhysVol to PVConstLink, the parameter type has. Consistent with the codebase changes, this modification is. Wise decision, it appears to be.
60-67
: Consistent error handling maintained, it is.
Properly updated the GeoShiftConverter calls with new parameter type, you have. Error handling remains robust, ensuring safe geometric transformations, it does.
Line range hint 1-152
: Verify geometric calculations, we must.
Complex geometric operations, this code performs. Validate the trapezoid merging logic with edge cases, we should.
Run these tests, you must:
Tests/UnitTests/Plugins/GeoModel/GeoTrdConverterTests.cpp (4)
37-43
: Wise changes these are, approve them I do!
Strong with the Force, this code is. Smart pointer usage, memory leaks prevent it does. PVConstLink, a powerful ally it has become.
75-80
: Good test coverage, have you achieved!
Different dimensions test you do. Symmetry in the Force, this brings. Well-structured, your test case is.
110-115
: Different perspective, tested well it is!
XZ plane, explored you have. Like a Jedi viewing all angles of combat, thorough your testing is.
145-150
: Complete, your test coverage is!
All angles of the trapezoid, covered they are. Balance in the tests, achieved you have.
Examples/Python/src/GeoModel.cpp (1)
117-118
: Hmmmm, wise addition this is, approve it I do!
Align with the force of Muon System integration, this change does. A new configuration option for subvolume conversion, introduced it has been.
Verify the impact on existing configurations, we must. Run this command, you shall:
✅ Verification successful
Hmmmm, safe this change is, proceed we shall!
Through the Force, seen I have that this configuration change:
- Only in Python bindings, used it is
- New property alongside existing ones like
nameList
andmaterialList
, added it was - Breaking changes, there are none, as initialization with default constructor preserved it is
Backwards compatibility maintained it is, and aligned with the ways of the Force, this change remains. No existing configurations disturbed, they shall be.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing Config usage patterns
rg -A 3 "GeoModelDetectorSurfaceFactory::Config" --type cpp
Length of output: 1457
Plugins/GeoModel/src/GeoModelBlueprintCreater.cpp (2)
43-44
: Verify impact on dependent systems, we shall.
Changed method signature may affect other parts of the system, it does. Verify all callers updated are, we must.
43-44
: Verify API change and add documentation, hmmmm.
Changed the method call has been, from getTableFromTableName
to getTableFromTableName_String
. Clear the intention is, yet documentation missing it remains. Handle errors properly, we must.
Add documentation we should:
// Retrieve table from database using string-based lookup
gmTree.geoReader->getTableFromTableName_String(options.table);
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelDetectorSurfaceFactory.hpp (4)
48-49
: New configuration parameter 'convertSubVolumes', appropriate it is.
72-72
: Default logging level changed, from INFO to WARNING it has.
Intentional this change is? Confirm you should, for logging verbosity affects it does.
95-103
: Helper method 'convertSensitive', added you have. Well-documented, it is.
104-111
: New method 'findAllSubVolumes', beneficial it is.
Implementation sound appears, and documentation clear is.
Plugins/GeoModel/src/GeoModelDetectorSurfaceFactory.cpp (1)
162-162
: Typo, there is. Correct nllptr
to nullptr
, you must.
Ensure error messages accurate are.
if(PluginGeoModel IN_LIST Acts_COMPONENTS) | ||
find_dependency(GeoModelCore @GeoModelCore_VERSION@ CONFIG EXACT) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate conditional blocks for PluginGeoModel, I sense. Remove this one, we must.
Hmmmm. Two identical conditional blocks for PluginGeoModel, there are. The first block at lines 70-72, incomplete it is, while a complete block with both dependencies later exists. Confusion and maintenance difficulties, this duplication brings.
Apply this change, you should:
-if(PluginGeoModel IN_LIST Acts_COMPONENTS)
- find_dependency(GeoModelCore @GeoModelCore_VERSION@ CONFIG EXACT)
-endif()
💡 Codebase verification
Inconsistency in the Force, I sense. Fix it, we must.
In the same file, two different version variables for GeoModel appear:
- Line 70:
@GeoModelCore_VERSION@
- Line 82-83:
@GeoModel_VERSION@
used for both GeoModelCore and GeoModelIO
A disturbance in the versioning, this creates. Same version variable throughout the file, we should use. Choose @GeoModelCore_VERSION@
for GeoModelCore and align GeoModelIO accordingly, we must.
- Line 82: Replace
@GeoModel_VERSION@
with@GeoModelCore_VERSION@
- Line 83: Replace
@GeoModel_VERSION@
with@GeoModelCore_VERSION@
🔗 Analysis chain
Version variables, different they are. Consistency we need.
Observe closely, I do, that @GeoModelCore_VERSION@
and @GeoModel_VERSION@
differently named they are. Ensure matching versions between GeoModelCore and GeoModelIO, we must, as together they work.
Run this verification script, you shall:
Also applies to: 82-85
The previous command failed, young padawan. A new approach, we must try. Search for GeoModel references in CMake files directly, we shall. Through the Force, guide us to understanding these version variables, it will.
Found a CMake file with GeoModel references, we have. But its contents, examine we must. The path to wisdom, through thorough investigation it lies, hmmmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check version variables in CMake files
# Search for GeoModel version variables
rg -A 1 "GeoModel.*_VERSION" --type cmake
# Check if variables are defined
cmake -LA -N . | grep -i "GeoModel.*VERSION"
Length of output: 186
Script:
#!/bin/bash
# Search for GeoModel version references in CMake files
fd -e cmake -x cat {} | grep -A 2 "GeoModel.*VERSION"
# Find all CMake files mentioning GeoModel
fd -e cmake -x grep -l "GeoModel" {}
Length of output: 128
Script:
#!/bin/bash
# Examine the content of the found CMake file
cat ./cmake/ActsExternSources.cmake
# Also check the config file under review
cat ./cmake/ActsConfig.cmake.in
Length of output: 7415
surfaceOptions = acts.svg.SurfaceOptions() | ||
surfaceOptions.style = surfaceStyle | ||
|
||
viewRange = acts.Extent([]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the unused variable, we must.
Unused, this variable is. Remove it, we should.
- viewRange = acts.Extent([])
🧰 Tools
🪛 Ruff (0.8.0)
196-196: Local variable viewRange
is assigned to but never used
Remove assignment to unused variable viewRange
(F841)
gmFactoryConfig.materialList = args.material_list | ||
gmFactoryConfig.nameList = args.name_list | ||
gmFactoryConfig.convertSubVolumes = args.convert_subvols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Validate the inputs, we must.
Safe handling of command line arguments, crucial it is. Add validation before assignment, you should.
+ # Validate material and name lists
+ def validate_list(lst, list_name):
+ if not all(isinstance(item, str) for item in lst):
+ raise ValueError(f"All {list_name} must be strings")
+
+ validate_list(args.material_list, "materials")
+ validate_list(args.name_list, "names")
gmFactoryConfig.materialList = args.material_list
gmFactoryConfig.nameList = args.name_list
gmFactoryConfig.convertSubVolumes = args.convert_subvols
📝 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.
gmFactoryConfig.materialList = args.material_list | |
gmFactoryConfig.nameList = args.name_list | |
gmFactoryConfig.convertSubVolumes = args.convert_subvols | |
# Validate material and name lists | |
def validate_list(lst, list_name): | |
if not all(isinstance(item, str) for item in lst): | |
raise ValueError(f"All {list_name} must be strings") | |
validate_list(args.material_list, "materials") | |
validate_list(args.name_list, "names") | |
gmFactoryConfig.materialList = args.material_list | |
gmFactoryConfig.nameList = args.name_list | |
gmFactoryConfig.convertSubVolumes = args.convert_subvols |
auto blueprintTable = | ||
gmTree.geoReader->getTableFromTableName_String(options.table); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle table retrieval errors, we must.
Protect against failed table retrieval, the code does not. Add error handling, you should.
auto blueprintTable =
gmTree.geoReader->getTableFromTableName_String(options.table);
+ if (blueprintTable.empty()) {
+ throw std::runtime_error(
+ "GeoModelBlueprintCreater: Failed to retrieve table '" + options.table + "'");
+ }
📝 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.
auto blueprintTable = | |
gmTree.geoReader->getTableFromTableName_String(options.table); | |
auto blueprintTable = | |
gmTree.geoReader->getTableFromTableName_String(options.table); | |
if (blueprintTable.empty()) { | |
throw std::runtime_error( | |
"GeoModelBlueprintCreater: Failed to retrieve table '" + options.table + "'"); | |
} |
if (converter == nullptr) { | ||
throw std::runtime_error("The converter for " + recType(*shape) + | ||
" is nullptr"); | ||
|
||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreachable return
after throw
, remove you should.
Since throw
exits the function, unnecessary the return
is.
Apply this diff to fix the issue:
if (converter == nullptr) {
throw std::runtime_error("The converter for " + recType(*shape) +
" is nullptr");
- return;
}
📝 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.
if (converter == nullptr) { | |
throw std::runtime_error("The converter for " + recType(*shape) + | |
" is nullptr"); | |
return; | |
} | |
if (converter == nullptr) { | |
throw std::runtime_error("The converter for " + recType(*shape) + | |
" is nullptr"); | |
} |
This was done for the summer student's project in order to use it and develop on top of it. So it was done, I am closing it. |
This PR introduces the usage of the Geomodel Converters for the conversion of the tubes of the MDTs chambers as a starting point. I guess there are some things for discussion.
I want to clarify some things though:
The sqlite database that I use is: ATLAS-R3-MUONTEST_v3.db
1.a I have checked it with the Inner Detector and the
geomodel.py
script by python3 geomodel.py -i ATLAS-R3-MUONTEST_v2.db -q PIXEL SCT --output-obj -o id -ml std::Silicon
, without using the blueprint for now just to get the sensitive surfaces with Pixel and SCT and it works, this is what I got:I needed to change some things in the
GeomodelDetectorSurfaceFactory
and the python scriptgeomodel.py
that we use to generate the converted surfaces and dump them in the obj file:2.a The Muon System's database apparently has a different hierarchy that the one of the ID. The top-level full physical volume is actually the MDT Multilayer, the children of which are the tubelayers and the tubefoam. I am interested in the children of the tubelayers which are the sensitive tubes I want to convert. For this reason:
convert-subvolumes
argument was introduced, that allows for subvolumes extraction from the hierarchy.PVConstLink
from Geomodel. So I also needed to change what the converters accept as arguments (Now they accept the PVConstLink volume and the transform that would be handled before passing to the converter).-The DetectorElement does not hold any more the full physical volume though and I do not know if this will be a problem (I do not know what is happening with the Alignment for example etc, if it will be needed there).
2.b The Muon System is huge, so if I build everything (all the tubes from all the chambers) the file will be huge. For visualization reasons I changed the
match function
.BIL3_MDT02_-1_4_2
and the name comes from the databse). In case no name and no material provided, it goes and builds everything.std::Aluminium
) and if I only say the nameTube
it will also build the layers and the foams that carry a different material. If the material list is empty or we have a full phys vol ---> it checks just the name list.If the name list is empty ---> it checks just the material list
converters
are picked up and turned it to a function with a map, in order not to loop every time to all the converters passed in the configuration.For the Muon System, if I run the python script and get the obj with the surfaces by:
python3 geomodel.py -i ATLAS-R3-MUONTEST_v2.db -q Muon --convert-subvols --output-obj -o muons -ml Aluminium -n Tube BIL3_MDT02_-1_4_2
I get this:
for now just floating tubes, but we will continue with the other technologies with the summer student.
Also, one last thing is that in order to use some functions that allow to get the subvolumes from the full physical volume, I neede to change the tag of Geomode. But this can also be discussed.
Also by
python3 geomodel.py -i ATLAS-R3-MUONTEST_v3.db -q Muon --output-obj -o muons -n MDT -ml Air
(this picks the names and the materials of the top-level physical volumes which are the bounding boxes of the tubes --> multilayers). For the moment it is only converted as plane surfaces. Next step with the summer student: Get the converters that convert them to detector volumes (most probably take advantage of what @Matthewharri has implemented!! )Sorry for the big explanation.
I am pinging some people that may be interested and I have touched their code: @asalzburger @benjaminhuth @noemina @junggjo9 @paulgessinger
Summary by CodeRabbit
New Features
--name-list
,--material-list
,--convert-subvols
, and--enable-blueprint
).GeoModelDetectorSurfaceFactory
for handling subvolumes and sensitive surfaces.Bug Fixes
Documentation
GeoModelCore
andGeoModelIO
.Tests