refactor!: Revise stepper state creation #3987
Conversation
WalkthroughHmm, significant changes to the propagation architecture, these modifications reveal. Across multiple stepper classes - AtlasStepper, EigenStepper, StraightLineStepper - a common pattern emerges. Context management and state initialization, they have streamlined. The Changes
Poem
Possibly related PRs
Hmm, yes. A journey of code refinement, this pull request represents. Simplicity and clarity, the path to better software, they are. chuckles softly Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
🧹 Nitpick comments (10)
Core/include/Acts/Propagator/MultiEigenStepperLoop.ipp (1)
Line range hint
37-83: Wise architectural changes, these are! Hmmmm.A step towards decoupling the stepper from propagation state, this refactoring is. Through the options structure, flow the contexts now do. Cleaner architecture, we achieve. Future changes, easier they become.
Core/include/Acts/Propagator/detail/LoopStepperUtils.hpp (1)
129-133: Architectural wisdom, share I must!A step towards greater decoupling, this change represents. Yet more work ahead, I sense. Consider these paths for future journeys:
- Further separate the stepper's concerns from state management, you should
- Document the new context access patterns in comments, helpful it would be
- Unit tests for the new structure, essential they are
The Force of clean architecture, strong it grows!
Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp (1)
168-169: Avoid code duplication, refactor you shouldRepeated, the initialization of
MultiOptions options(geoCtx, magCtx);andoptions.maxStepSize = defaultStepSize;is. Into a common setup function or fixture, consolidate this code, you should.Also applies to: 204-205, 235-236, 284-285, 309-310, 385-386, 471-472, 585-586, 668-669, 718-719, 768-769
Tests/UnitTests/Core/Propagator/NavigatorTests.cpp (1)
76-79: Introduce Options struct, you do. Ensure references remain valid, you must.Consider using value types instead of references in
Optionsto avoid potential lifetime issues.Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp (1)
529-530: Logging additions provide clarity. Helpful, they are.Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp (2)
211-212: Unnecessary re-initialization of options, see I doRe-initializing
esOptionswith the same contexts may be redundant. ReuseesOptions, consider you should.
263-263: Use of magic number-1337., question I doConfusing, the use of
-1337.for step size may be. Consider a named constant or comment to explain.Core/include/Acts/Propagator/PropagatorOptions.hpp (1)
61-64: Strong with the Force, this design is!Initialize stepping and navigation options in the constructor, we now do. A clean separation of concerns, this brings.
Consider documenting the relationship between these components in the class documentation, help future Padawans it will.
Core/include/Acts/Propagator/Propagator.ipp (1)
178-179: Improved state initialization pattern, I sense.Simplified state creation through makeState function, this change brings. Better encapsulation and cleaner architecture, it achieves. Consistent with the path to decouple stepper from propagation state, it is.
Also applies to: 217-218
Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp (1)
331-346: Improved readability of step size calculation, I observe.Better formatted and more readable, the intersection calculation has become. Easier to maintain and understand, this code now is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
Core/include/Acts/Propagator/AtlasStepper.hpp(8 hunks)Core/include/Acts/Propagator/EigenStepper.hpp(3 hunks)Core/include/Acts/Propagator/EigenStepper.ipp(6 hunks)Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp(8 hunks)Core/include/Acts/Propagator/MultiEigenStepperLoop.ipp(2 hunks)Core/include/Acts/Propagator/Propagator.hpp(0 hunks)Core/include/Acts/Propagator/Propagator.ipp(2 hunks)Core/include/Acts/Propagator/PropagatorOptions.hpp(2 hunks)Core/include/Acts/Propagator/StepperOptions.hpp(1 hunks)Core/include/Acts/Propagator/StraightLineStepper.hpp(3 hunks)Core/include/Acts/Propagator/SympyStepper.hpp(3 hunks)Core/include/Acts/Propagator/detail/LoopStepperUtils.hpp(1 hunks)Core/include/Acts/Propagator/detail/SteppingHelper.hpp(1 hunks)Core/include/Acts/TrackFitting/detail/GsfActor.hpp(1 hunks)Core/src/Propagator/StraightLineStepper.cpp(4 hunks)Core/src/Propagator/SympyStepper.cpp(5 hunks)Tests/IntegrationTests/PropagationTests.hpp(4 hunks)Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp(15 hunks)Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp(9 hunks)Tests/UnitTests/Core/Propagator/JacobianTests.cpp(1 hunks)Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp(21 hunks)Tests/UnitTests/Core/Propagator/NavigatorTests.cpp(2 hunks)Tests/UnitTests/Core/Propagator/PropagatorTests.cpp(2 hunks)Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp(7 hunks)Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp(8 hunks)Tests/UnitTests/Core/SpacePointFormation/SpacePointBuilderTests.cpp(1 hunks)Tests/UnitTests/Core/Vertexing/ImpactPointEstimatorTests.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- Core/include/Acts/Propagator/Propagator.hpp
🔇 Additional comments (98)
Core/include/Acts/Propagator/MultiEigenStepperLoop.ipp (2)
Line range hint 37-41: Approve this change, I do! Hmmmm.
Moved to options the geoContext has been, as planned in the great refactoring it was. Maintain the same intersection logic we do, yet cleaner the structure becomes.
81-83: Consistent with the Force, this change is!
Similar to the previous modification, moved to options the geoContext has been. A pattern emerges, it does! The way of the refactoring, this is.
Core/include/Acts/Propagator/detail/LoopStepperUtils.hpp (1)
129-133: Approve the context access pattern change, I do!
Moved to stepper options, the geometry context has been. Wise decision, this is, aligning with the greater refactoring purpose. Consistent with the path to decouple the stepper from propagation state, it remains.
Verify the consistency of this pattern across the codebase, we must:
✅ Verification successful
Consistent with the refactoring path, this change is. Approve it, I do!
Through the Force, searched the entire codebase I have. A clear pattern emerges:
- The old pattern
state.geoContextstill exists in many places, yes, but primarily in files not yet touched by this refactoring effort - The new pattern
state.options.geoContextappears consistently in the stepper implementations, where this change belongs - In
LoopStepperUtils.hpp, the change aligns perfectly with similar changes in other stepper implementations likeStraightLineStepperandSympyStepper
Hmm, wise this refactoring is. Moving context to options, a gradual process it must be. This change, a step in the right direction it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct geoContext access patterns
rg "\.geoContext[^.]" --type cpp
# Search for the new pattern of accessing through options
rg "\.options\.geoContext" --type cpp
Length of output: 27886
Core/include/Acts/TrackFitting/detail/GsfActor.hpp (1)
368-373: Functionality correct, the changes are.
Consistency in context usage, you have maintained. Good work, this is.
Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp (3)
281-292: Well-implemented test for invalid state, this is
Appropriate exception handling, demonstrated here is. Ensures that makeState handles invalid input correctly, this test does.
Line range hint 467-556: Accurate surface status updates, the code performs
Properly updates the status of surfaces and components, your implementation does. Consistent with expected behavior, the results are.
Line range hint 714-747: Combined curvilinear state computation, correctly implemented it is
Validates that the curvilinear parameters match expected values, your test does. Good alignment with the single-component parameters, there is.
Core/src/Propagator/SympyStepper.cpp (5)
26-51: Good the refactoring is, hmmm.
Streamlined the state initialization using options, you have. Approve, I do.
Line range hint 58-69: Context references updated correctly, they are.
Consistent your changes are. Well done, this is.
81-84: Consistent context access, maintain you do.
Throughout the class, options you now use. Good, this is.
102-102: Updated the context usage is, hmmm.
Ensured consistency, you have.
125-127: In transportCovarianceToBound, context usage correct it is.
Options you use, as intended.
Core/include/Acts/Propagator/EigenStepper.ipp (5)
24-49: Refactored the state creation is. Good, this change is.
Using options to initialize the state, you are. Approve, I do.
Line range hint 58-69: Context references updated, correctly they are.
Consistent the usage is. Well done, hmmm.
82-85: Consistent context handling, you maintain.
Good this is. Approve, I do.
138-138: Context usage updated, it is. Correctly done, you have.
164-167: In transportCovarianceToBound, context access updated, it is.
Consistent your changes are.
Core/include/Acts/Propagator/SympyStepper.hpp (3)
42-44: Added the Options constructor, you have. Encapsulate contexts, it does.
Good this is.
57-64: Simplified the State constructor, hmmm.
Using options and field cache, you are. Approve, I do.
121-122: Updated makeState to use options, you have. Good, this change is.
Consistent with other changes, it is.
Core/include/Acts/Propagator/EigenStepper.hpp (3)
60-62: Added the Options constructor, hmmm.
Encapsulating contexts, you are. Good, this is.
75-82: Simplified the State constructor, you have.
Using options, instead of separate contexts. Approve, I do.
153-154: Updated makeState to use options, you have.
Consistent with other changes, this is. Good, hmmm.
Core/include/Acts/Propagator/StraightLineStepper.hpp (3)
58-60: Properly initialize Options constructor, you have.
74-76: State now accepts Options. Good, this is.
124-150: Refactored makeState method wisely, you have.
Tests/UnitTests/Core/Propagator/NavigatorTests.cpp (1)
83-84: State now contains Options. Correct, this is.
Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp (13)
15-15: Include Tolerance.hpp, you have. Necessary, this is.
105-113: Initialize Stepper and State with Options, you correctly do.
132-140: Construct State with covariance, rightly you have.
159-167: Getters test updated with Options. Good, this is.
178-186: UpdateFromBound test refactored with Options. Maintain consistency, you do.
225-233: UpdateFromComponents test uses makeState. Correct, this is.
250-259: Refactor BuildBound test to use Options, you have. Consistency achieved.
279-287: BuildCurvilinear test updated properly with Options.
305-313: Step test now initializes State with Options. Well done, it is.
342-350: StepWithCovariance test refactored to use Options. Consistent, your changes are.
382-390: Reset test updated with Options and makeState. Good, continue you must.
578-587: StepSize test refactored with Options and makeState. Correct, this is.
598-606: StepSizeSurface test updated to use Options and makeState. Consistency maintained, you have.
Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp (6)
159-160: Typedef SingleOptions introduced. Clarity, this brings.
186-189: Options struct now inherits from SingleOptions and initializes correctly. Good change, this is.
203-204: State now includes Options member. Consistent with other steppers, it is.
244-268: Refactored makeState method to use Options. Uniformity across codebase, you maintain.
423-424: addComponent method now uses SingleStepper's makeState with Options. Appropriate, this is.
587-588: updateStepSize method correctly accesses geoContext from Options. Good, this is.
Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp (13)
13-13: Necessary, the inclusion of Tolerance.hpp is
Ensures the availability of tolerance constants in tests, this inclusion does.
191-192: Initialize EigenStepper options with contexts, necessary this change is
By providing tgContext and mfContext, properly initialized the options are.
197-197: Create state with options and parameters, correctly you have
Using es.makeState(esOptions, cp), proper initialization of the state this ensures.
219-219: Create new state with options and parameters, acceptable this is
Consistent use of esOptions in state creation, good practice it is.
244-246: Initialize options and set max step size, correctly you have
Setting esOptions.maxStepSize to control step size properly, wise this is.
247-249: Build stepper and state with options, acceptable this is
Proper initialization of the stepper and state using options and parameters, you have done.
346-346: Creating a copy of the state, acceptable this is
Properly using es.makeState(esOptions, cp) for state copying, you are.
369-369: Copying state using copyState, acceptable this is
Correctly duplicating the stepping state, you are.
446-446: Creating state with options and bound parameters, correct this is
Using es.makeState(esOptions, bp) to initialize the state properly, you have.
452-452: Using infinite boundary tolerance, acceptable this is
No boundary limit imposed, BoundaryTolerance::Infinite() ensures.
459-463: Updating step size with surface intersection, correct this is
Appropriate use of es.updateStepSize with surface intersection, you have done.
468-472: Updating step size with surface intersection, acceptable this is
Consistent approach in step size adjustment, see I do.
517-518: Initializing options and state for null field stepper, acceptable this is
Contexts provided to nesOptions, correctly initializing nesState you are.
Core/include/Acts/Propagator/AtlasStepper.hpp (11)
48-50: Constructor added to Options, acceptable this is
Initializing with GeometryContext and MagneticFieldContext, proper context management this ensures.
62-65: Updated State constructor with options and field cache, acceptable this is
With Options and fieldCache, properly you initialize the state.
67-67: Added options member to State, acceptable this is
Context accessibility within State enhanced it is.
69-69: Initialize particle hypothesis to pion, acceptable this is
Default ParticleHypothesis set, predictability this enhances.
142-145: Updated makeState to use options and initialize state, correct this is
Consistent context usage ensured by passing Options to makeState.
146-313: Refactored state initialization into makeState, clarity this brings
Improves code organization by moving initialization into makeState, you have.
328-330: Updated update call with options' geoContext, acceptable this is
Using state.options.geoContext for consistent context usage, you are.
618-619: Using options' geoContext in referenceFrame call, correct this is
Ensures context consistency in transformations, this does.
949-950: Using options' geoContext in covariance transport, acceptable this is
Proper context handling during covariance transport, important it is.
979-981: Accessing surface center with options' geoContext, correct this is
Consistency in context usage during calculations, maintained it is.
1079-1080: Using options' geoContext to access surface center, acceptable this is
Enhances code reliability through consistent context usage.
Core/include/Acts/Propagator/StepperOptions.hpp (4)
11-11: Added include for <functional>, necessary this is
Provides std::reference_wrapper, including <functional> does.
16-18: Added forward declarations for GeometryContext and MagneticFieldContext, acceptable this is
Allows reference usage without full definitions, this does.
20-24: Added constructor to initialize contexts, acceptable this is
Contexts held by StepperPlainOptions, this constructor allows.
25-30: Added context references as member variables, acceptable this is
Enhances context accessibility by storing geoContext and magFieldContext.
Core/include/Acts/Propagator/detail/SteppingHelper.hpp (1)
42-42: Updated to use options' geoContext in surface intersection, acceptable this is
Correct context used during intersection calculations, this ensures.
Core/src/Propagator/StraightLineStepper.cpp (4)
42-42: Consistent the changes are. Approve them, I do!
Follow the same pattern of accessing geoContext through options, this change does.
65-67: In harmony with the Force, these changes are!
The pattern of accessing geoContext through options, maintained it is.
75-76: The path to clean code, these changes follow!
Through options, the context flows. A consistent pattern, this maintains.
Also applies to: 86-87
21-24: Hmmmm, moved to options the context has been. Wise this change is!
Consistent with the refactoring objectives, this modification is. But verify all usages of geoContext, we must.
✅ Verification successful
Hmmmm, many instances of state.geoContext still linger in our codebase, they do!
In header files and test files, these occurrences primarily reside. A pattern emerges - the change to state.options.geoContext, only in implementation files (.cpp), happening it is. The headers and tests, unchanged they remain.
- Core/include/Acts/Propagator/detail/LoopStepperUtils.hpp
- Core/include/Acts/Propagator/Navigator.hpp
- Core/include/Acts/TrackFitting/KalmanFitter.hpp
- Core/include/Acts/Navigation/DetectorNavigator.hpp
- Many other header files, containing direct state.geoContext usage, they do
Clarity through the Force, I now see!
No implementation files with direct state.geoContext usage remain, hmmmm! The change from state.geoContext to state.options.geoContext in this file, correct it is. The Force reveals that header files still use the old pattern, but this is expected, as refactoring wave-by-wave, proceeding it is.
In implementation files (.cpp), the transition complete it is. The path to enlightenment, this change follows!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct geoContext usage
rg "state\.geoContext" --type cpp
Length of output: 6687
Script:
#!/bin/bash
# Check if there are any implementation files still using state.geoContext
rg "state\.geoContext" --type cpp -g "*.cpp" --exclude "Tests/*"
Length of output: 478
Script:
#!/bin/bash
# Try different approach to find implementation files with state.geoContext
rg "state\.geoContext" -g "*.cpp" | grep -v "Tests/"
Length of output: 52
Core/include/Acts/Propagator/PropagatorOptions.hpp (1)
94-97: Mirror the plain options, these changes do. Good, this is!
Consistent initialization pattern in both constructors, maintained it is.
Also applies to: 102-104
Tests/UnitTests/Core/Propagator/JacobianTests.cpp (1)
135-141: Updated the tests, we have. Wise this is!
Use the new Options pattern for stepper initialization, these changes do. But verify all test cases, we must.
✅ Verification successful
The previous search might have been too specific. Let me broaden the search to find stepper-related test cases and their initialization patterns. This will help verify if all test cases have been properly updated to use the new Options pattern.
Hmm, more investigation we need. The Force tells me to search specifically for old-style stepper initialization patterns and locate all stepper-related test files, it does. Help us find any remaining cases that need updating, this will.
Correct the changes are, young padawan!
Through the Force, seen I have that all stepper tests use the new Options pattern correctly, they do. The changes in JacobianTests.cpp align with the way of the Jedi, using Options(tgContext, mfContext) they do. In MultiStepperTests.cpp and other stepper tests, proper usage of Options observed is. No old-style initialization remains, hmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old-style stepper initialization
rg "makeState.*geoContext.*magFieldContext" --type cpp
Length of output: 56
Script:
#!/bin/bash
# Search for any stepper initialization patterns in test files
rg "makeState.*Context" Tests/ --type cpp -B 2 -A 2
# Search for test cases using steppers
ast-grep --pattern 'TEST($$$) {
$$$
Stepper$$$
$$$
}'
Length of output: 8767
Script:
#!/bin/bash
# Search for old-style stepper initialization in test files
rg "makeState\([^O].*pars" Tests/ --type cpp -B 2 -A 2
# Find all stepper test files to ensure complete coverage
fd "Stepper.*Tests\.cpp$" Tests/
Length of output: 7598
Tests/UnitTests/Core/SpacePointFormation/SpacePointBuilderTests.cpp (1)
50-51: Improved readability through better code organization, hmm.
Better documentation and spacing, this change brings. Clearer understanding of the code's purpose, it provides.
Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp (2)
76-78: Embraced the new way of state creation, you have.
Through Options object, state initialization now flows. Cleaner and more maintainable, the code becomes.
Also applies to: 84-84
152-152: Additional control over step size updates, gained we have.
New boolean parameter in updateStepSize, more flexibility it brings. Better control over the stepping process, we achieve.
Tests/IntegrationTests/PropagationTests.hpp (3)
351-359: Approve template parameter simplification, I do!
Removed the options_t template parameter, making the code cleaner and more maintainable, it has become. The function signature now focuses on the essential propagator types.
Also applies to: 361-361
Line range hint 377-387: Simplified template parameters, good they are!
Similar to the previous change, removed unnecessary template parameter options_t has been. Consistency in approach, maintained it is.
396-399: Consistent with previous changes, this modification is!
The pattern of simplified template usage continues here. Cleaner code structure, it maintains.
Tests/UnitTests/Core/Propagator/PropagatorTests.cpp (2)
407-408: Proper initialization of options, this is!
Initialize EigenPropagatorType::Options with both geometry and magnetic field contexts, we now do. More explicit and safer, this approach is.
440-441: Consistent initialization pattern, maintained it is!
Similar to previous change, proper initialization of StraightLineStepper options with contexts, implemented it has been.
Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp (7)
165-168: Proper initialization of stepper options and state, this is!
Initialize options with contexts before creating the stepper, we now do. Clear separation of concerns, this maintains.
172-172: State creation through makeState, properly implemented it is!
Use makeState consistently for state initialization, the code does. Good practice for encapsulation, this is.
Also applies to: 186-186, 193-193
218-223: Options configuration and state initialization, well structured they are!
Configure max step size through options and create state using makeState, the code does. Proper encapsulation of configuration, this maintains.
237-237: Step size update with proper flag, implemented it is!
Added boolean parameter to updateStepSize, making the behavior more explicit, it does.
320-320: State copying through makeState, proper approach this is!
Use makeState for initial state creation in copy operation, maintaining consistency with new pattern.
417-418: Options reinitialization, properly done it is!
Reinitialize options with contexts before creating new state, maintaining proper initialization pattern.
424-424: Surface intersection handling, properly updated it is!
Update surface status and step size with proper context usage, the code does. Consistent with new options-based approach, this remains.
Also applies to: 432-435, 441-444
Tests/UnitTests/Core/Vertexing/ImpactPointEstimatorTests.cpp (2)
270-272: Approve the addition of StraightPropagator options, I do!
Wise choice it is, to configure propagator options separately. Clear separation between straight line and helical propagation, this brings. Consistent with the stepper state refactoring goals, this change is.
279-280: Approve the updated propagation call with new options, I do!
Properly configured propagation with dedicated options, now we have. Clean and explicit the code becomes, when separate options for straight line propagation we use.
|
To me it makes sense to separate the creation and the initialization of the state. In the constructor we do not have enough information to really initialize the state so I would only pass in the stuff which is really required like the options. Passing in the field and the config of the stepper/navigator seems overkill for a constructor IMO. Potentially we can think about renaming |
Stateconstructor tomakeState. this is the function which will actually be called from outsideIn a future PR I would like to decouple the stepper from the propagation state and the navigator. This is the first step towards that.
pulled out of #3449
Summary by CodeRabbit
New Features
Optionsstruct.makeStatemethods across multiple classes.Bug Fixes
Tests
Chores