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 IVertexFinder interface, use in vertexing #2948

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Feb 14, 2024

This PR introduces a new IVertexFinder with an interface that can support the existing vertex finder implementations, with the exception of the seed vertex finder (which depends on specific spacepoint triplets).

The interface introduces a type-erased state holder that the client of the finder initializes via the makeState interface function, and which the finder implementation downcasts to manipulate the state. This enables the concrete interface without templating.

Since some vertex finders use other vertex finders as seeders, this also allows to have the seed finder property use this IVertexFinder interface.

I did not observe CPU performance degradation as documented in #2842.

Part of:

Blocked by:

@paulgessinger paulgessinger added this to the v33.0.0 milestone Feb 14, 2024
@paulgessinger paulgessinger added the 🛑 blocked This item is blocked by another item label Feb 14, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module Vertexing labels Feb 14, 2024
@paulgessinger paulgessinger force-pushed the refactor/vtx-templates-part7-ivertexfinder branch from 78a84d4 to 70455c4 Compare February 14, 2024 15:58
@paulgessinger paulgessinger force-pushed the refactor/vtx-templates-part7-ivertexfinder branch from 70455c4 to e7b6dc2 Compare February 14, 2024 16:07
Copy link

codecov bot commented Feb 14, 2024

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (0c753b1) 48.85% compared to head (ff46291) 48.82%.

Files Patch % Lines
...e/include/Acts/Vertexing/IterativeVertexFinder.ipp 47.36% 3 Missing and 7 partials ⚠️
...e/include/Acts/Vertexing/IterativeVertexFinder.hpp 20.00% 5 Missing and 3 partials ⚠️
...clude/Acts/Vertexing/AdaptiveMultiVertexFinder.hpp 42.85% 2 Missing and 2 partials ⚠️
...clude/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp 77.77% 0 Missing and 4 partials ⚠️
Core/include/Acts/Vertexing/ZScanVertexFinder.hpp 33.33% 1 Missing and 1 partial ⚠️
...Acts/Vertexing/AdaptiveGridDensityVertexFinder.hpp 83.33% 0 Missing and 1 partial ⚠️
...Acts/Vertexing/AdaptiveGridDensityVertexFinder.ipp 66.66% 0 Missing and 1 partial ⚠️
...include/Acts/Vertexing/FullBilloirVertexFitter.ipp 50.00% 0 Missing and 1 partial ⚠️
...include/Acts/Vertexing/GridDensityVertexFinder.hpp 83.33% 0 Missing and 1 partial ⚠️
...include/Acts/Vertexing/GridDensityVertexFinder.ipp 66.66% 0 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2948      +/-   ##
==========================================
- Coverage   48.85%   48.82%   -0.03%     
==========================================
  Files         495      496       +1     
  Lines       28906    28938      +32     
  Branches    13731    13751      +20     
==========================================
+ Hits        14121    14130       +9     
- Misses       4899     4909      +10     
- Partials     9886     9899      +13     

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

@paulgessinger paulgessinger force-pushed the refactor/vtx-templates-part7-ivertexfinder branch from e7b6dc2 to 01fd8e5 Compare February 14, 2024 16:52
@paulgessinger paulgessinger force-pushed the refactor/vtx-templates-part7-ivertexfinder branch 2 times, most recently from 23b8dc4 to 8a21b11 Compare February 15, 2024 14:16
@paulgessinger paulgessinger force-pushed the refactor/vtx-templates-part7-ivertexfinder branch from 80b258f to dee393a Compare February 16, 2024 08:56
@paulgessinger paulgessinger marked this pull request as ready for review February 16, 2024 08:56
@paulgessinger paulgessinger removed the 🛑 blocked This item is blocked by another item label Feb 16, 2024
@paulgessinger
Copy link
Member Author

@andiwand this is ready now.

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.

Left a few general comments. I am definitely in favor of removing such templated interfaces as I do not think we gain any runtime performance from it. On the other hand we lose time during compilation and non-templated code is easier to reason about IMO.

