Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

feisenhu
Copy link
Collaborator

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.

@@ -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)"};
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@feisenhu feisenhu enabled auto-merge (squash) October 28, 2024 21:44
alibuild
alibuild previously approved these changes Oct 28, 2024
Copy link
Collaborator

@alibuild alibuild left a comment

Choose a reason for hiding this comment

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

Auto-approving on behalf of @feisenhu.

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

@dsekihat dsekihat Oct 29, 2024

Choose a reason for hiding this comment

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

If cfgDCAcase = 3, what happens?

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) {
Copy link
Collaborator

@dsekihat dsekihat Oct 29, 2024

Choose a reason for hiding this comment

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

If cfgDCAcase = 3, what happens?

@@ -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)"};
Copy link
Collaborator

Choose a reason for hiding this comment

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

cfgDCAcase = 3 should not exist.

@@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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.);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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.);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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.);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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