ITS: add back the code for track following#15456
Conversation
mpuccio
requested review from
fprino,
mconcas,
mcoquet642 and
shahor02
as code owners
f3sch
left a comment
f3sch
left a comment
There was a problem hiding this comment.
Nice work, still have to go through the actual meat of the code but maybe this can be a starting point. Most comments are only of cosmetic nature
| } | ||
|
|
||
| // return clamped ROF range with strictly positive overlap with timestamp interval | ||
| GPUhdi() int2 getROFRange(TimeStamp ts) const noexcept |
There was a problem hiding this comment.
Let's stay consistent and use the RangeReference here instead of int2, e.g. adding to the class a using BCRange = RangeReference<BCType,BCType>;
| if (mNROFsTF == 0) { | ||
| return {1, 0}; | ||
| } |
There was a problem hiding this comment.
This makes no sense to me, how can we ever have zero ROFs in a TF?
| template <int NLayers> | ||
| GPUhdi() constexpr uint32_t makeAddedClustersPatternMask() | ||
| { | ||
| return (NLayers >= 32) ? 0xffffffffu : ((1u << NLayers) - 1u); | ||
| } | ||
|
|
||
| template <int NLayers> | ||
| GPUhdi() void applyExtendedClustersPattern(TrackITSExt& track, uint32_t diff) | ||
| { | ||
| diff &= makeAddedClustersPatternMask<NLayers>(); | ||
| track.setUserField(static_cast<uint16_t>(diff)); | ||
| if constexpr (NLayers <= kMaxLayersInTrackPattern) { | ||
| track.setPattern(track.getPattern() | (diff << kExtendedPatternShift)); | ||
| } else { | ||
| (void)track; | ||
| } | ||
| } | ||
|
|
||
| template <int NLayers> | ||
| GPUhdi() uint32_t getAddedClustersPattern(const TrackITSExt& track) | ||
| { | ||
| const auto mask = makeAddedClustersPatternMask<NLayers>(); | ||
| if constexpr (NLayers <= kMaxLayersInTrackPattern) { | ||
| const auto diff = (track.getPattern() >> kExtendedPatternShift) & mask; | ||
| if (diff) { | ||
| return diff; | ||
| } | ||
| } | ||
| return track.getUserField() & mask; | ||
| } | ||
|
|
||
| GPUhdi() void clearAddedClustersPattern(TrackITSExt& track) | ||
| { | ||
| track.setUserField(0); | ||
| track.getParamOut().setUserField(0); | ||
| } |
There was a problem hiding this comment.
Arguably, these only act on the class itself and should thus be part of it and not be in some other header.
| inline constexpr unsigned int kExtendedPatternShift = 24; | ||
| inline constexpr int kMaxLayersInTrackPattern = 8; |
| TrackITSExt track; | ||
| }; | ||
|
|
||
| inline constexpr int MaxTrackExtensionCandidatesPerTrack = 4; |
There was a problem hiding this comment.
Constants should go into the Constants.h header
|
|
||
| template <int NLayers> | ||
| struct TrackExtensionCandidate { | ||
| static constexpr float InvalidChi2 = 1.e20f; |
There was a problem hiding this comment.
Maybe use the constants::UnsetValue?
| template <int NLayers> | ||
| GPUhdi() bool isBetterTrackExtensionCandidate(const TrackExtensionCandidate<NLayers>& a, const TrackExtensionCandidate<NLayers>& b) | ||
| { | ||
| return (a.nAddedClusters > b.nAddedClusters) || (a.nAddedClusters == b.nAddedClusters && a.chi2 < b.chi2); | ||
| } |
There was a problem hiding this comment.
This and isBetterTrackExtensionHypothesis do essentially the same thing, is there not a way to reuse this logic? Also I would prefer to make these members of the classes themselves since it spiritually is the same as the TrackITS::isBetter.
There was a problem hiding this comment.
Although the same argument should have been made for track::isBetter
| template <int NLayers> | ||
| GPUhdi() void initialiseTrackExtensionHypothesis(const TrackITSExt& track, | ||
| const bool outward, | ||
| TrackExtensionHypothesis<NLayers>& hypo) | ||
| { | ||
| hypo.param = outward ? track.getParamOut() : track.getParamIn(); | ||
| hypo.time = track.getTimeStamp(); | ||
| hypo.chi2 = track.getChi2(); | ||
| hypo.nClusters = track.getNClusters(); | ||
| hypo.edgeLayer = outward ? getTrackExtensionLastClusterLayer<NLayers>(track) : getTrackExtensionFirstClusterLayer<NLayers>(track); | ||
| for (int iLayer{0}; iLayer < NLayers; ++iLayer) { | ||
| hypo.clusters[iLayer] = track.getClusterIndex(iLayer); | ||
| } | ||
| } |
There was a problem hiding this comment.
To me this seems also something be made as part of the class, e.g., in the constructor or as a initialiseFromTrack function.
| const auto rofTS = rofOverlaps.getLayer(iLayer).getROFTimeBounds(rof, true); | ||
| const auto& ts = updated.time; | ||
| const float lower = o2::gpu::CAMath::Max(ts.getTimeStamp() - ts.getTimeStampError(), static_cast<float>(rofTS.lower())); | ||
| const float upper = o2::gpu::CAMath::Min(ts.getTimeStamp() + ts.getTimeStampError(), static_cast<float>(rofTS.upper())); | ||
| updated.time.setTimeStamp(0.5f * (lower + upper)); | ||
| updated.time.setTimeStampError(0.5f * (upper - lower)); | ||
| addTrackExtensionHypothesisToBeam(updated, nextHypotheses, nNext, beamWidth); |
There was a problem hiding this comment.
We should not use the symmetric timestamp in 'intermediate' artefacts, lets stay consistent and ensure to only use the asymmetrical one, otherwise we have to argue what artefact uses what.
| #include "ITStracking/Cell.h" | ||
| #include "ITStracking/BoundedAllocator.h" | ||
| #include "DataFormatsITS/TimeEstBC.h" | ||
| #include "ReconstructionDataFormats/Track.h" |
There was a problem hiding this comment.
Do we actually need this header?
2 participants
By default, this is disabled for ITS, but it is required for ALICE3.
This changes the findRoads to avoid the double pass (first count then fill) on CPU, leading to marginal gain on speed while keeping the memory footprint equal (checked on a critical PbPb CTF that Felix gave me)