-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Avoid Nans in primary vertex #5490
Conversation
A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_7_3_X. Avoid Nans in primary vertex It involves the following packages: RecoVertex/AdaptiveVertexFit @cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks. |
}; | ||
|
||
if ( (**i).weight() >= theWeightThreshold ) ns_trks++; | ||
|
||
if ( fabs ( nVertex.position().z() ) > 10000. || |
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.
probably it's a silly point,
but could we get rid of such hard-coded numbers ?
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.
If somebody volunteers (the code was there already)
double t_z = ((*it).stateAtBeamLine().trackStateAtPCA()).position().z(); | ||
double phi=((*it).stateAtBeamLine().trackStateAtPCA()).momentum().phi(); | ||
double tantheta = std::tan( ((*it).stateAtBeamLine().trackStateAtPCA()).momentum().theta()); | ||
if (std::abs(t_z) > 1000.) continue; |
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.
This version of "1000." in the middle of the code is new. I would write it as a const with a suggestive name.
@cmsbuild: tests, comparison? |
Pull request #5490 was updated. @cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please check and sign again. |
As expected the main changes are to the pixel vertices, less for PV. I see some changes to CSV b-tagging (which was "more sensitive" I was told and not the main algo). I tested in wf202 (ttbar with PU) mainly for now. Below some results (red is this PR vs recent IB), please comment. Overall I see slight decrease in pixel vertices (and related objects), I thougt it had to be the opposite. |
@VinInn @StoyanStoynev |
It is consistent with what I observe as well. |
+1 |
sorry, was wrong noise on my side. |
RecoVertex -- Avoid Nans in primary vertex
void sortTracksByPt(std::vector<reco::TransientTrack> & cont) { | ||
auto s = cont.size(); | ||
float pt2[s]; int ind[s]; int i=0; | ||
for (auto const & tk : cont) { ind[i]=i; pt2[++i] = tk.impactPointState().globalMomentum().perp2();} |
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.
this should have been i++
, or, more straightforward and less error prone, pt2[i] = ...; ++i;
Valgrind reports errors here.
I suspect this is where our vertex/btag discrepancies originate.
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.
oops, is this in 72?
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.
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 think this is only in 73X
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.
While investigating the irreproducibility of relvals I discovered that in some cases no PixelVertices were reconstructed. I trace this down to some Nans.
Here I try to trap Nans and conditions that will generate Nans.
The effect on OfflineVertices is essentially negligible while evident to PixelVertices.
This is also one of the reason that prompted #5476
Few more optimizations have been added to the algorithms.
AdaptiveVertexFitter and KalmanVertexFitter (and the infrastructure used in there) would benefit of a complete re-engineering in light of C++11 and vectorization.
@mtosi, could you please test this for HLT?