-
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
Fix initialisation of maxChi2 data member in PVClusterComparer #31725
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31725/18919
|
A new Pull Request was created by @ats2008 (Aravind Sugunan) for master. It involves the following packages: RecoPixelVertexing/PixelVertexFinding @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild please test |
The tests are being triggered in jenkins.
|
@ats2008 |
@@ -78,7 +78,7 @@ double PVClusterComparer::pTSquaredSum(const reco::Vertex &v) { | |||
void PVClusterComparer::setChisquareQuantile() { | |||
std::vector<double> maxChi2(20, 0.); |
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.
please do not use variables that differ only for a "_"!
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.
are you sure 20 is safe?
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.
and, btw, why 20?
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 am not clear on why it was initialized with 20 elements.
there another function ( PVClusterComparer::updateChisquareQuantile ) that updates the maxChi2_ vector if elements with ndof more than 20 are required.
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.
and its called when those elements need to be accessed, as seen here, so it should be safe.
@@ -78,7 +78,7 @@ double PVClusterComparer::pTSquaredSum(const reco::Vertex &v) { | |||
void PVClusterComparer::setChisquareQuantile() { | |||
std::vector<double> maxChi2(20, 0.); | |||
if (track_prob_min_ >= 0. && track_prob_min_ <= 1.) | |||
for (size_t ndof = 0; ndof < maxChi2_.size(); ++ndof) | |||
for (size_t ndof = 0; ndof < maxChi2.size(); ++ndof) | |||
// http://root.cern.ch/root/html/TMath.html#TMath:ChisquareQuantile | |||
maxChi2[ndof] = TMath::ChisquareQuantile(1 - track_prob_min_, ndof); |
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.
why not filling maxChi2_ directly?
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.
yes .. it could have been done . I thought i will just make the minor code correction .. but now that its mentioned , I shall update the PR with the code that fills maxChi2_ directly
+1 |
Comparison job queued. |
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31725/18969
|
please test |
The tests are being triggered in jenkins.
|
+1 |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1
|
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Minor bug fix. The code to initialize the maxChi2 values does not do the job, due to an unfortunate typo in the loop break condition.