refactor: Disentangle stepper state creation and initialization #4069
Conversation
WalkthroughChanged, the code has. The state creation process now split it is into two steps: first by calling Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client/Propagator
participant Stepper as Stepper Instance
participant State as State Object
Caller->>Stepper: makeState(options)
Stepper-->>Caller: Return empty state
Caller->>Stepper: initialize(state, parameters)
Stepper-->>Caller: Return initialized state
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ 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
|
andiwand
changed the title
![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 (3)
Core/include/Acts/Propagator/SympyStepper.hpp (1)
120-127: Approve these interface changes, I do. But documentation, we must add.Split the state creation and initialization, wisely done it is. But document the new methods, we should. Help future padawans understand the purpose of each initialization path, it will.
Add documentation for the new methods:
+/// Create a new state object with the given options State makeState(const Options& options) const; +/// Initialize state with bound track parameters void initialize(State& state, const BoundTrackParameters& par) const; +/// Initialize state with individual components +/// @param boundParams The bound parameters vector +/// @param cov Optional covariance matrix +/// @param particleHypothesis The particle hypothesis +/// @param surface The reference surface void initialize(State& state, const BoundVector& boundParams, const std::optional<BoundMatrix>& cov, ParticleHypothesis particleHypothesis, const Surface& surface) const;Core/include/Acts/Propagator/EigenStepper.hpp (1)
152-159: Consistent with SympyStepper, these changes are. Approve them, I do.Follow the same pattern of separate state creation and initialization, EigenStepper does. Consistency across steppers, this brings. Strong with the Force, this design is.
Add documentation similar to SympyStepper, we should:
+/// Create a new state object with the given options State makeState(const Options& options) const; +/// Initialize state with bound track parameters void initialize(State& state, const BoundTrackParameters& par) const; +/// Initialize state with individual components +/// @param boundParams The bound parameters vector +/// @param cov Optional covariance matrix +/// @param particleHypothesis The particle hypothesis +/// @param surface The reference surface void initialize(State& state, const BoundVector& boundParams, const std::optional<BoundMatrix>& cov, ParticleHypothesis particleHypothesis, const Surface& surface) const;Core/include/Acts/Propagator/Propagator.hpp (1)
389-391: Complete the state initialization changes, this does. Approve it, I do.Pass the parameters during initialization, the Propagator now does. Work together in harmony with the stepper changes, this does. A balanced approach to state management, we have achieved.
Add documentation to explain the parameters:
+/// Initialize the propagator state with initial parameters +/// @param state The propagator state to initialize +/// @param start The initial track parameters +/// @param path_aborter_t The path aborter type template <typename propagator_state_t, typename parameters_t, typename path_aborter_t> void initialize(propagator_state_t& state, const parameters_t& start) const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
Core/include/Acts/Propagator/AtlasStepper.hpp(3 hunks)Core/include/Acts/Propagator/EigenStepper.hpp(1 hunks)Core/include/Acts/Propagator/EigenStepper.ipp(1 hunks)Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp(3 hunks)Core/include/Acts/Propagator/Propagator.hpp(1 hunks)Core/include/Acts/Propagator/Propagator.ipp(3 hunks)Core/include/Acts/Propagator/StepperConcept.hpp(0 hunks)Core/include/Acts/Propagator/StraightLineStepper.hpp(1 hunks)Core/include/Acts/Propagator/SympyStepper.hpp(1 hunks)Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp(2 hunks)Core/include/Acts/TrackFitting/KalmanFitter.hpp(3 hunks)Core/src/Propagator/StraightLineStepper.cpp(1 hunks)Core/src/Propagator/SympyStepper.cpp(2 hunks)Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp(18 hunks)Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp(7 hunks)Tests/UnitTests/Core/Propagator/JacobianTests.cpp(1 hunks)Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp(12 hunks)Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp(5 hunks)Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp(7 hunks)
💤 Files with no reviewable changes (1)
- Core/include/Acts/Propagator/StepperConcept.hpp
🧰 Additional context used
📓 Learnings (1)
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (2)
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:933-938
Timestamp: 2024-12-02T14:13:24.820Z
Learning: In `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the overloading of `findTracks` functions is intentional for convenience and not considered code duplication.
Learnt from: goetzgaycken
PR: acts-project/acts#3825
File: Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp:460-467
Timestamp: 2024-12-05T10:58:53.433Z
Learning: In the `Acts` project, within `Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp`, the cache `result.trackStateCandidates` is expected to be empty before calling `createTrackStates` in the function `filter` of `CombinatorialKalmanFilter`.
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / clang_tidy
🔇 Additional comments (37)
Core/src/Propagator/SympyStepper.cpp (4)
11-11: Include, well-placed it is.
Mmm, needed for track parameter definitions, yes. Necessity satisfied, I sense.
26-29: Creation method streamlined, good it is.
Separate concerns for state creation and initialization, wise you are. Calls to confirm usage throughout the code, verify you should.
31-35: Forwarding constructor, clarity it brings.
Efficient it seems, bridging old signature to the new, yes. The approach, well done.
37-66: Initialization logic, well structured, hmmm.
Separated creation from parameter setup, you have. Good stability and maintainability, the code shall have. Verify edge cases for null or empty covariance you must.Core/include/Acts/Propagator/EigenStepper.ipp (3)
23-26: Simplified makeState, a quick path to creation it is.
Yes, the clarity is strong here. Minimal, the overhead now.
29-33: Forward initialization, a worthy pattern.
Well does it convey the bound parameters, yes. Maintain consistency with other steppers, you should.
35-66: State initialization, robust it looks.
Reset the counters and covariance checks you do. To uncovered corner cases, remain alert.Core/include/Acts/Propagator/StraightLineStepper.hpp (3)
116-116: makeState removed from code no longer, it is?
Declared here, but ensure usage or removal consistent across code, you must.
118-118: Public initialize, unify your approach.
Yes, external calls to set up track parameters, simpler they become.
120-123: Optional covariance, flexible design it grants.
Separation of creation and setup in line with other steppers, see I do. Implementation synergy, maintain.Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (2)
24-24: New header, introduced you have.
No conflict, I sense. Helpful for the reorganized state management, it is.
373-376: Initialize stepper with filtered data, hmm.
Well-aligned with the new design, these calls are. Verify that reference surfaces never null are, you must.Core/src/Propagator/StraightLineStepper.cpp (4)
11-11: A new include, see I.
Matches usage of bound parameters, correct it seems. Continue you may.
17-21: State creation well separated, yes.
Clean and readable the logic is, next step is initialization.
23-28: Overloaded initialize bridging parameters, done it has.
A clear transition from short call to expanded usage, consistent it appears.
29-59: Initialize with optional covariance, wise it is.
Handles partial data gracefully, yes. No immediate issues I sense.Tests/UnitTests/Core/Propagator/JacobianTests.cpp (2)
137-138: Split calls for AtlasStepper, done.
Creation first, then initialize. A new approach, well aligned.
142-143: EigenStepper initialization, mirrored the approach you have.
Safety and clarity the separate steps bring.Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp (3)
82-84: Approve these changes, I do! Cleaner separation of concerns, they bring.Split the state creation from initialization, we have. Reuse existing state across multiple propagations, this allows. Hmmmm.
Also applies to: 104-104, 110-110
140-141: Consistent implementation throughout the tests, I observe.Follow the same pattern of separate state creation and initialization, all test cases do. Good practice, this is.
Also applies to: 269-270
242-243: More explicit state initialization, this change brings.Replaced
resetStatewithinitialize, we have. More control over state initialization, this provides. Parameters for covariance and particle hypothesis, now explicitly passed they are.Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp (1)
172-173: Hmm, consistent the changes are. Approve them, I do!Split the state creation and initialization have we. A cleaner separation this brings, yes. Follow the new pattern all test cases do:
- First create the state with options only
- Then initialize it with parameters
Good this refactoring is, matches the PR objectives it does.
Also applies to: 187-188, 224-225, 282-283, 319-322, 341-343, 372-373, 399-403, 457-459, 534-536
Core/include/Acts/Propagator/Propagator.ipp (1)
258-259: Wise this refactoring is, approve it I do!Separated the concerns have we:
- State creation with options only
- State initialization with parameters
- Clear the responsibilities now are
Match the PR objectives this does, hmm.
Also applies to: 297-298, 398-404
Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp (1)
113-114: Consistent these changes are, approve them I do!Follow the new pattern all test cases do:
- Create state with options only
- Initialize state with parameters separately
Maintain consistency across test suite this does, hmm.
Also applies to: 141-142, 169-170, 189-190, 237-238, 320-321, 358-359, 399-400, 420-422, 457-458, 493-494, 534-536
Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp (4)
202-203: Clear initialization, this constructor brings!Explicit the component construction now is, hmm. Good practice this follows.
246-249: Simple and focused, the makeState method becomes!Create state with options only it does. Separate concerns this does, yes.
251-274: Wise the initialization logic is!Handle all initialization in one place we do:
- Check for empty components
- Set particle hypothesis
- Initialize each component properly
- Handle covariance transport
Clear and maintainable the code becomes, hmm.
411-414: Consistent with new pattern, this change is!Use the new Component constructor we do. Match the initialization pattern this does.
Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp (3)
176-177: Approve the separation of state creation and initialization, I do!Wise changes these are, separating state creation from initialization. Clear and maintainable the code becomes.
210-211: Consistent pattern of separation, I observe!Follow the same wise path of separation, this test does. Good practices, consistency brings.
246-247: Comprehensive and consistent changes across all tests, I see!Throughout all test cases, the same pattern of separation flows. Like a well-choreographed lightsaber form, each test follows the same wise principles. Strong and maintainable, the codebase becomes.
Also applies to: 294-295, 333-338, 399-404
Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp (1)
196-197: In harmony with the Force, these changes are!Like twin suns of Tatooine, makeState and initialize now separate but connected remain. Consistent with other steppers, this implementation is. Well-tested, the changes are.
Also applies to: 212-213, 219-220
Core/include/Acts/Propagator/AtlasStepper.hpp (2)
142-145: Single responsibility, this method now has!Simple and focused, the makeState method becomes. Create state with options only, it does. Clean and clear, its purpose is.
147-331: Strong with the Force, this implementation is!Complete and thorough, the initialize methods are. Handle all cases of initialization, they do. Maintain the state's integrity while providing flexibility, they must. A masterful implementation, this is.
Core/include/Acts/TrackFitting/KalmanFitter.hpp (3)
547-550: Hmmmm, proper initialization of stepper state with particle hypothesis, I see.Wise changes these are, separating state creation from initialization they do. Particle hypothesis from stepper now passed explicitly, more clarity it brings.
1013-1016: Consistent initialization with first track state, it is.Match the pattern of initialization separation, this change does. Particle hypothesis from stepper, properly passed it is.
1019-1022: For last track state too, consistent initialization maintained is.The Force is strong with this one. Same pattern of initialization followed here is, maintaining consistency throughout the code.
Disentangles the state creation and initialization for the steppers. This is analog to what is done in the navigator. The idea is that a state can be used for multiple propagations so its creation should be decoupled from the initialization to allow the reuse of the existing state. At the same time
resetStateis removed as it has no use anymore.initializewill reset and reinitialize the state.Pulled out of #4036
Summary by CodeRabbit
Refactor
Tests