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: introduce navigation stream #3538

Merged
merged 21 commits into from
Aug 30, 2024

Conversation

asalzburger
Copy link
Contributor

@asalzburger asalzburger commented Aug 22, 2024

This PR introduces a NavigationStream object and a corresponding helper.

The NavigationStream should build the backbone of the Gen3 navigator, with having a TargetStream and a GeometryStream to deal with.

For details of the ongoing discussion, please see #3526.

@paulgessinger @andiwand

@asalzburger asalzburger added this to the next milestone Aug 22, 2024
@github-actions github-actions bot added the Component - Core Affects the Core module label Aug 22, 2024
Copy link

github-actions bot commented Aug 22, 2024

📊: Physics performance monitoring for c179649

Full contents

physmon summary

Core/include/Acts/Navigation/NavigationStream.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Navigation/NavigationStream.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Navigation/NavigationStreamHelper.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Intersection.hpp Outdated Show resolved Hide resolved
Core/src/Navigation/NavigationStreamHelper.cpp Outdated Show resolved Hide resolved
Core/src/Navigation/NavigationStreamHelper.cpp Outdated Show resolved Hide resolved
Core/src/Navigation/NavigationStreamHelper.cpp Outdated Show resolved Hide resolved
Core/src/Navigation/NavigationStreamHelper.cpp Outdated Show resolved Hide resolved
Core/src/Navigation/NavigationStreamHelper.cpp Outdated Show resolved Hide resolved
Core/include/Acts/Navigation/NavigationStreamHelper.hpp Outdated Show resolved Hide resolved
@asalzburger
Copy link
Contributor Author

Hi @andiwand - I've changed this quite a bit, so here's a summary to guide you:

  • changed NavigationStream into a class with private members
  • moved initialize() and update methods to navigation stream, also the fill methods and renamed them
  • removed targetAbort
  • removed NavigationStreamHelper

This is pretty much along the lines that you requested.

andiwand
andiwand previously approved these changes Aug 29, 2024
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

LGTM 👍

One question: I wonder if we want to re-converge Core/Navigation and Core/Propagator since we accumulate stepping, navigation and propagation there. I feel like splitting out Navigation requires also to split out Stepping.

@asalzburger
Copy link
Contributor Author

@andiwand - had a problem with the doc run, so I had to push another commit

@asalzburger
Copy link
Contributor Author

LGTM 👍

One question: I wonder if we want to re-converge Core/Navigation and Core/Propagator since we accumulate stepping, navigation and propagation there. I feel like splitting out Navigation requires also to split out Stepping.

All fine, I think we can move it back to Propagation and leave Navigation for the NavigaitonDelegates ?
The NavigationStream could still live in Navigation as it will communicate between the two, what do you think?

Copy link

sonarcloud bot commented Aug 30, 2024

@kodiakhq kodiakhq bot merged commit 3e0967b into acts-project:main Aug 30, 2024
42 checks passed
@asalzburger asalzburger deleted the feat-navigation-stream branch August 30, 2024 07:27
@andiwand andiwand modified the milestones: next, v36.3.0 Sep 2, 2024
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