-
Notifications
You must be signed in to change notification settings - Fork 483
Fixing handling of edge clusters - DON'T MERGE #14020
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
Conversation
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
|
Ping @davidrohr |
Please consider the following formatting changes to AliceO2Group#14020
| bool correct = (leftEdge) ? (pad < mPadMean) : (pad > mPadMean); | ||
| if (leftEdge && pad == 1) { // only check charge at boundary if maximum is at least one pad away from boundary | ||
| correct = correct && (padBoundaryCharges[0] > 0); // Only correct if cluster is asymmetric with charge > 0 towards sector boundary, otherwise all charge is found | ||
| } else if (!leftEdge && pad == (geo.NPads(pos.row()) - 1)) { |
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.
Should't this be
} else if (!leftEdge && pad == (geo.NPads(pos.row()) - 2)) {
correct = correct && (padBoundaryCharges[geo.NPads(pos.row()) - 1] > 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.
OK, I misunderstood the definition of padBoundatyCharges...
In any case, I think it should be pad == (geo.NPads(pos.row()) - 2 and not -1.
For the padBoundaryCharge, ok, that looks correct, but did you double-check it access the correct value, just to be sure?
| ChargePos pos = filteredPeakPositions[CAMath::Min(idx, clusternum - 1)]; | ||
| Charge charge = chargeMap[pos].unpack(); | ||
|
|
||
| Charge padBoundaryCharges[2] = {chargeMap[pos.delta({-1, 0})].unpack(), chargeMap[pos.delta({1, 0})].unpack()}; |
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.
Can't you define this further down, where it is actually used?
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.
Where excatly? Directly above pc.finalize?
| bool correct = (leftEdge) ? (pad < mPadMean) : (pad > mPadMean); | ||
| if (leftEdge && pad == 1) { // only check charge at boundary if maximum is at least one pad away from boundary | ||
| correct = correct && (padBoundaryCharges[0] > 0); // Only correct if cluster is asymmetric with charge > 0 towards sector boundary, otherwise all charge is found | ||
| } else if (!leftEdge && pad == (geo.NPads(pos.row()) - 1)) { |
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.
OK, I misunderstood the definition of padBoundatyCharges...
In any case, I think it should be pad == (geo.NPads(pos.row()) - 2 and not -1.
For the padBoundaryCharge, ok, that looks correct, but did you double-check it access the correct value, just to be sure?
|
No I didn't check it further. I'll compile and run it and let you know |
No description provided.