Updates to HFTrackEfficiency and KFParticle#4259
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds guarded HepMC mother rapidity computation and per-daughter rapidity storage to HFTrackEfficiency, restructures truth-matching into m_nDaughters==2 and ==3 paths, adds related TTree branches and resets; separately, KFParticle_nTuple now records DCA significance (PV and pair) per track/pair. ChangesHFTrackEfficiency: Rapidity and truth-matching
KFParticle_nTuple: DCA significance
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. 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: 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
Comment on lines
+218
to
+225
| 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.; | ||
| } |
Contributor
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.
Comment on lines
+311
to
+314
| if (Verbosity() >= VERBOSITY_MORE || true) // fix later | ||
| { | ||
| daughterG4->identify(); | ||
| } |
Contributor
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.
Suggested change
| if (Verbosity() >= VERBOSITY_MORE || true) // fix later | |
| { | |
| daughterG4->identify(); | |
| } | |
| if (Verbosity() >= VERBOSITY_MORE) | |
| { | |
| daughterG4->identify(); | |
| } |
Comment on lines
+212
to
+213
| float m_daughter_dca_sig[99]{0}; | ||
| float m_daughter_dca_sig_xy[99]{0}; |
Contributor
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 |
Build & test reportReport for commit 996eb322f88119c7074d68a145ba5c14d0eff77d:
Automatically generated by sPHENIX Jenkins continuous integration |
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.
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
Enhance heavy-flavor reconstruction and offline analysis by (1) adding DCA significance measurements to KFParticle nTuples to better quantify track-vertex and track-pair consistency, and (2) improving HFTrackEfficiency truth outputs and truth-matching for cascade-style decays that include an intermediate resonance so those decays can be analyzed correctly when only Geant4 (G4) truth is available.
Key Changes
Potential Risk Areas
Possible Future Improvements
Note: This summary was produced with AI assistance. AI can make mistakes—please review changes in code and tests carefully before relying on them in production.