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: use map instead of two vectors in AdaptiveGridTrackDensity #2338

Merged
merged 21 commits into from
Aug 14, 2023

Conversation

felix-russo
Copy link
Contributor

@felix-russo felix-russo commented Aug 2, 2023

Previously:
We used a vector containing z bins and a separate vector containing the corresponding density values.

Now:
We use a map relating
z bin -> density value

Future:
We will use a map relating
(z bin, time bin) -> density value
to include time information in the vertex finding.

@github-actions github-actions bot added Component - Core Affects the Core module Vertexing labels Aug 2, 2023
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Merging #2338 (9dd1a00) into main (7ae364d) will decrease coverage by 0.06%.
The diff coverage is 52.74%.

@@            Coverage Diff             @@
##             main    #2338      +/-   ##
==========================================
- Coverage   49.63%   49.57%   -0.06%     
==========================================
  Files         453      453              
  Lines       25530    25511      -19     
  Branches    11705    11706       +1     
==========================================
- Hits        12672    12648      -24     
  Misses       4579     4579              
- Partials     8279     8284       +5     
Files Changed Coverage Δ
...Acts/Vertexing/AdaptiveGridDensityVertexFinder.hpp 100.00% <ø> (ø)
...nclude/Acts/Vertexing/AdaptiveGridTrackDensity.hpp 100.00% <ø> (ø)
...Acts/Vertexing/AdaptiveGridDensityVertexFinder.ipp 50.63% <30.00%> (-2.38%) ⬇️
...nclude/Acts/Vertexing/AdaptiveGridTrackDensity.ipp 62.40% <55.55%> (-7.19%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@felix-russo felix-russo marked this pull request as ready for review August 3, 2023 08:21
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.

one general comment: it might be worth looking at flat implementations like the one from boost if this is performance critical

@felix-russo
Copy link
Contributor Author

felix-russo commented Aug 11, 2023

one general comment: it might be worth looking at flat implementations like the one from boost if this is performance critical

Yes, @paulgessinger made the same remark in the last stand-up meeting!

We agreed to the following:
I will first check if the physics performance improves after adding the time vertex seeding. Once the physmon looks good, I will investigate the CPU performance of the different implementations (i.e., unordered_map and Eigen SparseMatrix). Does this sound sensible?

Copy link
Contributor

@baschlag baschlag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@paulgessinger paulgessinger added this to the next milestone Aug 14, 2023
@kodiakhq kodiakhq bot merged commit f96b048 into acts-project:main Aug 14, 2023
55 checks passed
@paulgessinger paulgessinger modified the milestones: next, v28.2.0 Aug 17, 2023
@felix-russo felix-russo deleted the map-adaptive-grid-density branch August 30, 2023 12:43
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.

None yet

5 participants