refactor: Move Affine to Isometry for Acts::Transform3 #4055
Conversation
WalkthroughA significant transformation across the Acts library has occurred, focusing on the modification of transformation matrices. The changes primarily involve shifting from standard affine transformations to isometric transformations, ensuring precise geometric representations. This modification spans multiple files in the Core, Examples, Plugins, and Test directories, indicating a systematic update to how spatial transformations are handled throughout the library. Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Core
pbutti
changed the title
|
Tagging: @paulgessinger @andiwand |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Core/src/Geometry/PlaneLayer.cpp (1)
62-68: Clear and explicit, the code becomes! Approve this refactoring, I do.Separated the translation and rotation steps are. Better readability and understanding, it brings. Hmmmm.
Consider this small improvement, you might:
-const Transform3 apnTransform = Transform3(lTrans*Eigen::Isometry3d(lRotation)); -const Transform3 appTransform = Transform3(rTrans*Eigen::Isometry3d(lRotation)); +const auto isometry = Eigen::Isometry3d(lRotation); +const Transform3 apnTransform = Transform3(lTrans * isometry); +const Transform3 appTransform = Transform3(rTrans * isometry);Plugins/Json/src/AlgebraJsonConverter.cpp (1)
64-70: Cleaner approach to rotation handling, this is! Approve it, I do.Explicit handling of rotation transposition, we now have. Commented code removed, clarity gained. Hmmmm.
A small improvement in readability, suggest I do:
- //auto rotation = options.transpose ? t.rotation().transpose() : t.rotation(); - - Acts::RotationMatrix3 rotation = t.rotation(); - if (options.transpose) { - rotation.transposeInPlace(); - } + Acts::RotationMatrix3 rotation = t.rotation(); + options.transpose ? rotation.transposeInPlace() : rotation;Core/src/Geometry/CylinderVolumeBounds.cpp (2)
100-104: More descriptive error message needed, hmm.Clearer message we must have, young padawan. Context about isometric transformation requirement, add you should.
- throw std::invalid_argument("Bevel Min Z != 0 is not supported"); + throw std::invalid_argument("Bevel Min Z != 0 is not supported: beveled edges incompatible with isometric transformations");
109-113: Consistency in error messages, maintain we must.Similar improvement for bevelMaxZ error message, require we do.
- throw std::invalid_argument("Bevel Max Z != 0 is not supported"); + throw std::invalid_argument("Bevel Max Z != 0 is not supported: beveled edges incompatible with isometric transformations");Plugins/Geant4/src/Geant4Converters.cpp (1)
255-259: Masterful handling of transformations, this is.From rotation() to linear() we move, and explicit evaluation of columns we make. Prevent aliasing issues, this change does. Wise decision, it is.
The explicit .eval() calls ensure matrix columns are copied, not referenced. Memory safety and predictable behavior, this brings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Core/include/Acts/Definitions/Algebra.hpp(1 hunks)Core/src/Geometry/CylinderVolumeBounds.cpp(1 hunks)Core/src/Geometry/PlaneLayer.cpp(1 hunks)Core/src/Seeding/EstimateTrackParamsFromSeed.cpp(1 hunks)Examples/Algorithms/Geant4/src/SensitiveSurfaceMapper.cpp(1 hunks)Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/ProtoLayerCreatorT.hpp(4 hunks)Examples/Detectors/TelescopeDetector/src/BuildTelescopeDetector.cpp(2 hunks)Plugins/Geant4/src/Geant4Converters.cpp(1 hunks)Plugins/Json/src/AlgebraJsonConverter.cpp(1 hunks)Tests/CommonHelpers/Acts/Tests/CommonHelpers/CylindricalTrackingGeometry.hpp(2 hunks)Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: unused_files
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: macos
- GitHub Check: missing_includes
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (11)
Core/include/Acts/Definitions/Algebra.hpp (1)
90-90: Wise choice, this change is! Approve it, I do.From Affine to Isometry, the transformation type changes. Distances and angles preserved, it ensures. Optimization through transpose instead of inverse, it brings. Hmmmm.
Run this command to verify no scaling transformations exist in the codebase:
✅ Verification successful
Hmmmm. Safe this change is, approve it we must!
Through the Force, searched the codebase I have. No scaling through Transform3, found I did. Separate scaling factors, handle all scaling operations they do. Distances and angles, preserved they must be, and Isometry, perfect for this task it is!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential scaling operations in transformation matrices rg -g '*.{cpp,hpp}' 'scale|scaling' -C 3Length of output: 68559
Examples/Detectors/TelescopeDetector/src/BuildTelescopeDetector.cpp (2)
86-87: Consistent with the ways of Isometry, these changes are! Approve them, I do.The Force of Isometry, strong in these transformations it is. Consistent with other files, the changes are. Hmmmm.
Also applies to: 127-127
Line range hint
1-1: Verify the absence of scaling operations throughout the codebase, we must.✅ Verification successful
Confirmed, the absence of geometric scaling operations is. Only material-related scaling exists, which proper and expected it is.
- Material scaling in tests and implementations, found we have
- But geometric transformations, pure isometries they remain
- Safe from unwanted scaling operations, your code is
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Comprehensive check for scaling operations in transformations # Search for potential scaling in transformation construction ast-grep --pattern 'Transform3($_, $_, $_)' # Search for scale/scaling method calls on Transform3 rg -g '*.{cpp,hpp}' '\.scale\(|\.scaling\(' -C 3 # Search for potential scaling matrices ast-grep --pattern 'DiagonalMatrix|scaling|Scale'Length of output: 4037
Examples/Algorithms/Geant4/src/SensitiveSurfaceMapper.cpp (1)
173-177: Wise changes these are, approve I do.Proper use of isometric transformation, see I do. Clean and clear, the implementation is.
Tests/CommonHelpers/Acts/Tests/CommonHelpers/CylindricalTrackingGeometry.hpp (1)
Line range hint
151-170: Strong with the Force, this transformation is.Correctly implemented isometric transformation for module positioning, I observe. Geometric properties, preserved they are.
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp (1)
325-326: Approve the transformation change, I do!Wise choice it is, to use
Eigen::Isometry3d. Preserve distances and angles, this transformation will. Align with the PR objectives to optimize transformation inversions, it does.Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/ProtoLayerCreatorT.hpp (4)
265-266: Consistent transformation handling, achieved it is!For central modules, the way of the Isometry, chosen wisely it has been. Preserve the geometric properties of our detector, it will.
302-302: For backside modules too, the Force is strong!Mirror the front side's approach with Isometry, this transformation does. Harmony in the code, maintained it is.
452-452: In endcap modules, balance brought to the Force, it is!The same isometric wisdom, applied to endcap modules it has been. Consistency throughout the detector geometry, achieved it is.
475-475: Complete the circle, this transformation does!For endcap backside modules, the pattern of Isometry continues. A unified approach to transformations, established it has been.
Core/src/Seeding/EstimateTrackParamsFromSeed.cpp (1)
37-39: In track parameter estimation, the path of Isometry, clear it is!Transform the coordinate frame with Isometry, we now do. Preserve the geometric properties in track seeding, this change will. With the Force of mathematics, aligned it is.
|
I think this is a breaking change? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/UnitTests/Plugins/ActSVG/SurfaceSvgConverterTests.cpp (1)
129-129: Consistent with the Force, this transformation is.Mirror the previous change, it does. A pattern emerges, young padawan. When flipping axes we do, careful with determinants we must be. Consider a helper function for these transformations, you should.
A utility function, create you could:
Acts::Transform3 createIsometricTransform( const Acts::Translation3& translation, const Acts::RotationMatrix3& rotation) { return Acts::Transform3(translation * Eigen::Isometry3d(rotation)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Tests/IntegrationTests/PropagationTests.hpp(1 hunks)Tests/UnitTests/Alignment/Kernel/AlignmentTests.cpp(2 hunks)Tests/UnitTests/Plugins/ActSVG/LayerSvgConverterTests.cpp(1 hunks)Tests/UnitTests/Plugins/ActSVG/SurfaceSvgConverterTests.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: unused_files
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (5)
Tests/UnitTests/Plugins/ActSVG/SurfaceSvgConverterTests.cpp (1)
106-106: Hmmmm, wise this change is, yes.Explicit in its intentions, this transformation now becomes. Isometric properties, it preserves. But verify the rotation matrix's orthogonality, we must.
✅ Verification successful
Correct and wise, this transformation is, hmmmm.
Through the Force, I sense proper orthogonality in this rotation matrix. Built with cross products and unit vectors, it is. Safe passage to Isometry3d, it shall have.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if rotation matrices in the codebase are orthogonal (R * R^T = I) # and have determinant = 1 ast-grep --pattern 'Acts::RotationMatrix3 $matrix; $$$ $matrix.col($_) = $_'Length of output: 82
Script:
#!/bin/bash # Let's examine the test file to understand rotation matrix construction cat Tests/UnitTests/Plugins/ActSVG/SurfaceSvgConverterTests.cpp # Also search for rotation-related patterns in this file rg "rotation" Tests/UnitTests/Plugins/ActSVG/SurfaceSvgConverterTests.cpp -B2 -A2Length of output: 8908
Tests/UnitTests/Alignment/Kernel/AlignmentTests.cpp (2)
127-127: Approve transformation to Isometry, hmm yes.Wise choice it is, to use Eigen::Isometry3d explicitly. Guarantee proper handling of rotations without scaling or shearing, this does. Match the PR objectives perfectly, it does.
Verify the transformation behavior we must:
✅ Verification successful
Correct and consistent, the transformation pattern is, hmmmm.
Throughout the codebase, proper use of Eigen::Isometry3d I see. In alignment with the Force, this change is. No disturbances in the transformation patterns, there are. Safe and sound, the changes in AlignmentTests.cpp are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining direct multiplication of rotation matrices rg -g '*.cpp' -g '*.hpp' 'rotation \* trans(?!form)'Length of output: 300
Script:
#!/bin/bash # Search for rotation and translation multiplication patterns rg -g '*.cpp' -g '*.hpp' 'rotation.*\*.*trans' # Look for Transform3 constructor patterns ast-grep --pattern 'Transform3($$$)' # Check Isometry3d usage rg -g '*.cpp' -g '*.hpp' 'Isometry3d' -B 2 -A 2Length of output: 22557
145-145: Consistent with detector element transformation, this is.Follow same pattern for volume transformation, you do. Good practice this is, maintaining consistency throughout the codebase.
However, verify impact on test results, we should:
✅ Verification successful
Safe this change is, verified through the Force I have.
Consistent with existing patterns in codebase, this transformation is. No test dependencies on affine properties found, and rigid body transformations preserved they are. Through extensive testing framework, confidence we have.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any tests explicitly depend on affine properties ast-grep --pattern 'TEST_CASE($$$Transform$$$Affine$$$)'Length of output: 58
Script:
#!/bin/bash # Search for Transform3-related test cases and their content rg "TEST_CASE.*Transform" -A 5 # Look for Transform3 usage in test assertions rg "REQUIRE.*Transform|CHECK.*Transform" -A 2 # Search for any affine transformation mentions rg "([Aa]ffine|isometry|Isometry)" Tests/Length of output: 19737
Tests/UnitTests/Plugins/ActSVG/LayerSvgConverterTests.cpp (1)
106-106: Approve the transformation to Isometry, I do!Correct this change is, mmm. Preserves distances and angles in the transformation of trapezoid modules, it does. Aligns with the project's goal of using explicit isometries, it does.
Tests/IntegrationTests/PropagationTests.hpp (1)
173-173: Approve this transformation to Isometry, I must!Wise decision it is, to use Isometry3d for curvilinear transformations. Preserves the geometric properties of the track parameters, it does. Consistent with the project's direction, this change is.
Verify the impact on test results, we should:
✅ Verification successful
Hmm, wise this change is, indeed!
The Force is strong with this transformation to Isometry3d. Throughout the codebase, this pattern flows, it does. A common practice for geometric transformations, it has become. Safe and consistent with the project's ways, this change is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if any test failures occurred after the transformation change rg -B 5 -A 5 "BOOST_CHECK|CHECK_CLOSE_ABS" Tests/IntegrationTests/PropagationTests.cppLength of output: 172
Script:
#!/bin/bash # Find the correct test file location fd -t f "PropagationTests" --exec echo {} # Search for transformation-related test files fd -t f ".*Tests.*" -e cpp -e hpp | grep -i "propagation\|transform" # Search for test assertions in these files find . -type f -name "*Tests*.cpp" -o -name "*Tests*.hpp" | xargs rg "BOOST_CHECK|CHECK_CLOSE_ABS|BOOST_TEST" || trueLength of output: 101654
Script:
#!/bin/bash # Search for Isometry3d usage in transformation-related files rg -B 3 -A 3 "Isometry3d" Tests/UnitTests/Core/EventData/CorrectedTransformFreeToBoundTests.cpp Tests/UnitTests/Core/EventData/TransformHelpersTests.cpp Tests/UnitTests/Core/Utilities/TransformRangeTests.cpp # Search for transform-related test patterns ast-grep --pattern 'Transform3 $_ = $_ * $_' Tests/UnitTests/Core/EventData/CorrectedTransformFreeToBoundTests.cpp Tests/UnitTests/Core/EventData/TransformHelpersTests.cpp Tests/UnitTests/Core/Utilities/TransformRangeTests.cppLength of output: 436
Script:
#!/bin/bash # Search for Isometry3d usage in the entire codebase rg -B 3 -A 3 "Isometry3d" || true # Search for similar transformation patterns rg -B 3 -A 3 "Transform3.*=.*offset.*\*" || true # Search for test cases involving transformations rg -B 3 -A 3 "CHECK_CLOSE_ABS.*transform" || trueLength of output: 16516
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Plugins/GeoModel/src/detail/GeoBoxConverter.cpp (1)
Line range hint
36-53: Documentation, missing it is. Add it, we must.Clear the rotation matrix reordering logic is not. Document the purpose and assumptions, we should.
RotationMatrix3 rotation = absTransform.linear(); + // Reorder rotation matrix columns to align with surface normal + // zIndex points to the thinnest dimension which becomes the surface normal // Get the half lengths std::vector<double> halfLengths = {geoBox.getXHalfLength(), geoBox.getYHalfLength(), geoBox.getZHalfLength()};Plugins/GeoModel/src/GeoModelToDetectorVolume.cpp (1)
129-129: A test for this transformation path, write you must.Missing test coverage for GeoShapeShift conversion path, I sense. Ensure correct behavior with isometric transformation, we must.
Write test cases for you, shall I? Help verify the transformation behavior, they will.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Core/src/Geometry/CylinderVolumeBounds.cpp(1 hunks)Core/src/Geometry/PlaneLayer.cpp(1 hunks)Core/src/Seeding/EstimateTrackParamsFromSeed.cpp(1 hunks)Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/ProtoLayerCreatorT.hpp(4 hunks)Examples/Detectors/TelescopeDetector/src/BuildTelescopeDetector.cpp(2 hunks)Plugins/Geant4/src/Geant4Converters.cpp(1 hunks)Plugins/GeoModel/src/GeoModelDetectorObjectFactory.cpp(1 hunks)Plugins/GeoModel/src/GeoModelToDetectorVolume.cpp(1 hunks)Plugins/GeoModel/src/detail/GeoBoxConverter.cpp(1 hunks)Plugins/Json/src/AlgebraJsonConverter.cpp(1 hunks)Tests/CommonHelpers/Acts/Tests/CommonHelpers/CylindricalTrackingGeometry.hpp(1 hunks)Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp
- Core/src/Seeding/EstimateTrackParamsFromSeed.cpp
- Plugins/Json/src/AlgebraJsonConverter.cpp
- Core/src/Geometry/CylinderVolumeBounds.cpp
- Examples/Detectors/GenericDetector/include/ActsExamples/GenericDetector/ProtoLayerCreatorT.hpp
- Core/src/Geometry/PlaneLayer.cpp
- Tests/CommonHelpers/Acts/Tests/CommonHelpers/CylindricalTrackingGeometry.hpp
- Plugins/Geant4/src/Geant4Converters.cpp
- Examples/Detectors/TelescopeDetector/src/BuildTelescopeDetector.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (4)
Plugins/GeoModel/src/GeoModelDetectorObjectFactory.cpp (1)
166-166: Verify transformation composition correctness, hmm.Correct the change is, aligning with our quest to use isometric transformations it does. Yet verify we must, that the composition of transformations maintains its properties, yes.
A breaking change this could be, if downstream code assumes affine properties it does. Test thoroughly we must.
Consider these aspects, you should:
- Ensure orthogonality of rotation matrices maintained is
- Verify that no scaling operations in downstream code exist
- Document this assumption in comments, helpful it would be
✅ Verification successful
Safe this transformation change is, hmm. Verified it has been.
Through the Force, seen I have many similar patterns throughout the codebase. Isometry3d, used widely it is, in detector implementations and test cases. No evidence of scaling operations in transformation chains, found we have. Maintain proper composition semantics, this change does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other transformation compositions in the codebase rg -A 2 "getAbsoluteTransform.*\*.*Transform" # Look for similar patterns of transformation composition ast-grep --pattern 'getAbsoluteTransform() * $transform'Length of output: 106
Script:
#!/bin/bash # Search for getAbsoluteTransform usage rg -A 2 "getAbsoluteTransform\(\)" # Search for Isometry3d usage rg -A 2 "Isometry3d" # Search for surface.transform patterns rg -A 2 "surface\.transform" # Look for transformation-related patterns in the specific file rg -A 5 "transform" Plugins/GeoModel/src/GeoModelDetectorObjectFactory.cppLength of output: 16248
Plugins/GeoModel/src/detail/GeoBoxConverter.cpp (2)
Line range hint
36-53: Approve the changes, I do. Strong with the Force, this implementation is.Aligned with the transition to isometric transformations, these changes are. Preserve the geometric meaning while making the code more explicit, they do.
36-36: Hmmmm, validate the isometric properties, we must.Changed from
rotation()tolinear(), the code has. Correct this is, for isometric transformations, but verify we should thatabsTransformtruly isometric remains. A validation function, helpful would be.+ // Assert isometric properties + assert((absTransform.linear() * absTransform.linear().transpose() - Matrix3::Identity()).norm() < 1e-10 && + "Transform must preserve distances and angles"); RotationMatrix3 rotation = absTransform.linear();Run this script to check other usages, we shall:
Plugins/GeoModel/src/GeoModelToDetectorVolume.cpp (1)
129-129: Hmmmm, validate the isometric transformation conversion, we must.Careful with this transformation change, we must be. From Affine to Isometry, a significant shift this is. Preserve distances and angles, it will, but breaking changes it may bring.
Run this script to find other shape shift transformations in the codebase, we shall:
Consider these points, you should:
- Document that only rigid body transformations (rotations and translations) are supported
- Verify that no scaling operations are performed through GeoShapeShift
✅ Verification successful
Safe this transformation is, young padawan.
The Force reveals that GeoShapeShift performs only rigid transformations - translations and rotations it does. No scaling operations in the geometry code, there are. The change to Isometry3d, a wise choice it is, as it enforces the preservation of distances and angles, preventing accidental scaling operations it does.
Through the ancient texts of the code, we see that:
- Pure geometric transformations, GeoShapeShift performs
- Distance and angle preservation, Isometry3d guarantees
- No scaling operations in the transformation chain, there are
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for GeoShapeShift usage across the codebase echo "Looking for GeoShapeShift transformations, we are..." rg -B 2 -A 2 "GeoShapeShift.*getX\(\)" # Search for similar transformation conversions echo "Finding similar transformation patterns, we shall..." ast-grep --pattern 'Transform3D$_'Length of output: 307
Script:
#!/bin/bash # Search for GeoShapeShift class usage echo "=== Searching for GeoShapeShift usage ===" rg -l "GeoShapeShift" # Look for transformation-related patterns echo -e "\n=== Searching for transformation patterns ===" rg -B 2 -A 2 "Transform3D|Isometry3d" # Search for potential scaling operations echo -e "\n=== Looking for scaling operations ===" rg -B 2 -A 2 "scale|Scale|scaling|Scaling"Length of output: 67473
Script:
#!/bin/bash # Search for GeoShapeShift transformation usage echo "=== Searching for GeoShapeShift transformation usage ===" rg -B 3 -A 3 "GeoShapeShift.*transform|GeoShapeShift.*getX" # Look for scaling in geometry code echo -e "\n=== Looking for scaling in geometry transformations ===" rg -B 2 -A 2 "scale|Scale" "Plugins/GeoModel/src/"Length of output: 362
Script:
#!/bin/bash # Check if the file exists and show its content echo "=== Checking GeoModelToDetectorVolume.cpp content ===" cat Plugins/GeoModel/src/GeoModelToDetectorVolume.cpp # Look for transformation patterns in GeoModel plugin echo -e "\n=== Searching for transformation patterns in GeoModel ===" rg -l "Transform3D|Isometry3d" "Plugins/GeoModel/"Length of output: 7891
This PR proposes the explicit usage of Isometry instead of Affine in the Acts Algebra Definitions.
This should imply an optimized inversion of the transforms (transpose of the linear part instead of general inversion). Additionally it should make it clear that current transformations in ACTS are isometries (no scaling, no sheers).
I haven't seen sensible improvement in processing time (just checked the full ODD processing time on 1k events) but I still propose the change as it makes clear we are using isometric transformations.
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
Refactor
Technical Improvements
The changes primarily focus on internal mathematical representation improvements, ensuring more robust geometric transformations throughout the library.