Skip to content

[PWGCF] add 2 and 4 particle correlation and fix errors#16619

Draft
pykuan wants to merge 5 commits into
AliceO2Group:masterfrom
pykuan:master
Draft

[PWGCF] add 2 and 4 particle correlation and fix errors#16619
pykuan wants to merge 5 commits into
AliceO2Group:masterfrom
pykuan:master

Conversation

@pykuan

@pykuan pykuan commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions github-actions Bot added the pwgcf label Jun 11, 2026
@github-actions github-actions Bot changed the title add 2 and 4 particle correlation and fix errors [PWGCF] add 2 and 4 particle correlation and fix errors Jun 11, 2026
@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown

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

@alibuild

Copy link
Copy Markdown
Collaborator

Error while checking build/O2Physics/o2 for a7a4270 at 2026-06-11 12:50:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:556:16: error: 'Q' was not declared in this scope
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:1054:62: error: 'Cos' is not a member of 'o2::constants::math'
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:1055:62: error: 'Sin' is not a member of 'o2::constants::math'
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:814:11: error: unused variable 'histAcceptanceWeight' [-Werror=unused-variable]
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:814:11: error: unused variable 'histAcceptanceWeight' [-Werror=unused-variable]
ninja: build stopped: subcommand failed.

Full log here.

@alibuild

Copy link
Copy Markdown
Collaborator

Error while checking build/O2Physics/staging for a7a4270 at 2026-06-11 12:53:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:556:16: error: 'Q' was not declared in this scope
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:1054:62: error: 'Cos' is not a member of 'o2::constants::math'
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:1055:62: error: 'Sin' is not a member of 'o2::constants::math'
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:814:11: error: unused variable 'histAcceptanceWeight' [-Werror=unused-variable]
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:814:11: error: unused variable 'histAcceptanceWeight' [-Werror=unused-variable]
ninja: build stopped: subcommand failed.

Full log here.

@vkucera vkucera marked this pull request as draft June 11, 2026 11:05
@vkucera

vkucera commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

@pykuan You are supposed to test your changes before you make a PR. Please follow the contribution guidelines.

@pykuan

pykuan commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

@pykuan You are supposed to test your changes before you make a PR. Please follow the contribution guidelines.

Sorry, the clang-format in local didn't work properly. It caused even more errors than I expected... I'll try to fix O2 linter error and the code itself first.

@vkucera

vkucera commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

@pykuan You are supposed to test your changes before you make a PR. Please follow the contribution guidelines.

Sorry, the clang-format in local didn't work properly. It caused even more errors than I expected... I'll try to fix O2 linter error and the code itself first.

clang-format cannot cause errors if you use it as explained in the documentation and it has definitely nothing to do with your broken compilation.

@vkucera

vkucera commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

How exactly are you using clang-format?

@pykuan

pykuan commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

How exactly are you using clang-format?

I used "clang-format -i someFile" but it somehow breaks lines that are supposed to be in a single line, and causes errors. I saw there are many configurables in clang-format but didn't know which to switch off to satisfy the automatic checks.
In the last modification I tried to change them manually according to the error message.

@vkucera

vkucera commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

How exactly are you using clang-format?

I used "clang-format -i someFile" but it somehow breaks lines that are supposed to be in a single line, and causes errors. I saw there are many configurables in clang-format but didn't know which to switch off to satisfy the automatic checks. In the last modification I tried to change them manually according to the error message.

There is no such instruction in the documentation to use clang-format -i. Where did you get it from?

@abilandz

abilandz commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

How exactly are you using clang-format?

I used "clang-format -i someFile" but it somehow breaks lines that are supposed to be in a single line, and causes errors. I saw there are many configurables in clang-format but didn't know which to switch off to satisfy the automatic checks. In the last modification I tried to change them manually according to the error message.

There is no such instruction in the documentation to use clang-format -i. Where did you get it from?

From me. I was using that regularly for local formatting before committing code to the O2physics repository, starting in 2023. I also used it a few months ago when making my last commit, and there were no problems or complaints.

