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.

Sign up for GitHub

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

Jump to bottom

refactor!: Make BoundaryCheck constructor from bool explicit #2592

Merged
merged 11 commits into from
Nov 4, 2023
Merged

refactor!: Make BoundaryCheck constructor from bool explicit #2592

merged 11 commits into from
Nov 4, 2023

Conversation

andiwand
Copy link
Contributor

This increases readability IMO

@andiwand andiwand added this to the next milestone Oct 27, 2023
@paulgessinger
Copy link
Member

Yes please! CI fails at the moment, however.

@codecov
Copy link

codecov bot commented Oct 28, 2023 •
edited
Loading

Codecov Report

Merging #2592 (6d1da76) into main (c578923) will increase coverage by 0.00%.
The diff coverage is 26.00%.

@@           Coverage Diff           @@
##             main    #2592   +/-   ##
=======================================
  Coverage   49.50%   49.50%           
=======================================
  Files         473      473           
  Lines       26746    26770   +24     
  Branches    12338    12349   +11     
=======================================
+ Hits        13240    13253   +13     
- Misses       4753     4754    +1     
- Partials     8753     8763   +10
Files Coverage Δ
Core/include/Acts/Geometry/Layer.hpp 100.00% <ø> (ø)
Core/include/Acts/Geometry/NavigationLayer.hpp 84.61% <ø> (ø)
Core/include/Acts/Navigation/DetectorNavigator.hpp 52.14% <100.00%> (+0.69%) ⬆️
...include/Acts/Navigation/NavigationStateFillers.hpp 85.71% <ø> (ø)
Core/include/Acts/Surfaces/BoundaryCheck.hpp 48.14% <ø> (ø)
Core/include/Acts/Surfaces/ConeBounds.hpp 71.42% <ø> (ø)
Core/include/Acts/Surfaces/ConeSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/CylinderBounds.hpp 68.75% <ø> (ø)
Core/include/Acts/Surfaces/CylinderSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/DiscSurface.hpp 100.00% <ø> (ø)
... and 21 more

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@andiwand andiwand changed the title refactor: Make BoundaryCheck constructor from bool explicit refactor!: Make BoundaryCheck constructor from bool explicit Oct 28, 2023
@andiwand
Copy link
Contributor Author

this is running through now but I guess it is a breaking change?

@github-actions
Copy link

github-actions bot commented Oct 28, 2023 •
edited
Loading

📊: Physics performance monitoring for 6d1da76

Full contents

physmon summary

@andiwand andiwand modified the milestones: next, v31.0.0 Oct 28, 2023
@paulgessinger paulgessinger added the Breaking change This change breaks backwards compatibility label Nov 2, 2023
paulgessinger
paulgessinger previously approved these changes Nov 2, 2023
View reviewed changes
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.

Diff looks clean to me! 🎉

paulgessinger
paulgessinger previously approved these changes Nov 3, 2023
View reviewed changes
paulgessinger
paulgessinger previously approved these changes Nov 3, 2023
View reviewed changes
@acts-policybot acts-policybot bot dismissed paulgessinger’s stale review November 3, 2023 22:20

Invalidated by push of 7aeb762

@paulgessinger paulgessinger merged commit 2697f5a into acts-project:main Nov 4, 2023
52 of 53 checks passed
@acts-project-service
Copy link
Collaborator

acts-project-service commented Nov 4, 2023 •
edited
Loading

🔴 Athena integration test results [2697f5a]

Build job with this PR failed!

Please investigate the build job for the pipeline!

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Nov 4, 2023
@andiwand andiwand deleted the refactor-explicit-boundary-check branch November 4, 2023 12:45
@paulgessinger paulgessinger removed the Breaks Athena build This PR breaks the Athena build label Nov 16, 2023
@andiwand andiwand mentioned this pull request Jan 10, 2024
Merged
kodiakhq bot pushed a commit that referenced this pull request Jan 16, 2024
@andiwand
refactor: Remove BoundaryCheck to bool conversion (#2860)
a0d11fb 
After removing the from bool conversion #2592 I would also like to remove the to bool conversion from `BoundaryCheck` as this class is describing more than just a flag which can be shown by being more explicit
LaraCalic pushed a commit to LaraCalic/acts that referenced this pull request Feb 10, 2024
@andiwand @LaraCalic
refactor: Remove BoundaryCheck to bool conversion (acts-project#2860)
a76811e 
After removing the from bool conversion acts-project#2592 I would also like to remove the to bool conversion from `BoundaryCheck` as this class is describing more than just a flag which can be shown by being more explicit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulgessinger paulgessinger paulgessinger left review comments

Assignees
No one assigned
Labels
Breaking change This change breaks backwards compatibility Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Event Data Model Track Finding Track Fitting Vertexing
Projects
None yet
Milestone
v31.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@andiwand @paulgessinger @acts-project-service