fix: Use forced approach for MultiStepperSurfaceReached
#4025
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughIn the realm of Acts Core, a subtle transformation has occurred within the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Poem
Hmm, refactoring, the way of the code is. 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)
Core/include/Acts/Propagator/MultiStepperAborters.hpp (1)
16-16: Wise choice, the inheritance change is, hmm.From
SurfaceReachedtoForcedSurfaceReached, the path now leads. Align with the PR objectives, this change does, addressing the challenges of high curvature surfaces, it does.Consider documenting in comments, you should, why this approach better serves the curved surfaces than the previous one, yes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/Propagator/MultiStepperAborters.hpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: docs
- GitHub Check: build_debug
🔇 Additional comments (2)
Core/include/Acts/Propagator/MultiStepperAborters.hpp (2)
Line range hint
16-99: Test coverage for cylindrical surfaces, ensure we must.Given the changes to surface reaching behavior, comprehensive tests for cylindrical surfaces, essential they are. Particularly important for high curvature scenarios, this is.
Help you create additional test cases, I can, if needed they are.
84-85: Consistent with base class change, this modification is.Changed from
SurfaceReached::checkAborttoForcedSurfaceReached::checkAbort, the implementation has. But verify the impact on cylindrical surfaces, we must.Run this script to find usage patterns, we shall:
✅ Verification successful
Safe for cylindrical surfaces, this change is. Hmmmm.
The change from
SurfaceReachedtoForcedSurfaceReachedaffects only the distance limit for checking intersections, it does. Surface-type specific behavior, it does not modify. Work with all surface types including cylindrical ones, the underlying intersection logic does.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for cylindrical surface usage with MultiStepperSurfaceReached rg -l "CylinderSurface|CylinderBounds" | xargs rg "MultiStepperSurfaceReached" # Look for existing test cases with cylindrical surfaces ast-grep --pattern 'TEST_CASE($$$CylinderSurface$$$)'Length of output: 134
Script:
#!/bin/bash # Compare implementations of surface reached conditions ast-grep --pattern 'class SurfaceReached { $$$ static bool checkAbort($$$) { $$$ } $$$ }' ast-grep --pattern 'class ForcedSurfaceReached { $$$ static bool checkAbort($$$) { $$$ } $$$ }' # Find surface navigation tests rg -A 5 "TEST_CASE.*[Ss]urface.*[Nn]avigation" # Look for cylindrical surface usage in navigation context rg -A 3 "CylinderSurface.*navigator|navigator.*CylinderSurface"Length of output: 328
Script:
#!/bin/bash # Find files containing surface abort conditions fd . -e hpp -e cpp | xargs rg -l "SurfaceReached|ForcedSurfaceReached" # Look for surface navigation in propagator fd . -g "*Propagator*.hpp" -g "*Propagator*.cpp" | xargs rg -l "Surface" # Find surface-related test files fd Tests -e cpp | xargs rg -l "TEST_CASE.*Surface" # Get content of specific files if found cat Core/include/Acts/Propagator/StandardAborters.hppLength of output: 61669
|
Physmon changes but changes look fine to me. |
github-actions
bot
added
Infrastructure
|
…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 -->
Currently we respect the overstepping tolerance in the
MultiStepperSurfaceReachedaborter. 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 directlyForcedSurfaceReachedas 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
Summary by CodeRabbit
MultiStepperSurfaceReachedstruct