Updates to HFTrackEfficiency and KFParticle#4259
Conversation
📝 WalkthroughWalkthroughThe PR extends kinematics tracking in heavy-flavor reconstruction by adding rapidity computation for mother and daughter particles in HFTrackEfficiency, expanding truth-matching logic to handle different daughter multiplicities, and recording DCA significance quantities in KFParticle nTuples. Changes
Possibly related PRs
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41593349-771c-408d-9a5f-dd3fa30ee887
📒 Files selected for processing (4)
offline/packages/HFTrackEfficiency/HFTrackEfficiency.ccoffline/packages/HFTrackEfficiency/HFTrackEfficiency.hoffline/packages/KFParticle_sPHENIX/KFParticle_nTuple.ccoffline/packages/KFParticle_sPHENIX/KFParticle_nTuple.h
| if (mother->momentum().e() > abs(mother->momentum().pz())) | ||
| { | ||
| m_true_mother_rapidity = 0.5 * log((mother->momentum().e() + mother->momentum().pz())/(mother->momentum().e() - mother->momentum().pz())); | ||
| } | ||
| else | ||
| { | ||
| m_true_mother_rapidity = -999.; | ||
| } |
There was a problem hiding this comment.
Inconsistent sentinel value for invalid rapidity.
When the rapidity cannot be computed, -999. is used, but elsewhere in the class (e.g., resetBranches() at line 605), quiet_NaN() is used to indicate invalid/unset values. Downstream analysis code checking std::isnan() will miss these cases.
Consider using NaN for consistency:
Proposed fix
if (mother->momentum().e() > abs(mother->momentum().pz()))
{
m_true_mother_rapidity = 0.5 * log((mother->momentum().e() + mother->momentum().pz())/(mother->momentum().e() - mother->momentum().pz()));
}
- else
- {
- m_true_mother_rapidity = -999.;
- }
+ // else: m_true_mother_rapidity remains at its initialized NaN valueAlternatively, if -999. is intentional for compatibility with existing analysis code, please document this choice clearly.
| if (Verbosity() >= VERBOSITY_MORE || true) // fix later | ||
| { | ||
| daughterG4->identify(); | ||
| } |
There was a problem hiding this comment.
Debug code left in: || true bypasses verbosity check.
The condition Verbosity() >= VERBOSITY_MORE || true always evaluates to true, causing daughterG4->identify() to print for every matched daughter regardless of verbosity setting. The comment "fix later" confirms this is unintentional.
Proposed fix
- if (Verbosity() >= VERBOSITY_MORE || true) // fix later
+ if (Verbosity() >= VERBOSITY_MORE)
{
daughterG4->identify();
}📝 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.
| if (Verbosity() >= VERBOSITY_MORE || true) // fix later | |
| { | |
| daughterG4->identify(); | |
| } | |
| if (Verbosity() >= VERBOSITY_MORE) | |
| { | |
| daughterG4->identify(); | |
| } |
| float m_daughter_dca_sig[99]{0}; | ||
| float m_daughter_dca_sig_xy[99]{0}; |
There was a problem hiding this comment.
Pairwise DCA-significance buffers are undersized and can overflow.
At Line 212 and Line 213, capacity is 99, but pairwise loops in offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.cc can write up to m_num_tracks_nTuple*(m_num_tracks_nTuple-1)/2 entries. With max_tracks=20, that is 190 entries, so these arrays can be written out-of-bounds (memory corruption/crash risk).
💡 Proposed fix
--- a/offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.h
+++ b/offline/packages/KFParticle_sPHENIX/KFParticle_nTuple.h
@@
- float m_daughter_dca[99]{0};
- float m_daughter_dca_xy[99]{0};
- float m_daughter_dca_sig[99]{0};
- float m_daughter_dca_sig_xy[99]{0};
+ static constexpr int max_track_pairs{max_tracks * (max_tracks - 1) / 2};
+ float m_daughter_dca[max_track_pairs]{0};
+ float m_daughter_dca_xy[max_track_pairs]{0};
+ float m_daughter_dca_sig[max_track_pairs]{0};
+ float m_daughter_dca_sig_xy[max_track_pairs]{0};As per coding guidelines, **/*.{cc,cpp,cxx,c}: “Prioritize correctness, memory safety, error handling, and thread-safety.”
Build & test reportReport for commit dd98db258886298d85a63a02faedbb2e0eeafe31:
Automatically generated by sPHENIX Jenkins continuous integration |
1 participant
comment: For KFParticle this PR just adds DCA significance relative to the PV and other tracks in the decay as measurements to what gets stored in the output nTuple. For HFTrackEfficiency, this PR adds output variables for the truth mother rapidity and truth daughter track rapidities so those values can be used in offline analysis. Additionally, this PR fixes an issue in HFTrackEfficiency where decays with intermediate resonances would not be processed correctly if the G4 truth information needed to be accessed instead of the HepMC record. 2 prong decays should be unaffected by this update, but HFTrackEfficiency should now be able to handle decays with a bachelor track and an intermediate resonance decaying to 2 tracks (such as the cascade->Lambda(->proton+pion)pion). Be careful for decays outside of that format, however, as it does not generalize to any decay topology.
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)
Summary
Motivation / Context
These updates enhance the reconstruction and analysis capabilities for heavy-flavor physics at the sPHENIX collaboration. The changes add precision measurements of track-vertex distances and enable proper truth matching for cascade decay topologies that involve intermediate resonances.
Key Changes
HFTrackEfficiency Package:
m_true_mother_rapidityandm_true_track_rapidity[]to nTuple branches, enabling detailed kinematic studies of decay topologies(e, pz)formula with underflow protectionKFParticle Package:
DCA_sig,DCA_sig_xy) computed viaGetDeviationFromVertex()andGetDeviationFromVertexXY()DCA_sig,DCA_sig_xy) for all track pairs computed viaGetDeviationFromParticle()andGetDeviationFromParticleXY()Potential Risk Areas
I/O Format Changes: New TTree branches (
DCA_sig,DCA_sig_xy,true_mother_rapidity,true_track_rapidity) are added to output nTuples. Downstream analyses must account for these additions or filter them appropriately.Intermediate Resonance Fix Limitations: The implementation is specifically designed for "bachelor + 2-body intermediate" topology. The PR documentation explicitly cautions against generalizing to arbitrary decay topologies; users must verify compatibility with their specific decay modes.
Truth Matching Complexity: The expanded logic with branching on
m_nDaughters == 2vs.m_nDaughters == 3introduces more conditional paths. Edge cases with unusual barcode or PID values should be tested.Computation Overhead: DCA significance calculations (deviation measurements) are performed for all track-vertex and track-pair combinations; performance impact should be evaluated for high-occupancy events.
Backward Compatibility: Existing analyses using two-prong decays remain unaffected, but those processing cascade decays via G4 truth will see corrected mother assignments.
Possible Future Improvements
Note: AI summary may contain errors; code review best judgment recommended for production use.