Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: Revise stepper update functions and constrained step types #3986

Merged
merged 5 commits into from
Jan 9, 2025

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Dec 14, 2024

  • remove default parameters from stepper update functions
  • remove release flag from update functions. this is an important part of our propagation which should not be put under the rug
  • add ConstrainedStep::Type param so it can be used for other purposes than just the navigation slot
  • rename ConstrainedStep::Types
    • actor -> navigator since we only use it for navigation
    • aborter -> actor since we only use it in the actors (aborters have been merged into actors)

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced handling of step sizes and surface status updates across various classes, including AtlasStepper, EigenStepper, and StraightLineStepper, now incorporating a new parameter for step size type.
    • Introduction of a getStepSize method for better access to step sizes based on type.
  • Bug Fixes

    • Adjusted method calls to ensure consistent use of ConstrainedStep::navigator instead of ConstrainedStep::actor, improving accuracy in step size management.
  • Documentation

    • Updated comments and documentation across various components to enhance clarity regarding functionality and usage.
  • Tests

    • Comprehensive updates to unit tests to reflect changes in method signatures and ensure correct functionality of the stepper classes with the new parameters.

@andiwand andiwand added this to the v39.0.0 milestone Dec 14, 2024
Copy link

coderabbitai bot commented Dec 14, 2024

Walkthrough

Hmm, a significant refactoring of the ACTS propagation system, this is. Across multiple files, changes to step size management and navigation have been made. The core transformation involves renaming the ConstrainedStep enum types, particularly shifting from aborter to navigator. Methodical and systematic, these modifications are, touching stepper, navigator, and test implementations throughout the codebase.

Changes

File Category Changes
Propagator Headers Updated method signatures in multiple steppers to include ConstrainedStep::Type
Navigator Files Modified step size handling, replacing ConstrainedStep::actor with ConstrainedStep::navigator
Test Files Adjusted test cases to reflect new method signatures and step type handling

Suggested labels

automerge

Suggested reviewers

  • asalzburger
  • benjaminhuth

Poem

Steppers dance, navigators glide,
Constraints shift, parameters ride
Code flows like the Force, serene and bright
Refactoring brings algorithmic delight! 🚀✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8e8c35f and 3257c71.

📒 Files selected for processing (14)
  • Core/include/Acts/Propagator/AtlasStepper.hpp (3 hunks)
  • Core/include/Acts/Propagator/EigenStepper.hpp (2 hunks)
  • Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp (5 hunks)
  • Core/include/Acts/Propagator/Propagator.ipp (1 hunks)
  • Core/include/Acts/Propagator/StraightLineStepper.hpp (2 hunks)
  • Core/include/Acts/Propagator/SympyStepper.hpp (2 hunks)
  • Core/include/Acts/Propagator/detail/SteppingHelper.hpp (1 hunks)
  • Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp (3 hunks)
  • Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp (3 hunks)
  • Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp (4 hunks)
  • Tests/UnitTests/Core/Propagator/NavigatorTests.cpp (2 hunks)
  • Tests/UnitTests/Core/Propagator/PropagatorTests.cpp (1 hunks)
  • Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp (3 hunks)
  • Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • Tests/UnitTests/Core/Propagator/PropagatorTests.cpp
  • Core/include/Acts/Propagator/Propagator.ipp
  • Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp
  • Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp
  • Tests/UnitTests/Core/Propagator/NavigatorTests.cpp
  • Core/include/Acts/Propagator/StraightLineStepper.hpp
  • Core/include/Acts/Propagator/AtlasStepper.hpp
  • Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp
  • Core/include/Acts/Propagator/SympyStepper.hpp
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: merge-sentinel
  • GitHub Check: macos
🔇 Additional comments (12)
Core/include/Acts/Propagator/detail/SteppingHelper.hpp (1)

29-45: Wise decision, avoiding default values is!

Explicit parameter requirement for stype, a good practice it is. Previous discussions confirm, no good default value exists. Clear and intentional, this design choice is.

Core/include/Acts/Propagator/EigenStepper.hpp (2)

241-250: Approve this implementation, I do!

Properly forwarded, the stype parameter to the helper function is. Maintains consistency with the new design, it does.


262-280: Strong and consistent, these changes are!

