Phi wraparound bug in PFDisplacedVertexCandidateFinder?

[rappoccio]

Hi,

I think there is a phi wraparound bug in this line of PFDisplacedVertexCandidateFinder:

cmssw/RecoParticleFlow/PFTracking/src/PFDisplacedVertexCandidateFinder.cc

Line 194 in 6d2f660

if (pt1 > 2 && pt2 > 2 && std::abs(phi1 - phi2) > 1) {

std::abs(phi1 - phi2) > 1

should be

math::deltaPhi(phi1,phi2) > 1

This may be related to the issue reported here. Can someone confirm? We can fix it (and hopefully the other issue as well) if this is wrong as I expect.

@jpata @clacaputo @laurenhay

[cmsbuild]

A new Issue was created by @rappoccio .

@Dr15Jones, @perrotta, @dpiparo, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[makortel]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

@jpata,@slava77,@clacaputo you have been requested to review this Pull request/Issue and eventually sign? Thanks

[jpata]

cc @cms-sw/pf-l2

[jpata]

type pf

[jpata]

Digging through the code, it looks like Maxime was the original author. I don't know if he's on github, but I shot him an email.

[juska]

Isn't v1.DeltaPhi(v2) equivalent to abs(v1.Phi() - v2.Phi()) ?

[perrotta]

Isn't v1.DeltaPhi(v2) equivalent to abs(v1.Phi() - v2.Phi()) ?

Not if phi1 is 359 degrees and phi2 is 1 degree (e.g.)

[mmusich]

This may be related to the issue reported #31158. Can someone confirm? We can fix it (and hopefully the other issue as well) if this is wrong as I expect.

I'd propose to make the PR and check the performance....

[mmusich]

I'd propose to make the PR and check the performance....

here it is: #38080

[rappoccio]

Thanks @mmusich, we were really just asking if we were missing something, @laurenhay has the fix. It sounds like this is indeed really a bug so we will go ahead with the tests.

[jpata]

+reconstruction

• the physics bug was solved by PFDisplacedVertexCandidateFinder: fix deltaPhi cut, don't use std::abs #38080
• this had no effect on computational performance as far as we saw

[cmsbuild]

This issue is fully signed and ready to be closed.