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!: Move and Grid Density finders to cpp #2973

Conversation

paulgessinger
Copy link
Member

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:

Blocked by:

@paulgessinger paulgessinger added the 🛑 blocked This item is blocked by another item label Feb 21, 2024
@paulgessinger paulgessinger added this to the v33.0.0 milestone Feb 21, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Vertexing labels Feb 21, 2024
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

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

Project coverage is 48.70%. Comparing base (6a25545) to head (ffac673).

Files Patch % Lines
Core/src/Vertexing/GaussianGridTrackDensity.cpp 75.86% 0 Missing and 7 partials ⚠️
...nclude/Acts/Vertexing/GaussianGridTrackDensity.hpp 45.45% 4 Missing and 2 partials ⚠️
Core/src/Vertexing/GridDensityVertexFinder.cpp 50.00% 0 Missing and 2 partials ⚠️
...include/Acts/Vertexing/GridDensityVertexFinder.hpp 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2973      +/-   ##
==========================================
- Coverage   48.72%   48.70%   -0.03%     
==========================================
  Files         493      493              
  Lines       28968    28976       +8     
  Branches    13790    13798       +8     
==========================================
- Hits        14114    14112       -2     
- Misses       4930     4935       +5     
- Partials     9924     9929       +5     

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

@paulgessinger paulgessinger force-pushed the refactor/vtx-templates-part12-density-finders branch 2 times, most recently from 1fdd342 to 7216275 Compare February 21, 2024 16:34
@paulgessinger paulgessinger force-pushed the refactor/vtx-templates-part12-density-finders branch from 7216275 to 7ea5362 Compare February 27, 2024 15:38
@paulgessinger paulgessinger marked this pull request as ready for review February 27, 2024 15:43
@paulgessinger paulgessinger removed the 🛑 blocked This item is blocked by another item label Feb 27, 2024
@paulgessinger
Copy link
Member Author

paulgessinger commented Feb 27, 2024

Now this has some changes that we might want to discuss @andiwand: The grid size becomes a runtime parameter, which is taken from the configuration.

andiwand
andiwand previously approved these changes Feb 28, 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.

making this is runtime config makes total sense to me. no clue why this was a template parameter in the first place

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

Should be ready @andiwand.

@kodiakhq kodiakhq bot merged commit 5eb39c3 into acts-project:main Feb 28, 2024
54 checks passed
@paulgessinger paulgessinger deleted the refactor/vtx-templates-part12-density-finders branch February 29, 2024 22:18
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 

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

Successfully merging this pull request may close these issues.

3 participants