fix: Particle selector works on measurements, not simhits #3856
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Examples
There was a problem hiding this comment.
ah that went in accidentally
|
I'm not sure I understand when this makes a difference. Is it if we implement some kind of module inefficiency? When is the number of measurements per particle different from the number of truth hits? |
|
Yeah so I think simulated hit and digitized measurement are different concepts, right? E.g., geometric clustering could filter out clusters with a threshold, we can drop hits based on energy deposit, there could be split clusters... |
|
|
@benjaminhuth Right, that's what I was thinking of with module inefficiency. I guess a minimum charge threshold could have an affect like you say. Split clusters don't currently exist I don't think, and in that case we go from multiple hits to a single merged cluster that is then split again later using reco information only. In case of merged clusters, the cluster would be associated with both source particles. I think with how you implement this here, that should be ok, as the measurement is mapped back to each simhits particles, so would survive the map reversal you add here. |
| // Optionally construct a particles->measurement map | ||
| std::optional<boost::container::flat_multimap<ActsFatras::Barcode, Index>> | ||
| partMeasMap; | ||
| if (m_inputMeasPartMap.isInitialized()) { | ||
| const auto& measPartMap = m_inputMeasPartMap(ctx); | ||
| using InvPair = std::pair<ActsFatras::Barcode, Index>; | ||
| std::vector<InvPair> v(measPartMap.size()); | ||
| std::ranges::transform(measPartMap, v.begin(), [](const auto& p) { | ||
| return InvPair{p.second, p.first}; | ||
| }); | ||
| std::ranges::sort(v, std::less{}, &InvPair::first); | ||
| partMeasMap.emplace(boost::container::ordered_range_t{}, v.begin(), | ||
| v.end()); | ||
| } |
There was a problem hiding this comment.
I thought this inverted map is somewhere generated already and put on the whiteboard?
Also, isn't there a utility to do this kind of transformation? I can also try to look it up
|
Thanks for following up on this! I overlooked that hits are not enough for this |
The particle selection did work on the simhit count, but usually we rather desire to select on the actual number of measurements in the detector, which can be different.
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!