feat: add options to use spacepoint time information in TripletSeeder, with a new ROOT seeding analysis tool#5524
Conversation
github-actions
Bot
added
Component - Core
andiwand
left a comment
andiwand
left a comment
There was a problem hiding this comment.
hey @wangyanqi191025 - thanks for bringing this to ACTS! I left a few structural comments
There was a problem hiding this comment.
I believe we already have similar writers for this, like RootSeedWriter.hpp RootTrackParameterWriter.hpp. can they be used for your use case?
| const float tM = middleSp.time(); | ||
| const float varianceTM = middleSp.varianceT(); | ||
| const float t0M = | ||
| tM - std::sqrt(rM * rM + zM * zM) / Acts::PhysicalConstants::c; |
There was a problem hiding this comment.
I fear this might have quite some impact on the compute performance of the seeding without time. did you look at this?
I implemented this template mechanism for these kinds of features. that might be the better choice in this case
There was a problem hiding this comment.
Thanks @andiwand for having a look at this PR. CPU consumption is what we also worried about, for which we don't want to influence seeding without time too much. May I ask where can I find the template mechanism you mentioned so I can make some change to this PR? Also I plan to do more test on CPU consumption. Thanks!
There was a problem hiding this comment.
hey @wangyanqi191025 !
the templating mechanism you can find here
- for the doublet finder:
acts/Core/src/Seeding2/DoubletSeedFinder.cpp
Lines 23 to 25 in b040561
- for the triplet finder: https://github.com/acts-project/acts/blob/main/Core/src/Seeding2/TripletSeedFinder.cpp#L27-L28
this allows to generate different version of the seeding code and dispatch to it depending on what is available during runtime. I imagine that we could use this mechanism to use time information and only emit code when needed
| const float varianceTM = spM.varianceT(); | ||
| const float t0M = tM - std::sqrt(spM.zr()[0] * spM.zr()[0] + | ||
| spM.zr()[1] * spM.zr()[1]) / | ||
| Acts::PhysicalConstants::c; |
There was a problem hiding this comment.
note that c=1 in ACTS and we usually don't write the constant into formulas for readability reasons. not wrong but for consistency reasons I would tend to remove them
| for (const auto& [bottom, middle, top, bestSeedQuality, zOrigin, | ||
| qualitySeed] : sortedCandidates) { | ||
| // calculate chi2 of the triplet time compatibility | ||
| if (config().applyTripletTimeFilter) { |
There was a problem hiding this comment.
I believe this filtering should actually be done in the triplet seed finder
2 participants
feat: add options to use spacepoint time information in TripletSeeder, with a new ROOT seeding analysis tool
--- END COMMIT MESSAGE ---
Still a draft for the moment. Some potential concerns:
nanfor time and time variance. even causing problems when no time info in spacepoint?To @XiaocongAi @pbutti