Properly replaced, the release flag with stype parameter is. Handles both intersection and explicit updates well, it does. A unified approach to step size management, this brings.

Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp (3)

Line range hint 502-516: Approve this multi-component implementation, I do!

Properly handled, the stype parameter for each component is. Maintains consistency across all components, it does.


Line range hint 580-594: Strong with multi-component handling, these changes are!

Properly updates step size for each component with stype parameter, it does. A consistent approach across all components, this maintains.


604-606: Balanced and harmonious, this implementation is!

Properly handles explicit step size updates with stype parameter, it does. Maintains consistency with other update methods, this approach does.

Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp (3)

263-267: Approve the step size management changes, I do.

Correctly aligned with the navigator-based approach, these changes are. The test cases, both update and release of step size, properly verify they do.


452-455: Hmm, proper surface status update testing, this is.

Validate the step size after surface status update with navigator type, this test does. Consistent with the refactoring goals, it remains.


464-464: Strong with the Force, these step size update tests are.

Multiple scenarios of step size management, these tests cover. Properly aligned with the navigator-based approach, they remain. Comprehensive test coverage, they provide.

Also applies to: 467-467, 474-474

Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp (3)

588-592: Approve these changes, I do! Consistent with the refactoring, they are.

Updated correctly, the step size management tests have been. From actor to navigator, the transition is complete, hmm.


614-615: Properly extended, the surface status update test has been.

Added the navigator parameter has been, maintaining consistency with the new interface, yes. Good test coverage, this provides.


624-624: Complete, the surface intersection tests are.

Updated all step size management calls with the navigator parameter, you have. Thorough testing of surface interactions, this provides. Pleased with this comprehensive coverage, I am.

Also applies to: 629-629, 636-636


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added Component - Core Affects the Core module Component - Fatras Affects the Fatras module Component - Examples Affects the Examples module Track Finding labels Dec 14, 2024
Copy link

github-actions bot commented Dec 14, 2024

📊: Physics performance monitoring for e438f3d

Full contents

physmon summary

@andiwand andiwand marked this pull request as ready for review December 17, 2024 14:26
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (4)
Core/include/Acts/Propagator/AtlasStepper.hpp (1)

440-447: Improper use of direction parameter, confusion there is.

In updateStepSize, the direction parameter is ignored. Clarify its necessity, you should.

Consider removing the direction parameter if unused, or utilize it appropriately.

Fatras/include/ActsFatras/Kernel/detail/SimulationActor.hpp (1)

13-13: Lighter, the header burden becomes!

Include what you need, no more, no less. The path to PropagatorState.hpp, more direct it is. Unnecessary dependencies, eliminated they are.

Consider adding a comment explaining the PropagatorState.hpp dependency:

+// Include propagator state definitions required for simulation
 #include "Acts/Propagator/PropagatorState.hpp"
Core/include/Acts/Propagator/SympyStepper.hpp (1)

259-266: Simplified the step size updates have become, yes.

Removed the release flag, we have. More focused and clearer purpose, the method now shows. Through the Force, better code flows.

Consider documenting the relationship between Direction and ConstrainedStep::Type in the method comments, help future Padawans, it will.

Also applies to: 275-277

Core/include/Acts/Propagator/StraightLineStepper.hpp (1)

262-269: Simplified the step size update methods, you have.

Removed the redundant release parameter, making the API cleaner it does. Step type parameter now controls the behavior, more elegant solution this is.

Consider adding documentation to explain the valid step types and their effects:

  /// Update step size
  ///
  /// @param state [in,out] The stepping state (thread-local cache)
  /// @param stepSize [in] The step size value
  /// @param stype [in] The step size type to be set
+ /// @note Valid step types are:
+ ///   - ConstrainedStep::navigator: Used for navigation-related step size constraints
+ ///   - ConstrainedStep::actor: Used for actor-related step size constraints
+ ///   - ConstrainedStep::user: Used for user-defined step size constraints

Also applies to: 278-280

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9a2b74d and e97d8ab.

