Skip to content

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

Merged
abilandz merged 9 commits into
AliceO2Group:masterfrom
pykuan:master
Jun 12, 2026
Merged

[PWGCF] add 2 and 4 particle correlation and fix errors#16619
abilandz merged 9 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 previously approved these changes Jun 11, 2026

@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 again in the future.

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.

@vkucera

vkucera commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

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

Not that I am aware of. And even if it was the case, it would not prevent you from following the official instructions.

You could have replied that in your very first post here.

No, I could have not because I had no idea what @pykuan was doing locally. Hence my follow-up questions.

looking at the documentation of clang-format, this flag cannot be used on its own as it takes an argument:

The full command has been available on the main page of the coding guidelines for the last 7 years. It seems you never looked at them.

Can you please disclose what we use for argument <string> at the moment?

See the coding guidelines.

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

No, I cannot.

  • a) It is already part of the documentation.
  • b) It is not what a developer is encouraged to do.

This would completely eliminate the need for aliBuild to automatically generate trivial commits related to formatting.

We already provide a solution for that. Read the documentation.

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,...

That is exactly how it works now.

It's not extra work

It is unnecessary, therefore extra.

Doing formatting "by hand" would imply we are manually making an intervention in the source code locally...

It's invoking the command manually which requires action from the user. I call it "by hand".

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.

If you give students instructions which produce results incompatible with the results produced by the official instructions, then your instructions are clearly wrong and harmful to the students who have to deal with the resulting problems.

@abilandz abilandz marked this pull request as ready for review June 12, 2026 10:05
@abilandz abilandz enabled auto-merge (squash) June 12, 2026 10:06
@alibuild

Copy link
Copy Markdown
Collaborator

Error while checking build/O2Physics/o2 for 266f505 at 2026-06-12 12:09:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:728:11: error: unused variable 'histAcceptanceWeight' [-Werror=unused-variable]
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:728:11: error: unused variable 'histAcceptanceWeight' [-Werror=unused-variable]
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:499:3: error: control reaches end of non-void function [-Werror=return-type]
ninja: build stopped: subcommand failed.

Full log here.

@alibuild

Copy link
Copy Markdown
Collaborator

Error while checking build/O2Physics/staging for 266f505 at 2026-06-12 12:09:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:728:11: error: unused variable 'histAcceptanceWeight' [-Werror=unused-variable]
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:728:11: error: unused variable 'histAcceptanceWeight' [-Werror=unused-variable]
/sw/SOURCES/O2Physics/slc9_x86-64-slc9_x86-64/0/PWGCF/MultiparticleCorrelations/Tasks/multiparticleCumulants.cxx:499:3: error: control reaches end of non-void function [-Werror=return-type]
ninja: build stopped: subcommand failed.

Full log here.

@abilandz

Copy link
Copy Markdown
Collaborator

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

Not that I am aware of. And even if it was the case, it would not prevent you from following the official instructions.

You could have replied that in your very first post here.

No, I could have not because I had no idea what @pykuan was doing locally. Hence my follow-up questions.

looking at the documentation of clang-format, this flag cannot be used on its own as it takes an argument:

The full command has been available on the main page of the coding guidelines for the last 7 years. It seems you never looked at them.

Well, the fact is that in your reply, you have written that the --style option has to be used and you didn't specify its argument, which is just wrong syntax, also according to the official documentation...

I did have a detailed look at the O2 documentation before starting to develop and contribute code to O2. That's why most of my commits went through without any problems. Please stop inventing and posting wrong claims.

Can you please disclose what we use for argument <string> at the moment?

See the coding guidelines.

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

No, I cannot.

  • a) It is already part of the documentation.
  • b) It is not what a developer is encouraged to do.

If the developer is not encouraged to do it, why it is then part of the documentation?

Should we ourselves try to figure out which part of the documentation we are supposed to follow, and which not?

Please strip out of the documentation anything we are not supposed to use, or at least mark it clearly.

This would completely eliminate the need for aliBuild to automatically generate trivial commits related to formatting.

We already provide a solution for that. Read the documentation.

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,...

That is exactly how it works now.

It's not extra work

It is unnecessary, therefore extra.

Doing formatting "by hand" would imply we are manually making an intervention in the source code locally...

It's invoking the command manually which requires action from the user. I call it "by hand".

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.

If you give students instructions which produce results incompatible with the results produced by the official instructions, then your instructions are clearly wrong and harmful to the students who have to deal with the resulting problems.

You should perhaps finally start paying attention to what students (and not only them!) think about your comments, and out-of-the-blue interventions in their code which force them to stop their work just to start dealing with merging conflicts, etc.

This was my last post in this PR, I do not want to waste my time further on discussions like this.

auto-merge was automatically disabled June 12, 2026 10:42

Head branch was pushed to by a user without write access

@abilandz abilandz enabled auto-merge (squash) June 12, 2026 11:22

@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.

Thanks for fixing all the errors, also formatting problems! Please in the next PR try to address the remaining single O2Linter error!

@abilandz abilandz merged commit 52b6b7b into AliceO2Group:master Jun 12, 2026
12 of 13 checks passed
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