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: Curvature sorting in SeedFilter #1168

Merged
merged 12 commits into from
Mar 23, 2022

Conversation

LuisFelipeCoelho
Copy link
Member

This PR introduces an option to sort the Seed by curvature in the SeedFilter.

If the boolean curvatureSortingInFilter is true, the seeds will be sorted by curvature and if a seed has a curvature greater that the maximum (upperLimitCurv) it will break out of loop.

curvatureSortingInFilter is set to false by default.

@noemina @robertlangenberg @paulgessinger

@LuisFelipeCoelho LuisFelipeCoelho added this to the next milestone Feb 17, 2022
@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #1168 (fff55a8) into main (c9a5b53) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1168      +/-   ##
==========================================
- Coverage   47.77%   47.75%   -0.03%     
==========================================
  Files         360      360              
  Lines       18608    18616       +8     
  Branches     8770     8774       +4     
==========================================
  Hits         8890     8890              
- Misses       3666     3674       +8     
  Partials     6052     6052              
Impacted Files Coverage Δ
Core/include/Acts/Seeding/SeedFilter.ipp 0.00% <0.00%> (ø)
Core/include/Acts/Seeding/SeedFilterConfig.hpp 0.00% <ø> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@robertlangenberg
Copy link
Contributor

delete comments in SeedFilter.ipp line 70 & 71
// TODO: how much slower than sorting all vectors by curvature
// and breaking out of loop? i.e. is vector size large (e.g. in jets?)

as this PR addresses this :)

Did you test if this is faster respectively not significantly slower for small topSP vectors? If so, no need to make it conditional on curvatureSortingInFilter, and just always run it.

kodiakhq bot pushed a commit that referenced this pull request Mar 18, 2022
This PR tries to fix usage of mutable properties in #1168 and #1143.

Tagging: @paulgessinger @robertlangenberg @LuisFelipeCoelho 

BREAKING CHANGE: Remove assumption on constness of InternalSpacePoint. 
The need of this major change is to allow seed finding and filtering to evaluate and set properties of InternalSpacePoints as soon as they are candidates for triplet formation and seed quality evaluation.
Copy link
Contributor

@robertlangenberg robertlangenberg left a comment

Choose a reason for hiding this comment

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

Hey, looks good! Did you do tests if this speeds up performance? If not or if tests weren't showing it's speeding up or maintaining performance in +- all cases, we should probably just push this in. If you have tests that DO say it's always beneficial, we can also get rid of the switch curvatureSortingInFilter.

@robertlangenberg robertlangenberg added Feature Development to integrate a new feature automerge labels Mar 22, 2022
@LuisFelipeCoelho
Copy link
Member Author

LuisFelipeCoelho commented Mar 22, 2022

@robertlangenberg these plots compare the time per event with and without the curvature sorting. In the first one, I ran the seeding 100 times for 500 single muon events (100 GeV) and the second one 100 times for 20 ttbar events (14 TeV).
I used the single muon events to check for small topSP vector.

The performance seems to be the same.

compare_curvature_sorting_100MeV_singleMu_500evts
compare_curvature_sorting_ttbar_20evts

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Only a clang format change after @robertlangenberg's approval, so let's merge.

@kodiakhq kodiakhq bot merged commit 5e52dbc into acts-project:main Mar 23, 2022
@paulgessinger paulgessinger modified the milestones: next, v18.0.0 Apr 4, 2022
@LuisFelipeCoelho LuisFelipeCoelho deleted the add-sorting-filter branch November 10, 2022 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Feature Development to integrate a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants