refactor: Decouple stepping from navigation #4039
Conversation
WalkthroughSignificant changes in the propagation system, there are. Across multiple stepper classes and files, modifications flow like the Force. Removal of template parameters, simplification of method signatures, and introduction of a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Hehehe! 🧘♂️ ✨ 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 (
|
![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
🧹 Nitpick comments (12)
Core/include/Acts/Propagator/PropagatorState.hpp (1)
39-40: Approve these type aliases, I do! Clear and purposeful, they are.Hmm, wise addition of type aliases, this is. Clarity to the code, they bring. With the greater mission of decoupling stepping from navigation, aligned they are.
A path to better modularity, these aliases pave. In future refactoring endeavors, easier to maintain and understand, the code shall be. Strong with the Force, this change is!
Core/src/Propagator/SympyStepper.cpp (1)
87-91: Incomplete, this implementation remains. Track it properly, we must!Like a young Padawan without training, this method stands. Implementation following EigenStepper's path, it requires.
Create an issue to track this TODO, shall I? Help implement the proper curvilinear state preparation, I can.
Core/include/Acts/Propagator/Propagator.ipp (1)
Line range hint
322-386: Preparation of curvilinear state, ensure correctness you must.The call to
m_stepper.prepareCurvilinearState(state.stepping)is critical. If it fails, returningpropagationResult.error()might not provide sufficient context. Consider handling the error more explicitly to aid in debugging.Core/include/Acts/Propagator/EigenStepperDenseExtension.hpp (1)
Line range hint
496-498: Clarify the unusual test parameters, we shouldComments add, to explain 'stepSizeCutOff' and 'maxRungeKuttaStepTrials' extreme values. Maintainability, this aids.
Also applies to: 503-505
Core/include/Acts/Propagator/EigenStepperDefaultExtension.hpp (2)
40-48: Unused parameter, a mystery it remains!Document the future purpose of volumeMaterial, we should. Help future Jedi understand its role, this will.
Add a TODO comment explaining the intended future use of the volumeMaterial parameter.
72-76: Pattern of unused parameter, continues it does!Document the future purpose of volumeMaterial here too, we must. Consistency in documentation, important it is.
Add a TODO comment explaining the intended future use of the volumeMaterial parameter.
Core/include/Acts/Propagator/SympyStepper.hpp (2)
27-28: Wise decision, the introduction of IVolumeMaterial interface is, hmm.Separation of concerns, this brings. Material properties from navigation logic, it decouples. Clean architecture principles, it follows.
302-304: Simpler, the prepareCurvilinearState method becomes.Template parameters and navigator dependency, removed they are. Cleaner interface, we now have. Strong in SOLID principles, this change is.
Core/include/Acts/Propagator/EigenStepper.hpp (1)
241-252: More descriptive, the parameter name becomes.From navDir to propDir, renamed it is. Purpose of parameter, better reflected it becomes. Consistency in terminology, maintained it is.
Core/include/Acts/Propagator/Propagator.hpp (1)
95-97: Architectural improvement, this change brings.Navigator dependency from bound parameters support, removed it is. Simpler and cleaner, the propagator becomes. Decoupling strategy, complete it makes.
Core/include/Acts/Propagator/StraightLineStepper.hpp (1)
320-337: Simplified the interface has become, yet more detailed the implementation is.The Force flows stronger through this method now:
- Navigator dependency, removed it is
- Path accumulation check, more precise it has become
- Derivative calculations, more thorough they are
Consider adding documentation for the derivative calculations, help future Padawans it will.
Core/include/Acts/TrackFitting/KalmanFitter.hpp (1)
660-660: Hmm, wise this change is, aligns with the decoupling mission it does.Pass only the stepping state we now do, instead of entire propagation state we did before. Simplified and more focused the interface becomes, young padawan.
A step towards better separation of concerns this is. Like the Force, decoupled and balanced our components should be.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
Core/include/Acts/Propagator/AtlasStepper.hpp(9 hunks)Core/include/Acts/Propagator/EigenStepper.hpp(5 hunks)Core/include/Acts/Propagator/EigenStepper.ipp(8 hunks)Core/include/Acts/Propagator/EigenStepperDefaultExtension.hpp(4 hunks)Core/include/Acts/Propagator/EigenStepperDenseExtension.hpp(11 hunks)Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp(2 hunks)Core/include/Acts/Propagator/MultiEigenStepperLoop.ipp(6 hunks)Core/include/Acts/Propagator/Propagator.hpp(1 hunks)Core/include/Acts/Propagator/Propagator.ipp(2 hunks)Core/include/Acts/Propagator/PropagatorState.hpp(1 hunks)Core/include/Acts/Propagator/PropagatorTraits.hpp(1 hunks)Core/include/Acts/Propagator/StraightLineStepper.hpp(3 hunks)Core/include/Acts/Propagator/SympyStepper.hpp(4 hunks)Core/include/Acts/Propagator/SympyStepper.ipp(0 hunks)Core/include/Acts/Propagator/VoidNavigator.hpp(2 hunks)Core/include/Acts/Propagator/detail/LoopStepperUtils.hpp(4 hunks)Core/include/Acts/TrackFitting/KalmanFitter.hpp(1 hunks)Core/include/Acts/TrackFitting/detail/GsfActor.hpp(2 hunks)Core/include/Acts/TrackFitting/detail/KalmanUpdateHelpers.hpp(3 hunks)Core/src/Propagator/SympyStepper.cpp(5 hunks)Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp(10 hunks)Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp(6 hunks)Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp(4 hunks)Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp(5 hunks)Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp(6 hunks)
💤 Files with no reviewable changes (1)
- Core/include/Acts/Propagator/SympyStepper.ipp
👮 Files not reviewed due to content moderation or server errors (1)
- Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp
⏰ Context from checks skipped due to timeout of 90000ms (14)
- 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 / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_exatrkx
🔇 Additional comments (42)
Core/include/Acts/TrackFitting/detail/GsfActor.hpp (2)
238-239: Approve the direct covariance transport approach, I do!More efficient, this change is. Direct access to component state and transport of covariance, it provides. Aligned with the decoupling objectives, it remains.
642-643: Wise choice, using reference instead of copy, it is!Changed from copying state to using reference, performance benefits it brings. But careful we must be, for lifetime of reference we must ensure.
Run this verification we shall:
✅ Verification successful
Safe the reference usage is, verified it has been!
Through the Force, searched the codebase we have. Within loop scope the reference stays, never beyond its bounds it strays. Wise decision to avoid copying, it was!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any state lifetime issues in component handling ast-grep --pattern 'auto& state = $_.state(); $$$ return $_'Length of output: 66
Script:
#!/bin/bash # Search for component state access patterns ast-grep --pattern 'for ($_ : $_.componentIterable($_)) { $$$ }' # Also search for direct mentions of state access rg -B 2 -A 2 "\.state\(\)" --type cppLength of output: 1450
Core/include/Acts/Propagator/PropagatorState.hpp (1)
39-40: Verify consistent usage throughout the codebase, we must.Hmm, search for inconsistent type usage in the Force, I shall.
✅ Verification successful
Consistent with the Force, these type usages are, hmm.
In balance, the type system remains. Throughout the codebase, proper template parameters and type aliases flow like the Force itself. No disturbance in the type system, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for direct usage of raw template types instead of aliases # Search for direct usage of stepper_state_t and navigator_state_t rg "stepper_state_t(?!ype)" Core/include/Acts/Propagator/ rg "navigator_state_t(?!ype)" Core/include/Acts/Propagator/ # Look for potential inconsistencies in type usage ast-grep --pattern 'using $_ = stepper_state_t' Core/include/Acts/Propagator/ ast-grep --pattern 'using $_ = navigator_state_t' Core/include/Acts/Propagator/Length of output: 769
Script:
#!/bin/bash # Search with PCRE2 support for better pattern matching rg --pcre2 "stepper_state_t(?!ype)" Core/include/Acts/Propagator/ rg --pcre2 "navigator_state_t(?!ype)" Core/include/Acts/Propagator/ # Search for type definitions and usages rg "stepper_state_t|navigator_state_t" Core/include/Acts/Propagator/ # Look for template parameter declarations rg "template.*?(?:stepper_state_t|navigator_state_t)" Core/include/Acts/Propagator/ # Check type alias patterns ast-grep --pattern 'template <$$$> class $_ { using stepper_state_type = $_; }' ast-grep --pattern 'template <$$$> class $_ { using navigator_state_type = $_; }'Length of output: 6667
Core/src/Propagator/SympyStepper.cpp (3)
11-11: Wise addition of error handling, this include is!For proper error reporting in stepImpl, necessary this header becomes.
229-229: Consistent with the Force, this change is!Direction comparison now aligns with the new propagation direction parameter, bringing balance to the code.
136-142: Hmm, ignored the material parameter is. Intentional, this may be?Like the Force flowing through all things, material effects should not be ignored without reason. Document the intention or implement the material handling, we should.
Run this command to check if other steppers handle the material parameter:
Core/include/Acts/Propagator/MultiEigenStepperLoop.ipp (1)
Line range hint
94-225: Refactoredstepmethod enhances modularity, it does.The decoupling of the
stepfunction from the navigator by altering its signature to acceptState& state,Direction propDir, andconst IVolumeMaterial* materialaligns well with the PR objectives. Improves clarity and allows independent testing of steppers, this change does.Core/include/Acts/Propagator/EigenStepper.ipp (1)
174-190: Efficient error handling in Runge-Kutta step, commendable it is.The implementation of
tryRungeKuttaStepand the error estimation logic enhances robustness. Properly clamps the step size scaling and checks error tolerances, it does.Core/include/Acts/Propagator/Propagator.ipp (1)
84-86: Alignment with new stepper signature, correctly done it is.Updating the call to
m_stepper.stepto passstate.stepping,state.options.direction, andm_navigator.currentVolumeMaterial(state.navigation)reflects the changes in thestepmethod's signature. Consistent with the refactoring objectives, this is.Core/include/Acts/Propagator/EigenStepperDenseExtension.hpp (2)
75-82: Simplify the 'k' method signature, you haveBy removing 'navigator', dependencies reduced are. Modularity, this enhances.
160-160:⚠️ Potential issuePrevent division by zero, we must
Ensure 'newMomentum' is not zero before dividing at lines 160 and 162. Runtime errors, this avoids.
Also applies to: 162-162
Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp (1)
280-299: Thorough your tests areTesting with and without covariance transport, reliability you ensure.
Core/include/Acts/Propagator/PropagatorTraits.hpp (1)
15-21: Simplified the template structure, you haveBy reducing parameters, code clarity improved is. Readability, benefit it does.
Core/include/Acts/Propagator/VoidNavigator.hpp (3)
19-20: Clean and proper, these forward declarations are!Support the new interface methods, they do. Approve this change, I must.
61-63: Wisdom in simplicity, there is!Return nullptr, this method does. Align with VoidNavigator's purpose of doing nothing, it does. Approve this implementation, I do.
65-67: Consistent with the Force, this implementation is!Return nullptr, it does, like its siblings. Maintain the void nature of the navigator, it does. Approve this change, I must.
Core/include/Acts/Propagator/detail/LoopStepperUtils.hpp (3)
11-14: Well ordered, these includes are!Required for the new functionality, they are. Properly organized, they appear.
49-49: Strong with the Force, this const correctness is!Proper access to state, it provides. Maintain const correctness, it does. A well-crafted addition, this is.
Also applies to: 101-101
113-113: Balance between const and mutable, achieved it is!Complete the state access pattern, this does. Necessary for state modifications, it is.
Core/include/Acts/TrackFitting/detail/KalmanUpdateHelpers.hpp (2)
132-137: Simplified and focused, the interface has become!Decouple stepping from navigation, this change does. Direct access to stepper state, it provides. A step toward clarity, this is.
Line range hint
153-162: Consistent in its ways, the state access now is!Direct and clear, the state usage has become. No more through stepping it goes. Approve this transformation, I do.
Core/include/Acts/Propagator/EigenStepperDefaultExtension.hpp (2)
118-123: Direct and clear, the state access has become!Clean and efficient, this implementation is. Approve this change, I must.
160-164: In harmony with the Force, these changes are!Consistent state access patterns, they maintain. Clean and well-structured, the code remains.
Core/include/Acts/Propagator/SympyStepper.hpp (2)
366-375: Well documented and clear, the step method changes are.Material properties through dedicated interface, now passed they are. Navigation concerns, separated they become. Documentation, properly updated it is.
393-394: Simplified, the template specialization is.Navigator dependency, removed it is. Cleaner trait implementation, we achieve. With the Force of good design, this change flows.
Core/include/Acts/Propagator/EigenStepper.hpp (1)
399-408: Consistent with SympyStepper, these changes are.Same pattern of decoupling, followed it is. Harmony between implementations, achieved it is. Strong, the Force of consistency is.
Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp (1)
175-200: Simplified yet thorough, the tests remain.Direct state manipulation, clearer it becomes. Coverage of functionality, maintained it is. Step with and without covariance transport, tested they are. Approve these changes, I do.
Core/include/Acts/Propagator/StraightLineStepper.hpp (3)
39-40: Hmm, a new interface for volume material handling, I see.A forward declaration of
IVolumeMaterial, this is. Wise separation of concerns, it brings. Material properties from navigation logic, it decouples.
458-459: Simpler the template specialization has become.Navigator template parameter, removed it is. Cleaner and more focused, the code becomes.
402-452: Hmm, clearer path this method now follows.Strong improvements I sense:
- Direction and material parameters, explicit they have become
- Statistics tracking, more comprehensive it is
- Step calculations, better organized they are
But a small concern I have:
- ++state.nSteps; - ++state.nStepTrials; + state.statistics.nAttemptedSteps++; + state.statistics.nSuccessfulSteps++;Verify the statistics tracking we must:
✅ Verification successful
Consistent with the Force, these statistics changes are.
In harmony with other steppers - SympyStepper and EigenStepper - your implementation flows. The same pattern of tracking attempted, successful, and reverse steps, they all follow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other statistics tracking patterns rg "statistics\.(nAttempted|nSuccessful|nReverse)Steps" -A 2Length of output: 1831
Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp (2)
254-273: Clearer and more focused, these tests have become.The path to testing:
- State management, simplified it is
- Step verification, more thorough it has become
- Covariance transport, properly disabled it remains
456-458: Hmm, step tolerance testing, important it is.Wise to verify that step size remains unchanged when tolerance is large, it is.
Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp (5)
293-294: Simplified state initialization, approve I do!Cleaner the code becomes, when mock structures we remove. Direct state creation, a path to clarity it is.
297-303: Well structured test case, this is!Proper validation of step size and execution, the test performs. Strong with the Force, this code is.
306-317: Thorough testing of physical properties, I sense!All aspects of particle motion, this test validates:
- Position change it checks
- Time evolution it verifies
- Direction rotation it confirms
- Momentum and charge conservation it ensures
Well balanced, these tests are.
Line range hint
426-443: Complete state reset validation, observe I do!Thorough checking of all components after reset:
- Covariance transport
- Position vectors
- Direction normalization
- Momentum preservation
- Path accumulation
- Step size handling
Leave no stone unturned, a Jedi must.
Line range hint
446-483: Multiple reset scenarios, tested they are!Wisdom in testing different paths:
- Full reset with all parameters
- Partial reset without step size
- Minimal parameter reset
Like a lightsaber crystal, each facet examined must be.
Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp (2)
678-681: Simplified interface, appreciate I do!Removed complexity, yet maintained functionality we have. Template parameters, needed they were not. Elegant solution, this is.
736-745: Improved step method signature, I sense!Clear direction and material handling, this interface provides:
- Direction parameter, explicit it is
- Material pointer, added it was
- Documentation, improved it became
Like a well-crafted lightsaber, elegant and purposeful this design is.
Core/include/Acts/Propagator/AtlasStepper.hpp (3)
33-34: Forward declaration of IVolumeMaterial, wise this is!Material handling capabilities, prepare for we must. Clean and minimal, the declaration is.
533-536: Simplified state preparation, harmony brings!With other steppers, aligned this change is. Unnecessary complexity, removed it was. Path to clarity, found we have.
Line range hint
1156-1415: Improved step implementation, powerful it has become!Like a Jedi's training, refined this code is:
- Material handling, prepared for it is
- State management, clearer it became
- Path accumulation, accurate it remains
- Step trials, counted they are
Through the Force, better code flows.
acts-project-service
added
the
Breaks Athena build
|
Athena CI went through https://gitlab.cern.ch/acts/acts-athena-ci/-/pipelines/9931532 |
|
I'll look at this tomorrow! |
…pping-from-navigation
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Core/include/Acts/Propagator/EigenStepper.ipp (1)
83-85:⚠️ Potential issueIncorrect, this condition is. A disturbance in the Force, I sense.
The condition
if (state.pathAccumulated != 0)returns early when path is accumulated, preventing initialization of derivatives. The opposite of what we seek, this is.Apply this correction, you must:
- if (state.pathAccumulated != 0) { + if (state.pathAccumulated == 0) { return true; }
🧹 Nitpick comments (2)
Core/include/Acts/Propagator/Propagator.hpp (1)
92-97: Strong with the Force, this change is!Decoupling of stepper from navigator, achieved it has. Simplified template parameter check
SupportsBoundParameters_v<stepper_t>, more maintainable and focused the code makes.A wise path chosen you have. Independent testing and usage of steppers, this change enables. Modularity and separation of concerns, strengthened they are.
Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp (1)
411-412: Mysterious, this large step tolerance value is.A magic number of 4.4258e+09, I sense. Extract this to a named constant or document its significance, we should. Mask potential issues, such large tolerances might.
+// Maximum step tolerance value used for testing no step size adjustment cases +static constexpr double MAX_STEP_TOLERANCE = 4.4258e+09; + - esState.options.stepTolerance = 2. * 4.4258e+09; + esState.options.stepTolerance = 2. * MAX_STEP_TOLERANCE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Core/include/Acts/Propagator/AtlasStepper.hpp(9 hunks)Core/include/Acts/Propagator/EigenStepper.hpp(5 hunks)Core/include/Acts/Propagator/EigenStepper.ipp(8 hunks)Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp(2 hunks)Core/include/Acts/Propagator/Propagator.hpp(1 hunks)Core/include/Acts/Propagator/Propagator.ipp(2 hunks)Core/include/Acts/Propagator/StraightLineStepper.hpp(3 hunks)Core/include/Acts/Propagator/SympyStepper.hpp(4 hunks)Core/include/Acts/TrackFitting/KalmanFitter.hpp(1 hunks)Core/src/Propagator/SympyStepper.cpp(5 hunks)Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp(9 hunks)Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp(5 hunks)Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp(4 hunks)Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp(3 hunks)Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Core/include/Acts/TrackFitting/KalmanFitter.hpp
- Core/src/Propagator/SympyStepper.cpp
- Core/include/Acts/Propagator/SympyStepper.hpp
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: missing_includes
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu
🔇 Additional comments (19)
Core/include/Acts/Propagator/Propagator.hpp (1)
67-91: Documentation, accurate it remains!Well-documented the changes are, hmm. Clear explanation of the propagator's purpose and requirements, maintained it is. The Force, strong in this documentation it is.
Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp (1)
176-201: Strong and thorough, these tests are!Well-structured verification of stepping functionality, I see. Both with and without covariance transport, tested it is. Proper assertions for position, direction, momentum, and step size, included they are.
Core/include/Acts/Propagator/EigenStepper.hpp (1)
400-401: Wise, this decoupling is!Simplified and clear, the step method signature has become. No longer bound to navigation templates, it is. Through the Force of clean design, independent the stepper now stands.
Core/include/Acts/Propagator/StraightLineStepper.hpp (1)
376-420: Clean and focused, this implementation is!Properly handles the material parameter with void cast, it does. Clear stepping logic without navigation dependencies, it maintains. Statistics tracking, it remembers.
Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp (1)
255-276: Hmm, good these changes are!Simplified step method calls and state access, while maintaining test coverage for both covariance and non-covariance transport cases, I see. Clear and focused, the test remains.
Core/include/Acts/Propagator/Propagator.ipp (1)
84-86: Clearer, the stepping dependencies now are!Decoupled from navigation, the step method has become. Explicit passing of stepping state, direction and material, better design this is.
Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp (1)
300-303: Simpler, the state management has become!Removed the mock structure, we have. Direct state access, clearer and more maintainable it is.
Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp (2)
734-735: Wise, these signature changes are!Explicit direction and material parameters, better design they bring. With navigation decoupled, clearer responsibilities we have.
670-673: Mysterious, this simplification is.Always returns true, this method now does. Verify that preparation is indeed handled elsewhere or no longer needed, we must.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for curvilinear state preparation in other files rg -A 5 "prepareCurvilinearState"Length of output: 3933
Simplification by design, it is.
From the search results, clear evidence there is that many stepper implementations, such as in AtlasStepper and MultiEigenStepperLoop, return true immediately in prepareCurvilinearState. In contrast, EigenStepper contains a conditional on state.pathAccumulated, suggesting its extra check is specific to that implementation. Likely, the preparation responsibility is either handled elsewhere or simply unnecessary within MultiEigenStepperLoop. No concerns remain, verified this design choice is.
Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp (3)
311-313: Approve the simplified step method call, I do!Cleaner interface achieved by removing navigation dependency, this change brings. Simpler testing and better separation of concerns, it enables.
763-763: Direct state access, better it is!Removed unnecessary abstraction layer, we have. More straightforward and maintainable, the code becomes.
606-606: Consistent simplification, this change brings!Harmonious with other changes, it is. Navigation dependency removed, clarity gained.
Core/include/Acts/Propagator/AtlasStepper.hpp (4)
35-35: Forward declaration of IVolumeMaterial, wise it is!Decoupled material interaction support, this brings. Clean separation of concerns, it maintains.
530-532: Simpler interface, achieved we have!Template parameters removed, clarity gained. Direct state manipulation, more straightforward it becomes.
1407-1409: State tracking, properly maintained it is!Path accumulation and step statistics, correctly updated they are. Clean and clear, the implementation is.
1154-1157: Unused material parameter, noticed I have!Added for future use, the material parameter is. Implementation or removal, consider you should.
✅ Verification successful
🌐 Web query:
What is the recommended way to handle unused parameters in C++?💡 Result:
To handle unused parameters in C++, the recommended approaches balance explicitness, portability, and code clarity: ## Standard C++17 Method Use `[[maybe_unused]]` attribute: ```cpp void example([[maybe_unused]] int param) { // No compiler warnings // Function body } ``` This is **portable** and officially standardized since C++17[1][3]. ## Compiler-Specific Methods For pre-C++17 codebases or cross-platform projects: ```cpp // GCC/Clang void example(int __attribute__((unused)) param) {} // MSVC void example(int _UNUSED param) {} ``` Best wrapped in a macro for portability[1][4]: ```cpp #if defined(__GNUC__) # define UNUSED(x) x __attribute__((unused)) #elif defined(_MSC_VER) # define UNUSED(x) _UNUSED x #else # define UNUSED(x) x #endif void example(UNUSED(int param)) {} ``` ## Alternative Techniques 1. **Parameter name omission** (C++ only): ```cpp void example(int) {} // No name = no usage possible ``` 2. **Void cast** (works universally but less explicit): ```cpp void example(int param) { (void)param; // Silences warnings } ``` ## Key Considerations - **Polymorphism cases**: Unused parameters are common in interface implementations[10] - **Code evolution**: Macros that rename parameters (e.g., `UTILS_UNUSED_PARAM(x)`) help prevent accidental future use[1] - **Documentation**: Attributes/macros make intent clearer than name omission[3] The modern preference is `[[maybe_unused]]` where available[3][10], with macro-based solutions for cross-platform projects[1][4]. Avoid "harmless use" patterns like `x=x` which might trigger other warnings[4]. Citations: - 1: https://jmmv.dev/2015/02/unused-parameters-in-c-and-c.html - 2: https://www.modernescpp.com/index.php/c-core-guidelines-how-to-pass-function-parameters/ - 3: https://lms.fun-mooc.fr/asset-v1:inria+41020+session01+type@asset+block/forum-unused-parameters-attribute-unused-2181.html - 4: https://www.thecodingforums.com/threads/table-of-safe-methods-to-suppress-unused-parameter-warnings.970012/ - 5: https://blog.nexotips.com/mastering-c-function-parameters/ - 6: https://cs61.seas.harvard.edu/site/2019/Patterns/ - 7: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines - 8: https://learn.microsoft.com/th-th/cpp/cpp/errors-and-exception-handling-modern-cpp?view=msvc-160 - 9: https://rider-support.jetbrains.com/hc/en-us/community/posts/6116891602066-C-how-should-I-deal-with-deliberately-unused-parameters - 10: https://cplusplus.com/forum/beginner/250535/
Addressing the Unused Material Parameter, You Must
In file
Core/include/Acts/Propagator/AtlasStepper.hpparound lines 1154–1157, the parametermaterialis currently unused—its presence marked only by a void cast(void)material;. While its addition for future use is understood, modern C++ wisdom advises more explicit handling.
- Consider using the C++17
[[maybe_unused]]attribute onmaterialto clearly signal its intentional non-use.- Alternatively, retaining the current void cast is acceptable if backward compatibility or existing style is a priority.
Decide, you must, based on project guidelines and compiler support.
Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp (3)
281-300: Approve the simplified step function calls, I do.Cleaner and more focused, the step function calls have become. Direct usage of
esStateand explicit navigation direction, a better separation of concerns it brings. Passed all tests, these changes have.
343-344: Improved state copying mechanism, I sense.Stronger type safety through proper field type usage, these changes bring. Explicit state initialization, a path to better maintainability it is.
Also applies to: 448-452
437-438: Direct access to options through esState, wisdom this brings.Through esState.options, cleaner access to step tolerance and cutoff values we gain. Simplified configuration management, this represents.
Also applies to: 450-452
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Core/include/Acts/Propagator/Propagator.ipp (2)
84-86: Improved separation of concerns, I sense!A wise change this is, passing only the stepping state and material information to the stepper. Decoupled from navigation, the stepping component now stands.
322-322: Simplified interface, you have created!Only the stepping state, this function now requires. Easier to test and maintain, it becomes. Independent of navigation state, it now stands.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/Propagator/Propagator.ipp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: build_debug
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: docs
- GitHub Check: linux_ubuntu
andiwand
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Core/include/Acts/Propagator/StraightLineStepper.hpp (1)
289-291: Fix the path accumulation check, you must.The same disturbance in the Force, here I sense. Early return when path accumulated, incorrect it is.
Apply this fix:
- if (state.pathAccumulated != 0) { + if (state.pathAccumulated == 0) { return true;
🧹 Nitpick comments (2)
Core/include/Acts/Propagator/SympyStepper.hpp (1)
27-27: Document the new interface class, you must.For the new
IVolumeMaterialclass, documentation missing it is. Add class documentation to clarify its purpose and usage, hmm.+/// @brief Interface for volume material description +/// @note This class can be nullptr in the stepping methods class IVolumeMaterial;Core/include/Acts/Propagator/AtlasStepper.hpp (1)
1152-1153: Document the material parameter better, we should.Like in other steppers, document that nullptr is acceptable for the material parameter, we must. Consistency in documentation across all steppers, important it is.
- /// @param material is the optional volume material we are stepping through. - // This is simply ignored if `nullptr`. + /// @param material The volume material we are stepping through. + /// This parameter is optional and can be nullptr, in which case + /// material effects are ignored during the step.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Core/include/Acts/Propagator/AtlasStepper.hpp(9 hunks)Core/include/Acts/Propagator/EigenStepper.hpp(5 hunks)Core/include/Acts/Propagator/StraightLineStepper.hpp(3 hunks)Core/include/Acts/Propagator/SympyStepper.hpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Propagator/EigenStepper.hpp
⏰ Context from checks skipped due to timeout of 90000ms (21)
- 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_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: unused_files
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
- GitHub Check: docs
🔇 Additional comments (2)
Core/include/Acts/Propagator/SympyStepper.hpp (2)
298-298: LGTM! Simplified interface, I sense.The method signature change from templated to concrete state improves clarity and reduces complexity, it does. A step towards better separation of concerns, this is.
370-371: LGTM! Clear parameter documentation, it is.Well documented the material parameter is, with clear indication that nullptr is acceptable. Good practice this shows.
…pping-from-navigation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Core/include/Acts/Propagator/Propagator.hpp (1)
94-97: Approve the decoupling of stepper and navigator.Hmm, wise change this is. Removed dependency on navigator_t from SupportsBoundParameters_v, you have. Independent testing of steppers, this enables. Strong with SOLID principles, this change is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Core/include/Acts/Propagator/Propagator.hpp(1 hunks)Core/include/Acts/Propagator/Propagator.ipp(2 hunks)Core/include/Acts/Propagator/VoidNavigator.hpp(2 hunks)Core/include/Acts/TrackFitting/KalmanFitter.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Core/include/Acts/Propagator/VoidNavigator.hpp
- Core/include/Acts/TrackFitting/KalmanFitter.hpp
⏰ Context from checks skipped due to timeout of 90000ms (21)
- 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_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: unused_files
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu
- GitHub Check: docs
🔇 Additional comments (3)
Core/include/Acts/Propagator/Propagator.ipp (2)
84-86: Wise changes in the step method call, I see!Decoupled from navigation, the stepping has become. Only what is needed, passed it is - the stepping state, direction, and material information. Cleaner and more focused, the implementation now stands.
354-354: Consistent with the path of decoupling, this change is!Simplified the interface has become, using only the stepping state it does. A pattern of independence, throughout the changes I observe.
Core/include/Acts/Propagator/Propagator.hpp (1)
92-97: Verify impact on existing implementations.Changed the inheritance condition has been. Verify we must, that all existing steppers still compatible they are.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for all stepper implementations echo "Searching for stepper implementations..." ast-grep --pattern 'class $STEPPER : public $BASE { $$$ }' # Search for SupportsBoundParameters usage echo "Checking SupportsBoundParameters usage..." rg "SupportsBoundParameters" -A 2Length of output: 1866
Steady, the changes are.
- Existing steppers (EigenStepper, StraightLineStepper, and SympyStepper) indeed specialize SupportsBoundParameters to std::true_type.
- With this, Propagator's inheritance via BasePropagatorHelper is correctly selected for steppers supporting bound parameters.
- No evidence found indicates any incompatibility with current stepper implementations.
Verified, things remain harmonious.
acts-project-service
added
the
Fails Athena tests
andiwand
removed
the
Fails Athena tests
|
acts-project-service
added
the
Breaks Athena build
Very similar to #3449 this decouples the stepping from the navigation. Instead of passing the whole propagation state into stepping function we can just pass in the stepping state. This cuts all dependencies from the navigation and allows to use and test steppers independent from a propagation state and navigation.
Summary by CodeRabbit
Release Notes
New Features
IVolumeMaterialinterface across multiple propagation components.Improvements
Breaking Changes
Technical Refinements