[PWGCF] ESE flow task for GFW #12440

joachimckh · 2025-08-05T12:25:55Z

No description provided.

github-actions · 2025-08-05T12:26:08Z

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

vkucera · 2025-08-06T09:15:42Z

PWGCF/Flow/Tasks/flowGfwEse.cxx

+using namespace o2;
+using namespace o2::framework;
+
+#define O2_DEFINE_CONFIGURABLE(NAME, TYPE, DEFAULT, HELP) Configurable<TYPE> NAME{#NAME, DEFAULT, HELP};


@victor-gonzalez Why is this macro defined in 30 files?

I guess it started with the first task in O2Physics (O2 at that time) Tasks/correlations.cxx and people started replicating it
I personally don't use it and didn't like it but I find it not harmful

vkucera · 2025-08-06T09:17:33Z

PWGCF/Flow/Tasks/flowGfwEse.cxx

+    switch (cfgCentEstimator) {
+      case kCentFT0C:
+        sCentralityEstimator = "FT0C";
+        break;
+      case kCentFT0CVariant1:
+        sCentralityEstimator = "FT0C variant 1";
+        break;
+      case kCentFT0M:
+        sCentralityEstimator = "FT0M";
+        break;
+      case kCentFV0A:
+        sCentralityEstimator = "FV0A";
+        break;
+      case kCentNTPV:
+        sCentralityEstimator = "NTPV";
+        break;
+      case kCentNGlobal:
+        sCentralityEstimator = "NGlobals";
+        break;
+      case kCentMFT:
+        sCentralityEstimator = "MFT";
+        break;
+      default:
+        sCentralityEstimator = "FT0C";
+        break;


Use a map.

This is a very good suggestion for simplifying code in case you want to consider it

Changed this to use std::map instead,

victor-gonzalez

Please, have a look at my comments

victor-gonzalez · 2025-08-06T17:03:06Z

PWGCF/Flow/Tasks/flowGfwEse.cxx

+// granted to it by virtue of its status as an Intergovernmental Organization
+// or submit itself to any jurisdiction.
+
+/// \file FlowGfwEse.cxx


If it matches to the actual file name the linter error will be gone

Thank you! I could not figure out why it gave me the error here.. Completely missed the capital F. :)

The remaining linter errors should belong to different tasks now..

victor-gonzalez · 2025-08-06T17:06:05Z

PWGCF/Flow/Tasks/flowGfwEse.cxx

+
+#define O2_DEFINE_CONFIGURABLE(NAME, TYPE, DEFAULT, HELP) Configurable<TYPE> NAME{#NAME, DEFAULT, HELP};
+
+namespace o2::analysis::gfw


The task variables are not from the GFW namespace as such, use gfwflowese or something similar

Namespace should be changed now.

vkucera · 2025-08-07T12:09:42Z

PWGCF/Flow/Tasks/flowGfwEse.cxx

+
+  O2_DEFINE_CONFIGURABLE(cfgNbootstrap, int, 10, "Number of subsamples")
+  O2_DEFINE_CONFIGURABLE(cfgMpar, int, 4, "Highest order of pt-pt correlations")
+  O2_DEFINE_CONFIGURABLE(cfgCentEstimator, int, 0, "0:FT0C; 1:FT0CVariant1; 2:FT0M; 3:FT0A")


The documentation string is not correct.

I guess Vit is referring to not all the potential options considered in the documentation string

Indeed. One can just avoid listing all the options by referring to the CentEstimators enumerator instead.

This should match now.

vkucera · 2025-08-07T12:10:45Z

PWGCF/Flow/Tasks/flowGfwEse.cxx

+  TF1* fPtDepDCAxy = nullptr;
+
+  o2::framework::expressions::Filter collisionFilter = nabs(aod::collision::posZ) < cfgVtxZ;
+  o2::framework::expressions::Filter trackFilter = nabs(aod::track::eta) < cfgEta && aod::track::pt > cfgPtmin&& aod::track::pt < cfgPtmax && ((requireGlobalTrackInFilter()) || (aod::track::isGlobalTrackSDD == (uint8_t) true)) && (aod::track::itsChi2NCl < cfgChi2PrITSCls) && (aod::track::tpcChi2NCl < cfgChi2PrTPCCls) && nabs(aod::track::dcaZ) < cfgDCAz;


Do not use C-style casting, ever. Use static_cast instead.

@vkucera is right!
But shouldn't this have been a MegaLinter error?

It should have. But for some reason cpplint does not see these issues inside filter declarations.

I changed the cast.

vkucera · 2025-08-07T17:50:48Z

PWGCF/Flow/Tasks/flowGfwEse.cxx

+      {kCentNGlobal, "NGlobals"},
+      {kCentMFT, "MFT"}};
+
+    sCentralityEstimator = centEstimatorMap.count(cfgCentEstimator) ? centEstimatorMap[cfgCentEstimator] : "FT0C";


Don't you want to throw fatal for invalid values?

victor-gonzalez · 2025-08-07T17:53:28Z

PWGCF/Flow/Tasks/flowGfwEse.cxx

+      posRegionIndex = [&]() {
+        auto begin = cfgRegions->GetNames().begin();
+        auto end = cfgRegions->GetNames().end();
+        auto it = std::find(begin, end, "refP");
+        return (it != end) ? std::distance(begin, it) : -1;
+      }();
+      negRegionIndex = [&]() {
+        auto begin = cfgRegions->GetNames().begin();
+        auto end = cfgRegions->GetNames().end();
+        auto it = std::find(begin, end, "refN");
+        return (it != end) ? std::distance(begin, it) : -1;
+      }();
+      fullRegionIndex = [&]() {
+        auto begin = cfgRegions->GetNames().begin();
+        auto end = cfgRegions->GetNames().end();
+        auto it = std::find(begin, end, "refFull");
+        return (it != end) ? std::distance(begin, it) : -1;
+      }();
+      midRegionIndex = [&]() {
+        auto begin = cfgRegions->GetNames().begin();
+        auto end = cfgRegions->GetNames().end();
+        auto it = std::find(begin, end, "refMid");
+        return (it != end) ? std::distance(begin, it) : -1;
+      }();


Would not be better to have a single one and pass as a parameter the string to find?

Indeed, I have changed this expression now.

victor-gonzalez · 2025-08-11T09:28:19Z

PWGCF/Flow/Tasks/flowGfwEse.cxx

      {kCentMFT, "MFT"}};

-    sCentralityEstimator = centEstimatorMap.count(cfgCentEstimator) ? centEstimatorMap[cfgCentEstimator] : "FT0C";
+    sCentralityEstimator = centEstimatorMap.at(cfgCentEstimator);


I would suggest a controlled fatal in which if the configured estimator is not supported an appropriate message is given to the user together with the fatal crash. In that way finding the reason for the crash does not relay in the back-trace which sometimes is not too trustable

victor-gonzalez

Thanks for addressing our comments!
Have a look at the latest one regarding the checking of a potential not found centrality estimator in case you want to consider it for future iterations

ESE flow task for GFW

45ba5ac

github-actions bot added the pwgcf label Aug 5, 2025

trail whitespace

370f1d5

joachimckh marked this pull request as ready for review August 6, 2025 06:33

joachimckh requested review from alibuild, ariedel-cern, glromane, lauraser, lgraczykCern, otonvd, prchakra, saganatt, shouqiye, victor-gonzalez and zchochul as code owners August 6, 2025 06:33

vkucera reviewed Aug 6, 2025

View reviewed changes

victor-gonzalez requested changes Aug 6, 2025

View reviewed changes

resolve requests

13a2aa1

alibuild mentioned this pull request Aug 7, 2025

Please consider the following formatting changes to #12440 joachimckh/O2Physics#10

Closed

clang format

30537de

vkucera reviewed Aug 7, 2025

View reviewed changes

victor-gonzalez reviewed Aug 7, 2025

View reviewed changes

add requests from Vit and Victor

a518cff

victor-gonzalez reviewed Aug 11, 2025

View reviewed changes

victor-gonzalez approved these changes Aug 11, 2025

View reviewed changes

victor-gonzalez enabled auto-merge (squash) August 11, 2025 09:33

victor-gonzalez merged commit 2862a49 into AliceO2Group:master Aug 11, 2025
16 of 18 checks passed

jpxrk pushed a commit to jpxrk/O2Physics that referenced this pull request Aug 12, 2025

[PWGCF] ESE flow task for GFW (AliceO2Group#12440)

0fe7f55

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

[PWGCF] ESE flow task for GFW (AliceO2Group#12440)

6eac690

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

[PWGCF] ESE flow task for GFW (AliceO2Group#12440)

948aba5

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

[PWGCF] ESE flow task for GFW (AliceO2Group#12440)

5b23176


		#define O2_DEFINE_CONFIGURABLE(NAME, TYPE, DEFAULT, HELP) Configurable<TYPE> NAME{#NAME, DEFAULT, HELP};

		namespace o2::analysis::gfw

[PWGCF] ESE flow task for GFW #12440

[PWGCF] ESE flow task for GFW #12440

Uh oh!

Conversation

joachimckh commented Aug 5, 2025

Uh oh!

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

Uh oh!

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!

victor-gonzalez left a comment

Choose a reason for hiding this comment

Uh oh!

victor-gonzalez Aug 6, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victor-gonzalez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Aug 5, 2025 •

edited

Loading

victor-gonzalez Aug 6, 2025 •

edited

Loading