I do not remember from where I got it any longer in 2023, but for more than 2 years (at least), there were no problems with that. Can you please clarify to us why we cannot use suddenly clang-format -i any longer locally for formatting? Thanks!

@vkucera

vkucera commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Hi @abilandz

Can you please clarify to us why we cannot use suddenly clang-format -i any longer locally for formatting? Thanks!

You can use whatever you want but:

  • a) it's obvious that whatever you are doing now simply does not work and
  • b) there are already documented automated tools that do the formatting for you, so there is no reason to do it by hand.

@abilandz

Copy link
Copy Markdown
Collaborator

Hi @abilandz

Can you please clarify to us why we cannot use suddenly clang-format -i any longer locally for formatting? Thanks!

You can use whatever you want but:

If my memory does not betray me, when I was getting first formatting errors on my very first commits in O2Physics, I was getting automatically generated commits from aliBuild with the title "Please consider the following formatting changes", or something like that. I think that after inspecting the online log, I realized they were generated with clang-format. I tried to use myself clang-format locally in the same way, before making any new commit, and it worked - from that point onward I never had problems with the formatting of my commits.

And I completely eliminated from my workflow in O2Physics these unnecessary automatically generated commits from aliBuild related to the formatting.

Btw, the flag "-i" is irrelevant for this discussion - it merely updates the files in-place after the formatting (in the same fashion as sed -i does it, etc).

  • a) it's obvious that whatever you are doing now simply does not work and

