[PWGHF] Centrality changes in Lc correlator, Correlation task #11724

zeptotera · 2025-06-22T14:32:02Z

Updated the LcHadronPair table to include centrality

github-actions · 2025-06-22T14:32:17Z

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

vkucera · 2025-06-22T16:15:36Z

Hi @zeptotera , please notice the automatic formatting PR. You don't have to run clang-format at all. There is also the option to use pre-commit hooks. Both options are documented.

vkucera · 2025-06-22T16:17:38Z

Please fix the errors.

fgrosa

Hi @zeptotera, thanks for the implementation! See my comments below

PWGHF/HFC/TableProducer/correlatorLcHadrons.cxx

PWGHF/HFC/TableProducer/correlatorLcScHadrons.cxx

PWGHF/HFC/Tasks/taskCorrelationLcHadrons.cxx

vkucera · 2025-07-16T13:44:28Z

@zeptotera Please do not mark your PR as ready for review if it has conflicts or errors.

PWGHF/HFC/TableProducer/correlatorLcHadrons.cxx

vkucera · 2025-07-24T17:42:48Z

PWGHF/HFC/TableProducer/correlatorLcHadrons.cxx

+    if (isMultiplicityDependent) {
+      registry.add("hMassLcVsPtvsMult", "Lc candidates;inv. mass (p K #pi) (GeV/#it{c}^{2});entries", {HistType::kTH3F, {{axisMassLc}, {axisPtLc}, {axisCent}}});
+    } else {
+      registry.add("hMassLcVsPt", "Lc candidates;inv. mass (p K #pi) (GeV/#it{c}^{2});entries", {HistType::kTH2F, {{axisMassLc}, {axisPtLc}}});
+    }


I don't see any relation to multiplicity.

Why do you need hMassLcVsPt?

Okay, We have changed the name of the histogram to have hMassLcVsPtvsCent

hMassLcVsPt is filled so that other people who want the histogram without centrality can still access it and the merge creates a safe working environment for existing Analyzers, This was added based on Antonio’s suggestion.

What is unsafe about projecting hMassLcVsPtvsCent along the centrality axis?

Projecting along the centrality axis works fine for us too. We only kept the separate hMassLcVsPt upon Antonio’s suggestion, so that others using the same task without centrality wouldn’t need to change anything. If you think it's better to just keep one and project, we’re happy to update it — please suggest.

I suggest a single histogram.

Hi @vkucera I am not sure what is the recommended process in HF when adding new things to an exciting analysis task that is being used by others. During the discussion with coordinators, we agreed that while extending this task to perform a multiplicity based study, Tushar adds flags and additional histograms in a way that has minimal impact on current analyzers. Changing a 2D histogram to a 3D will disrupt others, which we would like to minimize. If you still insist on changing it, I would appreciate if you could share a reasoning to do it as well.

Hi @deepathoms @apalasciano , here is how I see the two scenarios:

split conditional histograms

pros

no change for current users

cons

bigger code

more code branching at many places -> less maintainable, more error-prone

lot of code duplication -> less maintainable, more error-prone

analysis-dependent output

train duplication for different analyses

no split histograms

pros

smaller code -> more maintainable, safer

no branching nor duplication -> more maintainable, safer

Same output can be used for all analyses.

cons

changing histogram name in the postprocessing code?

IMO, the drawbacks of the split scenario and the benefits of the no-split scenario are far more important than protecting users from changing a histogram name in their local scripts. One can even keep the current histogram name.

Concerning the projection, I don't see how it changes anything for the users. You said

Changing a 2D histogram to a 3D will disrupt others

but I don't understand what you mean here.

There is no change of a 2D histogram into a 3D histogram. There is a change from a kTHnSparseF histogram with N axes to a kTHnSparseF histogram with N + 1 axes.

The signature of the THnSparse::Projection method does not depend on the number of object axes.

In general, I don't agree that keeping the framework interface and the output intact should be a top priority. The framework is evolving based on the analysis needs and we should be able to collaborate on the evolution of the common code, not fragment it and protect it from changes.

We have introduced breaking changes even in the core code many times before. We communicated them to the community in an announcement and explained the consequences. It's just about communication.

PWGHF/HFC/Tasks/taskCorrelationLcHadrons.cxx

vkucera · 2025-07-24T17:44:46Z

PWGHF/HFC/Tasks/taskCorrelationLcHadrons.cxx

+      if (isMultiplicityDependent) {
+        registry.add("hCorrel2DVsPtSidebandsWithCentrality", stringLcHadron + stringSideband + stringDeltaPhi + stringDeltaEta + stringPtLc + stringPtHadron + "entries", {HistType::kTHnSparseF, {{axisDeltaPhi}, {axisDeltaEta}, {axisPtCorr}, {axisPtHadron}, {axisPoolBin}, {axisCentFT0M}}});
+      } else {
+        registry.add("hCorrel2DVsPtSidebands", stringLcHadron + stringSideband + stringDeltaPhi + stringDeltaEta + stringPtLc + stringPtHadron + "entries", {HistType::kTHnSparseF, {{axisDeltaPhi}, {axisDeltaEta}, {axisPtCorr}, {axisPtHadron}, {axisPoolBin}}});
+      }


Same comment about two histograms

hCorrel2DvsPtSidebands is filled so that other people who want the histogram without centrality can still access it and the merge creates a safe working environment for existing Analyzers, This was added based on Antonio’s suggestion.

zeptotera · 2025-07-30T17:23:27Z

Hi @fcatalan92 @fcolamar @gluparel @deepathoms @mmazzilli @mfaggin @hahassan7 @jpxrk @apalasciano @zhangbiao-phy @NicoleBastid @vkucera

I have addressed and implemented all the requested changes in the PR. A few of the questions raised seem to be beyond the scope of this PR. We would appreciate it if the PR could be approved at the earliest so we can proceed further.

Thanks!

vkucera · 2025-07-30T18:08:14Z

If you wish your PR to be reviewed, please use the button "Ready for review".
So far there seem to be still unaddressed comments though.

fgrosa

Ok for what concerns my comments!

zeptotera · 2025-08-16T14:58:41Z

Hi @fcatalan92 @fcolamar @gluparel @deepathoms @mmazzilli @mfaggin @hahassan7 @jpxrk @apalasciano @zhangbiao-phy @NicoleBastid @vkucera,

We’ve updated the tasks according to Vit’s suggestions. Could you please review and approve the PR?

vkucera

Hi @zeptotera , thanks for removing the split histograms. Please see my comments on a few remaining issues.

PWGHF/HFC/TableProducer/correlatorLcHadrons.cxx

PWGHF/HFC/Tasks/taskCorrelationLcHadrons.cxx

zeptotera · 2025-08-18T16:14:11Z

Hi @vkucera, We have made changes according to your latest suggestions, Can you please review and Approve the PR?

vkucera

Thanks @zeptotera for addressing my comments.

…2Group#11724)

zeptotera requested review from NicoleBastid, alibuild, apalasciano, deepathoms, fcatalan92, fcolamar, fgrosa, gluparel, hahassan7, jpxrk, mfaggin, mmazzilli, vkucera and zhangbiao-phy as code owners June 22, 2025 14:32

github-actions bot added pwghf PWG-HF datamodel labels Jun 22, 2025

github-actions bot changed the title ~~Centrality changes in Lc-H minimum Bias Analysis~~ [PWGHF] Centrality changes in Lc-H minimum Bias Analysis Jun 22, 2025

alibuild mentioned this pull request Jun 22, 2025

Please consider the following formatting changes to #11724 zeptotera/O2Physics#1

Closed

zeptotera changed the title ~~[PWGHF] Centrality changes in Lc-H minimum Bias Analysis~~ [PWGHF] Centrality changes in Lc correlator, Correlation task Jun 22, 2025

vkucera marked this pull request as draft June 22, 2025 16:17

fgrosa reviewed Jun 27, 2025

View reviewed changes

zeptotera force-pushed the centrality-changes branch from 867bb03 to 8858bd6 Compare July 16, 2025 13:25

zeptotera marked this pull request as ready for review July 16, 2025 13:32

vkucera marked this pull request as draft July 16, 2025 13:43

zeptotera force-pushed the centrality-changes branch from 2c95540 to 0b5e797 Compare July 21, 2025 09:50

github-actions bot removed the datamodel label Jul 21, 2025

zeptotera added 2 commits July 21, 2025 23:17

minor fix 2

afeb8c6

minor fix 3

8319896

vkucera reviewed Jul 22, 2025

View reviewed changes

PWGHF/HFC/TableProducer/correlatorLcHadrons.cxx Outdated Show resolved Hide resolved

zeptotera added 2 commits July 22, 2025 17:50

update for suggestion

9ee6768

Histogram update ThnSparseD to THnSparseF

a0804dc

zeptotera marked this pull request as ready for review July 24, 2025 17:41

vkucera requested changes Jul 24, 2025

View reviewed changes

vkucera marked this pull request as draft July 24, 2025 17:45

updates for comments

bec4492

histogram type change th1f->th1d

34f76e8

zeptotera marked this pull request as ready for review July 31, 2025 16:03

fgrosa previously approved these changes Aug 4, 2025

View reviewed changes

vkucera marked this pull request as draft August 13, 2025 07:55

Changes for Vit's comments

7e2b060

zeptotera dismissed fgrosa’s stale review via 7e2b060 August 16, 2025 14:47

minor fix-typo

c941adf

zeptotera marked this pull request as ready for review August 18, 2025 10:43

vkucera requested changes Aug 18, 2025

View reviewed changes

PWGHF/HFC/TableProducer/correlatorLcHadrons.cxx Outdated Show resolved Hide resolved

PWGHF/HFC/TableProducer/correlatorLcHadrons.cxx Outdated Show resolved Hide resolved

PWGHF/HFC/Tasks/taskCorrelationLcHadrons.cxx Outdated Show resolved Hide resolved

fix for Vit's comments

10803dd

vkucera approved these changes Aug 18, 2025

View reviewed changes

vkucera enabled auto-merge (squash) August 18, 2025 16:15

vkucera merged commit e9bd27c into AliceO2Group:master Aug 18, 2025
15 checks passed

zeptotera deleted the centrality-changes branch August 30, 2025 08:52

ThePhDane pushed a commit to ThePhDane/O2Physics that referenced this pull request Nov 3, 2025

[PWGHF] Centrality changes in Lc correlator, Correlation task (AliceO…

166fe05

…2Group#11724)

jloemker pushed a commit to jloemker/O2Physics that referenced this pull request Nov 11, 2025

[PWGHF] Centrality changes in Lc correlator, Correlation task (AliceO…

e224224

…2Group#11724)

alibuild pushed a commit to alibuild/O2Physics that referenced this pull request Dec 5, 2025

[PWGHF] Centrality changes in Lc correlator, Correlation task (AliceO…

27a37f2

…2Group#11724)

[PWGHF] Centrality changes in Lc correlator, Correlation task #11724

[PWGHF] Centrality changes in Lc correlator, Correlation task #11724

Uh oh!

Conversation

zeptotera commented Jun 22, 2025

Uh oh!

github-actions bot commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vkucera commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vkucera commented Jun 22, 2025

Uh oh!

fgrosa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vkucera commented Jul 16, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeptotera Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkucera Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeptotera commented Jul 30, 2025

Uh oh!

vkucera commented Jul 30, 2025

Uh oh!

fgrosa left a comment

Choose a reason for hiding this comment

Uh oh!

zeptotera commented Aug 16, 2025

Uh oh!

vkucera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeptotera commented Aug 18, 2025

Uh oh!

vkucera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

github-actions bot commented Jun 22, 2025 •

edited

Loading

vkucera commented Jun 22, 2025 •

edited

Loading

zeptotera Jul 25, 2025 •

edited

Loading

vkucera Jul 31, 2025 •

edited

Loading