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

Redundancy of bound and curvilinear state #2812

Closed
andiwand opened this issue Dec 11, 2023 · 4 comments
Closed

Redundancy of bound and curvilinear state #2812

andiwand opened this issue Dec 11, 2023 · 4 comments
Labels

Comments

@andiwand
Copy link
Contributor

While refactoring the CovarianceEngine and the JacobianEngine I noticed that we duplicate quite some code for handling bound and curvilinear states as they are very similar.

I am not sure if that is based on performance reasons or if this is just grown historically.

In any case I think we could remove the duplication by merging the concept or we could split the curvilinear parameters from the bound parameters which would remove the dependency on a shared pointer surface under the hood.

Ultimately this depends on the performance implications of merging them I guess since this would be the most general and easiest solution IMO.

@beomki-yeo
Copy link
Contributor

Curvilinear state is just a special case of bound state where the surface normal vector is direction.

I suppose we can make our interface smarter with surface normal vector argument. (ex) put a surface normal vector for bound state and put a track direction for curvilinear state)

@paulgessinger
Copy link
Member

Just want to add that in curvilinear the plane rotation around the normal is also fixed.

Copy link

This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.

@github-actions github-actions bot added the Stale label Jan 11, 2024
kodiakhq bot pushed a commit that referenced this issue Apr 9, 2024
I noticed that the concept of a curvilinear surface is a quite spread over the codebase and I wanted to improve this. Here I introduce a surface like class which does not actually inherit from `Acts::Surface` but shares some of the functionality. This way the jacobians are put in a single place and can be used from somewhere else in an expressive fashion.

Afterwards it might make sense to let the `CurvilinearTrackParameters` depend on this special surface instead of a `PlaneSurface` or to merge them with `BoundTrackParameters` completely.

related issues
- #2812 

blocked by
- #2789
- #2782
- #2781
- #2811
Ragansu pushed a commit to Ragansu/acts that referenced this issue Apr 19, 2024
I noticed that the concept of a curvilinear surface is a quite spread over the codebase and I wanted to improve this. Here I introduce a surface like class which does not actually inherit from `Acts::Surface` but shares some of the functionality. This way the jacobians are put in a single place and can be used from somewhere else in an expressive fashion.

Afterwards it might make sense to let the `CurvilinearTrackParameters` depend on this special surface instead of a `PlaneSurface` or to merge them with `BoundTrackParameters` completely.

related issues
- acts-project#2812 

blocked by
- acts-project#2789
- acts-project#2782
- acts-project#2781
- acts-project#2811
EleniXoch pushed a commit to EleniXoch/acts that referenced this issue May 6, 2024
I noticed that the concept of a curvilinear surface is a quite spread over the codebase and I wanted to improve this. Here I introduce a surface like class which does not actually inherit from `Acts::Surface` but shares some of the functionality. This way the jacobians are put in a single place and can be used from somewhere else in an expressive fashion.

Afterwards it might make sense to let the `CurvilinearTrackParameters` depend on this special surface instead of a `PlaneSurface` or to merge them with `BoundTrackParameters` completely.

related issues
- acts-project#2812 

blocked by
- acts-project#2789
- acts-project#2782
- acts-project#2781
- acts-project#2811
@andiwand
Copy link
Contributor Author

andiwand commented May 7, 2024

After having a deeper look at this it seems to me that this is an optimization step. We are basically doing the same math but with less expressions in case of the curvilinear surface.

@andiwand andiwand closed this as completed May 7, 2024
asalzburger pushed a commit to asalzburger/acts that referenced this issue May 21, 2024
I noticed that the concept of a curvilinear surface is a quite spread over the codebase and I wanted to improve this. Here I introduce a surface like class which does not actually inherit from `Acts::Surface` but shares some of the functionality. This way the jacobians are put in a single place and can be used from somewhere else in an expressive fashion.

Afterwards it might make sense to let the `CurvilinearTrackParameters` depend on this special surface instead of a `PlaneSurface` or to merge them with `BoundTrackParameters` completely.

related issues
- acts-project#2812 

blocked by
- acts-project#2789
- acts-project#2782
- acts-project#2781
- acts-project#2811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants