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

refactor!: Consistent naming for direction #2343

Merged
merged 8 commits into from
Aug 17, 2023

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Aug 3, 2023

@andiwand andiwand added this to the v29.0.0 milestone Aug 3, 2023
@github-actions github-actions bot added Component - Core Affects the Core module Component - Fatras Affects the Fatras module Component - Examples Affects the Examples module Event Data Model Vertexing Track Fitting labels Aug 3, 2023
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #2343 (a4c90ff) into main (49de0d3) will not change coverage.
The diff coverage is 40.00%.

@@           Coverage Diff           @@
##             main    #2343   +/-   ##
=======================================
  Coverage   49.57%   49.57%           
=======================================
  Files         453      453           
  Lines       25512    25512           
  Branches    11706    11706           
=======================================
  Hits        12648    12648           
  Misses       4580     4580           
  Partials     8284     8284           
Files Changed Coverage Δ
...ts/EventData/GenericCurvilinearTrackParameters.hpp 44.00% <0.00%> (ø)
Core/include/Acts/EventData/TrackProxy.hpp 78.70% <ø> (ø)
Core/include/Acts/Propagator/AtlasStepper.hpp 72.19% <ø> (ø)
Core/include/Acts/Propagator/EigenStepper.hpp 77.55% <0.00%> (ø)
.../include/Acts/Propagator/MultiEigenStepperLoop.ipp 40.62% <0.00%> (ø)
...re/include/Acts/Propagator/StraightLineStepper.hpp 73.52% <0.00%> (ø)
...re/include/Acts/TrackFitting/GaussianSumFitter.hpp 44.59% <ø> (ø)
.../include/Acts/Vertexing/HelicalTrackLinearizer.ipp 19.23% <ø> (ø)
...nclude/Acts/Vertexing/NumericalTrackLinearizer.ipp 16.36% <0.00%> (ø)
...ore/include/Acts/Visualization/EventDataView3D.hpp 48.78% <0.00%> (ø)
... and 6 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

We can do this if you want, but naively I don't think it's necessarily bad to communicate that these vectors are normalized.

@andiwand
Copy link
Contributor Author

andiwand commented Aug 4, 2023

I don't have a strong opinion on direction vs unitDirection but I think it should be consistent for track params, stepper and intersections. Happy to do it the other way by calling everything unitDirection

I guess if we opt for direction we could state somewhere that this always means unit direction across the codebase

Copy link
Contributor

@felix-russo felix-russo left a comment

Choose a reason for hiding this comment

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

I think the renaming is a good idea, since a direction is per definition a unit vector.
From Wikipedia:
image

@felix-russo
Copy link
Contributor

If this change alright for @paulgessinger we can merge

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 restarted the CI.

@andiwand
Copy link
Contributor Author

I would have kept this back for v29 because it might break Athena. what do you think @paulgessinger ?

@paulgessinger
Copy link
Member

You're right, I missed that. Let's hold it.

@kodiakhq kodiakhq bot merged commit 62ec2e1 into acts-project:main Aug 17, 2023
paulgessinger pushed a commit that referenced this pull request Aug 24, 2023
@andiwand andiwand deleted the consistent-naming-for-direction branch October 21, 2023 16:41
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 Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Event Data Model Track Fitting Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants