Conversation
|
O2 linter results: ❌ 0 errors, |
vkucera
left a comment
There was a problem hiding this comment.
@JinjooSeo Please add the location in the PR title. (See the contribution guidelines.)
@vkucera PWGDQ is already attached do I need to add another location? |
Yes. PWGDQ is just a tag. Did you read the guidelines? |
vkucera
left a comment
There was a problem hiding this comment.
Thanks for fixing the title.
mguilbau
left a comment
There was a problem hiding this comment.
Hi @JinjooSeo,
Could you please fix the o2 linter error here? You have duplicated some existing one so I would kindly ask you to fix these
fc7adcd to
3d4aefe
Compare
Hi @mguilbau Thanks! I've made a fix, but I'm not sure if it's what you meant. could you take a look and let me know if there iss anything else? |
5b56f1e to
3804722
Compare
| // globalIndex is unique only within a dataframe, so the dataframe sequence is part of the track identity | ||
| uint64_t dataFrameSequence = 0; | ||
| uint64_t trackGlobalIndex = 0; | ||
| // electric charge of the track; 0 means "not set" and disables the charge dependent pair variables | ||
| int8_t sign = 0; | ||
| bool IsSamePhysicalTrack(const MixingTrack& other) const | ||
| { | ||
| return dataFrameSequence == other.dataFrameSequence && trackGlobalIndex == other.trackGlobalIndex; | ||
| } |
There was a problem hiding this comment.
I don't think that this is needed. The tracks and events added to the mixing pools are different and don't need the DF or track index, neither the charge. For example, to distinguish the tracks when doing like-sign, one only needs to compare the iterators. Also, regarding the sign, the events are organized already such that they have two different track lists. In this particular case they are separated based on charge, but in other cases these can be particle species, or whatever (trigger and assoc particle, etc.). So this is a specialization that is not needed here, but it is implemented at the place where the mixing handler is actually used. So, since we need to add additional variables in this struct that are going to be needed for other purposes (such as mixing for flow, polarization, correlations), I strongly suggest to not add these new members. I agree they might be nice to have, but this is a part of code that is intensively used in our workflows and we should keep it at a minimum required.
There was a problem hiding this comment.
Thanks Ionut! I fully agree with you and I reverted them.
Just for the record, it was catching a track associated with two collisions of the same DF getting paired with itself in LS. Both copies have identical kinematics, so it will always be at 1 MeV LS only. Negligible for our analysis, but worth keeping in mind for possible analysis (maybe correlation..?)
| // dataframe counter, part of the track identity in the mixing pools | ||
| uint64_t fMixingDataFrameSequence = 0; |
There was a problem hiding this comment.
As commented above, this is not needed.
| uint64_t currentMixingDataFrameSequence = 0; | ||
| if (fConfigRunMixingAcrossTFs) { | ||
| currentMixingDataFrameSequence = ++fMixingDataFrameSequence; | ||
| } |
| } | ||
| auto t1 = assoc.template reducedtrack_as<TTracks>(); | ||
| MixingHandler::MixingTrack mixingTrack(t1.pt(), t1.eta(), t1.phi(), trackFilterForMixing); | ||
| MixingHandler::MixingTrack mixingTrack(t1.pt(), t1.eta(), t1.phi(), trackFilterForMixing, currentMixingDataFrameSequence, static_cast<uint64_t>(assoc.reducedtrackId()), static_cast<int8_t>(t1.sign())); |
There was a problem hiding this comment.
As commented above, we should not need these additional members.
| // check the two-track filter for the mixed pair and skip the same track associated to both collisions | ||
| uint32_t mixedTwoTrackFilter = t1.filteringFlags & t2.filteringFlags; | ||
| if (!mixedTwoTrackFilter) { | ||
| if (!mixedTwoTrackFilter || t1.IsSamePhysicalTrack(t2)) { |
There was a problem hiding this comment.
the check should be removed, you can just make a comparison of the iterators instead
| // check the two-track filter for the mixed pair and skip the same track associated to both collisions | ||
| uint32_t mixedTwoTrackFilter = t1.filteringFlags & t2.filteringFlags; | ||
| if (!mixedTwoTrackFilter) { | ||
| if (!mixedTwoTrackFilter || t1.IsSamePhysicalTrack(t2)) { |
There was a problem hiding this comment.
the check should be replaced with a comparison of the iterators
3804722 to
54969bf
Compare
Problems
UpdatePool()ages each stored event with its own mask instead of the arriving event's, socfgMixingDepthcounts arrivals rather than mixed partners: a selective track cut gets far fewer partners than a loose oneChanges
UpdatePool(event, poolDepth, agingMask): the across-TF path passes the mask of the arriving event, so an event is aged for a cut only when a mixed pair was produced for it