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!: ImpactPointEstimator moves to cpp file #2971

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Feb 21, 2024

This PR moves the code of the ImpactPointEstimator from the header into a compiled file. To allow this, I have to make some changes to the interface to remove templates.

In the process, I introduced Eigen::Map<ActsDynamicVector> as an argument to getDistanceAndMomentum and getVertexCompatibility.
Part of:

Blocked by:

Copy link

codecov bot commented Feb 21, 2024

Codecov Report

Attention: Patch coverage is 26.02740% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 48.72%. Comparing base (1cc1759) to head (630e743).

Files Patch % Lines
Core/src/Vertexing/ImpactPointEstimator.cpp 27.11% 8 Missing and 35 partials ⚠️
...re/include/Acts/Vertexing/ImpactPointEstimator.hpp 16.66% 1 Missing and 4 partials ⚠️
Core/src/Vertexing/AdaptiveMultiVertexFitter.cpp 25.00% 0 Missing and 3 partials ⚠️
Core/src/Vertexing/VertexingError.cpp 0.00% 2 Missing ⚠️
...clude/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2971      +/-   ##
==========================================
- Coverage   48.73%   48.72%   -0.02%     
==========================================
  Files         493      493              
  Lines       28943    28968      +25     
  Branches    13776    13790      +14     
==========================================
+ Hits        14106    14114       +8     
- Misses       4924     4930       +6     
- Partials     9913     9924      +11     

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

@paulgessinger paulgessinger added the 🛑 blocked This item is blocked by another item label Feb 21, 2024
@paulgessinger paulgessinger force-pushed the refactor/vtx-templates-part11-ipest-cpp branch from ab77ef0 to 9c485e6 Compare February 21, 2024 16:32
@paulgessinger paulgessinger removed the 🛑 blocked This item is blocked by another item label Feb 21, 2024
@paulgessinger paulgessinger marked this pull request as ready for review February 21, 2024 16:34
@paulgessinger paulgessinger force-pushed the refactor/vtx-templates-part11-ipest-cpp branch from 9c485e6 to 8e23310 Compare February 27, 2024 10:35
andiwand
andiwand previously approved these changes Feb 27, 2024
Core/src/Vertexing/ImpactPointEstimator.cpp Outdated Show resolved Hide resolved
Core/src/Vertexing/ImpactPointEstimator.cpp Show resolved Hide resolved
Core/src/Vertexing/ImpactPointEstimator.cpp Outdated Show resolved Hide resolved
Core/src/Vertexing/ImpactPointEstimator.cpp Outdated Show resolved Hide resolved
@kodiakhq kodiakhq bot merged commit 554afbd into acts-project:main Feb 27, 2024
56 checks passed
@acts-project-service
Copy link
Collaborator

🔴 Athena integration test results [554afbd]

Build job with this PR failed!

Please investigate the build job for the pipeline!

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Feb 27, 2024
@paulgessinger paulgessinger deleted the refactor/vtx-templates-part11-ipest-cpp branch February 27, 2024 15:54
kodiakhq bot pushed a commit that referenced this pull request Feb 28, 2024
This PR moves 

- `GaussianGridTrackDensity`
- `GridDensityVertexFinder`

to cpp files. It has to refactor the interface a bit to make this possible, mostly, the grid sizes become runtime arguments instead of compile-time parameters. This then also changes the internal grid eigen objects to be dynamic, which I think should be fine because we essentially only do lookups on them.

Part of:
- #2842

Blocked by:
- #2971
@paulgessinger paulgessinger removed the Breaks Athena build This PR breaks the Athena build label Mar 6, 2024
@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
This PR moves the code of the `ImpactPointEstimator` from the header into a compiled file. To allow this, I have to make some changes to the interface to remove templates.

In the process, I introduced `Eigen::Map<ActsDynamicVector>` as an argument to `getDistanceAndMomentum` and `getVertexCompatibility`.
Part of:
- acts-project#2842 

Blocked by:
- acts-project#2953
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
This PR moves 

- `GaussianGridTrackDensity`
- `GridDensityVertexFinder`

to cpp files. It has to refactor the interface a bit to make this possible, mostly, the grid sizes become runtime arguments instead of compile-time parameters. This then also changes the internal grid eigen objects to be dynamic, which I think should be fine because we essentially only do lookups on them.

Part of:
- acts-project#2842

Blocked by:
- acts-project#2971
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
This PR moves the code of the `ImpactPointEstimator` from the header into a compiled file. To allow this, I have to make some changes to the interface to remove templates.

In the process, I introduced `Eigen::Map<ActsDynamicVector>` as an argument to `getDistanceAndMomentum` and `getVertexCompatibility`.
Part of:
- acts-project#2842 

Blocked by:
- acts-project#2953
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
This PR moves 

- `GaussianGridTrackDensity`
- `GridDensityVertexFinder`

to cpp files. It has to refactor the interface a bit to make this possible, mostly, the grid sizes become runtime arguments instead of compile-time parameters. This then also changes the internal grid eigen objects to be dynamic, which I think should be fine because we essentially only do lookups on them.

Part of:
- acts-project#2842

Blocked by:
- acts-project#2971
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 Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants