-
Notifications
You must be signed in to change notification settings - Fork 177
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
More AMVF performance upgrades #277
More AMVF performance upgrades #277
Conversation
Somebody wants to review this small PR, @asalzburger, @robertlangenberg ? |
Did you do a proper test with >100 vertices to see if the reduction in physics performance is negligible? |
Yes, I checked on 300 events (i.e. 10536 vertices). The average time for running the AMVF decreased from 124ms to 120ms and the resulting vertex x,y,z positions, covariance matrices and number of tracks match 100%. |
Codecov Report
@@ Coverage Diff @@
## master #277 +/- ##
==========================================
+ Coverage 44.84% 48.38% +3.54%
==========================================
Files 376 318 -58
Lines 18866 16357 -2509
Branches 8970 7580 -1390
==========================================
- Hits 8461 7915 -546
+ Misses 4905 3180 -1725
+ Partials 5500 5262 -238
Continue to review full report at Codecov.
|
Just to clarify: The part 1. / std::tan(th) * (X * std::cos(phi) + Y * std::sin(phi)) is basically always |
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.
As discussed, removed part never exceeds 10e-16 in test with 10000 vertices, shouldn't influence end result.
* replace estimateDeltaZ method in AMVF by simple z0 diff check * replace z0 diff check by z position diff check in AMVF * fix format
This PR removes the
estimateDeltaZ
function (which contributed 5.5% to the overall AMVF timing, see below) and replaces it with a simple z position diff check which is way less expensive and gives the same outputs.