-
Notifications
You must be signed in to change notification settings - Fork 178
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: Deduplicate some code in SingleBoundTrackParameters #2104
refactor: Deduplicate some code in SingleBoundTrackParameters #2104
Conversation
andiwand
commented
May 6, 2023
- deduplicate some code
- add local param accessor
Codecov Report
@@ Coverage Diff @@
## main #2104 +/- ##
==========================================
+ Coverage 49.41% 49.43% +0.01%
==========================================
Files 434 434
Lines 24985 24981 -4
Branches 11522 11515 -7
==========================================
+ Hits 12347 12350 +3
Misses 4486 4486
+ Partials 8152 8145 -7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
📊 Physics performance monitoring for a76acd2Full report VertexingSeedingCKFAmbiguity resolutionTruth tracking (Kalman Filter)Truth tracking (GSF) |
@@ -168,6 +168,8 @@ class SingleBoundTrackParameters { | |||
return m_params[kIndex]; | |||
} | |||
|
|||
/// Local spatial position two-vector. | |||
Vector2 localPosition() const { return m_params.head<2>(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vector2 localPosition() const { return m_params.head<2>(); } | |
Vector2 localPosition() const { return m_params.segment<2>(eBoundLoc0); } |
for consistency maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't .head<2>()
preferable because it knows the offset at compile time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm since eBoundLoc0
is a compile time constant, I would expect the compiler to be able to optimize this, but not sure of course.
However, isn't it kind of our style to use these enum
-values? https://acts.readthedocs.io/en/latest/codeguide.html#a-indices-always-use-enum-values-to-access-vector-matrix-components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so, but you're making the assumption that eLoc0
and eLoc1
are consecutive in any case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats true, we could also enforce this in TrackParameterization.hpp
similar to the free coordinates I think...
Co-authored-by: Benjamin Huth <37871400+benjaminhuth@users.noreply.github.com>