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: Adding features for sorting space points in r in (phi, z) grid #1267

Merged
merged 3 commits into from
May 30, 2022

Conversation

noemina
Copy link
Contributor

@noemina noemina commented May 24, 2022

This PR includes two features for sorting space points in radius for each (phi, z) grid bin.

  • A new parameter, binSizeR, is added to the SeedFinderConfig in order to allow the user to configure the bin size in R. By default it stays 1 mm.
  • A new boolean, forceRadialSorting, is added to the SeedFinderConfig in order to allow the user to force the space point in each (phi, z) bin to be sorted in radius. This is false by default.

@noemina noemina added this to the next milestone May 24, 2022
@noemina
Copy link
Contributor Author

noemina commented May 24, 2022

FYI: @LuisFelipeCoelho

@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #1267 (08b929b) into main (a7ee09d) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1267      +/-   ##
==========================================
- Coverage   47.50%   47.48%   -0.02%     
==========================================
  Files         376      376              
  Lines       19810    19816       +6     
  Branches     9305     9308       +3     
==========================================
  Hits         9410     9410              
- Misses       4010     4016       +6     
  Partials     6390     6390              
Impacted Files Coverage Δ
Core/include/Acts/Seeding/BinnedSPGroup.ipp 0.00% <0.00%> (ø)
Core/include/Acts/Seeding/SeedfinderConfig.hpp 0.00% <0.00%> (ø)

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

@LuisFelipeCoelho
Copy link
Member

Good from my side

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.

is the sorting taken advantage of somewhere?

@noemina
Copy link
Contributor Author

noemina commented May 25, 2022

is the sorting taken advantage of somewhere?

We (ATLAS seeder) have use cases profiting of both options implemented in this PR.

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.

I'm confused.. I thought I had already commented a question where the sorting is used? was this a similar PR?

urgh never mind, the comments didn't show up for some reason.

@robertlangenberg robertlangenberg merged commit f77f377 into acts-project:main May 30, 2022
Scott-James-Hurley pushed a commit to Scott-James-Hurley/acts_profiling that referenced this pull request Jun 6, 2022
…cts-project#1267)

* adding features for sorting sp in r

* re-trigger CI

* re-trigger CI
@paulgessinger paulgessinger modified the milestones: next, v19.2.0 Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants