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

feat: Add TryAllOverstepNavigator #2850

Merged
merged 6 commits into from
Jun 11, 2024

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Dec 21, 2023

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

@andiwand andiwand added this to the next milestone Dec 21, 2023
@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module labels Dec 21, 2023
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: Patch coverage is 52.69461% with 79 lines in your changes missing coverage. Please review.

Project coverage is 47.46%. Comparing base (c8ce7cb) to head (0c93c4b).
Report is 272 commits behind head on main.

Files with missing lines Patch % Lines
Core/include/Acts/Propagator/TryAllNavigator.hpp 52.69% 19 Missing and 60 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2850      +/-   ##
==========================================
+ Coverage   47.43%   47.46%   +0.02%     
==========================================
  Files         510      510              
  Lines       30053    30198     +145     
  Branches    14564    14639      +75     
==========================================
+ Hits        14257    14332      +75     
- Misses       5323     5335      +12     
- Partials    10473    10531      +58     

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

@github-actions github-actions bot added Stale and removed Stale labels Jan 20, 2024
@github-actions github-actions bot added Stale and removed Stale labels Feb 19, 2024
@andiwand andiwand closed this Mar 6, 2024
@paulgessinger paulgessinger modified the milestones: next, v33.1.0 Mar 26, 2024
@andiwand andiwand reopened this Jun 5, 2024
@andiwand andiwand added the 🚧 WIP Work-in-progress label Jun 5, 2024
@andiwand andiwand modified the milestones: v33.1.0, v35.2.0 Jun 5, 2024
@andiwand andiwand changed the title feat: Try all "overstep" navigator feat: Add TryAllOverstepNavigator Jun 6, 2024
@andiwand andiwand force-pushed the try-all-overstep-navigator branch from 5a2ab0a to eb74c80 Compare June 6, 2024 18:39
@github-actions github-actions bot removed the Component - Examples Affects the Examples module label Jun 6, 2024
@andiwand andiwand marked this pull request as ready for review June 6, 2024 19:01
@andiwand andiwand added 🛑 blocked This item is blocked by another item and removed 🚧 WIP Work-in-progress labels Jun 6, 2024
@andiwand andiwand force-pushed the try-all-overstep-navigator branch from 99b02e3 to 87ea1bc Compare June 7, 2024 07:05
@andiwand andiwand force-pushed the try-all-overstep-navigator branch from 87ea1bc to c96277c Compare June 7, 2024 16:30
@andiwand andiwand removed the 🛑 blocked This item is blocked by another item label Jun 7, 2024
Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Just a comment regarding potential code sharing, I won't insist on any of this

Core/include/Acts/Propagator/TryAllOverstepNavigator.hpp Outdated Show resolved Hide resolved
Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Looks good!

@kodiakhq kodiakhq bot merged commit 6b2e205 into acts-project:main Jun 11, 2024
51 checks passed
@andiwand andiwand deleted the try-all-overstep-navigator branch June 11, 2024 11:17
kodiakhq bot pushed a commit that referenced this pull request Jun 12, 2024
After #2849 and #2850 it is time to update the documentation of our navigators. I also included a line about the `DetectorNavigator`.

blocked by
- #2850
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
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jun 14, 2024
After acts-project#2849 and acts-project#2850 it is time to update the documentation of our navigators. I also included a line about the `DetectorNavigator`.

blocked by
- acts-project#2850
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
Matthewharri pushed a commit to Matthewharri/acts that referenced this pull request Jun 18, 2024
After acts-project#2849 and acts-project#2850 it is time to update the documentation of our navigators. I also included a line about the `DetectorNavigator`.

blocked by
- acts-project#2850
kodiakhq bot pushed a commit that referenced this pull request Jun 20, 2024
## Issue
The test-time for build_debug grew significantly after:
- #2849
- #2850

Before:
```
        Start 127: Navigator
127/239 Test #127: Navigator ............................   Passed   22.29 sec
```
TryAllNavigator:
```
        Start 128: Navigator
128/240 Test #128: Navigator ............................   Passed  1041.82 sec
```
TryAllOverstepNavigator:
```
        Start 128: Navigator
128/240 Test #128: Navigator ............................   Passed  3994.42 sec
```

## Possible Solution
This PR reduces the number of tests with the try-all-references from 500 to 10. This results in a test time of around 200 sec.

Maybe we should adapt the test in the future to get more tests in a similar time.
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
After acts-project#2849 and acts-project#2850 it is time to update the documentation of our navigators. I also included a line about the `DetectorNavigator`.

blocked by
- acts-project#2850
timadye pushed a commit to andiwand/acts that referenced this pull request Jun 27, 2024
)

## Issue
The test-time for build_debug grew significantly after:
- acts-project#2849
- acts-project#2850

Before:
```
        Start 127: Navigator
127/239 Test acts-project#127: Navigator ............................   Passed   22.29 sec
```
TryAllNavigator:
```
        Start 128: Navigator
128/240 Test acts-project#128: Navigator ............................   Passed  1041.82 sec
```
TryAllOverstepNavigator:
```
        Start 128: Navigator
128/240 Test acts-project#128: Navigator ............................   Passed  3994.42 sec
```

## Possible Solution
This PR reduces the number of tests with the try-all-references from 500 to 10. This results in a test time of around 200 sec.

Maybe we should adapt the test in the future to get more tests in a similar time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants