refactor!: Decouple navigation and stepping #3449
Conversation
andiwand
added
Needs Decision
github-actions
bot
added
Component - Core
andiwand
force-pushed
the
decouple-navigation-stepping
branch
from
901d3e4 to
cf443f5
Compare
github-actions
bot
removed
the
Infrastructure
andiwand
force-pushed
the
decouple-navigation-stepping
branch
from
cf443f5 to
b2ef11b
Compare
github-actions
bot
added
the
Infrastructure
andiwand
force-pushed
the
decouple-navigation-stepping
branch
from
474eeca to
b680a36
Compare
github-actions
bot
removed
Infrastructure
andiwand
force-pushed
the
decouple-navigation-stepping
branch
from
f1eb04c to
3c2406b
Compare
This reverts commit 689ff9e.
…andiwand/acts into decouple-navigation-stepping
![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 (2)
Core/include/Acts/Propagator/MultiEigenStepperLoop.ipp (2)
121-130: Improved logging clarity and component management, I see.More descriptive logging messages and explicit component handling, you have added. Wise decision, this is. The force of clarity, strong with this change it is.
Yet a suggestion, I have. Consider adding the total component count alongside the remaining count, hmm? Like this, it should be:
- ACTS_VERBOSE(components.size() - << " components left after removing missed components"); + ACTS_VERBOSE(components.size() + << " components remaining out of initial " + << stepping.initialComponentSize + << " after removing missed components");
Line range hint
1-232: Architectural wisdom, share I must.In alignment with the great decoupling of navigation and stepping, these changes are. Yet, consider these suggestions for future enlightenment:
- A dedicated component state manager, beneficial it could be. Separate the concerns of state management from stepping logic, it would.
- Events or callbacks for state transitions, consider adding them you should. Help other parts of the system react to component changes, they would.
- Configuration options for logging verbosity levels, add you might. Help in production debugging, this would.
The path to clean architecture, long it is. But with each step, closer to enlightenment we come.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Propagator/MultiEigenStepperLoop.ipp(3 hunks)Core/include/Acts/Propagator/MultiStepperAborters.hpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Propagator/MultiStepperAborters.hpp
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: unused_files
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu
🔇 Additional comments (2)
Core/include/Acts/Propagator/MultiEigenStepperLoop.ipp (2)
190-191: Simplified component removal logic, you have.Clean and efficient, the new approach is. Using
std::remove_ifwitherase, a pathway to better code it shows. The dark side of manual iteration, avoided it has been.
224-232: Critical state management addition, this is.Invalidate component status after each step, you now do. Prevent stale state from corrupting future decisions, this will. Yet, verify the impact on performance, we must.
Run this command to measure the impact, we shall:
✅ Verification successful
Minimal impact on performance, this change shall have.
Simple loop with basic assignment, it is. Necessary for correctness and preventing stale state issues, this change remains. Modern CPUs, such operations optimize well, they do. Worth the negligible cost for maintaining correct state, it most certainly is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for performance-related tests rg -l "TEST.*MultiEigenStepperLoop.*[Pp]erformance" -g "*.cpp"Length of output: 64
Script:
#!/bin/bash # Search for test files containing MultiEigenStepperLoop rg -l "MultiEigenStepperLoop" -g "*Test*.cpp" # Search for benchmark files fd ".*[Bb]enchmark.*\.cpp$" # Search for performance tests in propagator rg -l "TEST.*[Pp]erformance" Tests/Propagator/Length of output: 894
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Examples/Scripts/Python/hashing_seeding.py (1)
Line range hint
212-224: Architectural wisdom in particle selection strategy, I sense.Part of the larger navigation and stepping decoupling effort, these changes are. Clear separation between:
- Spatial selection (navigation domain)
- Physics selection (propagation domain)
To future stepper decoupling mentioned in PR objectives, this pattern points.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Examples/Scripts/Python/hashing_seeding.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: macos
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
- GitHub Check: docs
🔇 Additional comments (2)
Examples/Scripts/Python/hashing_seeding.py (2)
Line range hint
212-224: Complementary selection criteria between pre and post selection, observe.A balanced approach to particle filtering, I sense:
- Pre-selection: Spatial constraints (rho, absZ) for initial volume
- Post-selection: Physics constraints (pt, eta) and quality cuts (hits)
Wise separation of concerns, this is. Improved performance in Fatras algorithm, as mentioned in PR comments, it explains.
212-213: Changed particle selection criteria, hmm.Replaced the previous eta and pt-based selection with spatial constraints for particle pre-selection, I see. A significant change in the way particles are filtered before simulation, this is.
Run this command to verify the impact, we must:
Wise to verify these values against detector geometry constraints, it would be:
- rho max of 24mm: aligns with typical beam pipe radius, it does
- absZ of 1m: reasonable for initial particle selection, it is
✅ Verification successful
Consistent with the Force, these changes are.
The spatial constraints chosen (rho=(0.0, 24 * u.mm), absZ=(0.0, 1.0 * u.m)) align with established patterns across many files in the codebase, they do. A common approach for particle selection in the ACTS project, this has become.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other instances of ParticleSelectorConfig to ensure consistency rg -A 5 "ParticleSelectorConfig" | grep -v "postSelectParticles"Length of output: 26169
Currently we respect the overstepping tolerance in the `MultiStepperSurfaceReached` aborter. This can lead to problems with overstepping in situations with high curvature or surfaces that bent like cylinders and perigees. To fix this we can use directly `ForcedSurfaceReached` as a base which will always target the requested surface without checking the overstep tolerance. This can cause problems with cylinders but we usually do not target them with the GSF or one of the solutions should be strictly closer which suppresses the problem. Discovered and pulled out of #3449 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Updated inheritance for `MultiStepperSurfaceReached` struct - Simplified constructor initialization for the struct <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…perSurfaceReached` (#4027) After facing another problem with the GSF in #3449 this might resolve it. The problem seems to be that the navigator and the surface reached aborter are fighting over the surface reached status which can lead to removing all components if the target surface is not reached and the maximum step trials for the multi stepper are reached. This might introduce other problems so waiting for physmon report. blocked by - #4025 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Updated inheritance for `MultiStepperSurfaceReached` struct to use `ForcedSurfaceReached` as base class - Removed explicit constructor for `MultiStepperSurfaceReached` - Modified `checkAbort` method to use new base class implementation - **Chores** - Improved component handling and logging in `MultiEigenStepperLoop` class - Enhanced state management for components during stepping process <!-- end of auto-generated comment: release notes by coderabbit.ai -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Core/include/Acts/TrackFitting/detail/GsfUtils.hpp (1)
207-207: Consider documenting weight modification behavior, we should.Clear the behavior when weights are skipped or modified, make it. Add documentation about edge cases, helpful it would be.
+ // Modify the weight only if both determinant and factor are valid weights.at(tip) *= factor;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/TrackFitting/detail/GsfUtils.hpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: build_debug
- 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: unused_files
- 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: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: linux_ubuntu
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: docs
- GitHub Check: macos
🔇 Additional comments (3)
Core/include/Acts/TrackFitting/detail/GsfUtils.hpp (3)
195-198: Wise handling of determinant, this is!Protect against invalid mathematical operations when determinant is not positive, we do. Skip weight modification in such cases, we shall.
202-204: Graceful handling of non-finite factors, implemented well it is!Skip weight modification when factor calculation yields non-finite results, we do. Robust error handling, this demonstrates.
195-207: Verify impact on numerical stability across test cases, we must.Changes in weight computation logic, significant they are. Ensure stability across various input ranges and edge cases, we should.
Run these commands to analyze the test coverage and numerical stability:
|
With cf2b28a I am trying to work around an FPE that popped up in the GSF. We already checked for What do you think about that handling @benjaminhuth @paulgessinger? |
|
Let's go 🚀 |
|
acts-project-service
added
the
Breaks Athena build
This is a proposal to decouple navigation from stepping by
Propagatorin charge of communicating the target and estimated distance to the stepperOriginally I wanted the navigator to return the next step size but that clashes with the multi stepper. The current solution allows the stepper to judge on the step size given a surface. This allows the stepper to tweak the step size or to just take what the navigator proposed. In case of the multi stepper we will still just run intersections with the individual components.
The navigator is reduced to providing surface candidates and the surface status will be purely determined by the stepper. The propagator passes the information so the navigator knows when to switch surface / layer / volume.
Ultimately this removes all the templating from the navigator and the interface captures well what the navigator is supposed to do by having dedicated and visible I/O paths.
In case this is where we want to go I would like to do something similar to the steppers.
blocked by
Summary by CodeRabbit
Release Notes
Navigation Improvements
NavigationTargetstructure to enhance navigation precision.Propagator Enhancements
Stepper Refinements
Testing and Validation
Performance Optimizations