I wonder how far we are from actually using virtual inheritance and keeping the state inside the algorithm. We could even use a templated frontend to keep the ACTS state / algorithm separation intact.

I stumbled across this vertex seed finder and vertex finder interface a few weeks ago and it did not really make sense to me. I think these should be two different interfaces but maybe I am missing something.

Core/include/Acts/Vertexing/IVertexFinder.hpp Show resolved Hide resolved
Core/include/Acts/Vertexing/IVertexFinder.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Vertexing/IVertexFinder.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Vertexing/IVertexFinder.hpp Outdated Show resolved Hide resolved
Core/src/Vertexing/KalmanVertexUpdater.cpp Outdated Show resolved Hide resolved
@paulgessinger
Copy link
Member Author

@andiwand The main argument against internal state is thread-safety, in my opinion. This separation defines well which parts are mutable and which ones are not, and we don't need to create / recreate the tools frequently.

@paulgessinger paulgessinger force-pushed the refactor/vtx-templates-part7-ivertexfinder branch 2 times, most recently from 38464ac to 29213f8 Compare February 16, 2024 15:03
andiwand
andiwand previously approved these changes Feb 17, 2024
Core/include/Acts/Vertexing/IVertexFinder.hpp Show resolved Hide resolved
Core/include/Acts/Vertexing/GridDensityVertexFinder.hpp Outdated Show resolved Hide resolved
@paulgessinger
Copy link
Member Author

I seem to have broken physmon with the updates I added.

andiwand
andiwand previously approved these changes Feb 19, 2024
@kodiakhq kodiakhq bot merged commit 84c039f into acts-project:main Feb 19, 2024
54 checks passed
@paulgessinger paulgessinger deleted the refactor/vtx-templates-part7-ivertexfinder branch February 19, 2024 10:37
kodiakhq bot pushed a commit that referenced this pull request Feb 19, 2024
This changes the interface found in `KalmanVertexUpdater` and `KalmanVertexTrackUpdater` to handling the dispatch into 3 and 4 dimensions at runtime inside their respective compilation units.

Part of:
- #2842 

Blocked by:
- #2948
@paulgessinger paulgessinger added the Breaks Athena build This PR breaks the Athena build label Mar 6, 2024
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…2948)

This PR introduces a new `IVertexFinder` with an interface that can support the existing vertex finder implementations, with the exception of the seed vertex finder (which depends on specific spacepoint triplets).

The interface introduces a type-erased state holder that the client of the finder initializes via the `makeState` interface function, and which the finder implementation downcasts to manipulate the state. This enables the concrete interface without templating.

Since some vertex finders use *other* vertex finders as seeders, this also allows to have the seed finder property use this `IVertexFinder` interface.

I did not observe CPU performance degradation as documented in acts-project#2842.

Part of:
- acts-project#2842 

Blocked by:
- acts-project#2946
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…#2955)

This changes the interface found in `KalmanVertexUpdater` and `KalmanVertexTrackUpdater` to handling the dispatch into 3 and 4 dimensions at runtime inside their respective compilation units.

Part of:
- acts-project#2842 

Blocked by:
- acts-project#2948
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…2948)

This PR introduces a new `IVertexFinder` with an interface that can support the existing vertex finder implementations, with the exception of the seed vertex finder (which depends on specific spacepoint triplets).

The interface introduces a type-erased state holder that the client of the finder initializes via the `makeState` interface function, and which the finder implementation downcasts to manipulate the state. This enables the concrete interface without templating.

Since some vertex finders use *other* vertex finders as seeders, this also allows to have the seed finder property use this `IVertexFinder` interface.

I did not observe CPU performance degradation as documented in acts-project#2842.

Part of:
- acts-project#2842 

Blocked by:
- acts-project#2946
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…#2955)

This changes the interface found in `KalmanVertexUpdater` and `KalmanVertexTrackUpdater` to handling the dispatch into 3 and 4 dimensions at runtime inside their respective compilation units.

Part of:
- acts-project#2842 

Blocked by:
- acts-project#2948
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaks Athena build This PR breaks the Athena build Component - Core Affects the Core module Component - Examples Affects the Examples module Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants