feat: Adding G4Trap converter in Geant4Converters #3775
Conversation
asalzburger
changed the title
refactor: Adding Trap converter in Geant4Converters (Draft)
Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4Converters.hpp
Outdated
Show resolved
Hide resolved
Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4Converters.hpp
Outdated
Show resolved
Hide resolved
asalzburger
changed the title
asalzburger
changed the title
Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4Converters.hpp
Outdated
Show resolved
Hide resolved
|
We are getting closer and closer. Ideally, you should also add a test-case to the Of course it is at the moment (as you could show me), but this will basically test consistency from now. |
| @@ -21,10 +21,6 @@ class Hep3Vector; | |||
| class HepRotation; | |||
| } // namespace CLHEP | |||
|
|
|||
There was a problem hiding this comment.
The lines below can not be removed: we need the forward declaration of the Transform here!
WalkthroughA new method, 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
Plugins/Geant4/src/Geant4Converters.cpp (1)
414-414: Correct the exception message, you must.Change "Can not" to "Cannot" in the error message for proper grammar and consistency.
Apply this diff to fix the typo:
- throw std::runtime_error("Can not convert 'G4Trap' into forced shape."); + throw std::runtime_error("Cannot convert 'G4Trap' into forced shape.");Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4Converters.hpp (1)
137-140: Update the parameter name in the comments, you should.In the documentation, the parameter is incorrectly named
g4Trd; it should beg4Trap.Apply this diff to correct the comment:
/// @brief Convert to trapezoid bounds - from Trap /// - /// @param g4Trd a Geant4 trapezoid shape + /// @param g4Trap a Geant4 trapezoid shape ///
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4Converters.hpp(1 hunks)Plugins/Geant4/src/Geant4Converters.cpp(3 hunks)Tests/UnitTests/Plugins/Geant4/Geant4ConvertersTests.cpp(1 hunks)
🔇 Additional comments (1)
Plugins/Geant4/src/Geant4Converters.cpp (1)
238-301: Implemented the new method well, you have.
The trapezoidBounds function for G4Trap is correctly added, and calculations appear accurate.
| G4Trap trapXY("trapXY", 2, 0.523599, 0.785398, 125, 200, 125, 0.174533, 50, | ||
| 125, 50, 0.174533); | ||
| auto [boundsXY, axesXY, thicknessZ] = | ||
| Acts::Geant4ShapeConverter{}.TrapezoidBounds(trapXY); |
There was a problem hiding this comment.
Incorrect method name used, you have.
Method TrapezoidBounds should be trapezoidBounds; follow naming conventions, you must.
Apply this diff to correct the method name in all instances:
- Acts::Geant4ShapeConverter{}.TrapezoidBounds(trapXY);
+ Acts::Geant4ShapeConverter{}.trapezoidBounds(trapXY);
- Acts::Geant4ShapeConverter{}.TrapezoidBounds(trapyX);
+ Acts::Geant4ShapeConverter{}.trapezoidBounds(trapyX);
- Acts::Geant4ShapeConverter{}.TrapezoidBounds(trapYZ);
+ Acts::Geant4ShapeConverter{}.trapezoidBounds(trapYZ);
- Acts::Geant4ShapeConverter{}.TrapezoidBounds(trapXz);
+ Acts::Geant4ShapeConverter{}.trapezoidBounds(trapXz);Also applies to: 236-236, 254-254, 272-272
| @@ -210,6 +210,80 @@ | |||
| CHECK_CLOSE_ABS(thicknessY, 2., 10e-10); | |||
| } | |||
|
|
|||
| BOOST_AUTO_TEST_CASE(Geant4TrapzoidConversion) { | |||
There was a problem hiding this comment.
Duplicate test case name, there is.
Geant4TrapzoidConversion test case already defined earlier, it is. Having duplicate test case names, compilation errors cause, it will.
Apply this diff to rename the test case:
- BOOST_AUTO_TEST_CASE(Geant4TrapzoidConversion) {
+ BOOST_AUTO_TEST_CASE(Geant4TrapConversion_G4Trap) {📝 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.
| BOOST_AUTO_TEST_CASE(Geant4TrapzoidConversion) { | |
| BOOST_AUTO_TEST_CASE(Geant4TrapConversion_G4Trap) { |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Plugins/Geant4/src/Geant4Converters.cpp (1)
238-301: Document the geometric transformations, you should.Complex calculations involving theta and phi angles, this method has. Add comments explaining:
- Relationship between G4Trap parameters and resulting trapezoid bounds
- Meaning of the geometric transformations
- Assumptions about coordinate systems
Add documentation like this, you should:
std::tuple<std::shared_ptr<Acts::TrapezoidBounds>, std::array<int, 2u>, Acts::ActsScalar> Acts::Geant4ShapeConverter::trapezoidBounds(const G4Trap& g4Trap) { + // Convert G4Trap to Acts::TrapezoidBounds + // The conversion considers: + // - Theta: polar angle from the z-axis to the line joining the centers + // - Phi: azimuthal angle in the x-y plane from the y-axis + // Half-lengths are adjusted based on these angles to maintain the shape
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4Converters.hpp(1 hunks)Plugins/Geant4/src/Geant4Converters.cpp(3 hunks)Tests/UnitTests/Plugins/Geant4/Geant4ConvertersTests.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Geant4/include/Acts/Plugins/Geant4/Geant4Converters.hpp
🔇 Additional comments (5)
Tests/UnitTests/Plugins/Geant4/Geant4ConvertersTests.cpp (3)
21-21: Correct, the include statement is.
Follow the project's naming conventions for bounds types, it does.
213-285: Well structured and comprehensive, this test case is.
Cover all orientations of G4Trap conversion, it does:
- Standard XY orientation
- Flipped YX coordinates
- YZ spanning trapezoid
- XZ spanning trapezoid
Validate bounds and thickness with appropriate tolerances, it does.
297-297: Correct naming convention now used, it is.
Changed from TrapezoidBounds to trapezoidBounds, as per project standards.
Plugins/Geant4/src/Geant4Converters.cpp (2)
40-40: Required for G4Trap conversion, this include is.
Confirmed by past review comments, its necessity was.
401-416: Well implemented, the G4Trap surface conversion is.
Follow established patterns for shape conversion, it does:
- Proper type checking with dynamic cast
- Correct error handling
- Consistent with G4Trd implementation
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Plugins/Geant4/src/Geant4Converters.cpp(3 hunks)
🔇 Additional comments (3)
Plugins/Geant4/src/Geant4Converters.cpp (3)
241-244: Clear documentation, it is. Approve, I do.
Well-documented explanation of the geometric parameters considered in the conversion, it is.
272-299: Wise choice of axes orientation, it is.
Correctly handles different orientations based on minimum dimension, the code does. Consistent with existing implementations, it remains.
404-419: Handle G4Trap conversion gracefully, we do.
Follows established pattern for shape conversion, it does. Error handling for unsupported forced shapes, present it is.
Remember to add test cases as suggested by Master Asalzburger, we must.
✅ Verification successful
Let me search for test files more broadly, as the specific name might be different, hmm. Test files, find we must!
Found the test file we have, hmm! But see the specific test cases for G4Trap, we must. Search deeper into the Force, we shall.
Test cases for G4Trap conversion, present they are!
Comprehensive test coverage exists, it does! Multiple orientations tested are:
- Standard XY trap configuration
- Flipped yX orientation
- YZ spanning trapezoid
- Xz spanning trapezoid
Each case, thoroughly checked with bounds and axes validation, it is. Master Asalzburger's wishes, fulfilled they have been.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the test file mentioned in PR comments
fd "Geant4ConverterTests.cpp" --type f
Length of output: 38
Script:
#!/bin/bash
# Search for test files in the Geant4 plugin directory with various possible names
fd ".*[Tt]est.*\.cpp" --type f | grep -i "geant"
# Also search for any test files that might contain G4Trap tests
rg -l "G4Trap" --type cpp | grep -i "test"
Length of output: 466
Script:
#!/bin/bash
# Search for G4Trap test cases in the converter tests
rg "G4Trap" -B 5 -A 10 "Tests/UnitTests/Plugins/Geant4/Geant4ConvertersTests.cpp"
Length of output: 2600
| ActsScalar hlX0 = (x1 + x2) * 0.5; | ||
| ActsScalar hlX1 = 2 * z * std::tan(theta) * std::cos(phi) + (x3 + x4) * 0.5; | ||
| ActsScalar hlY0 = y1; | ||
| ActsScalar hlY1 = y2 + 2 * z * std::tan(theta) * std::sin(phi); | ||
| ActsScalar hlZ = z; |
There was a problem hiding this comment.
💡 Codebase verification
Safeguards for numerical stability, add we must.
Through the Force, I sense many trigonometric calculations in the codebase, but no explicit checks for numerical stability in this context, I do. A pattern of using std::tan without validation, widespread it is. Protect against infinite or undefined results, we should.
- In
Geant4Converters.cpp, add validation forthetaangle before tangent calculation:
const ActsScalar tanTheta = std::tan(theta);
if (!std::isfinite(tanTheta)) {
throw std::runtime_error("Invalid theta angle in G4Trap resulting in infinite tangent");
}
ActsScalar hlX1 = 2 * z * tanTheta * std::cos(phi) + (x3 + x4) * 0.5;
ActsScalar hlY1 = y2 + 2 * z * tanTheta * std::sin(phi);🔗 Analysis chain
Careful with trigonometric calculations, we must be.
Potential numerical instability in calculations with extreme angles, there might be. Consider adding safeguards against numerical overflow.
-ActsScalar hlX1 = 2 * z * std::tan(theta) * std::cos(phi) + (x3 + x4) * 0.5;
+const ActsScalar tanTheta = std::tan(theta);
+if (!std::isfinite(tanTheta)) {
+ throw std::runtime_error("Invalid theta angle in G4Trap resulting in infinite tangent");
+}
+ActsScalar hlX1 = 2 * z * tanTheta * std::cos(phi) + (x3 + x4) * 0.5;🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar trigonometric calculations in the codebase
ast-grep --pattern 'std::tan($x)'
Length of output: 35
Script:
#!/bin/bash
# Search for trigonometric calculations with pattern variations
rg "std::tan|tan\(" -A 2 -B 2
# Also search for angle-related variable names and calculations
rg "theta|angle|phi" -A 2 -B 2
# Look for error handling around trigonometric calculations
rg "isfinite|isinf|isnan" -A 2 -B 2
Length of output: 70292
| ActsScalar y1 = static_cast<ActsScalar>(g4Trap.GetYHalfLength1()); | ||
| ActsScalar y2 = static_cast<ActsScalar>(g4Trap.GetYHalfLength2()); | ||
| ActsScalar x1 = static_cast<ActsScalar>(g4Trap.GetXHalfLength1()); | ||
| ActsScalar x2 = static_cast<ActsScalar>(g4Trap.GetXHalfLength2()); | ||
| ActsScalar x3 = static_cast<ActsScalar>(g4Trap.GetXHalfLength3()); | ||
| ActsScalar x4 = static_cast<ActsScalar>(g4Trap.GetXHalfLength4()); | ||
| ActsScalar phi = static_cast<ActsScalar>(g4Trap.GetPhi()); | ||
| ActsScalar theta = static_cast<ActsScalar>(g4Trap.GetTheta()); | ||
| ActsScalar z = static_cast<ActsScalar>(g4Trap.GetZHalfLength()); | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Validate the parameters, we must. Prevent the dark side of undefined behavior, this will.
For G4Trap parameters, essential validation is, hmm:
- Negative half-lengths, forbidden they are
- Theta angle, between 0 and π it must be
- Phi angle, between -π and π it must lie
ActsScalar y1 = static_cast<ActsScalar>(g4Trap.GetYHalfLength1());
+if (y1 < 0 || y2 < 0 || x1 < 0 || x2 < 0 || x3 < 0 || x4 < 0 || z < 0) {
+ throw std::invalid_argument("Negative half-lengths in G4Trap not allowed, they are");
+}
+if (theta < 0 || theta > M_PI) {
+ throw std::invalid_argument("Theta angle must between 0 and π be");
+}
+if (phi < -M_PI || phi > M_PI) {
+ throw std::invalid_argument("Phi angle must between -π and π be");
+}From the test cases seen, validate these parameters we must. Prevent invalid geometric configurations that lead to dark undefined behavior, this will. Strong with the Force, this validation is.
🔗 Analysis chain
Consider adding validation for input parameters, we must.
Validate we must that input parameters from G4Trap are within expected ranges. Prevent undefined behavior in calculations, this will.
ActsScalar y1 = static_cast<ActsScalar>(g4Trap.GetYHalfLength1());
+if (y1 < 0 || y2 < 0 || x1 < 0 || x2 < 0 || x3 < 0 || x4 < 0 || z < 0) {
+ throw std::invalid_argument("Negative half-lengths in G4Trap are not allowed");
+}
+if (!std::isfinite(theta) || !std::isfinite(phi)) {
+ throw std::invalid_argument("Invalid angles in G4Trap");
+}Let's search for existing validation patterns and trap-related checks in the codebase, hmm? Help us understand the current practices, this will.
Hmm, deeper into the codebase we must look. Validation patterns for G4Trap and similar geometric conversions, seek we shall.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing parameter validation patterns in the codebase
rg -A 2 "GetTheta|GetPhi" --type cpp
Length of output: 1168
Script:
#!/bin/bash
# Search for validation patterns in geometric converters
rg -A 3 "invalid_argument|isfinite|epsilon" --type cpp
# Look for other trap-related validations
rg -A 3 "Trap.*Half" --type cpp
Length of output: 69663
Script:
#!/bin/bash
# Search for existing validation patterns in Geant4 converters
rg -A 5 "G4Trap" --type cpp
# Look for existing validation patterns in similar geometric converters
rg -A 5 "static_cast.*GetTheta|static_cast.*GetPhi" --type cpp
Length of output: 6629
This PR adds the possibility to convert a
G4TrapintoActs::TrapezoidalBounds.Summary by CodeRabbit
New Features
Bug Fixes
G4Trap.Tests
G4Trapgeometries, ensuring accuracy across various scenarios.