HFTrackEfficiency updates#4232
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe changes enhance the HFTrackEfficiency module by adding azimuthal angle (phi) storage for mothers and daughter tracks, replacing a relative index offset ( Changes
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d252e1d6-dfbd-4dde-8fa7-a07d00f0a80b
📒 Files selected for processing (2)
offline/packages/HFTrackEfficiency/HFTrackEfficiency.ccoffline/packages/HFTrackEfficiency/HFTrackEfficiency.h
Comment on lines
393
to
+401
| if (m_dst_track->get_silicon_seed()) | ||
| { | ||
| m_reco_track_silicon_seeds[i - 1] = static_cast<int>(m_dst_track->get_silicon_seed()->size_cluster_keys()); | ||
| m_reco_track_silicon_seeds[index] = static_cast<int>(m_dst_track->get_silicon_seed()->size_cluster_keys()); | ||
| } | ||
| else | ||
| { | ||
| m_reco_track_silicon_seeds[i - 1] = 0; | ||
| m_reco_track_silicon_seeds[index] = 0; | ||
| } | ||
| m_reco_track_tpc_seeds[i - 1] = static_cast<int>(m_dst_track->get_tpc_seed()->size_cluster_keys()); | ||
| m_min_reco_track_pT = std::min(m_reco_track_pT[i - 1], m_min_reco_track_pT); | ||
| m_max_reco_track_pT = std::max(m_reco_track_pT[i - 1], m_max_reco_track_pT); | ||
| m_reco_track_tpc_seeds[index] = static_cast<int>(m_dst_track->get_tpc_seed()->size_cluster_keys()); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check SvtxTrack interface for get_tpc_seed() return semantics and existing null-check patterns
rg -n -C3 'get_tpc_seed\(\)' --type=cppRepository: sPHENIX-Collaboration/coresoftware
Length of output: 43826
Potential null pointer dereference: get_tpc_seed() not checked before use.
Lines 393–400 guard against a null get_silicon_seed(), but line 401 calls get_tpc_seed()->size_cluster_keys() unconditionally. If get_tpc_seed() returns nullptr, this will crash.
Proposed fix
if (m_dst_track->get_silicon_seed())
{
m_reco_track_silicon_seeds[index] = static_cast<int>(m_dst_track->get_silicon_seed()->size_cluster_keys());
}
else
{
m_reco_track_silicon_seeds[index] = 0;
}
- m_reco_track_tpc_seeds[index] = static_cast<int>(m_dst_track->get_tpc_seed()->size_cluster_keys());
+ if (m_dst_track->get_tpc_seed())
+ {
+ m_reco_track_tpc_seeds[index] = static_cast<int>(m_dst_track->get_tpc_seed()->size_cluster_keys());
+ }
+ else
+ {
+ m_reco_track_tpc_seeds[index] = 0;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (m_dst_track->get_silicon_seed()) | |
| { | |
| m_reco_track_silicon_seeds[i - 1] = static_cast<int>(m_dst_track->get_silicon_seed()->size_cluster_keys()); | |
| m_reco_track_silicon_seeds[index] = static_cast<int>(m_dst_track->get_silicon_seed()->size_cluster_keys()); | |
| } | |
| else | |
| { | |
| m_reco_track_silicon_seeds[i - 1] = 0; | |
| m_reco_track_silicon_seeds[index] = 0; | |
| } | |
| m_reco_track_tpc_seeds[i - 1] = static_cast<int>(m_dst_track->get_tpc_seed()->size_cluster_keys()); | |
| m_min_reco_track_pT = std::min(m_reco_track_pT[i - 1], m_min_reco_track_pT); | |
| m_max_reco_track_pT = std::max(m_reco_track_pT[i - 1], m_max_reco_track_pT); | |
| m_reco_track_tpc_seeds[index] = static_cast<int>(m_dst_track->get_tpc_seed()->size_cluster_keys()); | |
| if (m_dst_track->get_silicon_seed()) | |
| { | |
| m_reco_track_silicon_seeds[index] = static_cast<int>(m_dst_track->get_silicon_seed()->size_cluster_keys()); | |
| } | |
| else | |
| { | |
| m_reco_track_silicon_seeds[index] = 0; | |
| } | |
| if (m_dst_track->get_tpc_seed()) | |
| { | |
| m_reco_track_tpc_seeds[index] = static_cast<int>(m_dst_track->get_tpc_seed()->size_cluster_keys()); | |
| } | |
| else | |
| { | |
| m_reco_track_tpc_seeds[index] = 0; | |
| } |
Comment on lines
+497
to
499
| m_reco_mother_pT = std::numeric_limits<float>::quiet_NaN(); | ||
| m_true_mother_p = std::numeric_limits<float>::quiet_NaN(); | ||
| m_true_mother_eta = std::numeric_limits<float>::quiet_NaN(); |
Contributor
There was a problem hiding this comment.
Missing reset of m_true_mother_phi — stale data may persist between decays.
m_reco_mother_pT is reset here (line 497), and m_true_mother_eta at line 499, but m_true_mother_phi (added in the header) is never reset. This means if one decay sets m_true_mother_phi and the next does not populate it, old values will be written to the TTree.
Proposed fix
m_reco_mother_pT = std::numeric_limits<float>::quiet_NaN();
m_true_mother_p = std::numeric_limits<float>::quiet_NaN();
m_true_mother_eta = std::numeric_limits<float>::quiet_NaN();
+ m_true_mother_phi = std::numeric_limits<float>::quiet_NaN();
m_min_true_track_pT = std::numeric_limits<float>::max();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| m_reco_mother_pT = std::numeric_limits<float>::quiet_NaN(); | |
| m_true_mother_p = std::numeric_limits<float>::quiet_NaN(); | |
| m_true_mother_eta = std::numeric_limits<float>::quiet_NaN(); | |
| m_reco_mother_pT = std::numeric_limits<float>::quiet_NaN(); | |
| m_true_mother_p = std::numeric_limits<float>::quiet_NaN(); | |
| m_true_mother_eta = std::numeric_limits<float>::quiet_NaN(); | |
| m_true_mother_phi = std::numeric_limits<float>::quiet_NaN(); |
Build & test reportReport for commit 0743fbe8eaac9118f69a777521e2e65237f42c78:
Automatically generated by sPHENIX Jenkins continuous integration |
5 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
HFTrackEfficiency Updates
Motivation / Context
HFTrackEfficiency is a reconstruction analysis module for the sPHENIX experiment that studies heavy-flavor decay chain reconstruction efficiency. The module tracks true (generator-level) and reconstructed (detector-level) particle kinematics and stores them in ROOT TTrees for offline analysis. This PR adds new measurement capabilities and corrects an indexing bug affecting the alignment of truth-reco particle mappings.
Key Changes
Added azimuthal angle (φ) storage: Introduced
m_true_track_phi[m_maxTracks]andm_reco_track_phi[m_maxTracks]arrays to store azimuthal angles for each daughter track, along with corresponding TTree branches for both mother particles and individual tracks.Extended mother kinematic measurements: Added
m_reco_mother_pT(computed frommotherRecoLV.perp()when all daughters are found) andm_true_mother_phidata members for expanded mother particle characterization.Fixed daughter array indexing: Replaced the prior
i - 1offset with a dedicated sequentialindexcounter (initialized to-1), ensuring proper alignment between per-daughter kinematic arrays (m_true_track_pT,m_reco_track_pT, etc.) and thetrackableParticlesfiltering logic. This indexing change appears to correct a latent bug where truth/reco assignments could become misaligned if the loop index did not match array positions.Updated reconstructed track property bookkeeping: Extended write/read operations for kinematic limits, seed counts, and chi-squared values to use the corrected
indexcounter.Potential Risk Areas
Output File Format Change (IO): New TTree branches (
reco_mother_pT,true_mother_phi, per-track_phibranches) change the structure of output ROOT files. Analyses built on previous output files will require updates to handle the new branches or be regenerated with this version.Index Alignment Bug Fix: The shift from
i - 1to explicit indexing could affect backward compatibility and may change reconstructed efficiency distributions if the prior indexing was inadvertently hiding misalignments. Comparisons with previous efficiency measurements should be validated.Analysis Reproducibility: Results from analyses using prior HFTrackEfficiency output will differ slightly due to corrected indexing; historical results should not be directly compared without understanding this change.
Possible Future Improvements
Note: AI-assisted analysis was used to understand code changes. Code review best practices suggest manually verifying the index counter fix and testing with typical HFTrackEfficiency use cases to confirm the efficiency measurements are correct.