Skip to content

Conversation

@aimeric-landou
Copy link
Contributor

@aimeric-landou aimeric-landou commented May 16, 2025

in trackEfficiency.cxx file:

  • adds possibillity to use weights in efficiency and purity plots
  • introduces ptHat cuts in all processes
  • cleans file

@github-actions
Copy link

github-actions bot commented May 16, 2025

O2 linter results: ❌ 0 errors, ⚠️ 0 warnings, 🔕 0 disabled

Copy link
Collaborator

@vkucera vkucera left a 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.

@alibuild
Copy link
Collaborator

Error while checking build/O2Physics/o2 for f2e355c at 2025-05-16 11:37:

## sw/BUILD/O2Physics-latest/log
/sw/slc9_x86-64/O2/daily-20250516-0000-local1/include/Framework/HistogramRegistry.h:451:14: error: 'eventWeight' may be used uninitialized [-Werror=maybe-uninitialized]
ninja: build stopped: subcommand failed.

Full log here.

@aimeric-landou aimeric-landou requested a review from vkucera May 16, 2025 10:17
@aimeric-landou
Copy link
Contributor Author

Ah! i'm sorry, I didn't mean to click on this request review

@aimeric-landou aimeric-landou changed the title [PWGJE] trackEfficiency: allow weights to eff. and purity plots [PWGJE] trackEfficiency: weights in eff. plots, ptHat cuts May 16, 2025
@aimeric-landou
Copy link
Contributor Author

This last version should be much better.

@aimeric-landou
Copy link
Contributor Author

Hi @vkucera
is the new version looking good to merge?

@vkucera
Copy link
Collaborator

vkucera commented May 19, 2025

Hi @vkucera is the new version looking good to merge?

Hi @aimeric-landou , thanks for the fixes. Can you fix the rest too?

@alibuild
Copy link
Collaborator

Error while checking build/O2Physics/o2 for 8b28b50 at 2025-05-20 13:02:

No log files found

Full log here.

Comment on lines 88 to 89
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
Copy link
Collaborator

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?

Copy link
Collaborator

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 enum with meaningful names,
  • add a sanity check for acceptSplitCollisions in init which throws a fatal in case of invalid values.
  • branch the code with if/else if blocks without an else block.

Copy link
Contributor Author

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.

nzardosh
nzardosh previously approved these changes May 21, 2025
@nzardosh nzardosh enabled auto-merge (squash) May 21, 2025 19:29
auto-merge was automatically disabled May 28, 2025 10:14

Head branch was pushed to by a user without write access

Comment on lines 89 to 92
enum AcceptSplitCollisionsOptions {
nonSplitOnly = 0,
splitOkCheckAnyAssocColl, // 1
splitOkCheckFirstAssocCollOnly // 2
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

@nzardosh nzardosh enabled auto-merge (squash) May 30, 2025 10:12
@nzardosh nzardosh dismissed vkucera’s stale review May 30, 2025 10:13

fixes done

@nzardosh nzardosh merged commit e487ef9 into AliceO2Group:master May 30, 2025
14 of 15 checks passed
jinhyunni pushed a commit to jinhyunni/O2Physics that referenced this pull request Jun 11, 2025
hernasab pushed a commit to hernasab/O2Physics that referenced this pull request Jun 11, 2025
EmilGorm pushed a commit to EmilGorm/O2Physics that referenced this pull request Jun 12, 2025
prottayCMT pushed a commit to prottayCMT/O2Physics2024 that referenced this pull request Jun 12, 2025
ddobrigk pushed a commit to ddobrigk/O2Physics that referenced this pull request Jun 14, 2025
smaff92 pushed a commit to smaff92/O2Physics that referenced this pull request Jun 17, 2025
ddobrigk pushed a commit to ddobrigk/O2Physics that referenced this pull request Jul 16, 2025
alibuild pushed a commit to alibuild/O2Physics that referenced this pull request Aug 11, 2025
jloemker pushed a commit to jloemker/O2Physics that referenced this pull request Nov 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

4 participants