feat: Allow tracksummary plots on detector subparts #4058
Conversation
WalkthroughHmm, changes to tracking and performance writing, we have! Modifications span multiple files in the Examples framework, they do. Logging adjustments, method signatures expanded, new configuration options for sub-detector tracking introduced. Subtle yet significant improvements to track analysis capabilities, these changes represent. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Examples/Io/Root/src/TrackFinderPerformanceWriter.cpp (1)
234-257: Refactor the track state processing logic, we should!Complex, this section has become. Extract track state processing to a separate method, we must. Improve readability and maintainability, it will.
+ private: + void processTrackStatesForVolumes( + const Track& track, + const Acts::BoundTrackParameters& fittedParameters, + const std::string& key, + const std::set<int>& volumes) { + std::size_t nTrackStates{}, nMeasurements{}, nOutliers{}, nHoles{}, + nSharedHits{}; + + for (auto state : track.trackStatesReversed()) { + if (!state.hasReferenceSurface() || + !volumes.contains(state.referenceSurface().geometryId().volume())) { + continue; + } + + nTrackStates++; + nMeasurements += static_cast<std::size_t>( + state.typeFlags().test(Acts::MeasurementFlag)); + nOutliers += + static_cast<std::size_t>(state.typeFlags().test(Acts::OutlierFlag)); + nHoles += + static_cast<std::size_t>(state.typeFlags().test(Acts::HoleFlag)); + nSharedHits += static_cast<std::size_t>( + state.typeFlags().test(Acts::SharedHitFlag)); + } + + m_trackSummaryPlotTool.fill(m_subDetectorSummaryCaches.at(key), + fittedParameters, nTrackStates, nMeasurements, + nOutliers, nHoles, nSharedHits); + }Replace the existing loop with:
for (const auto& [key, volumes] : m_cfg.subDetectorTrackSummaryVolumes) { ACTS_VERBOSE("Fill track summary stats for subset " << key); - std::size_t nTrackStates{}, nMeasurements{}, nOutliers{}, nHoles{}, - nSharedHits{}; - for (auto state : track.trackStatesReversed()) { - if (!state.hasReferenceSurface() || - !volumes.contains(state.referenceSurface().geometryId().volume())) { - continue; - } - - nTrackStates++; - nMeasurements += static_cast<std::size_t>( - state.typeFlags().test(Acts::MeasurementFlag)); - nOutliers += - static_cast<std::size_t>(state.typeFlags().test(Acts::OutlierFlag)); - nHoles += - static_cast<std::size_t>(state.typeFlags().test(Acts::HoleFlag)); - nSharedHits += static_cast<std::size_t>( - state.typeFlags().test(Acts::SharedHitFlag)); - } - m_trackSummaryPlotTool.fill(m_subDetectorSummaryCaches.at(key), - fittedParameters, nTrackStates, nMeasurements, - nOutliers, nHoles, nSharedHits); + processTrackStatesForVolumes(track, fittedParameters, key, volumes); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackTruthMatcher.cpp(1 hunks)Examples/Framework/include/ActsExamples/Validation/TrackSummaryPlotTool.hpp(1 hunks)Examples/Framework/src/Validation/TrackSummaryPlotTool.cpp(1 hunks)Examples/Io/Root/include/ActsExamples/Io/Root/TrackFinderPerformanceWriter.hpp(2 hunks)Examples/Io/Root/src/TrackFinderPerformanceWriter.cpp(5 hunks)Examples/Python/src/Output.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TrackTruthMatcher.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (8)
Examples/Framework/include/ActsExamples/Validation/TrackSummaryPlotTool.hpp (1)
62-64: Approve the method signature change, I do!Well-documented and backward compatible, this change is. Support for sub-detector tracking plots, it provides.
Examples/Io/Root/include/ActsExamples/Io/Root/TrackFinderPerformanceWriter.hpp (2)
66-68: Wise addition to the Config struct, this is!Clear documentation and flexible configuration for detector regions, you provide. The Force is strong with this design.
104-105: Balanced and symmetrical, this member variable is!Mirror the Config structure, it does. Proper organization of sub-detector caches, it maintains.
Examples/Framework/src/Validation/TrackSummaryPlotTool.cpp (2)
28-32: Elegant solution for prefix handling, you have created!Lambda function
addPrefix, clarity it brings. Debug logging, improved it is.
34-71: Consistent and clean, the histogram creation remains!Wisely applied, the prefix handling is. Unchanged, the core functionality stays.
Examples/Io/Root/src/TrackFinderPerformanceWriter.cpp (2)
77-79: Balanced initialization and cleanup, achieved you have!Properly managed, the sub-detector caches are. Constructor and destructor, in harmony they work.
Also applies to: 87-89
176-177: Improved logging wisdom, you have shown!Clear visibility into track processing, these messages provide. Help in debugging, they will.
Also applies to: 272-278
Examples/Python/src/Output.cpp (1)
401-407: Approve the changes, I do! Yet verify parameter ordering, we must.Hmm, correctly placed the new parameter
subDetectorTrackSummaryVolumesis, grouped with other plot configurations it remains. Yet ensure consistent with C++ class declaration, the parameter ordering must be.✅ Verification successful
Correct, the parameter ordering is! Strong with the Force, this implementation is.
In perfect harmony with the C++ Config struct, the Python bindings are. Each parameter, in its rightful place it stands, from
inputTrackstowriteMatchingDetails. The new parametersubDetectorTrackSummaryVolumes, wisely placed it is, among its plot configuration companions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify parameter ordering consistency between Python bindings and C++ class declaration # Search for TrackFinderPerformanceWriter class declaration ast-grep --pattern 'class TrackFinderPerformanceWriter { $$$ struct Config { $$$ }; $$$ }'Length of output: 104
Script:
#!/bin/bash # Search for TrackFinderPerformanceWriter class and its Config struct rg -l "class TrackFinderPerformanceWriter" # Once we find the file, we'll examine the Config struct rg "struct Config" -A 20 -B 2 "TrackFinderPerformanceWriter"Length of output: 253
Script:
#!/bin/bash # Examine the Config struct in the header file cat "Examples/Io/Root/include/ActsExamples/Io/Root/TrackFinderPerformanceWriter.hpp"Length of output: 4780
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Examples/Io/Root/src/TrackFinderPerformanceWriter.cpp (4)
87-89: Safe cleanup, ensure we must!Throwing exceptions during cleanup, dangerous it is. Use operator[] instead of .at(), we should.
for (const auto& [key, _] : m_cfg.subDetectorTrackSummaryVolumes) { - m_trackSummaryPlotTool.clear(m_subDetectorSummaryCaches.at(key)); + m_trackSummaryPlotTool.clear(m_subDetectorSummaryCaches[key]); }
140-143: Handle write failures, we must!Hmm, silent failures in writing, dangerous they are. Check write status, we should.
for (const auto& [key, _] : m_cfg.subDetectorTrackSummaryVolumes) { - m_trackSummaryPlotTool.write(m_subDetectorSummaryCaches.at(key)); + if (!m_trackSummaryPlotTool.write(m_subDetectorSummaryCaches[key])) { + ACTS_WARNING("Failed to write summary cache for volume " << key); + } }
176-177: Improved logging messages, provide we should!More context in debug messages, helpful it would be. Track processing progress, show we must.
- ACTS_DEBUG("Collect information from " << tracks.size() << " tracks"); + ACTS_DEBUG("Begin processing " << tracks.size() << " tracks for event " << ctx.eventNumber); std::size_t unmatched = 0, missingRefSurface = 0; for (const auto& track : tracks) { // ... existing code ... if (!track.hasReferenceSurface()) { - ACTS_VERBOSE("No fitted track parameters for track, index = " - << track.index() << " tip index = " << track.tipIndex()); + ACTS_VERBOSE("Track " << track.index() << " (tip: " << track.tipIndex() + << ") missing fitted parameters at reference surface");Also applies to: 184-186
255-261: Debug information, enhance we shall!More detailed statistics, provide we must. Help in debugging track matching issues, it will.
if (unmatched > 0) { - ACTS_DEBUG("No matching information found for " << unmatched << " tracks"); + ACTS_DEBUG("Track matching summary: " << unmatched << "/" << tracks.size() + << " tracks unmatched (" + << (100.0 * unmatched / tracks.size()) << "%)"); } if (missingRefSurface > 0) { - ACTS_DEBUG("Reference surface was missing for " << missingRefSurface - << " tracks"); + ACTS_DEBUG("Reference surface missing for " << missingRefSurface << "/" + << tracks.size() << " tracks (" + << (100.0 * missingRefSurface / tracks.size()) << "%)"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Examples/Io/Root/src/TrackFinderPerformanceWriter.cpp(5 hunks)
🔇 Additional comments (1)
Examples/Io/Root/src/TrackFinderPerformanceWriter.cpp (1)
77-79: Validate the sub-detector volumes configuration, you must!Hmm, missing validation of volumes configuration, I sense. Empty volumes list, handle gracefully we should.
Add validation before the loop:
+ if (!m_cfg.subDetectorTrackSummaryVolumes.empty()) { for (const auto& [key, _] : m_cfg.subDetectorTrackSummaryVolumes) { m_trackSummaryPlotTool.book(m_subDetectorSummaryCaches[key], key); } + }
|
Hmm so I think the issue is that you never want efficiency/fakerate/duprate plots on subparts of the detector, right? But only hit content plots e.g... So multiple writer does not make as much sense I think... |
Potentially this is not the best writer to add it to I guess. Maybe the track state summary with a volume constrain it more suitable? |
|
Hmm but that does not do plots right? that only makes flat TTrees, right? |
|
I actually think that this is quite suited here, since when investigating the finding performance, one usually wants to look at all those hit content plots together, right? Like in IDPVM, where all these plots are under the same category. |
|
True for plotting this makes sense I think. I guess for finding performance this only tells you one side of the coin which is about the tracks you found but not about those you did not find I am just a bit worried that we add a bunch of semi-generic region specifier to our writers and they become unmaintainable again. I spent quite some time cleaning them up in the past |
|
Of course the new plots don't show the full truth (as none single plot does), but I think they are fairly common metrics, and I think it is good if we support those kind of plots. Of course one can alway handcraft those, but its kind of cumbersome. I see your fear of making the whole thing more convoluted, but I think that makes relatively good use of the existing infrastructure... |
|
Adds some infrastructure to produce additonal plots for detector regions (specified with a list of volume ids) in the `TrackFinderPerformanceWriter`. Also adjusts some log levels to be less verbose in `DEBUG` mode.
Adds some infrastructure to produce additonal plots for detector regions (specified with a list of volume ids) in the
TrackFinderPerformanceWriter. Also adjusts some log levels to be less verbose inDEBUGmode.--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates