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(nav): Gen3 Navigation policies and factory #3760

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Oct 18, 2024

Terminology:

  • "Navigation delegate": the function that is registered with a tracking volume. In principle, this can be anything
  • "Navigation policy": the object that is registered with the tracking volume. It contains a method that is connected to the "navigation delegate". It has extra methods
  • "Navigation policy factory": To delay construction of the actual policy object until after the volume is fully sized and has all of its internal structure registered, the blueprint tree only contains navigation policy factories. This is configurable. During construction, the factory is applied to volumes, and produces a policy that is registered with the volume. This is called "navigation policy factory" from a conceptual point of view.
  • "MultiNavigationPolicy": chains together multiple policies in a sort of composition. You can have one policy that only deals with portals, one for sensitive and one for passive surfaces, for example.
  • To make this less annoying to construct, as you would have to manage the factory, I'm adding a concrete class NavigationPolicyFactory. Its job is to make defining a factory that produces a "MultiNavigationPolicy" easy, like:
    using namespace Acts;
    using SurfaceArrayNavigationPolicy::LayerType;
    SurfaceArrayNavigationPolicy::Config config{
      .layerType = LayerType::Cylinder,
      .bins = {10, 10}
    };
    auto factory = NavigationPolicyFactory::make()
      .add<TryAllPortalNavigationPolicy>()
      .add<SurfaceArrayNavigationPolicy>(config);
    or in python:
    policy = (
      acts.NavigationPolicyFactory.make()
        .add(acts.TryAllPortalNavigationPolicy)
        .add(acts.TryAllSurfaceNavigationPolicy)
        .add(
          acts.SurfaceArrayNavigationPolicy,
          acts.SurfaceArrayNavigationPolicy.Config(
              layerType=acts.SurfaceArrayNavigationPolicy.LayerType.Plane,
              bins=(10, 10),
          ),
        )
    )

Part of #3502

@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module labels Oct 18, 2024
Copy link

github-actions bot commented Oct 18, 2024

📊: Physics performance monitoring for b8d52c4

Full contents

physmon summary

@paulgessinger paulgessinger mentioned this pull request Oct 11, 2024
@paulgessinger paulgessinger added this to the next milestone Oct 18, 2024
@paulgessinger paulgessinger marked this pull request as ready for review October 18, 2024 13:16
@paulgessinger
Copy link
Member Author

@asalzburger I'm making some changes to the NavigationStream here to make it support the Gen3 portals. I'm keeping the Gen2 portals in, but I'm prefixing them with "Gen2" to clarify.

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.

Some general comments and questions about the logic from my side

@acts-project acts-project deleted a comment from paulgessinger Oct 19, 2024
@paulgessinger
Copy link
Member Author

After discussion with @andiwand, I refactored the interface of the function contained in the NavigationDelegate to be called initializeCandidates in most cases, and I pull out the navigation stream and the logger as separate arguments to clarify the function's purpose.

I'm also wrapping the navigation stream into a separate wrapper object called AppendOnlyNavigationStream that only exposes methods to append to the navigation stream, thereby guaranteeing that the navigation policies will not be able to arbitrarily manipulate the candidate vector.

This is to be complemented (in a future PR) with a method like isValid where the navigation policies can return whether their candidates can still be considered valid, as well as dedicated function for volume entry and exit.

andiwand
andiwand previously approved these changes Oct 23, 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.

I think that makes a lot of sense!

Lets get this in!

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 Component - Examples Affects the Examples module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants