fix: Reset component state in MultiEigenStepperLoop over MultiStepperSurfaceReached
#4027
Conversation
WalkthroughHmm, changes to propagation components, there are. In the Changes
Poem
Hmm, yes. Poetic, this is. 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 (
|
|
This triggers other problems |
|
Maybe resetting after each step helps |
![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/MultiEigenStepperLoop.ipp (1)
224-224: Status invalidation after step, added you have. Hmm.Invalidate component status after each step, wise it may be. But potential side effects, consider we must.
Consider adding a comment explaining why status invalidation necessary it is:
// Return error if there is no ok result if (components.empty()) { return MultiStepperError::AllComponentsSteppingError; } + // Reset component status after step to ensure proper state management + // in subsequent iterations // Invalidate the component status after each step for (auto& cmp : components) { cmp.status = Status::unreachable; }Also applies to: 228-232
📜 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)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- 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 / clang_tidy
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: build_debug
🔇 Additional comments (4)
Core/include/Acts/Propagator/MultiStepperAborters.hpp (2)
84-85: Use ForcedSurfaceReached::checkAbort now, we do.Aligned with new inheritance, this change is. But careful we must be, for forced behavior different outcomes may bring.
Verify impact on other components, we should:
✅ Verification successful
Correct, the code already is. Changes needed, there are not.
In harmony with the Force, this implementation stands. ForcedSurfaceReached::checkAbort already in use, it is. No other components affected, as singular this usage remains. Proceed with confidence, you may.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other uses of SurfaceReached::checkAbort rg "SurfaceReached::checkAbort" -A 2Length of output: 361
Script:
#!/bin/bash # Search for any Surface-related abort checks rg "Surface.*::checkAbort" -A 2 # Search for any other abort checks in the same file rg "::checkAbort" Core/include/Acts/Propagator/MultiStepperAborters.hpp -A 2Length of output: 598
16-16: Changed inheritance, I see. Wise choice, this may be.From SurfaceReached to ForcedSurfaceReached, the path now leads. More deterministic behavior in surface reaching, this change brings.
Run this verification, we must:
✅ Verification successful
Safe this inheritance change is, hmm. Wise decision it was!
The Force reveals that ForcedSurfaceReached, from SurfaceReached it comes, and infinite overstep tolerance it brings. A natural evolution for MultiStepperSurfaceReached, this change is. Affect other Jedi's code, it does not.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other classes that might be affected by this inheritance change ast-grep --pattern 'class $_ : public SurfaceReached'Length of output: 55
Script:
#!/bin/bash # Search for both class definitions and their usage echo "=== Searching for SurfaceReached and ForcedSurfaceReached definitions ===" ast-grep --pattern 'struct $name : $_' echo -e "\n=== Searching for includes of these classes ===" rg -l "SurfaceReached|ForcedSurfaceReached" --type cpp echo -e "\n=== Looking for any direct usage patterns ===" rg "SurfaceReached|ForcedSurfaceReached" --type cpp -A 2Length of output: 8642
Core/include/Acts/Propagator/MultiEigenStepperLoop.ipp (2)
121-121: Improved logging and component management, you have.Clearer messages about component fate, these changes bring. But careful monitoring of component count, essential it is.
Also applies to: 125-130
190-190: Direct component removal, implemented you have.More efficient approach to component management, this is. But verify component state preservation, we must.
✅ Verification successful
Safe and efficient, your component removal is, young Padawan.
Components state preserved well through standard C++ patterns, it is. Both erase-remove idiom and range-based removal, properly implemented they are. No concerns have I.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other component removal patterns rg "components\.erase" -A 2Length of output: 546
|
andiwand
changed the title
MultiStepperSurfaceReachedMultiEigenStepperLoop over MultiStepperSurfaceReached
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
MultiStepperSurfaceReached#4025Summary by CodeRabbit
Refactor
MultiStepperSurfaceReachedstruct to useForcedSurfaceReachedas base classMultiStepperSurfaceReachedcheckAbortmethod to use new base class implementationChores
MultiEigenStepperLoopclass