PHSimpleVertexFinder INTT requirements#4248
Open
cdean-github wants to merge 10 commits intosPHENIX-Collaboration:masterfrom
Open
PHSimpleVertexFinder INTT requirements#4248cdean-github wants to merge 10 commits intosPHENIX-Collaboration:masterfrom
cdean-github wants to merge 10 commits intosPHENIX-Collaboration:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds verbosity-gated diagnostic logging across KFParticle tools, refactors silicon/INTT cluster requirement checks into a shared helper, renames BCO branches in nTuples, changes DecayFinder to return an integer count, and changes HFTrackEfficiency to compute seed counts by iterating SvtxTrackState detector IDs (seed counters initialized to -1). 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. Comment |
Build & test reportReport for commit 5a29f035df4f3cc6c65db7e16ee999f06ec9ba33:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit ce03df27c0acf8e596f77aa98a7c294ba62a8d64:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 11a9d5c35e4332b28eff3ba262e6e46eec9db3d7:
Automatically generated by sPHENIX Jenkins continuous integration |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
offline/packages/decayfinder/DecayFinder.cc (1)
455-459:⚠️ Potential issue | 🟠 MajorDon’t zero out an accumulated decay count on a null map entry.
Line 458 returns
false, which is0for thisintfunction. If an earlierPHHepMCGenEventMapentry already incrementedreconstructableDecayWasFound, a later null entry will still makeprocess_eventsee zero and potentially abort the event.🐛 Proposed fix
if (!m_genevt) { std::cout << "DecayFinder: Missing node PHHepMCGenEvent" << std::endl; - return false; + continue; }offline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.cc (1)
151-186:⚠️ Potential issue | 🟠 MajorUpdate BCO state before early event skips.
Because the BCO block now runs after the no-track/no-vertex returns, skipped events do not advance
m_prev_event_bco. The next filled candidate can write a stalelast_GL1_BCOfrom the previous accepted event rather than the previous GL1 event. Please hoist the BCO-state update before these early returns, or run it in each skip path before returning.Also applies to: 188-245, 273-274
🧹 Nitpick comments (2)
offline/packages/trackreco/PHSimpleVertexFinder.h (1)
49-52: Document downstream impact for the new INTT vertex-selection API.Please add a short compatibility note stating that defaults preserve current behavior, and whether enabling
setRequireINTT(true)changesSvtxVertexMapoutputs enough to require downstream validation or reprocessing.As per coding guidelines, “If interfaces change, ask for compatibility notes and any needed downstream updates.” Based on learnings, “If the PR changes reconstruction outputs, calibration constants, or simulation behavior, ensure the description states expected analysis impact and whether reprocessing is required.”
Also applies to: 96-99
offline/packages/decayfinder/DecayFinder.cc (1)
96-103: Document the analysis impact of the changed veto semantics.This now keeps an event when any reconstructable candidate is found, so accepted-event counts and saved DST contents can change relative to previous productions. Please state the expected analysis impact and whether existing samples require reprocessing. Based on learnings, “If the PR changes reconstruction outputs, calibration constants, or simulation behavior, ensure the description states expected analysis impact and whether reprocessing is required.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a20bd1d1-c22d-48cc-a032-0a680bfbe010
📒 Files selected for processing (10)
offline/packages/HFTrackEfficiency/HFTrackEfficiency.ccoffline/packages/KFParticle_sPHENIX/KFParticle_Tools.ccoffline/packages/KFParticle_sPHENIX/KFParticle_Tools.hoffline/packages/KFParticle_sPHENIX/KFParticle_eventReconstruction.ccoffline/packages/KFParticle_sPHENIX/KFParticle_nTuple.ccoffline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.ccoffline/packages/decayfinder/DecayFinder.ccoffline/packages/decayfinder/DecayFinder.hoffline/packages/trackreco/PHSimpleVertexFinder.ccoffline/packages/trackreco/PHSimpleVertexFinder.h
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Build & test reportReport for commit 51dd7c7c9e7aaefeb046e35310f7bc2c422aa9e9:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 8921a077a2c1350a53c4f66dbcdf8bb2942efd7c:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 04bbc4787c16b6adbd93fe0c91dba43cc3c895a7:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/HFTrackEfficiency/HFTrackEfficiency.cc (1)
381-418:⚠️ Potential issue | 🔴 CriticalOff-by-one: seed counters are left at the
-1sentinel and then incremented, undercounting by 1.
resetBranches()now initializesm_reco_track_silicon_seeds[iTrack]andm_reco_track_tpc_seeds[iTrack]to-1(the sentinel for "no reco track / not filled"), which is correct per the previous review. However, when a reco track is found we enter this block and start incrementing from-1, so a track with, say, 3 MVTX/INTT states ends up reportingsilicon_seeds = 2(and similarly 0 instead of 1 when only one silicon state exists, which is indistinguishable from the sentinel). The branches are written as/I, so this propagates directly into the nTuples and will bias downstream seed-count analyses.Reset the counters to
0once the track is confirmed, before the state loop, and keep-1strictly as the "track not reconstructed" sentinel.🔧 Proposed fix
if (recoTrackFound) { selectedTracks.insert(m_dst_track); if (Verbosity() >= VERBOSITY_MORE) { m_dst_track->identify(); } m_reco_track_exists[index] = true; m_reco_track_pT[index] = m_dst_track->get_pt(); m_reco_track_eta[index] = m_dst_track->get_eta(); m_reco_track_phi[index] = m_dst_track->get_phi(); m_reco_track_chi2nDoF[index] = m_dst_track->get_chisq() / m_dst_track->get_ndf(); + m_reco_track_silicon_seeds[index] = 0; + m_reco_track_tpc_seeds[index] = 0; for (auto state_iter = m_dst_track->begin_states(); state_iter != m_dst_track->end_states(); ++state_iter) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f2b6829d-5bca-4fa8-9f9a-c8776d8fe975
📒 Files selected for processing (5)
offline/packages/HFTrackEfficiency/HFTrackEfficiency.ccoffline/packages/KFParticle_sPHENIX/KFParticle_Tools.ccoffline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.ccoffline/packages/decayfinder/DecayFinder.ccoffline/packages/trackreco/PHSimpleVertexFinder.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- offline/packages/KFParticle_sPHENIX/KFParticle_Tools.cc
Comment on lines
156
to
+202
| if (evtHeader && gl1packet) | ||
| { | ||
| const int64_t run = evtHeader->get_RunNumber(); | ||
| const int64_t evn = evtHeader->get_EvtSequence(); | ||
| m_this_event_bco = static_cast<uint64_t>(gl1packet->lValue(0, "BCO")); | ||
|
|
||
| if (Verbosity() >= VERBOSITY_SOME) | ||
| { | ||
| std::cout << "Event start | run: " << run << " event: " << evn << " this_event_bco: " << m_this_event_bco << std::endl; | ||
| } | ||
| const int64_t run = evtHeader->get_RunNumber(); | ||
| const int64_t evn = evtHeader->get_EvtSequence(); | ||
| m_this_event_bco = static_cast<uint64_t>(gl1packet->lValue(0, "BCO")); | ||
|
|
||
| if (run != m_prev_runNumber || evn != m_prev_eventNumber) | ||
| { | ||
| if (Verbosity() >= VERBOSITY_A_LOT) | ||
| { | ||
| std::cout << "Event start | run: " << run << " event: " << evn << " this_event_bco: " << m_this_event_bco << std::endl; | ||
| } | ||
|
|
||
| if (Verbosity() >= VERBOSITY_SOME) | ||
| if (run != m_prev_runNumber || evn != m_prev_eventNumber) | ||
| { | ||
| std::cout << "New event detected" << std::endl; | ||
| std::cout << "Previous event BCO: " << m_prev_event_bco << std::endl; | ||
| } | ||
| //m_last_event_bco = m_prev_event_bco; | ||
| m_last_event_bco = (run == m_prev_runNumber) ? m_prev_event_bco : -1; | ||
| m_prev_event_bco = m_this_event_bco; | ||
|
|
||
| m_prev_runNumber = run; | ||
| m_prev_eventNumber = evn; | ||
| if (Verbosity() >= VERBOSITY_A_LOT) | ||
| { | ||
| std::cout << "New event detected" << std::endl; | ||
| std::cout << "Previous event BCO: " << m_prev_event_bco << std::endl; | ||
| } | ||
|
|
||
| m_last_event_bco = (run == m_prev_runNumber) ? m_prev_event_bco : -1; | ||
| m_prev_event_bco = m_this_event_bco; | ||
|
|
||
| if (Verbosity() >= VERBOSITY_SOME) | ||
| { | ||
| std::cout << "Updated values | last_event_bco: " << m_last_event_bco | ||
| << " stored_prev_event_bco: " << m_prev_event_bco | ||
| << std::endl; | ||
| } | ||
| m_prev_runNumber = run; | ||
| m_prev_eventNumber = evn; | ||
|
|
||
| if (Verbosity() >= VERBOSITY_A_LOT) | ||
| { | ||
| std::cout << "Updated values | last_event_bco: " << m_last_event_bco | ||
| << " stored_prev_event_bco: " << m_prev_event_bco | ||
| << std::endl; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
|
|
||
| if (Verbosity() >= VERBOSITY_SOME) | ||
| if (Verbosity() >= VERBOSITY_MORE) | ||
| { | ||
| std::cout << "KFParticle: EventHeader or GL1 packet not found" << std::endl; | ||
| } | ||
| m_this_event_bco = -1; | ||
| m_last_event_bco = -1; | ||
| m_prev_event_bco = -1; | ||
| m_prev_runNumber = -1; | ||
| m_prev_eventNumber = -1; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm declared types of the BCO/run/event members and their initializers.
fd -e h -e hpp KFParticle_sPHENIX
rg -nP -C1 '\b(m_this_event_bco|m_last_event_bco|m_prev_event_bco|m_prev_runNumber|m_prev_eventNumber)\b'
rg -nP -C2 '\bset_event_bcos\s*\('Repository: sPHENIX-Collaboration/coresoftware
Length of output: 5998
Fix unsigned member initialization — -1 sentinel wraps to 0xFFFFFFFFFFFFFFFF.
The members m_this_event_bco, m_last_event_bco, and m_prev_event_bco are declared as uint64_t in the header (KFParticle_sPHENIX.h:428–430), yet the code assigns -1 to them on Lines 176, 197–199. Under unsigned arithmetic, -1 wraps silently to 0xFFFFFFFFFFFFFFFF, the maximum uint64_t value. Downstream consumers—including set_event_bcos() (line 272) and nTuple readers—will misinterpret this wrapped sentinel as a valid (enormous) BCO and incorrectly process invalid events.
Additionally, set_event_bcos() expects int64_t parameters but receives uint64_t arguments, causing an implicit signed/unsigned conversion.
Either (a) change the BCO members to int64_t, (b) use an explicit sentinel like std::numeric_limits<uint64_t>::max() consistently and document it throughout, or (c) adopt std::optional<uint64_t> to avoid sentinels altogether.
Comment on lines
+205
to
215
| SvtxTrackMap *check_trackmap = findNode::getClass<SvtxTrackMap>(topNode, m_trk_map_node_name); | ||
| multiplicity = check_trackmap->size(); | ||
|
|
||
| if (check_trackmap->empty()) | ||
| { | ||
| std::cout << "EventHeader or GL1 packet not found" << std::endl; | ||
| } | ||
| m_this_event_bco = -1; | ||
| m_last_event_bco = -1; | ||
| m_prev_event_bco = -1; | ||
| m_prev_runNumber = -1; | ||
| m_prev_eventNumber = -1; | ||
| if (Verbosity() >= VERBOSITY_SOME) | ||
| { | ||
| std::cout << "KFParticle: Event skipped as there are no tracks" << std::endl; | ||
| } | ||
| return Fun4AllReturnCodes::EVENT_OK; | ||
| } |
Contributor
There was a problem hiding this comment.
Potential null-pointer dereference on check_trackmap.
findNode::getClass<SvtxTrackMap>(...) can return nullptr if the node is absent, but Line 206 immediately calls check_trackmap->size() before any null check. The subsequent empty() check on Line 208 is then also unsafe. If the track map node is ever missing (e.g. misconfigured DST, upstream module disabled), this will segfault rather than gracefully skipping the event.
🛡️ Proposed guard
SvtxTrackMap *check_trackmap = findNode::getClass<SvtxTrackMap>(topNode, m_trk_map_node_name);
- multiplicity = check_trackmap->size();
-
- if (check_trackmap->empty())
+ if (!check_trackmap || check_trackmap->empty())
{
if (Verbosity() >= VERBOSITY_SOME)
{
- std::cout << "KFParticle: Event skipped as there are no tracks" << std::endl;
+ std::cout << "KFParticle: Event skipped (no track map or empty)" << std::endl;
}
return Fun4AllReturnCodes::EVENT_OK;
}
+ multiplicity = check_trackmap->size();📝 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
| SvtxTrackMap *check_trackmap = findNode::getClass<SvtxTrackMap>(topNode, m_trk_map_node_name); | |
| multiplicity = check_trackmap->size(); | |
| if (check_trackmap->empty()) | |
| { | |
| std::cout << "EventHeader or GL1 packet not found" << std::endl; | |
| } | |
| m_this_event_bco = -1; | |
| m_last_event_bco = -1; | |
| m_prev_event_bco = -1; | |
| m_prev_runNumber = -1; | |
| m_prev_eventNumber = -1; | |
| if (Verbosity() >= VERBOSITY_SOME) | |
| { | |
| std::cout << "KFParticle: Event skipped as there are no tracks" << std::endl; | |
| } | |
| return Fun4AllReturnCodes::EVENT_OK; | |
| } | |
| SvtxTrackMap *check_trackmap = findNode::getClass<SvtxTrackMap>(topNode, m_trk_map_node_name); | |
| if (!check_trackmap || check_trackmap->empty()) | |
| { | |
| if (Verbosity() >= VERBOSITY_SOME) | |
| { | |
| std::cout << "KFParticle: Event skipped (no track map or empty)" << std::endl; | |
| } | |
| return Fun4AllReturnCodes::EVENT_OK; | |
| } | |
| multiplicity = check_trackmap->size(); |
Comment on lines
+577
to
+588
| if ((_require_mvtx || _require_intt) && !siliconseed) | ||
| { | ||
| continue; | ||
| } | ||
| if (_require_mvtx && !passClusterRequirement(tr1, "MVTX")) | ||
| { | ||
| continue; | ||
| } | ||
| if (_require_intt && !passClusterRequirement(tr1, "INTT")) | ||
| { | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
Handle zero required clusters as a pass.
The setters accept 0, but this path can still veto tracks when a requirement is enabled with zero required clusters. That makes configuration scans like setNinttRequired(0) silently stricter than requested, especially in zero-field mode where the precheck skips before the helper can decide.
Proposed fix
- if ((_require_mvtx || _require_intt) && !siliconseed)
+ const bool needs_mvtx_seed = _require_mvtx && _nmvtx_required > 0;
+ const bool needs_intt_seed = _require_intt && _nintt_required > 0;
+ if ((needs_mvtx_seed || needs_intt_seed) && !siliconseed)
{
continue;
} unsigned int nclus = 0;
- unsigned int _nclus_required = type == "MVTX" ? _nmvtx_required : _nintt_required;
+ const unsigned int nclus_required = type == "MVTX" ? _nmvtx_required : _nintt_required;
+
+ if (nclus_required == 0)
+ {
+ return true;
+ }
TrackSeed *siliconseed = track->get_silicon_seed();
if (!siliconseed)
{
return pass;
@@
- if (nclus >= _nclus_required)
+ if (nclus >= nclus_required)
{
pass = true;
}As per coding guidelines, **/*.{cc,cpp,cxx,c} changes should prioritize correctness.
Also applies to: 1264-1284
Build & test reportReport for commit 5fd977d86e0a8488a83e67ea5357b20acb3039ba:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
reviewed
Apr 20, 2026
Contributor
osbornjd
left a comment
osbornjd
left a comment
There was a problem hiding this comment.
I'm not sure why but this PR changes the pythia QA. It doesn't seem like the performance is any different but based on the code I can't see how the vertexer performance changes. @cdean-github do you have any thoughts?
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, ...)
I added the ability to require INTT clusters on a track when making primary vertices. The code defaults to not requiring this, and when enabled the default is a single cluster
and
I also patched a small bug in DecayFinder which would veto events if one candidate passed the trigger but another failed
Lastly, I cleaned up some KFParticle code. Its only cosmetic
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
PHSimpleVertexFinder INTT Requirements and Bug Fixes
Motivation
This PR extends primary vertex reconstruction flexibility by adding INTT (intermediate silicon tracker) cluster requirements to PHSimpleVertexFinder, which previously supported only MVTX (vertex tracker) constraints. Additionally, it fixes a logic bug in DecayFinder that could incorrectly veto events when multiple decay candidates are processed, and includes refactoring/debugging improvements across KFParticle code.
Key Changes
PHSimpleVertexFinder (Primary Feature)
setRequireINTT(bool)andsetNinttRequired(unsigned int)_require_intt(defaultfalse),_nintt_required(default1)_require_mvtx = true,_nmvtx_required = 2passClusterRequirement(SvtxTrack*, type_string)to centralize silicon cluster counting logic for both MVTX and INTTsetRequireMVTX,zeroField,set_pp_mode)DecayFinder Bug Fix
findDecay()return type frombooltoint(counts reconstructable decay candidates instead of binary pass/fail)HFTrackEfficiency Changes
TrackSeed::size_cluster_keys()m_reco_track_silicon_seedsandm_reco_track_tpc_seedsfrom0to-1(likely for cleaner NaN handling in ROOT output)KFParticle Debugging/Refactoring
printSelectionCheck()for formatted debug printingconstqualifier fromfindTwoProngs()methodGL1_BCO,last_GL1_BCO,Collision_BCO)Potential Risk Areas
HFTrackEfficiency seed counting change: The shift from using
TrackSeed::size_cluster_keys()to iterating throughSvtxTrackStateobjects changes the counting logic. Verification needed that:pathlength != 0)-1instead of0is compatible with downstream analysisDecayFinder return type change: Any external code calling
findDecay()expecting a boolean may require updates. The logic change from "any candidate passed" to "count candidates" could affect event selection in subtle ways.PHSimpleVertexFinder reconstruction behavior: INTT requirement defaults to
false(disabled), so backward compatibility is maintained by default. However, enabling INTT requirements changes track acceptance criteria and vertex finding behavior.Thread safety: Track state iteration in HFTrackEfficiency may have different thread-safety characteristics than the previous method.
Possible Future Improvements