feat: Add option for initial step size #4101
Conversation
WalkthroughChanged, the step size initialization has been. In several stepper classes, a single-step construction of Changes
Sequence Diagram(s)sequenceDiagram
participant Stepper
participant ConstrainedStep
Stepper->>ConstrainedStep: Construct default ConstrainedStep()
Stepper->>ConstrainedStep: setAccuracy(initialStepSize)
Stepper->>ConstrainedStep: setUser(maxStepSize)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 0
🧹 Nitpick comments (1)
Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp (1)
118-118: Good start this test is, but more coverage we need, hmmmm.Set the initial step size to 10 meters, this test does. But test the effects of different initial step sizes, we should.
Consider adding these test cases, you should:
- Test with very small initial step size
- Test with initial step size larger than maxStepSize
- Verify step size adaptation behavior with different initial values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Core/include/Acts/Propagator/AtlasStepper.hpp(1 hunks)Core/include/Acts/Propagator/EigenStepper.ipp(1 hunks)Core/include/Acts/Propagator/StepperOptions.hpp(1 hunks)Core/src/Propagator/StraightLineStepper.cpp(1 hunks)Core/src/Propagator/SympyStepper.cpp(1 hunks)Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp(1 hunks)Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp(2 hunks)Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: CI Bridge / lcg_107: [alma9, clang19]
- GitHub Check: CI Bridge / lcg_107: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_107: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- 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 / linux_ubuntu_2204
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- 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 / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: build_debug
🔇 Additional comments (7)
Core/include/Acts/Propagator/StepperOptions.hpp (1)
38-40: Hmmmm, good this addition is!Well placed and documented, the new
initialStepSizeoption is. A sensible default value of 1 meter, it has.Core/src/Propagator/StraightLineStepper.cpp (1)
42-44: Wise changes these are, hmmmm.A more flexible approach to step size initialization, this is. First construct with default, then set accuracy and user values separately, we do.
Core/src/Propagator/SympyStepper.cpp (1)
49-51: Consistency in the Force, I sense.Like its siblings, this stepper now follows the same path of initialization. Harmony in the codebase, this brings.
Core/include/Acts/Propagator/EigenStepper.ipp (1)
49-51: Approve the step size initialization changes, I do!A better way to control step size parameters, this two-step initialization is. Clear separation between initial accuracy and maximum step size, it provides. Consistent with the Force, this change is.
Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp (1)
195-195: Good test coverage, this provides!Properly test the new initialStepSize feature, this change does. Clear and meaningful test value of 10_m, it uses. Strong with the Force, this test is.
Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp (1)
220-220: Consistency in testing, I sense!Match the SympyStepper test configuration, this change does. Harmony in the test values, it maintains. Balance to the Force, this brings.
Core/include/Acts/Propagator/AtlasStepper.hpp (1)
163-165: Unified implementation across steppers, this achieves!Follow the same pattern as other steppers, this change does. Harmony in the codebase, it maintains. Strong and consistent, the Force is.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Core/include/Acts/Propagator/StepperOptions.hpp (1)
43-43:❓ Verification inconclusive
Careful we must be, when limits to infinity we stretch.
Changed the maxStepSize from 10 meters to infinity's edge, you have. Impact on the Force - performance and stability - carefully measured it must be. Consider adding configuration option for custom maximum, we should.
Consider these aspects you must:
- Performance impact when no practical limit exists
- Numerical stability with very large steps
- Whether configurable maximum better serves the users' needs
🌐 Web query:
What are the typical maximum step sizes used in particle tracking algorithms?💡 Result:
The maximum step sizes in particle tracking algorithms depend on both spatial resolution and temporal stability constraints, with key factors including:
Spatial Step Size
- Particle overlap tolerance: Algorithms can resolve overlapped particles up to 50% overlap ratio for larger particles ([1][2]). Smaller particles require stricter overlap limits.
- Displacement per frame: For velocity gradients up to 0.2 pixels per step ([1]), spatial steps must ensure particles remain trackable between frames.
- Subpixel precision: Centroid-based methods achieve subpixel accuracy with root mean square error < 0.07 pixels ([1][3]).
Temporal Step Size
- Explicit methods: Stability limits impose small timesteps for high Stokes number flows (e.g., small particles). For example:
- Forward Euler requires Δ𝑡 ≲ ( \tau ) (particle response time) ([5]).
- Runge-Kutta 4 allows slightly larger steps but remains constrained by ( \Delta t \propto \tau ) ([5]).
- Implicit methods: Backward Euler enables larger timesteps (e.g., ( \Delta t \approx 10\tau )) at the cost of reduced accuracy ([5]).
Practical Guidelines
- Frame rate vs. exposure time: Exposure time ( \sigma ) should be ≤ 10% of inter-frame time (( \tau_{min} )) to minimize motion blur ([4]).
- Microscale systems: Achieve displacement uncertainties of 0.62–0.77 µm per step with optimized imaging ([3]).
Algorithm-Specific Tradeoffs
Method Max Step Size Accuracy Use Case Explicit Euler Small (( \sim\tau )) Low Simple flows Implicit Euler Large (( >10\tau )) Moderate Stiff systems Hybrid PIV-PTV Adaptive High High particle density For microscale flows, systems like color-coded 3D PTV resolve ~400 vectors per frame with submicron precision ([3]). In macroscale applications, hybrid PTV techniques balance spatial resolution and computational cost by leveraging PIV results as initial guesses ([1][3]).
Citations:
- 1: https://www.aa.washington.edu/files/faculty/dabiri/pubs/A%20vision-based%20hybrid%20particle%20tracking%20velocimetry%20(PTV)%20technique%20using%20a%20modified%20cascade%20correlation%20peak-finding%20method%202012.pdf
- 2: https://research.stowers.org/imagejplugins/particle_tracking_tutorial.html
- 3: https://www.aa.washington.edu/sites/aa/files/faculty/dabiri/pubs/Color-coded%20three-dimensional%20micro%20particle%20tracking%20velocimetry%20and%20application%20to%20micro%20backward-facing%20step%20flows.pdf
- 4: https://lem.che.udel.edu/wiki/uploads/Main/Microrheology/Particle_tracking_handout.pdf
- 5: https://www.cfd-online.com/Forums/main/120910-particle-tracking-algorithms.html
- 6: https://imagej.net/imaging/particle-analysis
Attention: Impact on the Force – Review Step Size Change, You Must
Changed the maxStepSize from a fixed 10 meters to the maximum value of a double, you have. This significant change could impact performance and numerical stability, hmmm. Typical particle tracking step sizes are tightly constrained by spatial resolution and time stability—not near infinity they are. Consider these points:
- The change to an unbounded limit may allow simulation steps so large that performance degrades or physics becomes unreliable.
- Configuration options for setting a custom maximum step size, you might add. This would balance flexibility with the need for physical realism.
- Verify with simulations and review spatial/temporal constraints from typical particle tracking studies, you should.
Proceed with caution, you must, to ensure that the change does not propagate harmful side effects in your simulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/Propagator/StepperOptions.hpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (2)
Core/include/Acts/Propagator/StepperOptions.hpp (2)
14-14: Wise addition of limits header, I sense.For std::numeric_limits, necessary this include is. Well placed in the include order, it stands.
39-40:✅ Verification successful
Hmmmm... A new power to control initial steps, you seek.
Wise choice this is, allowing users to control their destiny. Default value of 10 meters, from the old maxStepSize it comes. But verify we must, that all steppers this new power they use correctly.
🏁 Script executed:
#!/bin/bash # Search for usage of initialStepSize across steppers ast-grep --pattern 'state.options.initialStepSize'Length of output: 372
Approved: Verified usage of initial step size correctly, it is.
Wise change this is. Verified, the new
initialStepSizemember is used in all steppers as intended:
- In
Core/include/Acts/Propagator/AtlasStepper.hppat line 164,- In
Core/src/Propagator/StraightLineStepper.cppat line 43,- And in
Core/src/Propagator/SympyStepper.cppat line 50.Control over initial steps, this new power gives, and consistent its application remains. Approved, I do.
|
hashes changed because the root file in question stores the constraint step which does not have the same entries as on main |
github-actions
bot
added
Component - Examples
|
Adds an option to allow the user steering the initial step size attempted by the stepper. Currently this was not set and only constrained by the maximum step size.
This will not make a big difference but is the correct thing to do IMO. Given an adaptive step size the stepper will figure out the appropriate step size after ~one failed attempt.
Summary by CodeRabbit