refactor!: Make Direction constructor explicit #3973
Conversation
This requires changing the static names values to static functions
WalkthroughChange significant, yes. 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 (
|
github-actions
bot
added
Component - Core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Core/src/Geometry/ConeVolumeBounds.cpp (1)
106-106: Strong with the Force, this refactoring is.OrientedSurface construction, more explicit it becomes. Direction values, through proper constructor now flow. Safer and clearer, the code path becomes.
Consider grouping related Direction constants into an enum class for even stronger type safety:
enum class SurfaceDirection { AlongNormal, OppositeNormal };Also applies to: 112-112, 121-121, 127-127, 137-137, 145-145, 160-160, 169-170
Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp (1)
380-380: Hmmmm. Inconsistent with other Direction usage in file, this initialization is.Different approach to Direction initialization here, I see. While functional, consistency with other changes in file maintain we should.
Consider this change, you should:
- Direction navDir = Direction::Forward(); + static constexpr Direction navDir = Direction::Forward();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (44)
Core/include/Acts/Definitions/Direction.hpp(5 hunks)Core/include/Acts/EventData/detail/CorrectedTransformationFreeToBound.hpp(1 hunks)Core/include/Acts/Geometry/BoundarySurfaceT.hpp(2 hunks)Core/include/Acts/Material/ISurfaceMaterial.hpp(1 hunks)Core/include/Acts/Propagator/DirectNavigator.hpp(2 hunks)Core/include/Acts/Propagator/PropagatorOptions.hpp(1 hunks)Core/include/Acts/TrackFitting/detail/GsfActor.hpp(2 hunks)Core/src/Detector/detail/CuboidalDetectorHelper.cpp(2 hunks)Core/src/Detector/detail/CylindricalDetectorHelper.cpp(12 hunks)Core/src/Geometry/ConeVolumeBounds.cpp(5 hunks)Core/src/Geometry/CuboidVolumeBounds.cpp(1 hunks)Core/src/Geometry/CutoutCylinderVolumeBounds.cpp(3 hunks)Core/src/Geometry/CylinderVolumeBounds.cpp(3 hunks)Core/src/Geometry/Portal.cpp(6 hunks)Core/src/Geometry/TrackingVolume.cpp(3 hunks)Core/src/Geometry/TrapezoidVolumeBounds.cpp(3 hunks)Core/src/Vertexing/ImpactPointEstimator.cpp(2 hunks)Examples/Algorithms/Geant4/src/SensitiveSteppingAction.cpp(1 hunks)Examples/Algorithms/Propagation/src/SimHitToSummaryConversion.cpp(1 hunks)Examples/Algorithms/Traccc/include/ActsExamples/Traccc/DetrayPropagator.hpp(1 hunks)Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp(1 hunks)Plugins/Json/src/PortalJsonConverter.cpp(1 hunks)Tests/IntegrationTests/NavigatorConsistency.cpp(2 hunks)Tests/IntegrationTests/PropagationTests.hpp(2 hunks)Tests/UnitTests/Core/Definitions/DirectionTests.cpp(1 hunks)Tests/UnitTests/Core/Detector/PortalTests.cpp(6 hunks)Tests/UnitTests/Core/Geometry/PortalShellTests.cpp(3 hunks)Tests/UnitTests/Core/Geometry/PortalTests.cpp(10 hunks)Tests/UnitTests/Core/Material/HomogeneousSurfaceMaterialTests.cpp(1 hunks)Tests/UnitTests/Core/Material/ISurfaceMaterialTests.cpp(1 hunks)Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp(6 hunks)Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp(3 hunks)Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp(3 hunks)Tests/UnitTests/Core/Propagator/LoopProtectionTests.cpp(1 hunks)Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp(2 hunks)Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp(5 hunks)Tests/UnitTests/Core/Propagator/NavigatorTests.cpp(1 hunks)Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp(3 hunks)Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp(3 hunks)Tests/UnitTests/Core/Propagator/VolumeMaterialInteractionTests.cpp(2 hunks)Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp(2 hunks)Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp(2 hunks)Tests/UnitTests/Core/TrackFitting/FitterTestsCommon.hpp(2 hunks)Tests/UnitTests/Plugins/Json/PortalJsonConverterTests.cpp(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- Core/src/Geometry/TrapezoidVolumeBounds.cpp
- Core/src/Vertexing/ImpactPointEstimator.cpp
- Core/src/Geometry/CuboidVolumeBounds.cpp
- Core/src/Geometry/CylinderVolumeBounds.cpp
- Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp
👮 Files not reviewed due to content moderation or server errors (2)
- Tests/UnitTests/Core/Propagator/LoopProtectionTests.cpp
- Core/include/Acts/Geometry/BoundarySurfaceT.hpp
🔇 Additional comments (58)
Tests/UnitTests/Core/Material/ISurfaceMaterialTests.cpp (1)
48-66: Consistent with the ways of the Force, these changes are!
Changed from static values to function calls, the Direction usage has been. Maintain test integrity while embracing explicit construction, it does. Approve these changes, I do!
Tests/UnitTests/Core/Definitions/DirectionTests.cpp (1)
21-25: Wise use of constexpr, I sense!
Compile-time evaluation preserved through constexpr, it is. Direction values through function calls obtained, yet performance maintained, it remains. Strong with the Force, this implementation is!
Core/include/Acts/EventData/detail/CorrectedTransformationFreeToBound.hpp (1)
99-99: Changed the default parameter has been, verify its impact we must!
Default parameter now uses explicit Direction construction, as intended it is. Yet verify we must, that all callers of this function prepared for this change they are.
✅ Verification successful
Safe this change is, proceed we may!
Searched through the Force, I have. No direct calls to operator() with default Direction parameter, found I have. Implementation file shows proper handling of Direction parameter it does, and test file reveals no conflicts with this change. Through the Dark Side and Light, no disturbance in the Force I sense.
The path is clear:
- Implementation in CorrectedTransformationFreeToBound.cpp uses navDir parameter correctly
- Test file shows no dependency on implicit Direction construction
- No other direct usage of operator() with default parameters exists
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of the operator() with default parameters
rg -l "CorrectedFreeToBoundTransformer.*operator.*\(" | xargs rg "operator\(\s*(?:[^,]+,){4}\s*(?!Direction::).*\)"
Length of output: 398
Script:
#!/bin/bash
# Search for files containing CorrectedFreeToBoundTransformer
fd -e hpp -e cpp | xargs rg -l "CorrectedFreeToBoundTransformer"
# Then search for operator() calls in those files
fd -e hpp -e cpp | xargs rg "CorrectedFreeToBoundTransformer.*operator\(" -B2 -A2
# Also search for any Direction-related parameters in the transformer usage
fd -e hpp -e cpp | xargs rg "CorrectedFreeToBoundTransformer.*Direction" -B2 -A2
Length of output: 1074
Script:
#!/bin/bash
# Search for the implementation and test files to see how the operator is called
rg -A 10 "CorrectedFreeToBoundTransformer::operator" "Core/src/EventData/CorrectedTransformationFreeToBound.cpp" "Tests/UnitTests/Core/EventData/CorrectedTransformFreeToBoundTests.cpp"
# Also check test file for any direction-related test cases
rg "Direction" "Tests/UnitTests/Core/EventData/CorrectedTransformFreeToBoundTests.cpp" -B2 -A2
Length of output: 1665
Core/include/Acts/Definitions/Direction.hpp (4)
30-41: Wise changes these are, approve them I do!
Strong with the Force, this refactoring is. Type safety it brings, and implicit conversions it prevents. Compile-time optimization with constexpr, preserved it remains.
Line range hint 49-61: Handle zero values wisely, these methods do!
Assert on zero in fromScalar(), wisdom shows. For fromScalarZeroAsPositive(), clear intention in name it carries. Consistent with the new way, both methods are.
Line range hint 69-96: Clear and efficient, these methods remain!
Convert between index and Direction, fromIndex does well. Invert the path does, without losing its way. Strong with the new pattern, both methods are.
102-105: The heart of the change, this is!
Explicit constructor, accidental conversions it prevents. Friend equality operator, symmetric comparison it enables. Strong foundation for the entire refactoring, these changes provide.
Tests/UnitTests/Core/Propagator/VolumeMaterialInteractionTests.cpp (1)
47-47: Updated to the new ways, these tests are!
From static values to function calls, the transition complete it is. Test logic unchanged remains, while new patterns it embraces.
Also applies to: 101-101
Tests/UnitTests/Plugins/Json/PortalJsonConverterTests.cpp (1)
80-80: Consistent with the new path, these portal tests are!
Direction function calls now used they are, where static values once stood. Portal attachment and navigation, tested well they remain.
Also applies to: 82-82, 128-128, 132-133
Examples/Algorithms/Propagation/src/SimHitToSummaryConversion.cpp (1)
102-102: Correct this change is, approve it I do!
The navigation direction initialization follows the new explicit constructor pattern, maintaining the same behavior it does.
Core/include/Acts/Material/ISurfaceMaterial.hpp (1)
140-142: Correct the changes are, but verify equality comparison we must!
The explicit constructor in Direction comparisons, a significant change it is. Ensure that Direction equality comparison behaves correctly with the new pattern, we must.
Run these tests, we shall:
Improve readability, we could.
Consider extracting direction comparisons to descriptive helper methods, clearer intent it would show:
+ // Helper methods for direction checks
+ bool isNegativeDirection(Direction dir) const { return dir == Direction::Negative(); }
+ bool isPositiveDirection(Direction dir) const { return dir == Direction::Positive(); }
+
double factor(Direction pDir, MaterialUpdateStage mStage) const {
if (mStage == Acts::MaterialUpdateStage::FullUpdate) {
return 1.;
} else if (mStage == Acts::MaterialUpdateStage::PreUpdate) {
- return pDir == Direction::Negative() ? m_splitFactor : 1 - m_splitFactor;
+ return isNegativeDirection(pDir) ? m_splitFactor : 1 - m_splitFactor;
} else /*if (mStage == Acts::MaterialUpdateStage::PostUpdate)*/ {
- return pDir == Direction::Positive() ? m_splitFactor : 1 - m_splitFactor;
+ return isPositiveDirection(pDir) ? m_splitFactor : 1 - m_splitFactor;
}
}✅ Verification successful
Hmm, more context we need, to verify the Direction comparison behavior. Search for class definition and usage patterns, we shall. Tests and material handling code, crucial they are for our understanding. Different comparison patterns, we must explore.
Safe the Direction comparison changes are, verified they have been!
Found the tests and implementation, I have. Clear evidence exists:
- In DirectionTests.cpp, proper equality comparison tests present they are:
BOOST_CHECK_EQUAL(bwd, Direction::Negative());
BOOST_CHECK_EQUAL(fwd, Direction::Positive());- In ISurfaceMaterial.hpp, consistent pattern of Direction comparison used throughout:
return pDir == Direction::Negative() ? m_splitFactor : 1 - m_splitFactor;
return pDir == Direction::Positive() ? m_splitFactor : 1 - m_splitFactor;- Direction class properly defined with explicit constructors and comparison operators, it is.
Correct behavior maintained the changes have. No impact on material scaling detected. Helper methods suggested in review, good to have they are, but not critical for functionality they remain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Direction equality comparison tests
rg "TEST.*Direction.*==" --type cpp
# Search for Direction comparison usage in material handling
ast-grep --pattern 'if ($expr == Direction::$dir()) {
$$$
}'
Length of output: 102
Script:
#!/bin/bash
# Search for Direction class definition and usage
rg "class.*Direction" --type cpp -A 5
rg "Direction::(Positive|Negative)" --type cpp -A 2
# Search for Direction comparison patterns with different syntax
rg "(==|!=).*Direction::(Positive|Negative)" --type cpp
# Look for Direction-related tests
fd "test.*\.cpp" --exec rg "Direction" {} \;
# Find material-related files using Direction
fd "Material" --type f --exec rg "Direction" {} \;
Length of output: 22014
Tests/UnitTests/Core/Material/HomogeneousSurfaceMaterialTests.cpp (1)
85-86: Approve these changes, I do!
Correctly updated to use explicit Direction function calls instead of static values, hmmmm. Maintains the same behavior while improving code clarity, it does.
Core/src/Geometry/CutoutCylinderVolumeBounds.cpp (1)
66-66: Consistent in the Force, these changes are!
Updated all Direction values to explicit function calls, we have. Maintains correct surface orientations while improving code clarity and type safety, it does. A balanced approach to geometry definition, this is!
Also applies to: 72-72, 83-83, 89-89, 98-98, 105-105, 113-113, 120-120
Examples/Algorithms/Traccc/include/ActsExamples/Traccc/DetrayPropagator.hpp (1)
132-134: Strong with the Force, this change is!
Updated navigation direction setting to use explicit function calls, we have. Maintains correct conditional logic while following the new way of the Force, it does.
Core/src/Geometry/Portal.cpp (1)
43-43: Approve this change, I do.
Explicit Direction constructor usage, clearer intent it shows. Safer code path, this creates.
Core/include/Acts/Propagator/DirectNavigator.hpp (1)
85-85: Consistent with the Force, these changes are.
Direction constructor, explicit now it becomes. Throughout the code, harmony this brings.
Also applies to: 98-98, 106-106
Core/src/Geometry/ConeVolumeBounds.cpp (1)
Line range hint 106-170: Verify the path to enlightenment, we must.
Throughout the codebase, consistent usage of Direction constructor, essential it is.
✅ Verification successful
Consistent usage of Direction throughout the codebase, I sense.
Through the Force, observed I have that Direction's usage follows proper patterns:
- All Direction constructors properly called with parentheses they are
- No static member access without constructor calls exists
- In the code under review, correct usage of Direction::AlongNormal() and Direction::OppositeNormal() maintained it is
The path to enlightenment, already found it has been. No disturbance in the Force, there is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining static Direction member access
echo "Searching for potential inconsistent Direction usage..."
rg --type cpp "Direction::(Forward|Backward|AlongNormal|OppositeNormal|Positive|Negative)[^(]"
Length of output: 217
Script:
#!/bin/bash
# Search for Direction constructor usage patterns
echo "Searching for Direction constructor patterns, we must..."
rg --type cpp "Direction::(Forward|Backward|AlongNormal|OppositeNormal|Positive|Negative)\(\)"
echo "\nSearching for Direction member access in header files, hmm..."
rg --type hpp "Direction::(Forward|Backward|AlongNormal|OppositeNormal|Positive|Negative)"
echo "\nInvestigate Direction class definition, we shall..."
fd -e hpp -e h "Direction.hpp" --exec cat {}
Length of output: 27209
Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp (1)
330-330: Hmmmm, proper usage of Direction constructor, this is.
Changed from static values to function calls, the Direction initialization has been. Consistent with the new explicit constructor pattern, these changes are.
Also applies to: 352-352
Examples/Algorithms/Geant4/src/SensitiveSteppingAction.cpp (1)
101-101: Wise choice, the explicit Direction constructor is.
From static value to function call, the Direction initialization has evolved. Clear intention and proper initialization, this change brings.
Plugins/Json/src/PortalJsonConverter.cpp (1)
322-323: Strong with the Force, this Direction initialization is.
Array of Direction values, now properly initialized with function calls it is. Consistency with the new explicit constructor pattern, maintained it has been.
Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp (2)
138-138: Approve the Direction constructor change, I do!
Changed from static value to explicit constructor call, the code has. Consistent with the PR objectives, this modification is, hmmmm.
Line range hint 211-211: Approve this direction constructor change too, I must!
Wise decision it is, to use explicit constructor calls. Maintains the same logic while improving code clarity, it does.
Core/src/Detector/detail/CuboidalDetectorHelper.cpp (1)
286-290: Approve these direction constructor changes, I shall!
Changed to explicit constructor calls for both Forward and Backward directions, the code has. Maintains the portal attachment logic while improving code clarity, it does.
Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp (2)
307-307: Approve this forward direction constructor change, I do!
Changed to explicit constructor call in test setup, it has. Maintains test integrity while following new direction pattern, it does.
365-365: Approve this backward direction constructor change, I shall!
Changed to explicit constructor call in backward test setup, it has. Maintains test integrity while following new direction pattern, it does.
Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp (1)
57-57: Consistent with explicit Direction constructor pattern, these changes are!
Changed from static values to function calls, the Direction initialization has been. Maintain test functionality while improving code clarity, these modifications do.
Also applies to: 128-128
Tests/IntegrationTests/NavigatorConsistency.cpp (1)
106-106: Hmmmm, consistent with the new Direction pattern, these changes are!
In both backward propagation test configurations, updated the Direction initialization has been. Maintain the same test behavior while following the new explicit constructor pattern, it does.
Also applies to: 192-192
Tests/IntegrationTests/PropagationTests.hpp (2)
272-272: In harmony with the Force, these changes are!
Updated both the code and comments to use explicit Direction constructor calls, we have. Documentation and implementation, aligned they now are.
Also applies to: 296-296
Line range hint 57-57: Verify consistency across the codebase, we must!
Search for any remaining static Direction usage that might need updating, we shall.
Also applies to: 272-272
✅ Verification successful
Consistent with the Force, the Direction usage is, hmmmm
Through the Force, searched the entire codebase I have. All instances of Direction usage follow the explicit constructor pattern they do, yes. No static access remains that updating requires. In the production code and examples both, Direction::Forward() and Direction::Backward() properly called they are.
The path to enlightenment shows:
- Core components: Proper Direction constructor usage they have
- Example algorithms: The new pattern they follow
- Plugin implementations: Aligned with the ways of the Force they are
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential remaining static Direction usage
echo "Searching for potential static Direction usage..."
rg --type cpp "Direction::(Forward|Backward|Both)" -g "!Tests"
echo "Searching for updated Direction constructor calls..."
rg --type cpp "Direction::(Forward|Backward|Both)\(\)" -g "!Tests"
Length of output: 10351
Tests/UnitTests/Core/Geometry/PortalShellTests.cpp (1)
317-317: Consistent with the Force, these changes are!
Changed the static Direction members to function calls, we have. Maintain the same behavior while improving explicitness, these modifications do. Hmmmm.
Examples of changes:
Direction::OppositeNormal→Direction::OppositeNormal()Direction::AlongNormal→Direction::AlongNormal()
Also applies to: 323-323, 335-335, 341-341, 731-731, 733-734, 736-739, 743-743, 746-747, 748-748, 750-752
Core/src/Geometry/TrackingVolume.cpp (1)
117-117: Strong with the Force, these changes are!
From static members to function calls, the Direction usage has evolved:
Direction::Positive→Direction::Positive()Direction::Negative→Direction::Negative()
Maintain the same behavior while improving clarity and explicitness, these modifications do. Hmmmm.
Also applies to: 136-136, 141-141
Core/include/Acts/TrackFitting/detail/GsfActor.hpp (1)
412-412: Wise these changes are, but verify we must!
From static members to function calls, the Direction comparisons have evolved:
state.options.direction == Direction::Forward→state.options.direction == Direction::Forward()
Critical for momentum calculations this change is. Though behavior remains same, verify thoroughly we should.
Run these tests to ensure momentum calculations remain correct:
Also applies to: 427-427
✅ Verification successful
Let me search for Direction usage patterns to verify the changes, hmm. Failed the first attempt did, but broader search we shall conduct now. Through the Force, sense the codebase structure we must.
Safe and sound, these Direction changes are!
Through the Force, seen I have that Direction::Forward() consistently used is, across many tests and core components. The new pattern, widely adopted it already is:
- In propagators and steppers
- In track fitting and finding
- In navigation systems
- In material interactions
No disruption in the Force I sense. The change from static member to function call, already established pattern it follows. Through extensive test coverage, verified this change is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify momentum calculations in GSF actor remain correct
# Search for test cases involving Direction::Forward
rg -l "Direction::Forward" Tests/UnitTests/TrackFitting/
Length of output: 126
Script:
#!/bin/bash
# Search for Direction usage in test files across the repository
fd "test.*\.(cpp|hpp)$" | xargs rg "Direction::Forward"
# Search for Direction usage in source files
rg "Direction::Forward" -A 2
# Find all files containing Direction class or enum
ast-grep --pattern 'enum Direction { $$$ }'
Length of output: 20641
Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp (7)
72-72: Approve the change in Direction initialization style, I do!
From static member access to function call, the change is. Consistent with the new pattern across the codebase, this remains.
483-483: Consistent with the Force, this change is!
Function call style for Direction, now used it is. Harmony in the codebase, this brings.
501-505: In balance with the Force, these changes are!
Multiple instances of Direction initialization, updated they have been. Consistency throughout the propagator state, maintained it is.
512-512: The path of consistency, this change follows!
Function call style for Direction, maintained it is. Wisdom in uniformity, there is.
528-528: Strong with the Force, this modification is!
The same pattern of Direction initialization, followed it has been. Harmony in the code, this maintains.
595-597: In the flow of the Force, these changes are!
Multiple instances of Direction initialization, updated they have been. Consistency in the propagator state, preserved it is.
602-604: The way of the Force, these changes follow!
Function call style for Direction, maintained it is. Balance in the code, this brings.
Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp (2)
237-237: The path of consistency, this change follows!
Navigation direction initialization, updated it has been. The new way of the Force, embraced it has.
Line range hint 373-374: In harmony with the Force, these vector initializations are!
Direction vectors, updated they have been. The new way of initialization, followed it is.
Also applies to: 389-390
Core/src/Detector/detail/CylindricalDetectorHelper.cpp (7)
713-720: The Force flows through these cylinder replacements, it does!
Direction initialization in cylinder creation, updated it has been. Consistency across the detector helper, maintained it is.
Also applies to: 727-727, 737-737
825-825: The way of the Force, this cylinder follows!
Direction initialization, consistent it remains. The path of function calls, chosen it has.
877-880: Index access with the Force, aligned it is!
Direction index retrieval, updated it has been. The new way of accessing indices, followed it is.
945-949: Through the Force, these indices flow!
Direction index access, consistent it remains. The path of function calls, maintained it is.
1058-1058: With the Force, this index aligns!
Direction index retrieval, updated it has been. The new way, followed it is.
1073-1073: Through the Force, this index flows!
Direction index access, consistent it remains. The path of function calls, chosen it has.
1087-1087: With the Force, this final index aligns!
Direction index retrieval, updated it has been. Complete, the transformation is.
Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp (3)
282-282: Forward and backward propagation tests updated correctly, they are.
In the forward/backward propagation tests, Direction initialization changed to explicit constructor calls has been. Maintains test behavior while improving code clarity, it does.
Also applies to: 298-298
442-442: Ambiguity handling tests properly modified, they have been.
For ambiguity test cases, Direction initialization now uses explicit constructor pattern. Consistent with other changes throughout codebase, this is.
Also applies to: 460-460
557-557: Multiple intersection tests updated appropriately, yes.
In multiple intersection test scenarios, Direction initialization follows new pattern. Clear and consistent with project standards, the changes are.
Also applies to: 575-575
Tests/UnitTests/Core/TrackFitting/FitterTestsCommon.hpp (1)
239-239: Test helper methods properly updated, they have been.
In both forward and backward test methods, Direction initialization now uses explicit constructor calls. Maintains consistency across test suite, it does. Clear and explicit the intention becomes.
Also applies to: 298-298
Tests/UnitTests/Core/Detector/PortalTests.cpp (1)
91-92: Strong with the Force, these changes are.
Consistently updated to use new Direction API, all test cases are. Maintained, the test coverage is.
Also applies to: 114-115, 209-210, 218-219, 231-232, 241-242, 252-253, 260-261
Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp (1)
73-73: Approve these changes, I do! Explicit Direction construction, a path to clarity it is!
Changed from static members to function calls for Direction values, maintaining the same behavior while improving code clarity and type safety.
Also applies to: 211-211, 321-321
Tests/UnitTests/Core/Geometry/PortalTests.cpp (1)
74-74: Hmmmm... Consistent changes throughout the portal tests, I sense!
From static members to function calls, the Direction values have evolved. Changed the syntax is, but preserved the behavior remains. A good refactoring, this is!
Also applies to: 90-90, 109-110, 116-117, 172-173, 176-176, 616-616, 621-621, 626-626, 628-628, 650-651, 654-655
Tests/UnitTests/Core/Propagator/NavigatorTests.cpp (1)
229-229: Simple but significant change, this is! Explicit Direction construction in navigation options, we now have!
Changed from static member to function call, aligning with the new Direction instantiation pattern across the codebase.
Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp (2)
70-70: LGTM! Proper use of explicit Direction constructor, it is!
Changed from static value to function call, the Direction initialization has been. Aligns with the refactoring goals, this change does.
87-87: LGTM! Consistent with the new Direction API, this change is!
Static initialization updated correctly to use the explicit constructor, it has been. Maintains consistency across the codebase, this does.
| @@ -25,7 +25,7 @@ namespace detail { | |||
| /// @brief Holds the generic pure propagator options | |||
| struct PurePropagatorPlainOptions { | |||
| /// Propagation direction | |||
| Direction direction = Direction::Forward; | |||
| Direction direction = Direction::Forward(); | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistencies in Direction usage, I sense. Fix them, we must!
Found several instances where Direction constructors lack parentheses, hmm. Update these locations, we should:
Core/include/Acts/Material/ISurfaceMaterial.hpp:Direction::Negative()andDirection::Positive()Core/include/Acts/Geometry/BoundarySurfaceT.hpp:Direction::Backward()Core/src/Geometry/TrackingVolume.cpp:Direction::Positive()Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp:Direction::Backward()Tests/UnitTests/Core/Propagator/VolumeMaterialInteractionTests.cpp:Direction::Forward()andDirection::Backward()Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp:Direction::Forward()andDirection::Backward()Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp:Direction::Backward()andDirection::Forward()
Consistent with the explicit constructor pattern, these changes must be. Yes, hmmm.
🔗 Analysis chain
Approve this change, I do! Verify consistency across the codebase, we must.
The change from Direction::Forward to Direction::Forward() aligns with the explicit constructor pattern, hmm.
Run this script to verify consistent usage, we shall:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining static Direction values that need updating
rg "Direction::(Forward|Backward|Positive|Negative)[^(]" --type cpp
# Search for updated pattern to confirm widespread adoption
rg "Direction::(Forward|Backward|Positive|Negative)\(\)" --type cpp
Length of output: 13240
| @@ -88,16 +92,17 @@ class Direction final { | |||
| /// | |||
| /// @return an opposite direction | |||
| constexpr Direction invert() const { | |||
| return (m_value == Value::Positive) ? Value::Negative : Value::Positive; | |||
| return Direction{m_value == Value::Positive ? Value::Negative | |||
| : Value::Positive}; | |||
| } | |||
|
|
|||
| std::string toString() const; | |||
|
|
|||
| constexpr Direction() = default; | |||
There was a problem hiding this comment.
should we get rid of the default constructor?
There was a problem hiding this comment.
Might be a good idea, I'll give it a try.
| static constexpr Direction Negative() { return Direction{Value::Negative}; } | ||
| static constexpr Direction Positive() { return Direction{Value::Positive}; } | ||
|
|
||
| static constexpr auto Backward = Value::Negative; | ||
| static constexpr auto Forward = Value::Positive; | ||
| static constexpr Direction Backward() { return Direction{Value::Negative}; } | ||
| static constexpr Direction Forward() { return Direction{Value::Positive}; } | ||
|
|
||
| static constexpr auto OppositeNormal = Value::Negative; | ||
| static constexpr auto AlongNormal = Value::Positive; | ||
| static constexpr Direction OppositeNormal() { | ||
| return Direction{Value::Negative}; | ||
| } | ||
| static constexpr Direction AlongNormal() { | ||
| return Direction{Value::Positive}; | ||
| } |
There was a problem hiding this comment.
One question here is whether the static factories should start with a lower case.
There was a problem hiding this comment.
I was also thinking about this and I don't really have come to a conclusion. Having the upper case is what Eigen does and I feel like it improved readability. But it seems also be against our usual convention.
I would be good with keeping them upper case
|
This requires changing the static names values to static functions
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation