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: cut to the maximum value of delta z between SPs in seedFinder #1209

Merged

Conversation

LuisFelipeCoelho
Copy link
Member

This PR adds a cut to the maximum value of delta z between SPs in seedFinder. By default the cut is not applied since deltaZMax is set to numeric_limits<float>::max()

This cut will be used for the strip SPs in the ITk implementation but not for the pixel SPs

@LuisFelipeCoelho LuisFelipeCoelho added Component - Core Affects the Core module Improvement Changes to an existing feature Impact - Minor Nuissance bug and/or affects only a single module labels Mar 28, 2022
@LuisFelipeCoelho LuisFelipeCoelho added this to the next milestone Mar 28, 2022
@LuisFelipeCoelho LuisFelipeCoelho removed the Improvement Changes to an existing feature label Mar 28, 2022
@stephenswat stephenswat added the Improvement Changes to an existing feature label Mar 28, 2022
@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #1209 (c1d4d63) into main (9953615) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1209      +/-   ##
==========================================
- Coverage   47.92%   47.90%   -0.02%     
==========================================
  Files         371      371              
  Lines       19388    19395       +7     
  Branches     9133     9135       +2     
==========================================
  Hits         9291     9291              
- Misses       3766     3773       +7     
  Partials     6331     6331              
Impacted Files Coverage Δ
Core/include/Acts/Seeding/Seedfinder.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

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.

can go in as is, if you want you can change the deltaZMax default to infinity.

@kodiakhq
Copy link
Contributor

kodiakhq bot commented Mar 29, 2022

This PR currently has a merge conflict. Please resolve this and then re-add the automerge label.

Copy link
Member

@stephenswat stephenswat left a comment

Choose a reason for hiding this comment

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

Re-signing off on this. 😄

@paulgessinger
Copy link
Member

I think the CI failure here is codecov complaining that coverage goes down too much (0.2%).

Is there a reasonable way for you to add some additional test coverage @LuisFelipeCoelho ?

Otherwise I can override this and merge manually.

@paulgessinger
Copy link
Member

Not sure why the codecov check is still pending, let me manually merge this one then.

@paulgessinger paulgessinger merged commit 6ae8a95 into acts-project:main Mar 30, 2022
@LuisFelipeCoelho LuisFelipeCoelho deleted the deltaZMax-seedFinder branch March 30, 2022 11:55
@paulgessinger paulgessinger modified the milestones: next, v18.0.0 Apr 4, 2022
kodiakhq bot pushed a commit that referenced this pull request Oct 18, 2022
same cut included in default seeding #1209
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Oct 20, 2022
paulgessinger pushed a commit that referenced this pull request Oct 20, 2022
same cut included in default seeding #1209
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Component - Core Affects the Core module Impact - Minor Nuissance bug and/or affects only a single module Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants