PWGJE: Add task for K* in jets by JimunLee · Pull Request #6798 · AliceO2Group/O2Physics

JimunLee · 2024-07-10T03:08:24Z

Dear Experts,

Hello I am Jimun Lee,
I have recently started working on the K*(892) analysis.
This PR is for reconstructing K* within jets.

Requesting a PR review

Please consider the following formatting changes to #6798

nzardosh

Thanks for the PR, I will finish the rest of my review after we discuss

nzardosh · 2024-07-10T07:32:30Z

.gitignore

@@ -1,31 +1 @@
-CMakeCache.txt


this file should be removed

nzardosh · 2024-07-10T07:32:55Z

PWGJE/Tasks/kstarInJets.cxx

+using namespace o2::framework::expressions;
+
+struct kstarInJets {
+  SliceCache cache;


what is this doing?

nzardosh · 2024-07-10T07:33:22Z

PWGJE/Tasks/kstarInJets.cxx

+  SliceCache cache;
+  HistogramRegistry JEhistos{"JEhistos", {}, OutputObjHandlingPolicy::AnalysisObject};
+
+  HistogramRegistry registry{"registry",


why do you have 2 different registries

nzardosh · 2024-07-10T07:34:05Z

PWGJE/Tasks/kstarInJets.cxx

+                              {"h_part_jet_eta", "particle level jet #eta;#eta_{jet part};entries", {HistType::kTH1F, {{100, -1.0, 1.0}}}},
+                              {"h_part_jet_phi", "particle level jet #phi;#phi_{jet part};entries", {HistType::kTH1F, {{80, -1.0, 7.}}}}}};
+
+  Configurable<std::string> cfgeventSelections{"cfgeventSelections", "sel8", "choose event selection"};


in the JE framework we tend not to start configurables with "cfg"

nzardosh · 2024-07-10T07:35:53Z

PWGJE/Tasks/kstarInJets.cxx

+    JEhistos.add("Resp_Matrix", "Resp_Matrix", HistType::kTHnSparseD, {PtAxis, axisPt, PtAxis, axisPt});                 // REC(Phi,Jet), GEN(Phi,Jet)
+    JEhistos.add("Resp_Matrix_MATCHED", "Resp_Matrix_MATCHED", HistType::kTHnSparseD, {PtAxis, axisPt, PtAxis, axisPt}); // REC(Phi,Jet), GEN(Phi,Jet)
+
+    JEhistos.add("ptGeneratedPion", "ptGeneratedPion", kTH1F, {PtAxis});


it seems you are trying to do all the analysis in histograms instead of trees. is this feasible for systematics

nzardosh · 2024-07-10T07:46:02Z

PWGJE/Tasks/kstarInJets.cxx

+    bool tpcPIDPassed{false}, tofPIDPassed{false};
+    if (std::abs(candidate.tpcNSigmaPi()) < cfgnTPCPID)
+      tpcPIDPassed = true;
+


nzardosh · 2024-07-10T07:48:37Z

PWGJE/Tasks/kstarInJets.cxx

+    if (!trackPIDKaon(trk1) || !trackPIDPion(trk2))
+      return;
+
+    if (trk1.index() == trk2.index())


you should put this first

and it should be globalIndex()

nzardosh · 2024-07-10T07:54:51Z

PWGJE/Tasks/kstarInJets.cxx

+    if (!jetderiveddatautilities::selectCollision(collision, jetderiveddatautilities::JCollisionSel::sel8))
+      return;
+
+    for (auto& [track1, track2] : combinations(o2::soa::CombinationsFullIndexPolicy(tracks, tracks))) {


can you point me to what this does and where its defined?

why do you do this for all pairs of tracks and not only the ones inside jets?
Maybe you intend these to be the jet tracks only but they are not. You have to do jer.tracks_as<soa::Join<aod::JTracks, aod::JTrackPIs>>()

so this then should be done inside the charged jet look for each jet

nzardosh · 2024-07-10T08:01:55Z

PWGJE/Tasks/kstarInJets.cxx

+    bool INELgt0 = false;
+    for (const auto& track : tracks) {
+      if (fabs(track.eta()) < cfgtrkMaxEta) {
+        INELgt0 = true;


why is this only done for reco and not for data

nzardosh · 2024-07-10T08:05:56Z

PWGJE/Tasks/kstarInJets.cxx

+            continue;
+        }
+
+        if (originalTrack.index() >= originalTrack2.index())


all of these should be globalIndex() ?

I think you should reformaulate the way you do the for loops. So that the second for loop over tracks starts from the track index after the current one in the first loop

vkucera · 2024-07-11T09:29:18Z

Hi @JimunLee , I have converted your PR to a draft, because it cannot be merged as it is now for several reasons, so there is no point running the compilation tests yet. You have wrongly removed .gitignore and committed PWGJE/.DS_Store and compile_commands.json. Please fix it and be careful to only commit files that concern your work.

Your task is derived from Adrian's task phiInJets.cxx. It would be more efficient to merge both analyses into a single file to avoid large duplication of code that both analyses have in common.

vkucera · 2024-07-14T06:22:23Z

@JimunLee There is no need for you to run clang-format. The formatting changes can be applied with the automatic PR. Please follow the instructions.

JimunLee · 2024-07-14T07:26:21Z

@JimunLee There is no need for you to run clang-format. The formatting changes can be applied with the automatic PR. Please follow the instructions.

Ah, Thank you ! Then, Can I just click on the link and proceed with the process by pressing "Merge"?

vkucera · 2024-07-14T08:28:08Z

@JimunLee There is no need for you to run clang-format. The formatting changes can be applied with the automatic PR. Please follow the instructions.

Ah, Thank you ! Then, Can I just click on the link and proceed with the process by pressing "Merge"?

Yes

Please consider the following formatting changes to #6798 yes, I did follow this process

jimun_lee added 4 commits July 8, 2024 15:48

Create kstarInJets.cxx

c4162c8

modified CMakeLists.txt

6aae1a0

Modify kstarInJets.cxx&CMakeLists.txt

f8d0edf

Modify authors

bf865f7

JimunLee requested review from a team, alibuild, ddobrigk, iarsene, jgrosseo, ktf, maoyx, mfasDa and nzardosh as code owners July 10, 2024 03:08

alibuild and others added 7 commits July 10, 2024 03:09

Please consider the following formatting changes

2ebf8a1

Merge pull request #1 from alibuild/alibot-cleanup-6798

03ee905

Please consider the following formatting changes to #6798

modify tap to space

4313d43

Merge branch 'master' of https://github.com/JimunLee/O2Physics

850b2c7

Apply clang-format changes

49433e8

Merge branch 'master' of https://github.com/AliceO2Group/O2Physics

bfd00a5

Remove temporary file

c239e5d

nzardosh reviewed Jul 10, 2024

View reviewed changes

Remove .gitignore file

731dfc4

vkucera marked this pull request as draft July 11, 2024 08:05

vkucera changed the title ~~Pull Request for PWGJE/Tasks/kstarInJets.cxx~~ PWGJE: Add tasks for K* in jets Jul 11, 2024

vkucera changed the title ~~PWGJE: Add tasks for K* in jets~~ PWGJE: Add task for K* in jets Jul 11, 2024

jimun_lee added 4 commits July 11, 2024 20:01

Remove unnecessary files

41da46b

Remove unnecessary files

9e1a0b0

Change tab to space

a0d45f6

Remove the trailing spaces

f470798

jimun_lee and others added 2 commits July 13, 2024 14:31

clang-format

618cf94

Please consider the following formatting changes

d5c2212

Merge pull request #3 from alibuild/alibot-cleanup-6798

70aca4a

Please consider the following formatting changes to #6798 yes, I did follow this process

JimunLee closed this Jul 18, 2024

		@@ -1,31 +1 @@
		CMakeCache.txt

Conversation

JimunLee commented Jul 10, 2024

Uh oh!

nzardosh left a comment

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!

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

Uh oh!

Uh oh!

vkucera commented Jul 14, 2024

Uh oh!

JimunLee commented Jul 14, 2024

Uh oh!

vkucera commented Jul 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

vkucera commented Jul 11, 2024 •

edited

Loading