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: Remove direction from NavigationOptions and drop constructor #2345

Merged
merged 10 commits into from
Aug 14, 2023

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Aug 4, 2023

I propose to drop the navigation direction from NavigationOptions as the direction vector parameter in the lookup functions is sufficient to provide the same functionality.

Additionally I removed the constructor of NavigationOptions which duplicated default values and was less explicit. I also removed the magic overstepping limit which will be overwritten by the one from the stepper anyways.

@andiwand andiwand added this to the next milestone Aug 4, 2023
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #2345 (08a3db7) into main (e9977db) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 46.51%.

@@            Coverage Diff             @@
##             main    #2345      +/-   ##
==========================================
- Coverage   49.63%   49.63%   -0.01%     
==========================================
  Files         453      453              
  Lines       25540    25530      -10     
  Branches    11709    11705       -4     
==========================================
- Hits        12678    12672       -6     
+ Misses       4580     4579       -1     
+ Partials     8282     8279       -3     
Files Changed Coverage Δ
Core/include/Acts/Navigation/DetectorNavigator.hpp 48.06% <ø> (ø)
Core/include/Acts/Propagator/DirectNavigator.hpp 75.71% <ø> (ø)
...de/Acts/TrackFinding/CombinatorialKalmanFilter.hpp 31.78% <0.00%> (ø)
Core/include/Acts/Utilities/UnitVectors.hpp 60.71% <ø> (ø)
Core/src/Geometry/Layer.cpp 57.46% <0.00%> (+0.21%) ⬆️
Core/src/Geometry/TrackingVolume.cpp 48.84% <25.00%> (+0.41%) ⬆️
Core/include/Acts/Propagator/Navigator.hpp 57.11% <62.96%> (-0.12%) ⬇️
Core/include/Acts/TrackFitting/KalmanFitter.hpp 42.89% <100.00%> (-0.29%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions github-actions bot added the Component - Examples Affects the Examples module label Aug 6, 2023
@andiwand
Copy link
Contributor Author

andiwand commented Aug 6, 2023

apparently this is fixing a small navigation issue. when we target a layer in backward navigation we used to propagate one step in the wrong direction and proceed to the target. this is not the case anymore and therefore results are slightly changing

@andiwand andiwand marked this pull request as ready for review August 6, 2023 09:00
@github-actions github-actions bot removed the Component - Examples Affects the Examples module label Aug 6, 2023
@github-actions github-actions bot added Component - Examples Affects the Examples module Changes Performance labels Aug 14, 2023
@benjaminhuth benjaminhuth self-assigned this Aug 14, 2023
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.

Let's get it in!

@kodiakhq kodiakhq bot merged commit 0f745ae into acts-project:main Aug 14, 2023
55 checks passed
@andiwand andiwand deleted the refactor-navigation-options branch August 14, 2023 13:07
@paulgessinger paulgessinger modified the milestones: next, v28.2.0 Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants