[PWGJE] Add Jet-hadron correlations task into the JE framework #13847
Conversation
yhou-git
requested review from
alibuild,
fjonasALICE,
maoyx,
mfasDa,
mhemmer-cern and
nzardosh
as code owners
|
O2 linter results: ❌ 1 errors, |
Please consider the following formatting changes to AliceO2Group#13847
|
Hi. Please squash your commits so we can perform a review |
Hello Nima, I'm not quite ready yet, but I will be ready for review once it's sorted. Thanks, Yongzhen |
yhou-git
force-pushed
the
jethadronBranch
branch
from
7ea1e7c to
80fcbc9
Compare
|
@yhou-git Please use a meaningful PR title. |
yhou-git
changed the title
Hi Vit, thanks for your reminder, have updated it, thanks a lots. Yongzhen |
Thanks to you! |
|
Hi @yhou-git thanks for letting me know. In general if you are developing your own task its best not to open the PR until you have it ready. In case you are working on a core part of the framework and need intermediate reviews you can open the PR in draft mode |
Hi @nzardosh, thanks! My task is now ready for review. There are no further changes planned at the moment. please let me know if anything needs adjustment. Thanks again, yongzhen |
yhou-git
force-pushed
the
jethadronBranch
branch
from
9812168 to
8331d11
Compare
nzardosh
left a comment
nzardosh
left a comment
There was a problem hiding this comment.
Dear @yhou-git Thanks for the PR. In general I think the code is quite a bit more complicated than it needs to be in places. I have tried to highlight examples so you can go through the code and fix each time the example appears. Thanks!
| template <typename TCollision> | ||
| float getCentrality(const TCollision& coll) const | ||
| { | ||
| if (cfgCentEstimator.value == 0) |
There was a problem hiding this comment.
why do you need the .value here instead of calling the variable directly?
There was a problem hiding this comment.
I need this centrality value to work for different systems. Pb-Pb uses FT0C for centrality, but pp uses FT0M.
Using .value makes the code flexible for both, or other systems.
PWGJE/Tasks/chargedJetHadron.cxx
Outdated
| // ========================================================== | ||
| template <typename TCollision> | ||
| bool isGoodCollision(const TCollision& coll, | ||
| const std::vector<int>& eventSelectionBits, bool skipMBGapEvents, |
There was a problem hiding this comment.
all the variables other than coll are defined globally in your struct and do not need to be passed to the function
There was a problem hiding this comment.
I see your point and will test it. My idea was to simplify the selections inside each process.
PWGJE/Tasks/chargedJetHadron.cxx
Outdated
| return false; | ||
| } | ||
| float cent = getCentrality(coll); | ||
| if (cent < centralityMin || cent > centralityMax) { |
There was a problem hiding this comment.
isnt this already taken care of by the eventCuts filter?
There was a problem hiding this comment.
Yes, it is already included.
I only added the extra lines as an additional safety check.
PWGJE/Tasks/chargedJetHadron.cxx
Outdated
| if (cent < centralityMin || cent > centralityMax) { | ||
| return false; | ||
| } | ||
| if (std::abs(coll.posZ()) > vertexZCut) { |
There was a problem hiding this comment.
this can go in the eventCuts filter too
There was a problem hiding this comment.
You are right, let me consider whether to retain them or remove them. I shall first test whether removing them changes my results.
PWGJE/Tasks/chargedJetHadron.cxx
Outdated
| double constituentPtMin = -98.0; | ||
| double constituentPtMax = 9998.0; | ||
| if (jetAreaFractionMin > jetAreaLimit) { | ||
| if (jet.area() < jetAreaFractionMin * o2::constants::math::PI * (jet.r() / 100.0) * (jet.r() / 100.0)) { |
There was a problem hiding this comment.
this cut is typically done in the jetFinder already, but you can do it here too if you want to be safe
There was a problem hiding this comment.
Thanks very much for pointing it out. I will check if it influences.
| auto tracksTuple = std::make_tuple(jets, tracks); | ||
| Pair<TCollisions, TJets, TTracks, BinningType> pairData{corrBinning, numberEventsMixed, -1, collisions, tracksTuple, &cache}; | ||
|
|
||
| for (const auto& [c1, jets1, c2, tracks2] : pairData) { |
There was a problem hiding this comment.
how does this work? what is it iterating over?
There was a problem hiding this comment.
This part follows the event mixing tutorial.
It builds the mixing pools and iterates over the mixed event pairs.
https://github.com/AliceO2Group/O2Physics/blob/master/Tutorials/src/eventMixing.cxx#L220C25-L220C28.
PWGJE/Tasks/chargedJetHadron.cxx
Outdated
| if (!jetderiveddatautilities::selectCollision(collision, eventSelectionBits, skipMBGapEvents)) { | ||
| continue; | ||
| } | ||
| if (collision.trackOccupancyInTimeRange() < trackOccupancyInTimeRangeMin || trackOccupancyInTimeRangeMax < collision.trackOccupancyInTimeRange()) { |
There was a problem hiding this comment.
these can all be in filters
There was a problem hiding this comment.
Ok, I see and will try to improve it, thanks
PWGJE/Tasks/chargedJetHadron.cxx
Outdated
|
|
||
| if (acceptSplitCollisions == SplitOkCheckFirstAssocCollOnly || acceptSplitCollisions == NonSplitOnly) { | ||
| centrality = getCentrality(collisions.begin()); | ||
| multiplicity = getMultiplicity(collisions.begin()); |
There was a problem hiding this comment.
does the SmallGroups essentially slice the collision table?
There was a problem hiding this comment.
I think so, smallgroups gives only the collisions associated to the current mccollision.
PWGJE/Tasks/chargedJetHadron.cxx
Outdated
| } | ||
| } | ||
| } | ||
| if (!hasSel8Coll || !centralityIsGood || !occupancyIsGood) { |
There was a problem hiding this comment.
instead of doing this you can put a return in each if statement. For example
if (!jetderiveddatautilities::selectCollision(collision, eventSelectionBits, skipMBGapEvents)) {
return;
}
PWGJE/Tasks/chargedJetHadron.cxx
Outdated
| } | ||
| float jetweight = jet.eventWeight(); | ||
| double pTHat = 10. / (std::pow(jetweight, 1.0 / pTHatExponent)); | ||
| for (int N = 1; N < ptHadbins; N++) { |
There was a problem hiding this comment.
what is this pTHadbins doing?
There was a problem hiding this comment.
Sorry, the purpose was to perform the JJ samples. But I have not updated this section recently. I would like to first review my data points, mainly focusing on the Data and MCD levels. I will update this process accordingly. This pTHadbins value and the related loop will also be removed.
Dear Nima @nzardosh, thank you very for the review and suggestions. I will go through this task, simplify it as recommended, and update the PR. I will let you know once the optimizations are completed. Thanks again. |
yhou-git
force-pushed
the
jethadronBranch
branch
from
0e8106f to
d7a9deb
Compare
3 participants
No description provided.