This is not a fair summary. We have used `clang-format' successfully for more than two years, basically in all my commits in the past 2+ years. @pykuan also used it successfully in her commits in the past few weeks. So something very recently has changed in the workflow related to the formatting.

Could you please clarify what has changed? Do we use some other tool now instead of clang-format, or do we still use it, but with some new non-default options, etc?

  • b) there are already documented automated tools that do the formatting for you, so there is no reason to do it by hand.

I do not consider executing clang-format -i someFile before making a commit to be that difficult... It's trivial in fact.

Are you suggesting now that we should not attempt to format ourselves the code locally, and just rely on these automated tools you are mentioning?

But if I can format in the very same way my code locally, that should only ease the burden on those automated tools... Therefore, I do not fully understand your point here.

@abilandz abilandz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see most of the formatting problems are fixed, therefore I approve this one. Please address the remaining 1 error from O2linter in the next PR.

@vkucera

vkucera commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

@abilandz

... I was getting automatically generated commits from aliBuild with the title "Please consider the following formatting changes..."

It is still the case. See above.

  • a) it's obvious that whatever you are doing now simply does not work and

This is not a fair summary.

I don't know what you find "not fair" but the facts are simple. If your local formatting worked, there would be no formatting PR created. It was created, therefore your local formatting does not work.

So something very recently has changed in the workflow related to the formatting.

No, nothing has changed very recently. The last change of the clang-format version was in March.

Do we use some other tool now instead of clang-format, or do we still use it, but with some new non-default options, etc?

No. The options have not changed as far as I can remember.

I do not consider executing clang-format -i someFile before making a commit to be that difficult... It's trivial in fact.

It's trivial and wrong because you are missing the --style option which applies the project configuration. That's why we provide and encourage automated solutions.

Are you suggesting now that we should not attempt to format ourselves the code locally, and just rely on these automated tools you are mentioning?

Yes. It's written in the documentation.

But if I can format in the very same way my code locally, that should only ease the burden on those automated tools...

If you prefer extra work and want to do it locally by hand, do it at least properly and do not confuse students with wrong instructions. Otherwise, there is a configured and documented pre-commit hook for this.

@vkucera

vkucera commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

@abilandz

... I was getting automatically generated commits from aliBuild with the title "Please consider the following formatting changes..."

It is still the case. See above.

  • a) it's obvious that whatever you are doing now simply does not work and

This is not a fair summary.

I don't know what you find "not fair" but if your local formatting worked, there would be no formatting PR created. It was created, therefore your local formatting does not work.

So something very recently has changed in the workflow related to the formatting.

No, nothing has changed very recently. The last change of the clang-format version was in March.

Do we use some other tool now instead of clang-format, or do we still use it, but with some new non-default options, etc?

No. The options have not changed as far as I can remember.

I do not consider executing clang-format -i someFile before making a commit to be that difficult... It's trivial in fact.

It's trivial and also wrong because you are missing the --style option which applies the project configuration. That's why we provide and encourage automated solutions.

Are you suggesting now that we should not attempt to format ourselves the code locally, and just rely on these automated tools you are mentioning?

Yes. It's written in the documentation.

But if I can format in the very same way my code locally, that should only ease the burden on those automated tools...

If you prefer extra work and want to do it locally by hand, do it at least properly and do not confuse students with wrong instructions. Otherwise, there is a configured and documented pre-commit hook for this.

@abilandz

abilandz commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

@abilandz

... I was getting automatically generated commits from aliBuild with the title "Please consider the following formatting changes..."

It is still the case. See above.

  • a) it's obvious that whatever you are doing now simply does not work and

This is not a fair summary.

I don't know what you find "not fair" but if your local formatting worked, there would be no formatting PR created. It was created, therefore your local formatting does not work.

Our local formatting was working for 2+ years. Without any change on our side, it stopped working suddenly.

This looks like a change was introduced to the central workflow related to the formatting, which is not backward-compatible.

So something very recently has changed in the workflow related to the formatting.

No, nothing has changed very recently. The last change of the clang-format version was in March.

Do we use some other tool now instead of clang-format, or do we still use it, but with some new non-default options, etc?

No. The options have not changed as far as I can remember.

I do not consider executing clang-format -i someFile before making a commit to be that difficult... It's trivial in fact.

It's trivial and also wrong because you are missing the --style option which applies the project configuration.

You could have replied that in your very first post here. But your reply now is nevertheless incomplete - looking at the documentation of clang-format, this flag cannot be used on its own as it takes an argument:

  --style=<string>               - Set coding style. <string> can be:
                                   1. A preset: LLVM, GNU, Google, Chromium, Microsoft,
                                      Mozilla, WebKit.
                                   2. 'file' to load style configuration from a
                                      .clang-format file in one of the parent directories
                                      of the source file (for stdin, see --assume-filename).
                                      If no .clang-format file is found, falls back to
                                      --fallback-style.
                                      --style=file is the default.
                                   3. 'file:<format_file_path>' to explicitly specify
                                      the configuration file.
                                   4. "{key: value, ...}" to set specific parameters, e.g.:
                                      --style="{BasedOnStyle: llvm, IndentWidth: 8}"

Can you please disclose what we use for argument <string> at the moment? Did we start using a customized configuration file instead of relying on default formatting options - perhaps that's what changed recently...

That's why we provide and encourage automated solutions.

Are you suggesting now that we should not attempt to format ourselves the code locally, and just rely on these automated tools you are mentioning?

Yes. It's written in the documentation.

Please can you re-think that part of the documentation, and add the following line:

Before making a commit for files file1 file2 ... fileN, a developer is encouraged to execute locally:
clang-format -i --style=<specify-what> file1 file2 ... fileN

This would completely eliminate the need for aliBuild to automatically generate trivial commits related to formatting. I guess we all agree that we should not clog our O2 repository with unnecessary commits.

If the local formatting failed for whatever reason, only then aliBuild can generate the commit related to formatting, but with the instruction at the end on how a developer can run the very same formatting tool locally, so that such a trivial and unnecessary formatting commit does NOT have to be generated by aliBuild itself.

But if I can format in the very same way my code locally, that should only ease the burden on those automated tools...

If you prefer extra work and want to do it locally by hand,

It's not extra work, and it's a really trivial one-liner - I am sure you understand that. Doing formatting "by hand" would imply we are manually making an intervention in the source code locally to fix the formatting - nobody normal would ever attempt to do that.

And, btw, we get all clang suite installed with the O2 package in any case...

do it at least properly and do not confuse students with wrong instructions.
I have conveyed to my students the procedure that worked 2+ years for me without the slightest problem. And they started using the very same procedure a few weeks ago in their first commits without any problems. If that means that I gave them wrong instructions, so be it.

Otherwise, there is a configured and documented pre-commit hook for this.

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