📒 Files selected for processing (26)
  • Core/include/Acts/Navigation/DetectorNavigator.hpp (3 hunks)
  • Core/include/Acts/Propagator/AtlasStepper.hpp (3 hunks)
  • Core/include/Acts/Propagator/ConstrainedStep.hpp (3 hunks)
  • Core/include/Acts/Propagator/DirectNavigator.hpp (3 hunks)
  • Core/include/Acts/Propagator/EigenStepper.hpp (2 hunks)
  • Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp (5 hunks)
  • Core/include/Acts/Propagator/Navigator.hpp (16 hunks)
  • Core/include/Acts/Propagator/Propagator.ipp (1 hunks)
  • Core/include/Acts/Propagator/StandardAborters.hpp (2 hunks)
  • Core/include/Acts/Propagator/StepperConcept.hpp (1 hunks)
  • Core/include/Acts/Propagator/StraightLineStepper.hpp (2 hunks)
  • Core/include/Acts/Propagator/SympyStepper.hpp (2 hunks)
  • Core/include/Acts/Propagator/TryAllNavigator.hpp (8 hunks)
  • Core/include/Acts/Propagator/detail/SteppingHelper.hpp (1 hunks)
  • Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (1 hunks)
  • Examples/Io/Root/src/RootPropagationStepsWriter.cpp (1 hunks)
  • Fatras/include/ActsFatras/Kernel/detail/SimulationActor.hpp (1 hunks)
  • Tests/IntegrationTests/Fatras/FatrasSimulationTests.cpp (1 hunks)
  • Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp (4 hunks)
  • Tests/UnitTests/Core/Propagator/ConstrainedStepTests.cpp (2 hunks)
  • Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp (4 hunks)
  • Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp (6 hunks)
  • Tests/UnitTests/Core/Propagator/NavigatorTests.cpp (2 hunks)
  • Tests/UnitTests/Core/Propagator/PropagatorTests.cpp (1 hunks)
  • Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp (4 hunks)
  • Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp (4 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
  • Examples/Io/Root/src/RootPropagationStepsWriter.cpp
  • Tests/IntegrationTests/Fatras/FatrasSimulationTests.cpp
  • Core/include/Acts/Propagator/StandardAborters.hpp
🔇 Additional comments (48)
Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp (3)

13-13: LGTM! Include Tolerance.hpp, a wise addition it is.

For surface tolerance handling, necessary this include is.


244-248: Correctly updated step size management, you have.

Changed from actor to navigator the parameter has been, aligning with the broader refactoring goals. Proper sequence of update and release operations, maintained it is.


434-437: Surface status updates, properly modified they are.

Added surface tolerance parameter and navigator type, consistent with the architectural changes they are. Hmmmm, verify the impact of these changes across the test suite, we must.

Also applies to: 446-446, 449-449, 456-456

✅ Verification successful

Consistent with the Force, these changes are.

Through the test files, searched I have. Clear it is that all stepper tests properly use ConstrainedStep::navigator, and no remnants of actor in the test suite remain. The surface status updates, with proper tolerance parameters they are equipped.

  • In AtlasStepperTests.cpp, EigenStepperTests.cpp, MultiStepperTests.cpp, StraightLineStepperTests.cpp, and SympyStepperTests.cpp, correctly implemented the navigator pattern is.
  • The boundary tolerance parameters, consistently used they are, though some use Infinite() while others use None() - intentional this difference appears to be.
  • No instances of the old actor pattern in these test files, found I have.

Balance to the Force, these changes bring. Hmmmm, yes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other test files that might need similar updates
echo "Searching for similar test patterns in other files..."
rg -l "updateSurfaceStatus.*BoundaryTolerance::None" --type cpp

# Look for potentially missed updates from actor to navigator
echo "Checking for potentially missed actor to navigator updates..."
rg "ConstrainedStep::actor" --type cpp

Length of output: 1298


Script:

#!/bin/bash
# Check the specific test file for any remaining actor references
echo "Examining the specific test file for actor references..."
rg "ConstrainedStep::actor" Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp

# Look for the full context of updateSurfaceStatus usage in the test file
echo "Checking surface status update patterns in the test file..."
rg -B2 -A2 "updateSurfaceStatus" Tests/UnitTests/Core/Propagator/SympyStepperTests.cpp

# Check if there are any other similar test files that might need updates
echo "Finding similar stepper test files..."
fd "StepperTests.cpp$"

Length of output: 1090


Script:

#!/bin/bash
# Check for surface status updates in other stepper test files
echo "Examining surface status patterns in other stepper tests..."
for file in Tests/UnitTests/Core/Propagator/*StepperTests.cpp; do
    echo "=== $file ==="
    rg "updateSurfaceStatus" "$file" -B1 -A1
done

# Check for navigator vs actor usage in these files
echo "Checking ConstrainedStep type usage in stepper tests..."
for file in Tests/UnitTests/Core/Propagator/*StepperTests.cpp; do
    echo "=== $file ==="
    rg "ConstrainedStep::(navigator|actor)" "$file"
done

Length of output: 6356

Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp (4)

Line range hint 517-531: Added stype parameter, well done this is.

Enhancing updateSurfaceStatus with ConstrainedStep::Type stype, clarity and flexibility it brings.


Line range hint 595-608: Consistency maintained in updateStepSize, yes.

Introducing stype parameter aligns methods across steppers, good practice this is.


618-620: Overload updated with stype, correct this is.

Ensuring both overloads of updateStepSize accept stype, thorough you have been.


Line range hint 623-627: New method getStepSize added, useful it will be.

Providing step size retrieval based on stype, functionality improved has been.

Core/include/Acts/Propagator/TryAllNavigator.hpp (2)

395-395: ⚠️ Potential issue

Release step size properly, you should.

In preStep, ensure releaseStepSize uses the correct stype.

Apply this diff:

-          stepper.releaseStepSize(state.stepping);
+          stepper.releaseStepSize(state.stepping, ConstrainedStep::navigator);

Likely invalid or redundant comment.


822-823: ⚠️ Potential issue

Consistency lacking in updateSurfaceStatus, rectify you must.

Again, stype parameter missing. Include ConstrainedStep::navigator.

Apply this diff:

-                state.options.surfaceTolerance, logger());
+                state.options.surfaceTolerance, ConstrainedStep::navigator, logger());

Likely invalid or redundant comment.

Core/include/Acts/Propagator/AtlasStepper.hpp (3)

421-430: Enhanced updateSurfaceStatus, good this is.

Adding stype parameter improves control over step types.


456-458: Correct update of step size with stype, noted it is.

Adjusting updateStepSize to accept stype, consistent with other steppers.


469-475: getStepSize method added, valuable addition it is.

Providing access to step size by type enhances functionality.

Tests/UnitTests/Core/Propagator/ConstrainedStepTests.cpp (3)

25-27: Update test cases with new stype, attentive you are.

Replacing ConstrainedStep::actor with ConstrainedStep::navigator, correctness maintained.


39-42: Consistent use of navigator in tests, good this is.

Ensuring test coverage aligns with code changes.


46-47: Release step size with correct stype, you do.

Updating test to release ConstrainedStep::navigator, appropriate it is.

Core/include/Acts/Propagator/detail/SteppingHelper.hpp (3)

34-38: Improved parameter clarity and flexibility, I sense.

Wise changes these are. Renamed parameter 'direction' more intuitive it is, and new parameter 'stype' greater control it provides. Clear and purposeful, the function signature has become.


57-57: Consistent step size management, achieved it has been.

The force is strong with this change. Step size release and update now use the same 'stype' parameter, maintaining consistency throughout the propagation process.

Also applies to: 69-70


62-63: Verify the impact of ConstrainedStep::actor usage, we must.

Changed from 'aborter' to 'actor', the far limit reference has. Ensure all callers aware of this change, they are.

Core/include/Acts/Propagator/StepperConcept.hpp (2)

55-57: Updated concept requirements, clarity they bring.

Wise decision it was, to add ConstrainedStep::Type parameter. Stronger type safety and clearer intentions, this change provides.


63-64: Simplified updateStepSize signature, more focused it becomes.

Removed the boolean parameter has been, cleaner interface it creates. Yet verify all implementations we must.

✅ Verification successful

The pattern needs adjustment, young padawan. Search for Stepper implementations in different way, we must. Also examine the updateStepSize method implementations, we shall.


Verified the updateStepSize changes have been, across all stepper implementations.

Examined the implementations I have. All steppers - EigenStepper, AtlasStepper, StraightLineStepper, SympyStepper, and MultiEigenStepperLoop - aligned with the new signature they are. Removed the boolean parameter, yet maintained their functionality they have. The Force is in balance.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find all stepper implementations
ast-grep --pattern 'class $_ : public $$$Stepper' 

Length of output: 51


Script:

#!/bin/bash
# Search for stepper implementations with different pattern
rg -l "class.*Stepper.*\{" --type cpp

# For each found file, show the updateStepSize method implementation
rg "updateStepSize.*\(" -A 5 --type cpp

Length of output: 21662

Core/include/Acts/Propagator/ConstrainedStep.hpp (3)

45-48: Renamed enum values, clarity they bring.

Strong with the Force, these changes are. 'navigator' instead of 'actor', and 'actor' instead of 'aborter', better reflect their true purpose they do.


109-109: Simplified update method signature, wisdom it shows.

Removed the releaseStep parameter has been. Separate release method already exists, redundant it was.


134-134: Stream output alignment, maintained it is.

Updated stream output to use new enum values, consistency it maintains. Yet verify all usages we must.

Also applies to: 136-136

✅ Verification successful

I need to check the implementation of streamValue and any test cases to ensure the streaming behavior is consistent. This will help verify if the changes maintain proper alignment with the enum values.


Need to verify the implementation of streamValue and the enum values it uses, I do. Help us understand the streaming behavior better, this will.


Verified the streaming alignment is, hmmmm. Safe the changes are.

Examined the implementation I have. Uses proper enum values it does - navigator, actor, and user. Consistent with the Type enum the streaming remains. Format values correctly the streamValue lambda does, handling both finite and infinite cases with proper alignment it maintains.

Through the Force, see I do that all usages in tests maintain consistency with these enum values. No disturbance in the Force there is.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any streaming operations with ConstrainedStep
rg "<<.*ConstrainedStep"

Length of output: 156


Script:

#!/bin/bash
# Search for the implementation of streamValue and its usage
ast-grep --pattern 'streamValue($_)'

# Also check for any test cases or usage of ConstrainedStep streaming
rg -A 5 "ConstrainedStep" Tests/

Length of output: 29227


Script:

#!/bin/bash
# Search for the implementation of streamValue in ConstrainedStep.hpp
rg -A 5 "streamValue" Core/include/Acts/Propagator/ConstrainedStep.hpp

# Check for the enum values used in ConstrainedStep
rg "enum.*Type" Core/include/Acts/Propagator/ConstrainedStep.hpp -A 5

Length of output: 702

Core/include/Acts/Propagator/DirectNavigator.hpp (2)

Line range hint 235-241: Wise, this change is!

From actor to navigator, the step release now flows. More precise naming, it brings. The Force, strong with this refactoring, it is.


259-259: Consistent, these changes are!

The path of the navigator, clearer it becomes. From actor to navigator, the surface status updates now flow. Balance to the code, this brings.

Also applies to: 313-313

Core/include/Acts/Propagator/Propagator.ipp (1)

70-71: Balanced, the Force of step size release now is!

Release the constraints, we must, but only those that matter. The aborter constraint, removed it was, for in the propagation process, its significance lies.

Run this command, you shall, to verify the constraint handling:

✅ Verification successful

The Force guides me to look deeper into the constraints, hmm. See the enum definition and its usage patterns, we must.


Hmm, deeper into the Force of constraints, we must look. The enum definition and class structure, reveal to us they will.


Through the Force, clarity I have found!

From the extensive search results shown earlier, observe I do that ConstrainedStep::navigator and ConstrainedStep::actor are the primary constraints in use throughout the codebase. No reference to ConstrainedStep::aborter remains, I see. The removal of the aborter constraint release aligns with the natural flow of the Force, it does.

  • In Core/include/Acts/Propagator/Propagator.ipp, correct the changes are, releasing only navigator and actor constraints
  • Throughout the codebase, only these two types consistently used they are
  • The aborter constraint, abandoned it has been, as the PR intended
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to aborter in step size handling
rg -l "ConstrainedStep::aborter" 

Length of output: 32


Script:

#!/bin/bash
# Search for ConstrainedStep enum to understand all possible values
ast-grep --pattern 'enum $_ {
  $$$
}'

# Also search for any references to ConstrainedStep in the codebase
rg "ConstrainedStep::" -A 2

Length of output: 32963


Script:

#!/bin/bash
# Search for ConstrainedStep enum definition
ast-grep --pattern 'enum $_ {
  $$$
  aborter
  $$$
}'

# Also search for the ConstrainedStep class definition
ast-grep --pattern 'class ConstrainedStep {
  $$$
}'

Length of output: 111

Core/include/Acts/Navigation/DetectorNavigator.hpp (1)

217-217: Hmmmm, wise changes to surface status updates, these are.

Consistent with the navigation framework, this modification is. Pass the step type explicitly now, we do, making the intention clearer, it does.

Also applies to: 247-247

Core/include/Acts/Propagator/SympyStepper.hpp (1)

238-247: Strong with the Force, these changes are.

Explicit step type handling, we now have. Clearer intent and better control over navigation steps, this brings.

Core/include/Acts/Propagator/EigenStepper.hpp (1)

271-280: In harmony with the Force, these changes are.

Mirror the changes in SympyStepper, they do. A consistent pattern across the steppers, we see. Wisdom in uniformity, there is.

Strong points, observe I do:

  • Explicit step type handling
  • Removal of release flag
  • Consistent implementation across steppers

Verify the consistency across all stepper implementations, we must:

Also applies to: 292-299, 308-310

✅ Verification successful

Consistent across all steppers, the changes are.

Through the Force, searched the entire codebase I have. A pattern most clear emerges:

  • In EigenStepper, SympyStepper, AtlasStepper, and StraightLineStepper, identical implementation of updateSurfaceStatus and updateStepSize methods, there is
  • The stype parameter, consistently used across all stepper implementations, it is
  • The step type handling, uniform across the codebase, remains

Strong in the Force, this consistency is. No disturbance in the implementation, I sense.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for updateSurfaceStatus and updateStepSize implementations
echo "Searching for stepper implementations..."
rg -A 5 "updateSurfaceStatus|updateStepSize" --type cpp

Length of output: 40608

Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp (3)

13-13: Hmm, a new header inclusion, I see.

Added Tolerance.hpp has been, for surface tolerance definitions it provides. Wise move, this is.


154-158: Renamed from 'actor' to 'navigator', the step type has been.

Reflect the true purpose of the step type, this change does. For navigation purposes only, it is used. Clear and precise, the naming now becomes.


332-334: Surface tolerance parameter, added it has been.

More precise control over surface intersection checks, this provides. Good practice, this is, for robust geometric calculations.

Tests/UnitTests/Core/Propagator/PropagatorTests.cpp (1)

123-129: Step size management, updated it has been.

Changed from actor to navigator, the step type has been. Consistent with the broader refactoring, this change is. Clear intention in the code, it shows.

Core/include/Acts/Propagator/StraightLineStepper.hpp (1)

243-252: Added step type parameter to surface status update, hmm.

Explicit about step type intentions, the API now becomes. Good practice this is, for clear and maintainable code.

Tests/UnitTests/Core/Propagator/NavigatorTests.cpp (2)

20-20: Wise addition of transformation header, this is!

The inclusion of CorrectedTransformationFreeToBound.hpp aligns with the refactoring of stepper update functions, it does.


141-145: Updated surface status parameters, hmm, yes!

Consistent with the PR objectives, the addition of s_onSurfaceTolerance and ConstrainedStep::Type stype parameters is. Better control over surface interactions, this provides.

Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp (3)

15-15: Added tolerance header, essential it is!

The inclusion of Tolerance.hpp supports the surface tolerance changes throughout the codebase.


562-566: Updated step size test cases, properly aligned they are!

Changed from ConstrainedStep::actor to ConstrainedStep::navigator, reflecting the PR's objective to better represent navigation context, it does.


584-586: Surface status update parameters, properly modified they are!

Added s_onSurfaceTolerance and updated to ConstrainedStep::navigator, consistent with the stepper interface changes, this is.

Tests/UnitTests/Core/Propagator/MultiStepperTests.cpp (3)

13-13: Header additions, well-structured they are!

The inclusion of Tolerance.hpp and ConstrainedStep.hpp headers, proper foundation for the updated tests, they provide.

Also applies to: 23-23


483-484: Surface status update tests, consistently modified they are!

Multiple test cases updated with s_onSurfaceTolerance and ConstrainedStep::navigator, thorough testing of the new interface, this ensures.

Also applies to: 513-514, 530-531


596-599: Component-wise bound state tests, properly adapted they are!

Surface status updates in component tests, correctly modified to use the new parameters, they have been. Consistency across single and multi-component scenarios, this maintains.

Also applies to: 604-607

Tests/UnitTests/Core/Propagator/EigenStepperTests.cpp (2)

269-273: Hmm, correct the step size management is, young padawan!

The transition from old ways to new ways, I see. Using ConstrainedStep::navigator instead of actor, wisdom shows. Proper step size management, this change brings.


461-464: Consistent with the Force, this change is!

The surface status update with ConstrainedStep::navigator, proper alignment with navigation context it brings. Clear intention, the code now shows.

Core/include/Acts/Propagator/Navigator.hpp (3)

445-445: Release the step size with navigator's wisdom, we must!

In navigation break handling, proper step size release through ConstrainedStep::navigator, implemented it is. Clear ownership of step management, this brings.

Also applies to: 464-464


571-571: When no volume remains, release the step we must!

At the end of known space, proper step release through navigator, essential it is. Clean navigation termination, this ensures.


870-870: When boundaries unclear are, step release through navigator, wisdom shows!

In boundary resolution failure, proper step management through navigator context, implemented it is. Clear error handling path, this provides.

Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp (1)

625-625: In Kalman's realm, navigator's way we follow!

When target surface reached is, step release through proper channel - the navigator - happen it must. Consistency with the new ways, this maintains.

@kodiakhq kodiakhq bot merged commit 567db0c into acts-project:main Jan 9, 2025
42 checks passed
@andiwand andiwand deleted the refactor-stepper-update branch January 9, 2025 15:29
@github-actions github-actions bot removed the automerge label Jan 9, 2025
@acts-project-service
Copy link
Collaborator

🔴 Athena integration test results [567db0c]

Build job with this PR failed!

Please investigate the build job for the pipeline!

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Jan 9, 2025
kodiakhq bot pushed a commit that referenced this pull request Jan 15, 2025
This is a proposal to decouple navigation from stepping by
- changing the navigator interface to only consume **its** state and not the propagation state
- providing position and direction as an input to the navigation
- putting the `Propagator` in charge of communicating the target and estimated distance to the stepper

Originally 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
- #3984
- #3985
- #3986

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit

## Release Notes

- **Navigation Improvements**
	- Introduced a new `NavigationTarget` structure to enhance navigation precision.
	- Streamlined navigator initialization and target resolution processes.
	- Added more robust surface and target validation mechanisms.
	- Removed unnecessary member variables from navigators to simplify state management.

- **Propagator Enhancements**
	- Updated propagation logic to improve step size management.
	- Introduced configurable maximum target skipping limit.
	- Enhanced error handling for navigation scenarios.
	- Updated error enumerations to reflect new conditions.

- **Stepper Refinements**
	- Simplified stepper state initialization.
	- Improved step size update and release mechanisms.
	- Added more flexible context and options handling.
	- Removed redundant tolerance parameters from stepper states.

- **Testing and Validation**
	- Updated test cases to reflect new navigation and propagation interfaces.
	- Enhanced logging and error reporting.
	- Improved test coverage for various navigation scenarios.
	- Streamlined test setup and initialization processes.

- **Performance Optimizations**
	- Removed redundant state management variables.
	- Simplified navigation and stepping logic.
	- Improved memory management in propagation states.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
kodiakhq bot pushed a commit that referenced this pull request Jan 15, 2025
…l initial letter (#4013)

Followup from #3986

- Strongly typed enum `ConstrainedStep::Type`
- capital initial letter for `Type` instances

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit

Based on the comprehensive summary, here are the updated release notes:

- **Code Improvement**
  - Updated `ConstrainedStep` enumeration to use scoped enum (`enum class`) for improved type safety.
  - Renamed enum values to use PascalCase (`Navigator`, `Actor`, `User`).

- **Type System Changes**
  - Modified method signatures to use `ConstrainedStep::Type::Navigator/Actor/User` instead of previous flat enum references.
  - Standardized parameter naming conventions across multiple files.

- **Compatibility**
  - Existing functionality preserved while enhancing code clarity and type checking.
  - Minimal impact on overall system behavior.

The changes represent a systematic refactoring to improve code quality and type safety across the project's propagation and stepping mechanisms.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaks Athena build This PR breaks the Athena build Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Track Finding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants