PFDisplacedVertexFinder speedups #20997
Conversation
|
The code-checks are being triggered in jenkins. |
|
a double precision version of this code saved ~0.5% of reco 0.82 24.04 PFDisplacedVertexFinder::getLongDiff(Point3DBase<float, GlobalTag> const&, Point3DBase<float, GlobalTag> const&) const [256] goes to 1.0 29.56 PFDisplacedVertexFinder::getTransvLongDiff(Point3DBase<float, GlobalTag> const&, Point3DBase<float, GlobalTag> const&) const |
|
+code-checks |
|
please test |
|
The tests are being triggered in jenkins. |
|
A new Pull Request was created by @davidlange6 (David Lange) for master. It involves the following packages: RecoParticleFlow/PFTracking @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
On 10/24/17 9:59 AM, David Lange wrote:
a double precision version of this code saved ~0.5% of reco
0.82 24.04 PFDisplacedVertexFinder::getLongDiff(Point3DBase<float,
GlobalTag> const&, Point3DBase<float, GlobalTag> const&) const [256]
0.76 22.42 PFDisplacedVertexFinder::getTransvDiff(Point3DBase<float,
GlobalTag> const&, Point3DBase<float, GlobalTag> const&) const [271]
goes to
1.0 29.56 PFDisplacedVertexFinder::getTransvLongDiff(Point3DBase<float,
GlobalTag> const&, Point3DBase<float, GlobalTag> const&) const
where was this tested?
https://slava77sk.web.cern.ch/slava77sk/reco/cgi-bin/igprof-navigator/CMSSW_9_4_X_2017-10-16-1100-sign974.10224.step3.IgProf.ne30.minval.pp/1203
I see the total at 0.12% of the full job in 10224.
…
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#20997 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbqGnZAJqnWC3WooiaJtMuzRmAAHuks5svZjOgaJpZM4QD_ut>.
|
| Basic3DVector<float>vToProject(ToProject); | ||
| double oneOverMag = 1.0/vRef.mag(); | ||
|
|
||
| return std::make_pair(fabs(vRef.cross(vToProject).mag()*oneOverMag),fabs((vRef.dot(vToProject)-vRef.mag2())*oneOverMag)); |
There was a problem hiding this comment.
IMHO, this can all be in float.
all possible essential precision loss can happen in .cross and .dot and .dot-mag2, which all apparently will be made in float.
The extra scaling by 1/mag done in double seems unnecessary.
|
in fact I intended it to be ...will fix
… On Oct 24, 2017, at 10:20 AM, Slava Krutelyov ***@***.***> wrote:
@slava77 commented on this pull request.
In RecoParticleFlow/PFTracking/src/PFDisplacedVertexFinder.cc:
> @@ -734,6 +732,17 @@ PFDisplacedVertexFinder::getTransvDiff(const GlobalPoint& Ref, const GlobalPoint
}
+std::pair<float,float>
+PFDisplacedVertexFinder::getTransvLongDiff(const GlobalPoint& Ref, const GlobalPoint& ToProject) const {
+
+ Basic3DVector<float>vRef(Ref);
+ Basic3DVector<float>vToProject(ToProject);
+ double oneOverMag = 1.0/vRef.mag();
+
+ return std::make_pair(fabs(vRef.cross(vToProject).mag()*oneOverMag),fabs((vRef.dot(vToProject)-vRef.mag2())*oneOverMag));
IMHO, this can all be in float.
all possible essential precision loss can happen in .cross and .dot and .dot-mag2, which all apparently will be made in float.
The extra scaling by 1/mag done in double seems unnecessary.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
cmsbuild
added
code-checks-pending
tests-pending
and removed
code-checks-approved
tests-started
labels
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
The code-checks are being triggered in jenkins. |
|
so this works - the timing is down to ~21 units in my tests (so more than 50% saved from the original implementation)
… On Oct 25, 2017, at 9:28 AM, David Lange ***@***.***> wrote:
somehow i had convinced myself otherwise - I'll give it a try...
> On Oct 25, 2017, at 8:56 AM, Slava Krutelyov ***@***.***> wrote:
>
> @slava77 commented on this pull request.
>
> In RecoParticleFlow/PFTracking/src/PFDisplacedVertexFinder.cc:
>
> > @@ -734,6 +732,17 @@ PFDisplacedVertexFinder::getTransvDiff(const GlobalPoint& Ref, const GlobalPoint
>
> }
>
> +std::pair<float,float>
> +PFDisplacedVertexFinder::getTransvLongDiff(const GlobalPoint& Ref, const GlobalPoint& ToProject) const {
> +
> + Basic3DVector<float>vRef(Ref);
> + Basic3DVector<float>vToProject(ToProject);
> + float oneOverMag = 1.0f/vRef.mag();
> +
> + return std::make_pair(fabs(vRef.cross(vToProject).mag()*oneOverMag),fabs((vRef.dot(vToProject)-vRef.mag2())*oneOverMag));
>
> it looks like the following should work
> Ref.basicVector.dot(ToProject.basicVector())
> same for .cross.
> So, the conversion/copy is not really necessary.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
+code-checks |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
|
merge |
merge two calculations and covert them to float (as the inputs are floats). Regression is possible, though I found the calculations agreed to 5 digits (changing by 1 in the 6th digit from time to time).