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

fix!: helixCut is renamed to helixCutTolerance and moved to SeedFinderOptions #2653

Merged
merged 11 commits into from
Nov 16, 2023

Conversation

LuisFelipeCoelho
Copy link
Member

@LuisFelipeCoelho LuisFelipeCoelho commented Nov 13, 2023

Following up on PR #2472 I have:

  • Changed the name helixCut to helixCutTolerance
  • Moved helixCutTolerance to SeedFinderOptions
  • Added it to Orthogonal config and SeedFinderFTFConfig

BREAKING CHANGE:

The option helixCut is renamed to helixCutTolerance and moved to SeedFinderOptions

@github-actions github-actions bot added Component - Core Affects the Core module Seeding labels Nov 13, 2023
@LuisFelipeCoelho LuisFelipeCoelho changed the title fix: fix helixCut tolerance parameter fix: Fix helixCut tolerance parameter Nov 13, 2023
Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ebb7d40) 49.73% compared to head (e46a6a8) 49.73%.

Files Patch % Lines
Core/include/Acts/Seeding/SeedFinder.ipp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2653   +/-   ##
=======================================
  Coverage   49.73%   49.73%           
=======================================
  Files         474      474           
  Lines       26872    26872           
  Branches    12364    12364           
=======================================
  Hits        13366    13366           
  Misses       4702     4702           
  Partials     8804     8804           

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

Core/include/Acts/Seeding/SeedFinderConfig.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/SeedFinderFTFConfig.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/SeedFinderOrthogonalConfig.hpp Outdated Show resolved Hide resolved
LuisFelipeCoelho and others added 4 commits November 13, 2023 15:02
Co-authored-by: Alexander J. Pfleger <70842573+AJPfleger@users.noreply.github.com>
Co-authored-by: Alexander J. Pfleger <70842573+AJPfleger@users.noreply.github.com>
Co-authored-by: Alexander J. Pfleger <70842573+AJPfleger@users.noreply.github.com>
AJPfleger
AJPfleger previously approved these changes Nov 13, 2023
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.

I don't see the removed config parameter in the diff. Where was this before?
Also I suppose this is a breaking change?

EDIT: indeed the helixCut parameter is still there. Is this intentional?

@acts-policybot acts-policybot bot dismissed AJPfleger’s stale review November 13, 2023 15:37

Invalidated by push of d8b0be2

@LuisFelipeCoelho LuisFelipeCoelho changed the title fix: Fix helixCut tolerance parameter fix!: Fix helixCut tolerance parameter Nov 13, 2023
@LuisFelipeCoelho
Copy link
Member Author

I don't see the removed config parameter in the diff. Where was this before? Also I suppose this is a breaking change?

EDIT: indeed the helixCut parameter is still there. Is this intentional?

Thank you @paulgessinger. I removed the parameter now.

Yes, I guess this is breaking change because of the rename.

@paulgessinger paulgessinger added this to the v31.0.0 milestone Nov 13, 2023
@LuisFelipeCoelho LuisFelipeCoelho changed the title fix!: Fix helixCut tolerance parameter fix!: helixCut is renamed to helixCutTolerance and moved to SeedFinderOptions Nov 15, 2023
@CarloVarni
Copy link
Collaborator

just to comment that in Athena we are currently not manually setting helixCut so this requires no change from the Athena side, despite being a breaking change

@kodiakhq kodiakhq bot merged commit 07732c3 into acts-project:main Nov 16, 2023
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Seeding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants