[PWGEM] Add AnalysisType kDCA to analyse DCA 3D,xy,z together #8188

feisenhu · 2024-10-28T14:59:16Z

Include an additional AnalysisType kDCA to use when studding DCAxy, DCAz and DCA3D together in the same output.

To keep the option of choosing which component "3D,xy" or now also "z" should be used as DCA axis, I changed from a boolean to use DCAxy, to a string in which the component can be written cfgDCAcase.

dsekihat · 2024-10-28T15:05:58Z

PWGEM/Dilepton/Core/Dilepton.h

@@ -115,7 +115,7 @@ struct Dilepton {
  Configurable<int> cfgNtracksPV08Min{"cfgNtracksPV08Min", -1, "min. multNTracksPV"};
  Configurable<int> cfgNtracksPV08Max{"cfgNtracksPV08Max", static_cast<int>(1e+9), "max. multNTracksPV"};
  Configurable<bool> cfgApplyWeightTTCA{"cfgApplyWeightTTCA", false, "flag to apply weighting by 1/N"};
-  Configurable<bool> cfgUseDCAxy{"cfgUseDCAxy", false, "flag to use DCAxy, instead of DCA3D"};
+  Configurable<std::string> cfgDCAcase{"cfgDCAcase", "3D", "use string to choose which DCA component is used (default:3D, xy, z)"};


Can you use int, instead of string? 0 : 3D, 1:xy, 2:z.
Default should be 0.

I agree with Daiki. The C++ string comparison is rather slow. I think this would not be a big hit in performance but I would consider it a good style to use int or an enum.
Some more optimisations should be taken care of in the code like unused variables.
However, a more fundamental question from my side would be if one needs all 3 pair-DCA values in one histogram. This is only necessary if the correlation between the three is to be studied. If I recall correctly this was done before. I am also unsure if we should not use 3D and adjust the templates properly. Also far as I remember the main problem was that the prompt templates (Jpsi) needed to be constructed at the reconstructed mass to account for a bias in the DCA xy due to Bremsstrahlung effects.

I have adapted it to use int.

From my point of view I think that we should keep looking into all three components. We already have prioritised different components for different reasons and it changed already a few times and I don't think that we should neglect any of those, yet. Especially when looking at the effect of the Bremsstrahlung. I agree that at some point we just use once of the component which might be the 3D case. But I don't think we are at that point yet.

Furthermore, this change is just adding an additional AnalysisTyp. It adds the two additional DCA components only for this AnalysisType and not for all, so it will not affect other analysis if they don't use it.

So. The most interesting is the 3D DCA; from my point of view, this should be the one we follow up on. The only need to have all 3 versions of the DCA in the histogram is if you want to do a correlation of DCAz and DCAxy for example. If not, a setup with 3 sub wagons for each DCA is more efficient. 3 histograms with 3 dimensions should take less disc space than a 5D histogram.
And the problem with 3D was only the Jpsi peak moving in the tail right? This can be dealt with by constructing the template at the correct mass.
So: Is there any reason to have DCAxy, DCAz and DCA3D in the same histogram, or do you need one histogram for each one?
I made some more comments on the readability and performance of the code. This can be done later and is not necessary now.

To make the problem clear. At the moment the default value of bins is 30 bins in DCA. So adding two axes with 30 bins increases the memory consumption by this Sparse up to a factor 900. And this is only for the standard up to DCA = 10. I guess you have more bins in your analysis.
In ULS and LS this might be Ok since a lot of bins are empty and in the THnSparse implementation, this is not counting then. However, in mixed events, most bins should be filled and the memory will go through the roof. So, if not absolutely necessary (and right now I don't see why it is) I suggest using sub wagons.

alibuild

Auto-approving on behalf of @feisenhu.

dsekihat · 2024-10-29T00:37:03Z

PWGEM/Dilepton/Core/Dilepton.h

+        pair_dcaxy = std::sqrt((dcaxy_t1 * dcaxy_t1 + dcaxy_t2 * dcaxy_t2) / 2.);
+        pair_dca = pair_dcaxy;
+      }
+      if (cfgDCAcase == 2 || cfgDCAcase == 3) {


If cfgDCAcase = 3, what happens?

dsekihat · 2024-10-29T00:37:11Z

PWGEM/Dilepton/Core/Dilepton.h

+      dca3D_t2 = dca3DinSigma(t2);
+      pair_dca3D = std::sqrt((dca3D_t1 * dca3D_t1 + dca3D_t2 * dca3D_t2) / 2.);
+      pair_dca = pair_dca3D;
+      if (cfgDCAcase == 1 || cfgDCAcase == 3) {


If cfgDCAcase = 3, what happens?

dsekihat · 2024-10-29T00:40:31Z

PWGEM/Dilepton/Core/Dilepton.h

@@ -115,7 +115,7 @@ struct Dilepton {
  Configurable<int> cfgNtracksPV08Min{"cfgNtracksPV08Min", -1, "min. multNTracksPV"};
  Configurable<int> cfgNtracksPV08Max{"cfgNtracksPV08Max", static_cast<int>(1e+9), "max. multNTracksPV"};
  Configurable<bool> cfgApplyWeightTTCA{"cfgApplyWeightTTCA", false, "flag to apply weighting by 1/N"};
-  Configurable<bool> cfgUseDCAxy{"cfgUseDCAxy", false, "flag to use DCAxy, instead of DCA3D"};
+  Configurable<int> cfgDCAcase{"cfgDCAcase", 0, "DCA component for output (3D:0, xy:1, z:2, all:3)"};


cfgDCAcase = 3 should not exist.

hscheid · 2024-10-29T08:12:29Z

PWGEM/Dilepton/Core/Dilepton.h

@@ -946,7 +977,26 @@ struct Dilepton {
      } else if (t1.sign() < 0 && t2.sign() < 0) { // LS--
        fRegistry.fill(HIST("Pair/") + HIST(event_pair_types[ev_id]) + HIST("lsmm/hs"), v12.M(), v12.Pt(), pair_dca, dphi, deta, weight);
      }
-
+    } else if (cfgAnalysisType == static_cast<int>(o2::aod::pwgem::dilepton::utils::pairutil::DileptonAnalysisType::kDCA)) {
+      float deta = v1.Eta() - v2.Eta();


In case of the kDCA, I dont think you need to calculate the dphi and data, right?

hscheid · 2024-10-29T12:18:54Z

PWGEM/Dilepton/Core/Dilepton.h

-        pair_dca = std::sqrt((dca_t1 * dca_t1 + dca_t2 * dca_t2) / 2.);
+      dca3D_t1 = dca3DinSigma(t1);
+      dca3D_t2 = dca3DinSigma(t2);
+      pair_dca3D = std::sqrt((dca3D_t1 * dca3D_t1 + dca3D_t2 * dca3D_t2) / 2.);


For readability: there should be a function to do this operation. My suggestion: just give t1 and t2 as arguments, similar to the single functions above.
There is also no reason to do these operations in cfgDCAcase = 1 and cfgDCAcase = 2.

hscheid · 2024-10-29T12:20:08Z

PWGEM/Dilepton/Core/Dilepton.h

+      if (cfgDCAcase == 1 || cfgDCAcase == 3) {
+        dcaxy_t1 = t1.dcaXY() / std::sqrt(t1.cYY());
+        dcaxy_t2 = t2.dcaXY() / std::sqrt(t2.cYY());
+        pair_dcaxy = std::sqrt((dcaxy_t1 * dcaxy_t1 + dcaxy_t2 * dcaxy_t2) / 2.);


Should be done in a function for readability.
See comment https://github.com/AliceO2Group/O2Physics/pull/8188/files#r1820689972

hscheid · 2024-10-29T12:20:25Z

PWGEM/Dilepton/Core/Dilepton.h

+      if (cfgDCAcase == 2 || cfgDCAcase == 3) {
+        dcaz_t1 = t1.dcaZ() / std::sqrt(t1.cZZ());
+        dcaz_t2 = t2.dcaZ() / std::sqrt(t2.cZZ());
+        pair_dcaz = std::sqrt((dcaz_t1 * dcaz_t1 + dcaz_t2 * dcaz_t2) / 2.);


https://github.com/AliceO2Group/O2Physics/pull/8188/files#r1820689972

hscheid · 2024-10-29T12:22:56Z

PWGEM/Dilepton/Core/Dilepton.h

+        dcaxy_t1 = t1.dcaXY() / std::sqrt(t1.cYY());
+        dcaxy_t2 = t2.dcaXY() / std::sqrt(t2.cYY());
+        pair_dcaxy = std::sqrt((dcaxy_t1 * dcaxy_t1 + dcaxy_t2 * dcaxy_t2) / 2.);
+        pair_dca = pair_dcaxy;


Using this syntax makes case cfgDCAcase = 3 obsolete. You only fill the histogram once.

feisenhu requested review from alibuild, mikesas, rbailhac, dsekihat and ivorobye as code owners October 28, 2024 14:59

github-actions bot added the pwgem label Oct 28, 2024

dsekihat reviewed Oct 28, 2024

View reviewed changes

feisenhu enabled auto-merge (squash) October 28, 2024 21:44

alibuild previously approved these changes Oct 28, 2024

View reviewed changes

dsekihat reviewed Oct 29, 2024

View reviewed changes

dsekihat disabled auto-merge October 29, 2024 00:38

dsekihat requested review from hscheid, dsekihat and alibuild October 29, 2024 00:38

dsekihat reviewed Oct 29, 2024

View reviewed changes

hscheid reviewed Oct 29, 2024

View reviewed changes

feisenhu added 3 commits October 29, 2024 09:40

[PWGEM] Add AnalysisType kDCA to analyse DCA 3D,xy,z together

d7105a4

[PWGEM] Use Int instead of string to select DCA case

d26a7a5

[PWGEM] Remove dEta, dPhi calculation

902ec4b

feisenhu dismissed alibuild’s stale review via 902ec4b October 29, 2024 08:40

feisenhu force-pushed the master branch from 84889cb to 902ec4b Compare October 29, 2024 08:40

hscheid reviewed Oct 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PWGEM] Add AnalysisType kDCA to analyse DCA 3D,xy,z together #8188

[PWGEM] Add AnalysisType kDCA to analyse DCA 3D,xy,z together #8188

feisenhu commented Oct 28, 2024

dsekihat Oct 28, 2024

hscheid Oct 28, 2024

feisenhu Oct 28, 2024

hscheid Oct 29, 2024

hscheid Oct 29, 2024

alibuild left a comment

dsekihat Oct 29, 2024 •

edited

Loading

dsekihat Oct 29, 2024 •

edited

Loading

dsekihat Oct 29, 2024

hscheid Oct 29, 2024

hscheid Oct 29, 2024

hscheid Oct 29, 2024

hscheid Oct 29, 2024

hscheid Oct 29, 2024

[PWGEM] Add AnalysisType kDCA to analyse DCA 3D,xy,z together #8188

Are you sure you want to change the base?

[PWGEM] Add AnalysisType kDCA to analyse DCA 3D,xy,z together #8188

Conversation

feisenhu commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alibuild left a comment

Choose a reason for hiding this comment

dsekihat Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

dsekihat Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsekihat Oct 29, 2024 •

edited

Loading

dsekihat Oct 29, 2024 •

edited

Loading