-
Notifications
You must be signed in to change notification settings - Fork 614
[PWGJE] trackEfficiency: weights in eff. plots, ptHat cuts #11220
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
|
O2 linter results: ❌ 0 errors, |
vkucera
left a comment
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.
Fix the errors. You have bugs in the code.
|
Error while checking build/O2Physics/o2 for f2e355c at 2025-05-16 11:37: Full log here. |
|
Ah! i'm sorry, I didn't mean to click on this request review |
|
This last version should be much better. |
|
Hi @vkucera |
Hi @aimeric-landou , thanks for the fixes. Can you fix the rest too? |
PWGJE/Tasks/trackEfficiency.cxx
Outdated
| int acceptSplitCollisionsCase0 = 0; // acceptSplitCollisions case: only look at mcCollisions that are not split | ||
| int acceptSplitCollisionsCase2 = 2; // acceptSplitCollisions case: accept split mcCollisions but only look at the first reco collision associated with it |
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 are these values defined?
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 implementation is not easy to read, understand, use and maintain, because the names do not reflect the meanings of the values and there is not sanity check to validate the input values provided in acceptSplitCollisions.
I recommend to:
- replace these with
enumwith meaningful names, - add a sanity check for
acceptSplitCollisionsininitwhich throws afatalin case of invalid values. - branch the code with
if/else ifblocks without anelseblock.
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.
Hi @vkucera
I was away until this morning, I just updated my file to address your comments, I hope everything looks better now.
Head branch was pushed to by a user without write access
PWGJE/Tasks/trackEfficiency.cxx
Outdated
| enum AcceptSplitCollisionsOptions { | ||
| nonSplitOnly = 0, | ||
| splitOkCheckAnyAssocColl, // 1 | ||
| splitOkCheckFirstAssocCollOnly // 2 |
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.
Thanks for fixing the enum name. The linter does not check this but the names of the values should also follow the UpperCamelCase convention because they are compile-time constants.
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 just pushed a fix for those three variables; I don't understand what theMegaLinter is now complaining about, do you know?
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.
Thanks @aimeric-landou .
The MegaLinter error occurred because your branch is too old. If you merge the latest master in it, the check should run normally.
in trackEfficiency.cxx file: