KFParticle Track to Calo Matching QA#4251
KFParticle Track to Calo Matching QA#4251viewolfe321 wants to merge 1 commit intosPHENIX-Collaboration:masterfrom
Conversation
📝 WalkthroughWalkthroughA new QA module Changes
Sequence Diagram(s)sequenceDiagram
participant Event as Event Loop
participant Module as QAKFParticleTracktoCalo
participant NodeTree as Fun4All Node Tree
participant KFP as KFParticle Container
participant EMCal as EMCal Clusters
participant Histos as Histograms
Event->>Module: InitRun()
Module->>NodeTree: Locate geometry/tower containers
Module->>Histos: SetUpHistos()
Event->>Module: process_event()
Module->>NodeTree: LoadNodes() - fetch SvtxTrackMap, KFParticle, clusters
Module->>KFP: Validate KFParticle container present
Module->>KFP: Iterate mothers
loop For each mother
Module->>KFP: GetVectorOfDaughters()
loop For each daughter
Module->>KFP: Apply angle/kinematic cuts
Module->>KFP: Check vertex ID consistency
Module->>EMCal: GetMatchedCluster(track state → cluster)
alt Cluster matched
Module->>Histos: Fill residual plots (Δz, Δφ, energy)
Module->>Histos: Fill correlation plots
end
end
end
Event->>Module: End()
Module->>Histos: Write to ROOT file (if not using histogram manager)
Event->>Module: EndEvent()
Module->>Module: Clear event state
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}
There was a problem hiding this comment.
Actionable comments posted: 11
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4918c2db-a194-47aa-bcc0-5ec372c25c21
📒 Files selected for processing (5)
offline/QA/KFParticle_Track2Calo/Makefile.amoffline/QA/KFParticle_Track2Calo/QAKFParticleTracktoCalo.ccoffline/QA/KFParticle_Track2Calo/QAKFParticleTracktoCalo.hoffline/QA/KFParticle_Track2Calo/autogen.shoffline/QA/KFParticle_Track2Calo/configure.ac
| libQAKFParticleTracktoCalo_la_LDFLAGS = \ | ||
| -L$(libdir) \ | ||
| -L$(OFFLINE_MAIN)/lib \ | ||
| -lKFParticle \ | ||
| -lcalo_io \ | ||
| -lCLHEP \ | ||
| -lfun4all |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how peer Makefile.am files declare link libraries vs LDFLAGS,
# and which libraries export the symbols/headers this module uses.
set -e
fd -a 'Makefile.am' offline/QA | xargs -I{} sh -c 'echo "=== {} ==="; cat {}' | sed -n '1,2000p'
echo "--- headers that declare the classes we depend on ---"
for sym in KFParticle_Container SvtxTrackMap ActsGeometry SvtxVertexMap QAHistManagerDef TrkrClusterContainer; do
echo "### $sym"
rg -n --no-heading -g '*.h' "class\s+$sym\b" offline || true
doneRepository: sPHENIX-Collaboration/coresoftware
Length of output: 32575
🏁 Script executed:
# Examine the QAKFParticleTracktoCalo source files
cd offline/QA/KFParticle_Track2Calo
echo "=== QAKFParticleTracktoCalo.h ==="
cat -n QAKFParticleTracktoCalo.h
echo ""
echo "=== QAKFParticleTracktoCalo.cc (first 150 lines) ==="
head -150 QAKFParticleTracktoCalo.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 16688
🏁 Script executed:
# Search for QAHistManagerDef
rg -n "QAHistManagerDef" offline --type hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 522
🏁 Script executed:
# Check the KFParticle/Makefile.am sibling for comparison
cat -n offline/QA/KFParticle/Makefile.amRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1346
Use LIBADD instead of LDFLAGS and add missing library dependencies.
Verified: This module uses LDFLAGS (incorrect for libtool targets), while all sibling QA modules correctly use LIBADD. Additionally, the source code includes headers from several libraries not currently linked:
KFParticle_Container,KFParticle_Tools→-lkfparticle_sphenix(line 26–27 of header)SvtxTrack,SvtxTrackMap→-ltrack_io(lines 29–30)ActsGeometry,TrkrCluster*,TrkrDefs→-ltrackbase_historic(lines 31–37)SvtxVertexMap,SvtxVertex→-lglobalvertex_io(lines 22–23)QAHistManagerDef→-lqautils(qautils/QAHistManagerDef.h in source)SubsysReco→-lSubsysReco(line 4; not always transitively exported by-lfun4all)-lphoolis also needed (used throughout; included by peer modules)
Suggested rework
-libQAKFParticleTracktoCalo_la_LDFLAGS = \
- -L$(libdir) \
- -L$(OFFLINE_MAIN)/lib \
- -lKFParticle \
- -lcalo_io \
- -lCLHEP \
- -lfun4all
+libQAKFParticleTracktoCalo_la_LIBADD = \
+ -lKFParticle \
+ -lkfparticle_sphenix \
+ -lcalo_io \
+ -lCLHEP \
+ -lfun4all \
+ -lSubsysReco \
+ -lphool \
+ -lqautils \
+ -lglobalvertex_io \
+ -ltrack_io \
+ -ltrackbase_historic📝 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.
| libQAKFParticleTracktoCalo_la_LDFLAGS = \ | |
| -L$(libdir) \ | |
| -L$(OFFLINE_MAIN)/lib \ | |
| -lKFParticle \ | |
| -lcalo_io \ | |
| -lCLHEP \ | |
| -lfun4all | |
| libQAKFParticleTracktoCalo_la_LIBADD = \ | |
| -lKFParticle \ | |
| -lkfparticle_sphenix \ | |
| -lcalo_io \ | |
| -lCLHEP \ | |
| -lfun4all \ | |
| -lSubsysReco \ | |
| -lphool \ | |
| -lqautils \ | |
| -lglobalvertex_io \ | |
| -ltrack_io \ | |
| -ltrackbase_historic |
| return -1; | ||
| } |
There was a problem hiding this comment.
Return a proper Fun4All return code from process_event.
return -1; is not a Fun4All return code and can produce confusing behavior in the scheduler (it is not ABORTEVENT/ABORTRUN/EVENT_OK). Given the PR description mentions that this module sometimes sees an empty KFParticle container when run alongside the general KFParticle QA, please also distinguish "container absent" from "container present but empty" — silently aborting when absent is fine, but an empty container per event is probably not fatal.
- if (!m_kfpContainer){
- std::cout << "KFP CONTAINER - Fatal Error - unable to find DST node KFParticle_QA" << std::endl;
- emptyKFPcontainer++;
- return -1;
- }
- else filledKFPcontainer++;
+ if (!m_kfpContainer) {
+ if (m_verbosity > 0)
+ std::cout << __PRETTY_FUNCTION__ << " : missing KFParticle_Container node " << m_KFParticleNodeName << "_KFParticle_Container" << std::endl;
+ emptyKFPcontainer++;
+ return Fun4AllReturnCodes::ABORTEVENT;
+ }
+ ++filledKFPcontainer;| { std::cout << __LINE__ << std::endl; | ||
| // Setup // std::vector<std::pair<SvtxTrack*,KFParticle*>> v_daughters={}; // Holds all daughters belonging to a (single) candidate mother | ||
| std::vector<QAKFParticleTracktoCalo::MatchedCluster*> v_matched_clusterstracks = {}; // Holds only duaghters with matches | ||
| SvtxVertex *m_vertex; | ||
|
|
||
| //Get mother | ||
| KFParticle *mother = iter.second; | ||
| if (!mother){ | ||
| if (m_verbosity > 0) std::cout << __PRETTY_FUNCTION__ << " Error: null mother pointer" << std::endl; | ||
| continue; | ||
| } std::cout << __LINE__ << std::endl; |
There was a problem hiding this comment.
Remove leftover __LINE__ debug prints.
std::cout << __LINE__ << std::endl; in the hot per-event loop will produce one or two lines per KFParticle mother per event. In a QA job over many events this floods logs and materially slows the module. Please drop (or gate on m_verbosity).
- for (auto &iter : *m_kfpContainer)
- { std::cout << __LINE__ << std::endl;
+ for (auto &iter : *m_kfpContainer)
+ {
...
- } std::cout << __LINE__ << std::endl;
+ }📝 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.
| { std::cout << __LINE__ << std::endl; | |
| // Setup // std::vector<std::pair<SvtxTrack*,KFParticle*>> v_daughters={}; // Holds all daughters belonging to a (single) candidate mother | |
| std::vector<QAKFParticleTracktoCalo::MatchedCluster*> v_matched_clusterstracks = {}; // Holds only duaghters with matches | |
| SvtxVertex *m_vertex; | |
| //Get mother | |
| KFParticle *mother = iter.second; | |
| if (!mother){ | |
| if (m_verbosity > 0) std::cout << __PRETTY_FUNCTION__ << " Error: null mother pointer" << std::endl; | |
| continue; | |
| } std::cout << __LINE__ << std::endl; | |
| { | |
| // Setup // std::vector<std::pair<SvtxTrack*,KFParticle*>> v_daughters={}; // Holds all daughters belonging to a (single) candidate mother | |
| std::vector<QAKFParticleTracktoCalo::MatchedCluster*> v_matched_clusterstracks = {}; // Holds only duaghters with matches | |
| SvtxVertex *m_vertex; | |
| //Get mother | |
| KFParticle *mother = iter.second; | |
| if (!mother){ | |
| if (m_verbosity > 0) std::cout << __PRETTY_FUNCTION__ << " Error: null mother pointer" << std::endl; | |
| continue; | |
| } |
| std::pair<float,float> anglesbetweendaughters = {std::numeric_limits<float>::quiet_NaN(),std::numeric_limits<float>::quiet_NaN()}; | ||
| if(m_cut_on_angle_between_tracks){ | ||
| anglesbetweendaughters = GetEtaAndPhiBetweenDaughters(v_daughters); | ||
| if(!GoodAngleBetweenDaughters(anglesbetweendaughters)){ | ||
| failedcut_dAngle++; | ||
| std::cout << "Bad dAngle: " << anglesbetweendaughters.first << " " << anglesbetweendaughters.second << std::endl; | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
NaN silently passes the daughter-angle cut.
When v_daughters.size() != 2 or the daughters have the same-sign charge, GetEtaAndPhiBetweenDaughters returns {NaN, NaN}. In GoodAngleBetweenDaughters, std::abs(NaN) > threshold is false, so NaN is treated as passing the cut. That means same-sign or non-two-body candidates slip through m_cut_on_angle_between_tracks untouched, and the filled h_dEta_tracks/h_dPhi_tracks entries on Lines 441-442 will receive NaNs.
- if((float) std::abs(dEta_tracks) > m_dEta_Tracks) {return false;}
- if((float) std::abs(dPhi_tracks) > m_dPhi_Tracks) {return false;}
+ if (!std::isfinite(dEta_tracks) || !std::isfinite(dPhi_tracks)) return false;
+ if (std::abs(dEta_tracks) > m_dEta_Tracks) return false;
+ if (std::abs(dPhi_tracks) > m_dPhi_Tracks) return false;| // Get vertex | ||
| m_vertex = m_vertexMap->get(v_daughters[0].first->get_vertex_id()); |
There was a problem hiding this comment.
Potential null-pointer dereference on m_vertexMap.
LoadNodes only prints an error when SvtxVertexMap is missing and leaves m_vertexMap == nullptr. This line then calls m_vertexMap->get(...) unconditionally, which will crash. Either bail out of process_event when the vertex map (or any other mandatory node) is missing, or guard the call here. The commented-out return Fun4AllReturnCodes::ABORTRUN; paths in LoadNodes suggest the original intent.
// Get vertex
+ if (!m_vertexMap) { continue; }
m_vertex = m_vertexMap->get(v_daughters[0].first->get_vertex_id());A cleaner fix is to return ABORTEVENT from process_event when required containers are missing, rather than silently continuing with half-loaded state (which may be related to the crash described in the PR description when running alongside the general KFParticle QA).
📝 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.
| // Get vertex | |
| m_vertex = m_vertexMap->get(v_daughters[0].first->get_vertex_id()); | |
| // Get vertex | |
| if (!m_vertexMap) { continue; } | |
| m_vertex = m_vertexMap->get(v_daughters[0].first->get_vertex_id()); |
| if(MaCl->track_charge<0){ | ||
| // std::cout << "Filling histos for electrons..." << std::endl; | ||
| pt_m = MaCl->track_pt_svtx; | ||
| h_pt_m->Fill(pt_p); |
There was a problem hiding this comment.
Wrong variable fills negative-daughter pT histogram.
h_pt_m is filled with pt_p instead of pt_m, so the "minus" pT spectrum is a copy of the "plus" spectrum — a silent QA bug that will quietly corrupt every conversion study using this plot.
- h_pt_m->Fill(pt_p);
+ h_pt_m->Fill(pt_m);📝 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.
| h_pt_m->Fill(pt_p); | |
| h_pt_m->Fill(pt_m); |
| QAKFParticleTracktoCalo::MatchedCluster* QAKFParticleTracktoCalo::GetMatchedCluster(SvtxTrackState *thisState, SvtxVertex *m_vertex){ | ||
|
|
||
| // Objects | ||
| MatchedCluster* MaCl = new MatchedCluster; | ||
| RawCluster *cluster; | ||
| bool is_match; | ||
| int index; | ||
|
|
||
| // Make sure there's a state | ||
| if (thisState == nullptr || m_vertex==nullptr) return MaCl; | ||
|
|
||
| // Vertex | ||
| float vx = m_vertex->get_x(); | ||
| float vy = m_vertex->get_y(); | ||
| float vz = m_vertex->get_z(); | ||
|
|
||
| // Tracking variables | ||
| float _track_phi_emc = std::numeric_limits<float>::quiet_NaN(); | ||
| float _track_eta_emc = std::numeric_limits<float>::quiet_NaN(); | ||
| float _track_x_emc = std::numeric_limits<float>::quiet_NaN(); | ||
| float _track_y_emc = std::numeric_limits<float>::quiet_NaN(); | ||
| float _track_z_emc = std::numeric_limits<float>::quiet_NaN(); | ||
|
|
||
| // EMCal vectors | ||
| std::vector<float> v_emcal_phi; | ||
| std::vector<float> v_emcal_eta; | ||
| std::vector<float> v_emcal_clusE; | ||
| std::vector<float> v_emcal_dphi; | ||
| std::vector<float> v_emcal_deta; | ||
| std::vector<float> v_emcal_dr; | ||
| std::vector<float> v_emcal_dz; | ||
|
|
||
| //Track state kinematics | ||
| _track_x_emc = thisState->get_x()-vx; | ||
| _track_y_emc = thisState->get_y()-vy; | ||
| _track_z_emc = thisState->get_z()-vz; | ||
| _track_phi_emc = std::atan2(_track_y_emc, _track_x_emc); | ||
| _track_eta_emc = std::asinh(_track_z_emc / std::sqrt((_track_x_emc * _track_x_emc) + (_track_y_emc * _track_y_emc))); | ||
|
|
||
| // Set variables for matching | ||
| is_match = false; | ||
| index = -1; | ||
| // Create objects, containers, iterators for clusters | ||
| cluster = nullptr; | ||
| RawClusterContainer::Range begin_end_EMC = clustersEM->getClusters(); | ||
| RawClusterContainer::Iterator clusIter_EMC; | ||
|
|
||
| // Loop over the EMCal clusters | ||
| for (clusIter_EMC = begin_end_EMC.first; clusIter_EMC != begin_end_EMC.second; ++clusIter_EMC) | ||
| { | ||
| //Get cluster with respect to PV, not (0,0,0) | ||
| cluster = clusIter_EMC->second; | ||
| CLHEP::Hep3Vector v_vertex; | ||
| v_vertex.set(vx,vy,vz); | ||
| CLHEP::Hep3Vector E_vec_cluster = RawClusterUtility::GetEVec(*cluster, v_vertex); | ||
|
|
||
| bool skipthis = false; | ||
|
|
||
| //CUT -- CLUSTER ENERGY | ||
| float cluster_energy = E_vec_cluster.mag(); | ||
| if (cluster_energy < m_emcal_e_low_cut){ | ||
| skipthis=true; failedcut_energy++; | ||
| std::cout << "Bad energy: " << cluster_energy << std::endl; | ||
| } //continue;} | ||
|
|
||
| //CUT -- DPHI | ||
| float clPhi = E_vec_cluster.phi(); | ||
| float dphi = PiRange(_track_phi_emc - clPhi); | ||
| if (dphi > m_dphi_cut_high || dphi < m_dphi_cut_low){ | ||
| skipthis=true; failedcut_dPhi++; | ||
| std::cout << "Bad dPhi: " << dphi << std::endl; | ||
| } // continue;} | ||
|
|
||
| //CUT -- DZ | ||
| float clZ = cluster->get_z() - vz; | ||
| float dz = _track_z_emc - clZ; | ||
| if (dz > m_dz_cut_high || dz < m_dz_cut_low){ | ||
| skipthis=true; failedcut_dZ++; | ||
| std::cout << "Bad dZ: " << dz << std::endl; | ||
| }// continue;} | ||
|
|
||
| // CUT -- DETA // Currently not being used, but want to include functionality now | ||
| float clEta = E_vec_cluster.pseudoRapidity(); | ||
| float deta = _track_eta_emc - clEta; | ||
| // if (deta > m_deta_cut_high || deta < m_deta_cut_low) continue; | ||
|
|
||
| if(skipthis) continue; | ||
|
|
||
| //Calculate dr | ||
| float tmparg = caloRadiusEMCal * dphi; | ||
| float dr = std::sqrt((tmparg * tmparg) + (dz * dz)); // sqrt((R*dphi)^2 + (dz)^2 | ||
| // float dr = sqrt((dphi*dphi + deta*deta)); //previous version | ||
|
|
||
| // Add potential match's information to vectors | ||
| v_emcal_phi.push_back(clPhi); | ||
| v_emcal_eta.push_back(clEta); | ||
| v_emcal_clusE.push_back(cluster_energy); | ||
| v_emcal_dphi.push_back(dphi); | ||
| v_emcal_dz.push_back(dz); | ||
| v_emcal_deta.push_back(deta); | ||
| v_emcal_dr.push_back(dr); | ||
| is_match = true; | ||
| number_of_matches++; | ||
| } | ||
|
|
||
| // Find the closest match from all potential matches | ||
| if (is_match == true) | ||
| { | ||
| float tmp = 99999; | ||
| for (long unsigned int i = 0; i < v_emcal_dr.size(); i++) | ||
| { | ||
| if (v_emcal_dr[i] < tmp) | ||
| { | ||
| index = i; | ||
| tmp = v_emcal_dr[i]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (is_match == false) | ||
| { | ||
| return MaCl; | ||
| } | ||
| else | ||
| { | ||
| MaCl->track_phi_emc=_track_phi_emc; | ||
| MaCl->track_eta_emc=_track_eta_emc; | ||
| MaCl->track_x_emc=_track_x_emc; | ||
| MaCl->track_y_emc=_track_y_emc; | ||
| MaCl->track_z_emc=_track_z_emc; | ||
| MaCl->cluster_phi=v_emcal_phi[index]; | ||
| MaCl->cluster_eta=v_emcal_eta[index]; | ||
| MaCl->cluster_E=v_emcal_clusE[index]; | ||
| MaCl->dphi_trackcluster=v_emcal_dphi[index]; | ||
| MaCl->deta_trackcluster=v_emcal_deta[index]; | ||
| MaCl->dz_trackcluster=v_emcal_dz[index]; | ||
| MaCl->matched=true; | ||
| return MaCl; | ||
| } | ||
| } |
There was a problem hiding this comment.
MatchedCluster leaks on every call; use a value (or unique_ptr) instead of raw new.
GetMatchedCluster unconditionally news a MatchedCluster and returns the raw pointer. The callers at Lines 210–214 and 228–230 never delete it: unmatched candidates are dropped on the floor, and matched ones are pushed into v_matched_clusterstracks and also never freed at the end of process_event. Every event leaks one heap allocation per EMCal-matching attempt, which in a long QA run is noticeable and, more importantly, unnecessary — MatchedCluster is a trivially copyable POD and doesn't need heap allocation at all.
🛠️ Suggested shape
Prefer returning by value (and store by value in the vector):
MatchedCluster QAKFParticleTracktoCalo::GetMatchedCluster(SvtxTrackState* thisState, SvtxVertex* m_vertex)
{
MatchedCluster MaCl; // matched=false by default
if (!thisState || !m_vertex) return MaCl;
...
return MaCl; // NRVO
}
// caller:
std::vector<MatchedCluster> v_matched_clusterstracks;
auto matched = GetMatchedCluster(thisState, m_vertex);
if (!matched.matched) { failedcut_nomtch++; continue; }
...
v_matched_clusterstracks.push_back(matched);If you want to keep pointer semantics, at minimum use std::unique_ptr<MatchedCluster> and a std::vector<std::unique_ptr<MatchedCluster>>, or ensure all allocations are deleted before process_event returns.
| m_outputpath = path; | ||
| m_outtrailer = "_" + m_runnumber + ext; | ||
| std::string file = m_outputpath + filename + m_outtrailer; |
There was a problem hiding this comment.
Undefined behavior in m_outtrailer construction.
"_" + m_runnumber + ext does not do what it looks like. "_" is a const char[2] that decays to const char*; const char* + int is pointer arithmetic, not string concatenation. For any run number ≥ 2 you walk past the end of the string literal — undefined behavior, and in practice you will get a garbage trailer (or crash) appended before ext. The header also declares m_runnumber = std::numeric_limits<int>::quiet_NaN(), which for int is just 0, so this hasn't tripped in trivial tests but will in production.
- m_outtrailer = "_" + m_runnumber + ext;
+ m_outtrailer = "_" + std::to_string(m_runnumber) + ext;And in the header:
- int m_runnumber = std::numeric_limits<int>::quiet_NaN();
+ int m_runnumber{0};| void setTrackClusterDeltaZCut(float in){ m_dz_cut_low = in; m_dz_cut_high = in;} | ||
| void setTrackClusterDeltaZMin(float in){ m_dz_cut_high = in;} | ||
| void setTrackClusterDeltaZMax(float in){ m_dz_cut_low = in;} | ||
| void setTrackClusterDeltaPhiCut(float in){ m_dphi_cut_low = in; m_dphi_cut_high = in;} | ||
| void setTrackClusterDeltaPhiMin(float in){ m_dphi_cut_high = in;} | ||
| void setTrackClusterDeltaPhiMax(float in){ m_dphi_cut_low = in;} |
There was a problem hiding this comment.
Broken public API: cut setters are swapped and the "Cut" variants are not symmetric.
These setters will silently mis-configure users' cuts:
setTrackClusterDeltaZCut(in)sets bothm_dz_cut_low = inandm_dz_cut_high = in, so the cut atprocess_event(dz > high || dz < low) only acceptsdz == in. Presumably meant a symmetric cut (-in,+in).setTrackClusterDeltaZMin(in)writes intom_dz_cut_high— i.e. the max member.setTrackClusterDeltaZMax(in)writes intom_dz_cut_low— i.e. the min member.- The same three problems exist for the three
...DeltaPhi...setters.
Anyone calling these in a macro will get the opposite or an empty window.
- void setTrackClusterDeltaZCut(float in){ m_dz_cut_low = in; m_dz_cut_high = in;}
- void setTrackClusterDeltaZMin(float in){ m_dz_cut_high = in;}
- void setTrackClusterDeltaZMax(float in){ m_dz_cut_low = in;}
- void setTrackClusterDeltaPhiCut(float in){ m_dphi_cut_low = in; m_dphi_cut_high = in;}
- void setTrackClusterDeltaPhiMin(float in){ m_dphi_cut_high = in;}
- void setTrackClusterDeltaPhiMax(float in){ m_dphi_cut_low = in;}
+ // Symmetric window: |dz| <= in
+ void setTrackClusterDeltaZCut(float in) { m_dz_cut_low = -std::abs(in); m_dz_cut_high = std::abs(in); }
+ void setTrackClusterDeltaZMin(float in) { m_dz_cut_low = in; }
+ void setTrackClusterDeltaZMax(float in) { m_dz_cut_high = in; }
+ void setTrackClusterDeltaPhiCut(float in) { m_dphi_cut_low = -std::abs(in); m_dphi_cut_high = std::abs(in); }
+ void setTrackClusterDeltaPhiMin(float in) { m_dphi_cut_low = in; }
+ void setTrackClusterDeltaPhiMax(float in) { m_dphi_cut_high = in; }📝 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.
| void setTrackClusterDeltaZCut(float in){ m_dz_cut_low = in; m_dz_cut_high = in;} | |
| void setTrackClusterDeltaZMin(float in){ m_dz_cut_high = in;} | |
| void setTrackClusterDeltaZMax(float in){ m_dz_cut_low = in;} | |
| void setTrackClusterDeltaPhiCut(float in){ m_dphi_cut_low = in; m_dphi_cut_high = in;} | |
| void setTrackClusterDeltaPhiMin(float in){ m_dphi_cut_high = in;} | |
| void setTrackClusterDeltaPhiMax(float in){ m_dphi_cut_low = in;} | |
| // Symmetric window: |dz| <= in | |
| void setTrackClusterDeltaZCut(float in) { m_dz_cut_low = -std::abs(in); m_dz_cut_high = std::abs(in); } | |
| void setTrackClusterDeltaZMin(float in) { m_dz_cut_low = in; } | |
| void setTrackClusterDeltaZMax(float in) { m_dz_cut_high = in; } | |
| void setTrackClusterDeltaPhiCut(float in) { m_dphi_cut_low = -std::abs(in); m_dphi_cut_high = std::abs(in); } | |
| void setTrackClusterDeltaPhiMin(float in) { m_dphi_cut_low = in; } | |
| void setTrackClusterDeltaPhiMax(float in) { m_dphi_cut_high = in; } |
| int m_runnumber = std::numeric_limits<int>::quiet_NaN(); | ||
| void runnumber(const int run) { m_runnumber = run; } |
There was a problem hiding this comment.
quiet_NaN() is meaningless for int.
std::numeric_limits<int>::quiet_NaN() returns 0 (integers have no NaN representation), so the "sentinel" is just zero — indistinguishable from a legitimate run number. Combined with the bogus concatenation in GetOutputFileNameAndPath (flagged separately), this is how you silently end up with wrong output filenames.
Also: runnumber(int) is a public configuration entry point but lives in the public: verbosity block without a set prefix; consider setRunNumber for API consistency with the rest of the setters.
- int m_runnumber = std::numeric_limits<int>::quiet_NaN();
- void runnumber(const int run) { m_runnumber = run; }
+ int m_runnumber{0};
+ void setRunNumber(const int run) { m_runnumber = run; }📝 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.
| int m_runnumber = std::numeric_limits<int>::quiet_NaN(); | |
| void runnumber(const int run) { m_runnumber = run; } | |
| int m_runnumber{0}; | |
| void setRunNumber(const int run) { m_runnumber = run; } |
Build & test reportReport for commit cdc4af2acdb9bf97016b82d889b534ae958405ce:
Automatically generated by sPHENIX Jenkins continuous integration |
1 participant
This PR adds software that creates QA histograms for track to calo matching, focused on photon conversion.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
This PR adds new software to be used with KFParticle and for data QA.
I describe how this code works and what it does here.
TODOs (if applicable)
This software needs more testing, and there is at least one known bug: If this module and the general KFParticle QA are both run in the same macro, either the macro will crash or the KFP container accessed by this module will be empty. I'm not sure how to resolve this problem, so I am asking the HF conveners for input.
Links to other PRs in macros and calibration repositories (if applicable)
KFParticle Track-to-Calorimeter Matching QA
Motivation
This PR introduces a new QA module to characterize track-to-calorimeter matching performance for KFParticle-reconstructed particles, with particular emphasis on photon conversion pair analysis. The module produces histograms tracking the quality of SVTX track projections to EMCal clusters, providing essential diagnostic information for reconstruction efficiency studies.
Key Changes
New QA subsystem:
QAKFParticleTracktoCalo(SubsysReco module, ~1200 lines total)Configuration flexibility: Setters for all matching thresholds (track-cluster distance cuts, cluster energy minimum, projection radius), optional angle cuts between daughter tracks, maximum MVTX hits per daughter, and histogram output prefix
Dual output modes: Via QAHistManager (default) or standalone ROOT file (with run-number suffix)
Build system: Standard Automake/Autoconf configuration with explicit dependencies on KFParticle, calobase, calo_io, CLHEP, and fun4all libraries
Potential Risk Areas
Possible Future Improvements
Note: AI-assisted analysis used to interpret code structure and implementation details. Manual verification of matching algorithms, cut thresholds, and container access patterns recommended before deployment.