refactor!: seeding always sorts SPs #1910
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LuisFelipeCoelho
added
Component - Core
📊 Physics performance monitoring for 5d0dfc5Full report VertexingVertexing vs. muIVF seededIVF truth_smearedIVF truth_estimatedIVF orthogonalSeedingSeeding seededSeeding truth_estimatedSeeding orthogonalCKFCKF seededCKF truth_smearedCKF truth_estimatedCKF orthogonalAmbiguity resolutionseededTruth tracking (Kalman Filter)Truth trackingTruth tracking (GSF)Truth tracking |
Codecov Report
@@ Coverage Diff @@
## main #1910 +/- ##
==========================================
+ Coverage 49.53% 49.55% +0.02%
==========================================
Files 408 408
Lines 22705 22695 -10
Branches 10362 10356 -6
==========================================
Hits 11247 11247
+ Misses 4249 4239 -10
Partials 7209 7209
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
CarloVarni
reviewed
Mar 2, 2023
CarloVarni
reviewed
Mar 3, 2023
CarloVarni
reviewed
Mar 3, 2023
|
It doesn't seem to decrease the performance so I updated the root hashes |
CarloVarni
approved these changes
Mar 7, 2023
paulgessinger
changed the title
paulgessinger
added a commit
to paulgessinger/acts
that referenced
this pull request
Mar 10, 2023
This reverts commit c942f23.
kodiakhq bot
pushed a commit
that referenced
this pull request
Mar 14, 2023
…ms and tops (#1926) This is build on top of #1919 and also assumes the space points in the grid bins are already sorted in radius. Thus #1910 is required as well Idea here is that in a tt-bar sample the grid bins may be very populated. However, the grid collects space points according to their z-phi values, with no consideration for the r-coordinate. While looking for compatible bottoms and tops candidates we iterate on all the space points in the selected grid bins and this is a huge waste of time since just a few percentage of the space points are to be considerate. Of course, we check the distance of the candidate w.r.t. the middle space point... yet it is a O(N) operation. With this change I'm introducing here (excluding what's already in #1919) I first performs a binary search on the space points candidates to identify those in the proper radius range. This should be a O(logN) operation. And only then I run on them. Since `getCompatibleDoublets` is one of the main contributors in the seeding time, this may improve the CPU performance
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sorting has been shown to be less expensive then looping over all elements to select bottom and top sps for seeds. By sorting the SPs, we can avoid a considerable number of iterations, especially for high pile-up.
This PR removes the option to not sort the SPs during the seeding algorithm. @noemina