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: Backport navigation rewrite changes #2846

Merged

Conversation

andiwand
Copy link
Contributor

Backporting more changes from #2625 to main. This is a followup to #2768

Summary

  • remove overstepping from steppers
  • use surface tolerance for geometry lookups
  • add self consistency navigation test

@andiwand andiwand added this to the next milestone Dec 20, 2023
@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module labels Dec 20, 2023
@andiwand andiwand changed the title refactor Backport navigation rewrite changes refactor: Backport navigation rewrite changes Dec 20, 2023
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Attention: Patch coverage is 40.65934% with 54 lines in your changes missing coverage. Please review.

Project coverage is 47.42%. Comparing base (24f3582) to head (8b80a34).
Report is 251 commits behind head on main.

Files Patch % Lines
Core/src/Geometry/TrackingVolume.cpp 34.69% 1 Missing and 31 partials ⚠️
Core/include/Acts/Propagator/Navigator.hpp 31.81% 0 Missing and 15 partials ⚠️
Core/src/Geometry/Layer.cpp 53.33% 0 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2846      +/-   ##
==========================================
- Coverage   47.51%   47.42%   -0.09%     
==========================================
  Files         499      499              
  Lines       28292    28278      -14     
  Branches    13832    13831       -1     
==========================================
- Hits        13442    13410      -32     
- Misses       4970     4989      +19     
+ Partials     9880     9879       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

untemplate nav options
fix nav failure for endless bounds intersection
maybe more
@andiwand andiwand marked this pull request as ready for review December 20, 2023 21:47
Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Very nice!

Core/include/Acts/Propagator/Navigator.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Intersection.hpp Outdated Show resolved Hide resolved
Core/src/Geometry/Layer.cpp Outdated Show resolved Hide resolved
Core/src/Geometry/TrackingVolume.cpp Show resolved Hide resolved
Core/src/Geometry/TrackingVolume.cpp Show resolved Hide resolved
Core/src/Geometry/TrackingVolume.cpp Show resolved Hide resolved
Core/include/Acts/Propagator/Navigator.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/Navigator.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/Navigator.hpp Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Stale label Feb 27, 2024
@andiwand andiwand closed this Mar 6, 2024
Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

Good to go.

@andiwand andiwand added automerge and removed 🚧 WIP Work-in-progress labels May 24, 2024
@kodiakhq kodiakhq bot merged commit 57fd232 into acts-project:main May 24, 2024
51 checks passed
@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label May 24, 2024
@andiwand andiwand deleted the backport-navigation-rewrite-changes branch May 25, 2024 06:37
kodiakhq bot pushed a commit that referenced this pull request May 25, 2024
In #2846 I removed a constructor parameter which was used by Athena and maybe other clients. This would classify as a breaking change. To work around that I reintroduced the parameter and marked the constructor as deprecated.
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label May 29, 2024
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 31, 2024
Backporting more changes from acts-project#2625 to main. This is a followup to acts-project#2768 

Summary
- remove overstepping from steppers
- use surface tolerance for geometry lookups
- add self consistency navigation test
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 31, 2024
In acts-project#2846 I removed a constructor parameter which was used by Athena and maybe other clients. This would classify as a breaking change. To work around that I reintroduced the parameter and marked the constructor as deprecated.
@andiwand andiwand modified the milestones: next, v35.1.0 Jun 1, 2024
kodiakhq bot pushed a commit that referenced this pull request Jun 7, 2024
This PR adds a "try all" navigator which can be used as a reference to validate other navigators

depends on
- #2846
kodiakhq bot pushed a commit that referenced this pull request Jun 11, 2024
This PR adds another "try all" navigator which deliberately oversteps and runs intersections backwards. It can be used as a reference to validate other navigators.

The idea of intersecting backwards is to have a better approximation with the helix. Instead of using the tangent we will use a ray that goes through the trajectory twice.

depends on
- #2846
- #2849
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jun 14, 2024
This PR adds another "try all" navigator which deliberately oversteps and runs intersections backwards. It can be used as a reference to validate other navigators.

The idea of intersecting backwards is to have a better approximation with the helix. Instead of using the tangent we will use a ray that goes through the trajectory twice.

depends on
- acts-project#2846
- acts-project#2849
Matthewharri pushed a commit to Matthewharri/acts that referenced this pull request Jun 18, 2024
Backporting more changes from acts-project#2625 to main. This is a followup to acts-project#2768 

Summary
- remove overstepping from steppers
- use surface tolerance for geometry lookups
- add self consistency navigation test
Matthewharri pushed a commit to Matthewharri/acts that referenced this pull request Jun 18, 2024
In acts-project#2846 I removed a constructor parameter which was used by Athena and maybe other clients. This would classify as a breaking change. To work around that I reintroduced the parameter and marked the constructor as deprecated.
Matthewharri pushed a commit to Matthewharri/acts that referenced this pull request Jun 18, 2024
This PR adds a "try all" navigator which can be used as a reference to validate other navigators

depends on
- acts-project#2846
Matthewharri pushed a commit to Matthewharri/acts that referenced this pull request Jun 18, 2024
This PR adds another "try all" navigator which deliberately oversteps and runs intersections backwards. It can be used as a reference to validate other navigators.

The idea of intersecting backwards is to have a better approximation with the helix. Instead of using the tangent we will use a ray that goes through the trajectory twice.

depends on
- acts-project#2846
- acts-project#2849
kodiakhq bot pushed a commit that referenced this pull request Jun 19, 2024
As previously removed by #2846 and then reintroduced in #3223 to keep compatibility, I propose to remove the `overstepLimit` parameter from the `EigenStepper` since it does not have any effect anymore.

The propagation still handles overstepping but the hard cutoff parameter is pushed out of the stepper interface. Right now we allow overstepping of any distance. If we need a cutoff value I would suggest putting it into the navigator or propagator.
timadye pushed a commit to andiwand/acts that referenced this pull request Jun 27, 2024
This PR adds another "try all" navigator which deliberately oversteps and runs intersections backwards. It can be used as a reference to validate other navigators.

The idea of intersecting backwards is to have a better approximation with the helix. Instead of using the tangent we will use a ray that goes through the trajectory twice.

depends on
- acts-project#2846
- acts-project#2849
timadye pushed a commit to andiwand/acts that referenced this pull request Jun 27, 2024
)

As previously removed by acts-project#2846 and then reintroduced in acts-project#3223 to keep compatibility, I propose to remove the `overstepLimit` parameter from the `EigenStepper` since it does not have any effect anymore.

The propagation still handles overstepping but the hard cutoff parameter is pushed out of the stepper interface. Right now we allow overstepping of any distance. If we need a cutoff value I would suggest putting it into the navigator or propagator.
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 Changes Performance Component - Core Affects the Core module Component - Examples Affects the Examples module Fails Athena tests This PR causes a failure in the Athena tests Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants