-
Notifications
You must be signed in to change notification settings - Fork 15
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
invpt sign fix and numerical precision improvements for high pt #334
invpt sign fix and numerical precision improvements for high pt #334
Conversation
…s to above 1 MeV (bad result protection); use float precision in invCos computation
nice! for curiosity: it looks like you implemented a few fixes... any idea of their relative importance? |
@@ -1309,6 +1309,17 @@ void MkFinder::BkFitFitTracksBH(const EventOfHits & eventofhits, | |||
Err[iP], Par[iP], msErr, msPar, Err[iC], Par[iC], tmp_chi2, N_proc); | |||
} | |||
|
|||
//fixup invpt sign and charge |
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.
Shouldn't we put this in KalmanOperation[Endcap], if (update-requested)?
At least it should also go in BkFitFitTracks() as this is the one I'm switching to for backward search --- I'd have to remember to also put it in there.
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 couldn't quickly figure out how to pass a non-const charge
|
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.
here are some notes from PropagationMPlex.optrpt.
It looks like in a combination, there is some improvement.
@@ -295,7 +295,7 @@ void helixAtRFromIterativeCCSFullJac(const MPlexLV& inPar, const MPlexQI& inChg, | |||
errorPropTmp(n,4,4) = 1.f; | |||
errorPropTmp(n,5,5) = 1.f; | |||
|
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.
in this loop
from optreport original ==> this pr
remark #15475: --- begin vector cost summary ---
remark #15476: scalar cost: 1190 ==> 1201
remark #15477: vector cost: 427.250 ==> 430.75
remark #15478: estimated potential speedup: 2.420 ==> 2.420
@@ -627,10 +630,11 @@ void helixAtZ(const MPlexLV& inPar, const MPlexQI& inChg, const MPlexQF &msZ, | |||
|
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.
in this loop
remark #15476: scalar cost: 1375 ==> 1124
remark #15477: vector cost: 144.370 ==> 121.870
remark #15478: estimated potential speedup: 8.470 ==> 8.720
remark #15482: vectorized math library calls: 3
remark #15486: divides: 16 ==> 9
remark #15487: type converts: 1
is apparently in a good direction
@@ -717,7 +729,7 @@ void applyMaterialEffects(const MPlexQF &hitsRl, const MPlexQF& hitsXi, const MP | |||
const float beta2 = p2/(p2+mpi2); |
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.
in this loop
remark #15476: scalar cost: 748 ==> 748
remark #15477: vector cost: 154.120 ==> 141.000
remark #15478: estimated potential speedup: 4.590 ==> 5.000
remark #15482: vectorized math library calls: 5
remark #15486: divides: 14
remark #15487: type converts: 3 ==> not mentioned
apparently 1. -> 1.f
mattered
I added some notes from ttbar MTV in the PR description. The vectorization report analysis in #334 (review) looks good as well. |
http://uaf-10.t2.ucsd.edu/~slava77/figs/mic/mtv/10muHS_CKF_vs_mkFit_310_absPt_77a7f37/plots_initialStep.html
has plots for the high-pt muon sample
initial step built tracks: recently merged v3.1.0 (red); fix to the invpt charge flip (black) and this PR as of 77a7f37 (orange)
It looks like some of the inner hits are recovered (I think that this is from the recovered tracks that make it all the way inside without failing to a bad state in the backward fit)
The pulls are better
phi resolution look better
.. however, something odd is happening in the same vs pt (it looks like the fits did not converge)
The higher eta region apparently still needs more work.
For ttbar pu50 there are no significantly visible differences roughly as expected since most of the changes were targeting high-pt
http://uaf-10.t2.ucsd.edu/~slava77/figs/mic/mtv/ttbar_CKF_vs_mkFit_310_77a7f37/
e.g. all tracks OOB (this PR line is in black here)
timing with AVX2 looks consistent between two mkfit versions (v3.1.0 and this PR)
pixel-less has some more noticeable increase in timing, but I think that it's within uncertainties (also, there are no significant changes in the variables in this iteration)
vectorization report is suggestive of an overall moderate improvement in PropagationMPlex (
helixAtZ
andapplyMaterialEffects
) #334 (review)A couple of points that were observed during debugging of a few cases: