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: Hashing seeding algorithm #3148

Merged
merged 31 commits into from
Aug 30, 2024
Merged

Conversation

CouthuresJeremy
Copy link
Contributor

@CouthuresJeremy CouthuresJeremy commented Apr 25, 2024

Adds the Hashing for the seeding algorithm.

Instead of doing the seeding on every space points at once, this approach create small groups of space points called buckets and do the standard seeding on each of those buckets independently.

As there is overlaps between the buckets, the same seed might be reconstructed several times (due to independent reconstruction in several buckets). A set container of seeds is used to naturally handle this duplication.

The SeedingAlgorithmHashing example provided can be seen as a generalization of the standard SeedingAlgorithm. A unique bucket containing all space points of the events is expected to give strictly the same seeds than the SeedingAlgorithm on the event as the SeedingAlgorithmHashing behave like a wrapper around the standard algorithm.

This approach mitigate the filtering of good seeds due to an upper limit on the number of seeds sharing the same middle space point (the maxSeedsPerSpM parameter).

More details are in the poster presented on CTD 2023.

A third party software called Annoy is used to create the buckets. Some modifications of the software code has been done with respect to the official repository linked previously.

@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Component - Documentation Affects the documentation Event Data Model Seeding Track Finding labels Apr 25, 2024
Copy link

github-actions bot commented Apr 25, 2024

📊: Physics performance monitoring for 3eafd39

Full contents

physmon summary

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.

A few early general comments

Core/include/Acts/Seeding/Hashing/Annoylib.hpp Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/Hashing/HashingAnnoy.ipp Outdated Show resolved Hide resolved
@AJPfleger AJPfleger added this to the next milestone Apr 26, 2024
@CouthuresJeremy CouthuresJeremy force-pushed the Hashing branch 2 times, most recently from e05b5bd to d5b7f83 Compare May 3, 2024 14:58
@krasznaa
Copy link
Member

Jeremy pinged me privately on it, but I thought it would make more sense to answer here, publicly.

What you forgot about updating / extending is this file: https://github.com/acts-project/acts/blob/main/cmake/ActsConfig.cmake.in

You are now introducing a new external, which ActsCore would now publicly depend on. (Which in itself is a bit worrisome to me, but that's a separate issue...) So you must make sure that when somebody calls find_package(Acts), ActsConfig.cmake would call find_dependency(Annoy) itself.

Since in this PR's setup Annoy would always be needed, you should add

find_dependency(Annoy)

without any if(...) statements around here: https://github.com/acts-project/acts/blob/main/cmake/ActsConfig.cmake.in#L55

Actually, the logic of only calling find_dependency(Boost) and find_dependency(Eigen3) is not great over there. Since even if Acts was the one building / installing those, the Acts installation should still include everything required to make those find_dependency(...) calls work. 🤔 (So they should always be called.)

But that's a separate issue. For this PR, just add find_dependency(Annoy ...) with all the right arguments.

@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label May 28, 2024
Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 47.43%. Comparing base (75ef913) to head (857e6f1).
Report is 3 commits behind head on main.

Files Patch % Lines
Core/include/Acts/Seeding/ContainerPolicy.hpp 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3148      +/-   ##
==========================================
- Coverage   47.66%   47.43%   -0.23%     
==========================================
  Files         509      511       +2     
  Lines       29425    30053     +628     
  Branches    14131    14561     +430     
==========================================
+ Hits        14026    14257     +231     
- Misses       5285     5326      +41     
- Partials    10114    10470     +356     

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

@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... and removed Infrastructure Changes to build tools, continous integration, ... labels Jun 10, 2024
@CouthuresJeremy CouthuresJeremy marked this pull request as ready for review July 9, 2024 13:18
@andiwand andiwand added automerge and removed 🛑 blocked This item is blocked by another item labels Aug 30, 2024
Copy link

sonarcloud bot commented Aug 30, 2024

@kodiakhq kodiakhq bot merged commit 2e0a7d1 into acts-project:main Aug 30, 2024
42 checks passed
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Aug 30, 2024
@andiwand andiwand modified the milestones: next, v36.3.0 Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Documentation Affects the documentation Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Fails Athena tests This PR causes a failure in the Athena tests Infrastructure Changes to build tools, continous integration, ... Track Finding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants