New QA module to make cluster-state residual plots#4118
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new QA module, StateClusterResidualsQA, plus its header and Makefile entries. The module computes per-track state vs. cluster residuals, creates per-config histograms, and integrates with the QA histogram manager during InitRun/process_event/EndRun. Changes
Sequence Diagram(s)sequenceDiagram
participant EventLoop as Event Loop
participant QAMod as StateClusterResidualsQA
participant PHNode as PHCompositeNode
participant TrackMap as SvtxTrackMap
participant Clusters as TrkrClusterContainer
participant Geom as ActsGeometry
participant HistMgr as QAHistogramManager
participant Hist as Histograms
EventLoop->>QAMod: InitRun(top_node)
QAMod->>PHNode: get_node("SvtxTrackMap")
QAMod->>PHNode: get_node("TRKR_CLUSTER")
QAMod->>PHNode: get_node("ActsGeometry")
QAMod->>HistMgr: acquire manager
QAMod->>QAMod: createHistos()
QAMod->>HistMgr: register histograms
loop per event
EventLoop->>QAMod: process_event(top_node)
QAMod->>TrackMap: iterate tracks
loop per track
QAMod->>TrackMap: access track states
QAMod->>Clusters: lookup cluster for state
QAMod->>Geom: convert cluster to global position
alt selection matched
QAMod->>Hist: fill residual histograms (x,y,z)
end
end
end
EventLoop->>QAMod: EndRun(runnumber)
QAMod->>HistMgr: finalize (no extra processing)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Comment on lines
+113
to
+118
| for (const auto& cfg : m_pending) | ||
| { | ||
| m_histograms_x.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_x")))); | ||
| m_histograms_y.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_y")))); | ||
| m_histograms_z.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_z")))); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing null check on retrieved histogram pointers.
hm->getHisto() may return nullptr if the histogram name lookup fails. These pointers are stored and later dereferenced unconditionally in process_event() (lines 195-197), which would cause a crash.
Consider validating each histogram pointer after retrieval:
Proposed fix
for (const auto& cfg : m_pending)
{
- m_histograms_x.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_x"))));
- m_histograms_y.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_y"))));
- m_histograms_z.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_z"))));
+ auto* hx = dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_x")));
+ auto* hy = dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_y")));
+ auto* hz = dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_z")));
+ if (!hx || !hy || !hz)
+ {
+ std::cout << PHWHERE << "\n"
+ << "\tCould not retrieve histogram for config: " << cfg.name << "\n"
+ << "\tAborting\n" << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+ }
+ m_histograms_x.push_back(hx);
+ m_histograms_y.push_back(hy);
+ m_histograms_z.push_back(hz);
}
Comment on lines
+185
to
+198
| auto *cluster = cluster_map->findCluster(state->get_cluskey()); | ||
| float state_x = state->get_x(); | ||
| float state_y = state->get_y(); | ||
| float state_z = state->get_z(); | ||
| Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster); | ||
| float cluster_x = glob.x(); | ||
| float cluster_y = glob.y(); | ||
| float cluster_z = glob.z(); | ||
| if (cluster) | ||
| { | ||
| m_histograms_x[h]->Fill(state_x - cluster_x); | ||
| m_histograms_y[h]->Fill(state_y - cluster_y); | ||
| m_histograms_z[h]->Fill(state_z - cluster_z); | ||
| } |
Contributor
There was a problem hiding this comment.
Null pointer dereference: cluster used before null check.
findCluster() may return nullptr, but cluster is passed to geometry->getGlobalPosition() on line 189 before the null check on line 193. This will cause undefined behavior when the cluster is not found.
Proposed fix
auto *cluster = cluster_map->findCluster(state->get_cluskey());
+ if (!cluster)
+ {
+ continue;
+ }
float state_x = state->get_x();
float state_y = state->get_y();
float state_z = state->get_z();
Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster);
float cluster_x = glob.x();
float cluster_y = glob.y();
float cluster_z = glob.z();
- if (cluster)
- {
- m_histograms_x[h]->Fill(state_x - cluster_x);
- m_histograms_y[h]->Fill(state_y - cluster_y);
- m_histograms_z[h]->Fill(state_z - cluster_z);
- }
+ m_histograms_x[h]->Fill(state_x - cluster_x);
+ m_histograms_y[h]->Fill(state_y - cluster_y);
+ m_histograms_z[h]->Fill(state_z - cluster_z);📝 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
| auto *cluster = cluster_map->findCluster(state->get_cluskey()); | |
| float state_x = state->get_x(); | |
| float state_y = state->get_y(); | |
| float state_z = state->get_z(); | |
| Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster); | |
| float cluster_x = glob.x(); | |
| float cluster_y = glob.y(); | |
| float cluster_z = glob.z(); | |
| if (cluster) | |
| { | |
| m_histograms_x[h]->Fill(state_x - cluster_x); | |
| m_histograms_y[h]->Fill(state_y - cluster_y); | |
| m_histograms_z[h]->Fill(state_z - cluster_z); | |
| } | |
| auto *cluster = cluster_map->findCluster(state->get_cluskey()); | |
| if (!cluster) | |
| { | |
| continue; | |
| } | |
| float state_x = state->get_x(); | |
| float state_y = state->get_y(); | |
| float state_z = state->get_z(); | |
| Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster); | |
| float cluster_x = glob.x(); | |
| float cluster_y = glob.y(); | |
| float cluster_z = glob.z(); | |
| m_histograms_x[h]->Fill(state_x - cluster_x); | |
| m_histograms_y[h]->Fill(state_y - cluster_y); | |
| m_histograms_z[h]->Fill(state_z - cluster_z); |
Comment on lines
+56
to
+101
| StateClusterResidualsQA& setNMvtx(int min, int max) | ||
| { | ||
| m_pending.back().min_mvtx_clusters = min; | ||
| m_pending.back().max_mvtx_clusters = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setNIntt(int min, int max) | ||
| { | ||
| m_pending.back().min_intt_clusters = min; | ||
| m_pending.back().max_intt_clusters = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setNTpc(int min, int max) | ||
| { | ||
| m_pending.back().min_tpc_clusters = min; | ||
| m_pending.back().max_tpc_clusters = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setPhiRange(float min, float max) | ||
| { | ||
| m_pending.back().phi_min = min; | ||
| m_pending.back().phi_max = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setEtaRange(float min, float max) | ||
| { | ||
| m_pending.back().eta_min = min; | ||
| m_pending.back().eta_max = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setPtRange(float min, float max) | ||
| { | ||
| m_pending.back().pt_min = min; | ||
| m_pending.back().pt_max = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setPositiveTracks() | ||
| { | ||
| m_pending.back().charge = 1; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setNegativeTracks() | ||
| { | ||
| m_pending.back().charge = -1; | ||
| return *this; | ||
| } |
Contributor
There was a problem hiding this comment.
Undefined behavior: m_pending.back() called on potentially empty vector.
All fluent setter methods (setNMvtx, setNIntt, setNTpc, setPhiRange, setEtaRange, setPtRange, setPositiveTracks, setNegativeTracks) access m_pending.back() without verifying that m_pending is non-empty. If a user calls any of these before addHistogram(), this results in undefined behavior.
Consider adding an assertion or guard:
Proposed fix (example for setNMvtx)
StateClusterResidualsQA& setNMvtx(int min, int max)
{
+ assert(!m_pending.empty() && "Must call addHistogram() before setNMvtx()");
m_pending.back().min_mvtx_clusters = min;
m_pending.back().max_mvtx_clusters = max;
return *this;
}Apply similar guards to all other setter methods, or extract a helper that returns a reference with the check.
Build & test reportReport for commit d1152d0f8d158c3f7abd88006dfda222a3c0ffc3:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
offline/QA/Tracking/StateClusterResidualsQA.cc (2)
185-198: Null pointer dereference:clusterpassed togetGlobalPositionbefore null check.
findCluster()may returnnullptr, but the pointer is used on line 189 before it is validated on line 193. This causes undefined behavior when a cluster is not found.Move the null check immediately after
findCluster().
113-118: Missing null check on retrieved histogram pointers.
hm->getHisto()may returnnullptrif lookup fails. These pointers are later dereferenced unconditionally inprocess_event(), causing a crash.Validate each histogram pointer after retrieval and return
ABORTRUNon failure.
Comment on lines
+125
to
+127
| auto* track_map = findNode::getClass<SvtxTrackMap>(top_node, m_track_map_node_name); | ||
| auto *cluster_map = findNode::getClass<TrkrClusterContainer>(top_node, m_clusterContainerName); | ||
| auto *geometry = findNode::getClass<ActsGeometry>(top_node, "ActsGeometry"); |
Contributor
There was a problem hiding this comment.
No null checks on re-fetched node pointers.
These pointers are dereferenced unconditionally (e.g., *track_map at line 129, cluster_map->findCluster at line 185, geometry->getGlobalPosition at line 189). If the nodes become unavailable after InitRun, this will crash.
While the framework typically guarantees node persistence, defensive null checks would prevent silent corruption if assumptions are violated.
Proposed fix
auto* track_map = findNode::getClass<SvtxTrackMap>(top_node, m_track_map_node_name);
auto *cluster_map = findNode::getClass<TrkrClusterContainer>(top_node, m_clusterContainerName);
auto *geometry = findNode::getClass<ActsGeometry>(top_node, "ActsGeometry");
+
+ if (!track_map || !cluster_map || !geometry)
+ {
+ return Fun4AllReturnCodes::ABORTEVENT;
+ }📝 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
| auto* track_map = findNode::getClass<SvtxTrackMap>(top_node, m_track_map_node_name); | |
| auto *cluster_map = findNode::getClass<TrkrClusterContainer>(top_node, m_clusterContainerName); | |
| auto *geometry = findNode::getClass<ActsGeometry>(top_node, "ActsGeometry"); | |
| auto* track_map = findNode::getClass<SvtxTrackMap>(top_node, m_track_map_node_name); | |
| auto *cluster_map = findNode::getClass<TrkrClusterContainer>(top_node, m_clusterContainerName); | |
| auto *geometry = findNode::getClass<ActsGeometry>(top_node, "ActsGeometry"); | |
| if (!track_map || !cluster_map || !geometry) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } |
Comment on lines
+160
to
+202
| int h = 0; | ||
| for (const auto& cfg : m_pending) | ||
| { | ||
| if (cfg.charge != 0) | ||
| { | ||
| if ((cfg.charge < 0) && track->get_positive_charge()) | ||
| { | ||
| continue; | ||
| } | ||
| if ((cfg.charge > 0) && !(track->get_positive_charge())) | ||
| { | ||
| continue; | ||
| } | ||
| } | ||
| if (cfg.min_mvtx_clusters <= counters[TrkrDefs::mvtxId] && cfg.max_mvtx_clusters >= counters[TrkrDefs::mvtxId] | ||
| && cfg.min_intt_clusters <= counters[TrkrDefs::inttId] && cfg.max_intt_clusters >= counters[TrkrDefs::inttId] | ||
| && cfg.min_tpc_clusters <= counters[TrkrDefs::tpcId] && cfg.max_tpc_clusters >= counters[TrkrDefs::tpcId] | ||
| && cfg.phi_min <= track_phi && cfg.phi_max >= track_phi | ||
| && cfg.eta_min <= track_eta && cfg.eta_max >= track_eta | ||
| && cfg.pt_min <= track_pt && cfg.pt_max >= track_pt) | ||
| { | ||
| for (auto const& [path_length, state] : range_adaptor(track->begin_states(), track->end_states())) | ||
| { | ||
| if (path_length == 0) { continue; } | ||
|
|
||
| auto *cluster = cluster_map->findCluster(state->get_cluskey()); | ||
| float state_x = state->get_x(); | ||
| float state_y = state->get_y(); | ||
| float state_z = state->get_z(); | ||
| Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster); | ||
| float cluster_x = glob.x(); | ||
| float cluster_y = glob.y(); | ||
| float cluster_z = glob.z(); | ||
| if (cluster) | ||
| { | ||
| m_histograms_x[h]->Fill(state_x - cluster_x); | ||
| m_histograms_y[h]->Fill(state_y - cluster_y); | ||
| m_histograms_z[h]->Fill(state_z - cluster_z); | ||
| } | ||
| } | ||
| } | ||
| ++h; | ||
| } |
Contributor
There was a problem hiding this comment.
Critical bug: Histogram index h becomes misaligned after charge-filter continue.
When the charge filter (lines 165-172) triggers continue, the loop skips the ++h at line 201. Subsequent configs will then use incorrect histogram indices, filling the wrong histograms.
The index must be incremented regardless of whether the track passes the charge filter.
Proposed fix
int h = 0;
for (const auto& cfg : m_pending)
{
+ // Increment index early so continue statements don't skip it
+ int current_h = h++;
+
if (cfg.charge != 0)
{
if ((cfg.charge < 0) && track->get_positive_charge())
{
- continue;
+ continue; // h already incremented above
}
if ((cfg.charge > 0) && !(track->get_positive_charge()))
{
continue;
}
}
if (cfg.min_mvtx_clusters <= counters[TrkrDefs::mvtxId] && cfg.max_mvtx_clusters >= counters[TrkrDefs::mvtxId]
&& cfg.min_intt_clusters <= counters[TrkrDefs::inttId] && cfg.max_intt_clusters >= counters[TrkrDefs::inttId]
&& cfg.min_tpc_clusters <= counters[TrkrDefs::tpcId] && cfg.max_tpc_clusters >= counters[TrkrDefs::tpcId]
&& cfg.phi_min <= track_phi && cfg.phi_max >= track_phi
&& cfg.eta_min <= track_eta && cfg.eta_max >= track_eta
&& cfg.pt_min <= track_pt && cfg.pt_max >= track_pt)
{
for (auto const& [path_length, state] : range_adaptor(track->begin_states(), track->end_states()))
{
if (path_length == 0) { continue; }
auto *cluster = cluster_map->findCluster(state->get_cluskey());
+ if (!cluster)
+ {
+ continue;
+ }
float state_x = state->get_x();
float state_y = state->get_y();
float state_z = state->get_z();
Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster);
float cluster_x = glob.x();
float cluster_y = glob.y();
float cluster_z = glob.z();
- if (cluster)
- {
- m_histograms_x[h]->Fill(state_x - cluster_x);
- m_histograms_y[h]->Fill(state_y - cluster_y);
- m_histograms_z[h]->Fill(state_z - cluster_z);
- }
+ m_histograms_x[current_h]->Fill(state_x - cluster_x);
+ m_histograms_y[current_h]->Fill(state_y - cluster_y);
+ m_histograms_z[current_h]->Fill(state_z - cluster_z);
}
}
- ++h;
}📝 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
| int h = 0; | |
| for (const auto& cfg : m_pending) | |
| { | |
| if (cfg.charge != 0) | |
| { | |
| if ((cfg.charge < 0) && track->get_positive_charge()) | |
| { | |
| continue; | |
| } | |
| if ((cfg.charge > 0) && !(track->get_positive_charge())) | |
| { | |
| continue; | |
| } | |
| } | |
| if (cfg.min_mvtx_clusters <= counters[TrkrDefs::mvtxId] && cfg.max_mvtx_clusters >= counters[TrkrDefs::mvtxId] | |
| && cfg.min_intt_clusters <= counters[TrkrDefs::inttId] && cfg.max_intt_clusters >= counters[TrkrDefs::inttId] | |
| && cfg.min_tpc_clusters <= counters[TrkrDefs::tpcId] && cfg.max_tpc_clusters >= counters[TrkrDefs::tpcId] | |
| && cfg.phi_min <= track_phi && cfg.phi_max >= track_phi | |
| && cfg.eta_min <= track_eta && cfg.eta_max >= track_eta | |
| && cfg.pt_min <= track_pt && cfg.pt_max >= track_pt) | |
| { | |
| for (auto const& [path_length, state] : range_adaptor(track->begin_states(), track->end_states())) | |
| { | |
| if (path_length == 0) { continue; } | |
| auto *cluster = cluster_map->findCluster(state->get_cluskey()); | |
| float state_x = state->get_x(); | |
| float state_y = state->get_y(); | |
| float state_z = state->get_z(); | |
| Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster); | |
| float cluster_x = glob.x(); | |
| float cluster_y = glob.y(); | |
| float cluster_z = glob.z(); | |
| if (cluster) | |
| { | |
| m_histograms_x[h]->Fill(state_x - cluster_x); | |
| m_histograms_y[h]->Fill(state_y - cluster_y); | |
| m_histograms_z[h]->Fill(state_z - cluster_z); | |
| } | |
| } | |
| } | |
| ++h; | |
| } | |
| int h = 0; | |
| for (const auto& cfg : m_pending) | |
| { | |
| // Increment index early so continue statements don't skip it | |
| int current_h = h++; | |
| if (cfg.charge != 0) | |
| { | |
| if ((cfg.charge < 0) && track->get_positive_charge()) | |
| { | |
| continue; | |
| } | |
| if ((cfg.charge > 0) && !(track->get_positive_charge())) | |
| { | |
| continue; | |
| } | |
| } | |
| if (cfg.min_mvtx_clusters <= counters[TrkrDefs::mvtxId] && cfg.max_mvtx_clusters >= counters[TrkrDefs::mvtxId] | |
| && cfg.min_intt_clusters <= counters[TrkrDefs::inttId] && cfg.max_intt_clusters >= counters[TrkrDefs::inttId] | |
| && cfg.min_tpc_clusters <= counters[TrkrDefs::tpcId] && cfg.max_tpc_clusters >= counters[TrkrDefs::tpcId] | |
| && cfg.phi_min <= track_phi && cfg.phi_max >= track_phi | |
| && cfg.eta_min <= track_eta && cfg.eta_max >= track_eta | |
| && cfg.pt_min <= track_pt && cfg.pt_max >= track_pt) | |
| { | |
| for (auto const& [path_length, state] : range_adaptor(track->begin_states(), track->end_states())) | |
| { | |
| if (path_length == 0) { continue; } | |
| auto *cluster = cluster_map->findCluster(state->get_cluskey()); | |
| if (!cluster) | |
| { | |
| continue; | |
| } | |
| float state_x = state->get_x(); | |
| float state_y = state->get_y(); | |
| float state_z = state->get_z(); | |
| Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster); | |
| float cluster_x = glob.x(); | |
| float cluster_y = glob.y(); | |
| float cluster_z = glob.z(); | |
| m_histograms_x[current_h]->Fill(state_x - cluster_x); | |
| m_histograms_y[current_h]->Fill(state_y - cluster_y); | |
| m_histograms_z[current_h]->Fill(state_z - cluster_z); | |
| } | |
| } | |
| } |
Build & test reportReport for commit 6c11c5d5a51c7dcb4eb0ff7fcf70a64280d90d90:
Automatically generated by sPHENIX Jenkins continuous integration |
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.
comment: New QA module that lets the user check the residuals between the x, y, and z cluster and track state positions to monitor the quality of the track fitting for different conditions. Current available cuts are on nMVTX, nINTT, nTPC, track eta, track phi, track pT, and positive/negative charge. Any suggestions or comments are welcome!
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
This PR introduces a new QA module, StateClusterResidualsQA, to monitor track-fitting quality in the sPHENIX tracking system by producing residual histograms between fitted track state positions and cluster positions (x, y, z) across tracking detectors.
Motivation / Context
Residual distributions between track states and cluster positions are key diagnostics for alignment, systematic biases, and track-fitting performance. This module provides configurable, per-track-population monitoring to help identify detector- and reconstruction-level issues during commissioning and routine QA.
Key Changes
Potential Risk Areas
Possible Future Improvements
Note: AI-generated summaries can be wrong or miss details — please review the implementation and the assumptions about node lifetimes, histogram naming, and thread-safety before merging.