CaloVtxReco Refactor for new algorithms - #4341
Open
danjlis wants to merge 26 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a pluggable calorimeter vertex reconstruction framework with CaloZ, JetSkew, MLP, CNN, and ViT algorithms. Algorithm metadata is stored in calorimeter vertices, while GlobalVertexv4 preserves algorithm-specific calorimeter inputs and updated reconstruction paths. ChangesAlgorithm contracts and vertex metadata
Algorithm execution and registration
MLP vertex inference
CNN and ViT inference
Global vertex integration
Sequence Diagram(s)sequenceDiagram
participant CaloVtxReco
participant CaloVtxAlgo
participant CaloVertexv1
participant GlobalVertexReco
participant GlobalVertexv4
CaloVtxReco->>CaloVtxAlgo: CalculateVertex(topNode, z)
CaloVtxAlgo-->>CaloVtxReco: calculated z and Algo()
CaloVtxReco->>CaloVertexv1: store z and calorimeter algorithm
GlobalVertexReco->>CaloVertexv1: read get_calo_algo()
GlobalVertexReco->>GlobalVertexv4: insert CALO subtype vertex
GlobalVertexv4-->>GlobalVertexReco: provide position and fit accessors
Merge Risk: 🟠 High · up to The refactor can publish a NaN calorimeter vertex while reporting successful reconstruction when insufficient jet inputs are available, which can corrupt downstream vertex selection and analysis. Merge should be held until failed vertex calculation is propagated and invalid vertices are not inserted. 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 bcfc96289546a6482f14707407554902ac498d4c:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit a1b46f65d373f08094d7f3bced187a80c3a563c1:
Automatically generated by sPHENIX Jenkins continuous integration |
For now I am only supplying z. I can add z_err (per event sigma) in another PR.
danjlis
marked this pull request as ready for review
July 12, 2026 13:19
Add CNN calo vertex implementation
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
offline/packages/globalvertex/GlobalVertexReco.cc (2)
264-306: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCALO subtype gating doesn't cover the new subtypes.
useVertexType(GlobalVertex::VTXTYPE::CALO)only returns true when_vtxtypeisCALOorUNDEFINED. If a user configures_vtxtypetoCALO_JETSKEW,CALO_AVGZ, orCALO_JETMLP(to run only that algorithm), this outer gate is false and the entire loop — including the per-vertex algo mapping at lines 290-305 — is skipped. Those three new vertex types can never be produced via explicit type selection.🐛 Proposed fix
- if (calomap && useVertexType(GlobalVertex::VTXTYPE::CALO)) + if (calomap && (useVertexType(GlobalVertex::VTXTYPE::CALO) || + useVertexType(GlobalVertex::VTXTYPE::CALO_JETSKEW) || + useVertexType(GlobalVertex::VTXTYPE::CALO_AVGZ) || + useVertexType(GlobalVertex::VTXTYPE::CALO_JETMLP)))
388-402: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMBD_CALO fallback path loses algorithm metadata.
Unlike the standalone CALO loop above (which now routes to
CALO_JETSKEW/CALO_AVGZ/CALO_JETMLPbased oncalo->get_calo_algo()), this branch (Line 391) always inserts under the genericGlobalVertex::CALOtype, regardless of the actual algorithm that producedcalo. Combined MBD+CALO vertices will silently lose their algorithm identity, undermining the PR's goal of propagatingCALOALGOthroughGlobalVertexv4.🐛 Proposed fix
GlobalVertex *vertex = new GlobalVertexv4(); vertex->set_id(globalmap->size()); - - vertex->clone_insert_vtx(GlobalVertex::CALO, calo); + + switch (calo->get_calo_algo()) + { + case VertexDefs::CALOALGO::JETSKEW: + vertex->clone_insert_vtx(GlobalVertex::CALO_JETSKEW, calo); + break; + case VertexDefs::CALOALGO::AVGZ: + vertex->clone_insert_vtx(GlobalVertex::CALO_AVGZ, calo); + break; + case VertexDefs::CALOALGO::JETMLP: + vertex->clone_insert_vtx(GlobalVertex::CALO_JETMLP, calo); + break; + default: + vertex->clone_insert_vtx(GlobalVertex::CALO, calo); + break; + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3fc14317-fae2-4cf4-a672-707dd033715b
📒 Files selected for processing (22)
offline/packages/calovtxreco/CaloVtxAlgo.hoffline/packages/calovtxreco/CaloVtxAlgoCaloZ.ccoffline/packages/calovtxreco/CaloVtxAlgoCaloZ.hoffline/packages/calovtxreco/CaloVtxAlgoJetSkew.ccoffline/packages/calovtxreco/CaloVtxAlgoJetSkew.hoffline/packages/calovtxreco/CaloVtxAlgoMLP.ccoffline/packages/calovtxreco/CaloVtxAlgoMLP.hoffline/packages/calovtxreco/CaloVtxReco.ccoffline/packages/calovtxreco/CaloVtxReco.hoffline/packages/calovtxreco/Makefile.amoffline/packages/calovtxreco/VertexMLP.hoffline/packages/globalvertex/CaloVertex.hoffline/packages/globalvertex/CaloVertexv1.ccoffline/packages/globalvertex/CaloVertexv1.hoffline/packages/globalvertex/GlobalVertex.hoffline/packages/globalvertex/GlobalVertexReco.ccoffline/packages/globalvertex/GlobalVertexv4.ccoffline/packages/globalvertex/GlobalVertexv4.hoffline/packages/globalvertex/GlobalVertexv4LinkDef.hoffline/packages/globalvertex/Makefile.amoffline/packages/globalvertex/Vertex.hoffline/packages/globalvertex/VertexDefs.h
| int iphi = emcal_towers->getTowerPhiBin(towerkey); | ||
|
|
||
| const RawTowerDefs::keytype key = RawTowerDefs::encode_towerid(RawTowerDefs::CalorimeterId::CEMC, ieta, iphi); | ||
| float tower_z = tower_geomEM->get_tower_geometry(key)->get_center_z(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm get_tower_geometry can return nullptr (no internal assertion/guarantee).
fd -e h -e cc RawTowerGeomContainer | xargs -I{} ast-grep run --lang cpp \
--pattern 'get_tower_geometry($_)' {} 2>/dev/null
rg -nP 'RawTowerGeom\s*\*\s*get_tower_geometry' -A5 $(fd RawTowerGeomContainer)Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the call sites in the target file.
sed -n '60,145p' offline/packages/calovtxreco/CaloVtxAlgoCaloZ.cc
# Inspect the concrete geometry container implementation.
sed -n '1,140p' offline/packages/CaloBase/RawTowerGeomContainerv1.h
sed -n '1,220p' offline/packages/CaloBase/RawTowerGeomContainerv1.cc
# Find any local null checks or assertions around get_tower_geometry usage.
rg -n "get_tower_geometry\\(|center_z|nullptr|assert\\(" offline/packages/calovtxreco offline/packages/CaloBaseRepository: sPHENIX-Collaboration/coresoftware
Length of output: 14220
Guard the tower-geometry lookup before dereferencing. get_tower_geometry() returns nullptr when a key is absent, so the three ->get_center_z() calls here can crash on a missing CEMC/IHCal/OHCal geometry entry. Add a null check at each call site.
Comment on lines
+136
to
+140
| double b_calo_vertex_z = (average_z[0] + average_z[1] + average_z[2]) / (total_E[0] + total_E[1] + total_E[2]); | ||
|
|
||
| zvtx = b_calo_vertex_z; | ||
|
|
||
| return Fun4AllReturnCodes::EVENT_OK; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard against zero total energy before dividing.
If no towers survive the good/energy cuts, total_E[0]+total_E[1]+total_E[2] == 0 and this becomes 0.0/0.0 (NaN) or x/0 (inf). The function then stores that in zvtx and returns EVENT_OK, so a bogus vertex is propagated. Per the CaloVtxAlgo contract, return a nonzero code (e.g. ABORTEVENT) when no vertex can be found.
🐛 Suggested guard
- double b_calo_vertex_z = (average_z[0] + average_z[1] + average_z[2]) / (total_E[0] + total_E[1] + total_E[2]);
-
- zvtx = b_calo_vertex_z;
-
- return Fun4AllReturnCodes::EVENT_OK;
+ float total = total_E[0] + total_E[1] + total_E[2];
+ if (total <= 0.f)
+ {
+ return Fun4AllReturnCodes::ABORTEVENT;
+ }
+ zvtx = (average_z[0] + average_z[1] + average_z[2]) / total;
+
+ return Fun4AllReturnCodes::EVENT_OK;📝 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
| double b_calo_vertex_z = (average_z[0] + average_z[1] + average_z[2]) / (total_E[0] + total_E[1] + total_E[2]); | |
| zvtx = b_calo_vertex_z; | |
| return Fun4AllReturnCodes::EVENT_OK; | |
| float total = total_E[0] + total_E[1] + total_E[2]; | |
| if (total <= 0.f) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| zvtx = (average_z[0] + average_z[1] + average_z[2]) / total; | |
| return Fun4AllReturnCodes::EVENT_OK; |
| ~CaloVtxAlgoCaloZ() override = default; | ||
|
|
||
| int Init(PHCompositeNode *topNode) override; | ||
| int CalculateVertex(PHCompositeNode *topNode, float &zvtx) override; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard against division by zero in CalculateVertex.
The implementation (visible in the context snippet from CaloVtxAlgoCaloZ.cc:130-132) computes:
double b_calo_vertex_z = (average_z[0] + average_z[1] + average_z[2]) / (total_E[0] + total_E[1] + total_E[2]);If all towers fail the energy cut or get_isGood() check, total_E remains zero, producing inf/NaN. The result is unconditionally assigned to zvtx and EVENT_OK is returned, so callers cannot distinguish a valid vertex from a degenerate event. Add a zero-total-energy guard that returns a non-zero code (e.g. Fun4AllReturnCodes::ABORTEVENT) or leaves zvtx as NaN with an appropriate return value.
| ~CaloVtxAlgoJetSkew() override = default; | ||
|
|
||
| int Init(PHCompositeNode *topNode) override; | ||
| int CalculateVertex(PHCompositeNode *topNode, float &zvtx) override; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Geometry mismatch in CalculateVertex — CEMC towers paired with HCALIN geometry.
The implementation (visible in the context snippet from CaloVtxAlgoJetSkew.cc:113-118) uses m_towers[0] (CEMC_RETOWER) with m_geom[1] (HCALIN geometry) and CalorimeterId::HCALIN for the comp.first == 13 || 28 || 25 branch:
TowerInfo *tower = m_towers[0]->get_tower_at_channel(channel);
...
float neweta = new_eta(channel, m_towers[0], m_geom[1], RawTowerDefs::CalorimeterId::HCALIN, testz);The HCALOUT branch (comp.first == 7 || 27) correctly pairs m_towers[2] with m_geom[2] and CalorimeterId::HCALOUT. The CEMC branch should similarly pair m_towers[0] with m_geom[0] and CalorimeterId::CEMC, or if these components are actually HCALIN, use m_towers[1] with m_geom[1]. Mixing CEMC tower data with HCALIN geometry will produce incorrect eta values and corrupt the skew metric.
Comment on lines
+24
to
+36
| RawTowerGeomContainer *tower_geomEM = findNode::getClass<RawTowerGeomContainer>(topNode, "TOWERGEOM_CEMC"); | ||
| RawTowerGeomContainer *tower_geomOH = findNode::getClass<RawTowerGeomContainer>(topNode, "TOWERGEOM_HCALOUT"); | ||
| RawTowerGeomContainer *tower_geomIH = findNode::getClass<RawTowerGeomContainer>(topNode, "TOWERGEOM_HCALIN"); | ||
| if(!tower_geomEM || !tower_geomOH) | ||
| { | ||
| return Fun4AllReturnCodes::ABORTRUN; | ||
| } | ||
| unsigned int emkey = RawTowerDefs::encode_towerid(RawTowerDefs::CalorimeterId::CEMC, 48, 128); | ||
| m_radius_EM = tower_geomEM->get_tower_geometry(emkey)->get_center_radius(); | ||
| unsigned int ihkey = RawTowerDefs::encode_towerid(RawTowerDefs::CalorimeterId::HCALIN, 12, 0); | ||
| m_radius_IH = tower_geomIH->get_tower_geometry(ihkey)->get_center_radius(); | ||
| unsigned int ohkey = RawTowerDefs::encode_towerid(RawTowerDefs::CalorimeterId::HCALOUT, 12, 0); | ||
| m_radius_OH = tower_geomOH->get_tower_geometry(ohkey)->get_center_radius(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Null dereference: tower_geomIH is used but never null-checked.
Line 27 guards only tower_geomEM and tower_geomOH, but line 34 dereferences tower_geomIH->get_tower_geometry(ihkey). If TOWERGEOM_HCALIN is absent, tower_geomIH is nullptr and this crashes. Also, get_tower_geometry(...) can return nullptr for an invalid key on any of the three calorimeters, so lines 32/34/36 each need a guard before calling get_center_radius().
🛡️ Suggested guard
- if(!tower_geomEM || !tower_geomOH)
+ if(!tower_geomEM || !tower_geomOH || !tower_geomIH)
{
return Fun4AllReturnCodes::ABORTRUN;
}📝 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
| RawTowerGeomContainer *tower_geomEM = findNode::getClass<RawTowerGeomContainer>(topNode, "TOWERGEOM_CEMC"); | |
| RawTowerGeomContainer *tower_geomOH = findNode::getClass<RawTowerGeomContainer>(topNode, "TOWERGEOM_HCALOUT"); | |
| RawTowerGeomContainer *tower_geomIH = findNode::getClass<RawTowerGeomContainer>(topNode, "TOWERGEOM_HCALIN"); | |
| if(!tower_geomEM || !tower_geomOH) | |
| { | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| unsigned int emkey = RawTowerDefs::encode_towerid(RawTowerDefs::CalorimeterId::CEMC, 48, 128); | |
| m_radius_EM = tower_geomEM->get_tower_geometry(emkey)->get_center_radius(); | |
| unsigned int ihkey = RawTowerDefs::encode_towerid(RawTowerDefs::CalorimeterId::HCALIN, 12, 0); | |
| m_radius_IH = tower_geomIH->get_tower_geometry(ihkey)->get_center_radius(); | |
| unsigned int ohkey = RawTowerDefs::encode_towerid(RawTowerDefs::CalorimeterId::HCALOUT, 12, 0); | |
| m_radius_OH = tower_geomOH->get_tower_geometry(ohkey)->get_center_radius(); | |
| RawTowerGeomContainer *tower_geomEM = findNode::getClass<RawTowerGeomContainer>(topNode, "TOWERGEOM_CEMC"); | |
| RawTowerGeomContainer *tower_geomOH = findNode::getClass<RawTowerGeomContainer>(topNode, "TOWERGEOM_HCALOUT"); | |
| RawTowerGeomContainer *tower_geomIH = findNode::getClass<RawTowerGeomContainer>(topNode, "TOWERGEOM_HCALIN"); | |
| if(!tower_geomEM || !tower_geomOH || !tower_geomIH) | |
| { | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| unsigned int emkey = RawTowerDefs::encode_towerid(RawTowerDefs::CalorimeterId::CEMC, 48, 128); | |
| m_radius_EM = tower_geomEM->get_tower_geometry(emkey)->get_center_radius(); | |
| unsigned int ihkey = RawTowerDefs::encode_towerid(RawTowerDefs::CalorimeterId::HCALIN, 12, 0); | |
| m_radius_IH = tower_geomIH->get_tower_geometry(ihkey)->get_center_radius(); | |
| unsigned int ohkey = RawTowerDefs::encode_towerid(RawTowerDefs::CalorimeterId::HCALOUT, 12, 0); | |
| m_radius_OH = tower_geomOH->get_tower_geometry(ohkey)->get_center_radius(); |
Comment on lines
+32
to
+35
| void registerAlgo(CaloVtxAlgo* algo) { m_algos.push_back(std::move(algo)); } | ||
|
|
||
| private: | ||
| std::vector<CaloVtxAlgo*> m_algos{}; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clarify ownership of registered algorithms (RAII).
m_algos holds raw CaloVtxAlgo* and the destructor is defaulted, so nothing here deletes them. Either the algorithms are owned elsewhere (in which case please document the required lifetime — they must outlive this reco module across the whole run) or they leak. Since these are heap-allocated new objects in typical Fun4All macros, storing std::unique_ptr<CaloVtxAlgo> makes ownership explicit and leak-free. Note also that std::move on a raw pointer is a no-op.
As per path instructions: "Focus on API clarity/stability, ownership semantics (RAII), and avoiding raw new/delete."
♻️ Suggested ownership-explicit API
- void registerAlgo(CaloVtxAlgo* algo) { m_algos.push_back(std::move(algo)); }
+ void registerAlgo(std::unique_ptr<CaloVtxAlgo> algo) { m_algos.push_back(std::move(algo)); }
private:
- std::vector<CaloVtxAlgo*> m_algos{};
+ std::vector<std::unique_ptr<CaloVtxAlgo>> m_algos{};📝 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
| void registerAlgo(CaloVtxAlgo* algo) { m_algos.push_back(std::move(algo)); } | |
| private: | |
| std::vector<CaloVtxAlgo*> m_algos{}; | |
| void registerAlgo(std::unique_ptr<CaloVtxAlgo> algo) { m_algos.push_back(std::move(algo)); } | |
| private: | |
| std::vector<std::unique_ptr<CaloVtxAlgo>> m_algos{}; |
Source: Path instructions
Comment on lines
+106
to
+131
| if (!ok || gMedian.GetNrows() != kNFeatures) { | ||
| std::cerr << "VertexMLP::Load: failed to load a complete model from " << filename << std::endl; | ||
| return false; | ||
| } | ||
|
|
||
| gLoaded = true; | ||
| std::cout << "VertexMLP::Load: loaded " << filename << std::endl; | ||
| return true; | ||
| } | ||
|
|
||
| // Predicts the vertex z [cm] given the 21 input features (see the ordering | ||
| // table above). Returns 0 and prints an error if Load() hasn't succeeded. | ||
| inline double PredictVertexZ(const std::array<double, kNFeatures> &features) { | ||
| if (!gLoaded) { | ||
| std::cerr << "VertexMLP::PredictVertexZ: model not loaded -- call " | ||
| "VertexMLP::Load() in InitRun() first" | ||
| << std::endl; | ||
| return 0.0; | ||
| } | ||
|
|
||
| // Same preprocessing as training: percentile clip, then median/IQR scale. | ||
| TVectorD x(kNFeatures); | ||
| for (int i = 0; i < kNFeatures; i++) { | ||
| double v = std::clamp(features[i], gLo[i], gHi[i]); | ||
| x[i] = (v - gMedian[i]) / gIqr[i]; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Load() only validates gMedian's length, leaving PredictVertexZ() open to out-of-range access.
PredictVertexZ() indexes gLo[i], gHi[i], gMedian[i], and gIqr[i] for i < kNFeatures, and multiplies by gW1..gW4, but Load() only checks gMedian.GetNrows() == kNFeatures. A weights file whose iqr/lo/hi vectors (or W* dimensions) don't match kNFeatures will pass loading and then trip ROOT's bounds check (abort) at inference time. Additionally, gIqr[i] == 0 yields a divide-by-zero (inf/nan) that propagates silently into the network. Recommend validating all scaler vector lengths (and ideally gW1 row/col dims) against kNFeatures, and guarding against zero IQR.
🛡️ Suggested validation in Load()
- if (!ok || gMedian.GetNrows() != kNFeatures) {
+ if (!ok || gMedian.GetNrows() != kNFeatures ||
+ gIqr.GetNrows() != kNFeatures || gLo.GetNrows() != kNFeatures ||
+ gHi.GetNrows() != kNFeatures) {
std::cerr << "VertexMLP::Load: failed to load a complete model from " << filename << std::endl;
return false;
}📝 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 (!ok || gMedian.GetNrows() != kNFeatures) { | |
| std::cerr << "VertexMLP::Load: failed to load a complete model from " << filename << std::endl; | |
| return false; | |
| } | |
| gLoaded = true; | |
| std::cout << "VertexMLP::Load: loaded " << filename << std::endl; | |
| return true; | |
| } | |
| // Predicts the vertex z [cm] given the 21 input features (see the ordering | |
| // table above). Returns 0 and prints an error if Load() hasn't succeeded. | |
| inline double PredictVertexZ(const std::array<double, kNFeatures> &features) { | |
| if (!gLoaded) { | |
| std::cerr << "VertexMLP::PredictVertexZ: model not loaded -- call " | |
| "VertexMLP::Load() in InitRun() first" | |
| << std::endl; | |
| return 0.0; | |
| } | |
| // Same preprocessing as training: percentile clip, then median/IQR scale. | |
| TVectorD x(kNFeatures); | |
| for (int i = 0; i < kNFeatures; i++) { | |
| double v = std::clamp(features[i], gLo[i], gHi[i]); | |
| x[i] = (v - gMedian[i]) / gIqr[i]; | |
| } | |
| if (!ok || gMedian.GetNrows() != kNFeatures || | |
| gIqr.GetNrows() != kNFeatures || gLo.GetNrows() != kNFeatures || | |
| gHi.GetNrows() != kNFeatures) { | |
| std::cerr << "VertexMLP::Load: failed to load a complete model from " << filename << std::endl; | |
| return false; | |
| } | |
| gLoaded = true; | |
| std::cout << "VertexMLP::Load: loaded " << filename << std::endl; | |
| return true; | |
| } | |
| // Predicts the vertex z [cm] given the 21 input features (see the ordering | |
| // table above). Returns 0 and prints an error if Load() hasn't succeeded. | |
| inline double PredictVertexZ(const std::array<double, kNFeatures> &features) { | |
| if (!gLoaded) { | |
| std::cerr << "VertexMLP::PredictVertexZ: model not loaded -- call " | |
| "VertexMLP::Load() in InitRun() first" | |
| << std::endl; | |
| return 0.0; | |
| } | |
| // Same preprocessing as training: percentile clip, then median/IQR scale. | |
| TVectorD x(kNFeatures); | |
| for (int i = 0; i < kNFeatures; i++) { | |
| double v = std::clamp(features[i], gLo[i], gHi[i]); | |
| x[i] = (v - gMedian[i]) / gIqr[i]; | |
| } |
Comment on lines
+319
to
+345
| // okay now put in zero | ||
| if (useVertexType(GlobalVertex::VTXTYPE::ZERO)) | ||
| { | ||
| if (Verbosity()) | ||
| { | ||
| std::cout << "GlobalVertexReco::process_event - zero" << std::endl; | ||
| } | ||
|
|
||
| CaloVertex *cvertex = new CaloVertexv1(); | ||
| cvertex->set_z(0); | ||
| cvertex->set_t(0); | ||
| cvertex->set_z_err(0); | ||
| cvertex->set_t_err(0); | ||
| cvertex->set_id(0); | ||
| GlobalVertex *vertex = new GlobalVertexv4(); | ||
|
|
||
| vertex->clone_insert_vtx(GlobalVertex::ZERO, cvertex); | ||
| vertex->set_id(globalmap->size()); | ||
| //used_calo_vtxids.insert(cvertex->get_id()); | ||
|
|
||
| globalmap->insert(vertex); | ||
|
|
||
| if (Verbosity() > 1) | ||
| { | ||
| vertex->identify(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Memory leak: cvertex is never freed.
clone_insert_vtx clones cvertex internally (CloneMe()) and stores the clone; the original heap-allocated cvertex at Line 327 is left dangling with nothing to delete it. This leaks on every event that exercises the ZERO-vertex path.
🐛 Proposed fix
vertex->clone_insert_vtx(GlobalVertex::ZERO, cvertex);
+ delete cvertex;
vertex->set_id(globalmap->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
| // okay now put in zero | |
| if (useVertexType(GlobalVertex::VTXTYPE::ZERO)) | |
| { | |
| if (Verbosity()) | |
| { | |
| std::cout << "GlobalVertexReco::process_event - zero" << std::endl; | |
| } | |
| CaloVertex *cvertex = new CaloVertexv1(); | |
| cvertex->set_z(0); | |
| cvertex->set_t(0); | |
| cvertex->set_z_err(0); | |
| cvertex->set_t_err(0); | |
| cvertex->set_id(0); | |
| GlobalVertex *vertex = new GlobalVertexv4(); | |
| vertex->clone_insert_vtx(GlobalVertex::ZERO, cvertex); | |
| vertex->set_id(globalmap->size()); | |
| //used_calo_vtxids.insert(cvertex->get_id()); | |
| globalmap->insert(vertex); | |
| if (Verbosity() > 1) | |
| { | |
| vertex->identify(); | |
| } | |
| } | |
| // okay now put in zero | |
| if (useVertexType(GlobalVertex::VTXTYPE::ZERO)) | |
| { | |
| if (Verbosity()) | |
| { | |
| std::cout << "GlobalVertexReco::process_event - zero" << std::endl; | |
| } | |
| CaloVertex *cvertex = new CaloVertexv1(); | |
| cvertex->set_z(0); | |
| cvertex->set_t(0); | |
| cvertex->set_z_err(0); | |
| cvertex->set_t_err(0); | |
| cvertex->set_id(0); | |
| GlobalVertex *vertex = new GlobalVertexv4(); | |
| vertex->clone_insert_vtx(GlobalVertex::ZERO, cvertex); | |
| delete cvertex; | |
| vertex->set_id(globalmap->size()); | |
| //used_calo_vtxids.insert(cvertex->get_id()); | |
| globalmap->insert(vertex); | |
| if (Verbosity() > 1) | |
| { | |
| vertex->identify(); | |
| } | |
| } |
Comment on lines
+94
to
+114
| float GlobalVertexv4::get_t() const | ||
| { | ||
| auto it = _vtxs.find(GlobalVertex::VTXTYPE::MBD); | ||
| if (it == _vtxs.end()) | ||
| { | ||
| return std::numeric_limits<float>::quiet_NaN(); | ||
| } | ||
|
|
||
| return it->second[0]->get_t(); | ||
| } | ||
|
|
||
| float GlobalVertexv4::get_t_err() const | ||
| { | ||
| auto it = _vtxs.find(GlobalVertex::VTXTYPE::MBD); | ||
| if (it == _vtxs.end()) | ||
| { | ||
| return std::numeric_limits<float>::quiet_NaN(); | ||
| } | ||
|
|
||
| return it->second[0]->get_t_err(); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
get_t()/get_t_err() don't fall back to CALO/ZERO vertices.
get_position() was extended to fall back through CALO/CALO_JETSKEW/CALO_AVGZ/CALO_JETMLP/ZERO/TRUTH, but get_t()/get_t_err() still only look at MBD. Any GlobalVertex populated exclusively from a calorimeter algorithm (or the ZERO placeholder, which explicitly sets t=0/t_err=0 in GlobalVertexReco.cc) will report NaN timing even though valid values exist on the underlying vertex.
🐛 Proposed fix
float GlobalVertexv4::get_t() const
{
auto it = _vtxs.find(GlobalVertex::VTXTYPE::MBD);
- if (it == _vtxs.end())
- {
- return std::numeric_limits<float>::quiet_NaN();
- }
-
- return it->second[0]->get_t();
+ if (it != _vtxs.end())
+ {
+ return it->second[0]->get_t();
+ }
+ for (auto type : {GlobalVertex::VTXTYPE::CALO, GlobalVertex::VTXTYPE::CALO_JETSKEW,
+ GlobalVertex::VTXTYPE::CALO_AVGZ, GlobalVertex::VTXTYPE::CALO_JETMLP,
+ GlobalVertex::VTXTYPE::ZERO})
+ {
+ auto caloit = _vtxs.find(type);
+ if (caloit != _vtxs.end())
+ {
+ return caloit->second[0]->get_t();
+ }
+ }
+ return std::numeric_limits<float>::quiet_NaN();
}(analogous change for get_t_err())
| void identify(std::ostream& os = std::cout) const override; | ||
| void Reset() override; | ||
| int isValid() const override; | ||
| PHObject* CloneMe() const override { return new GlobalVertexv4(*this); } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether prior GlobalVertex versions have the same shallow-copy CloneMe(), and whether CloneMe() is invoked anywhere on GlobalVertex-derived objects.
fd 'GlobalVertexv[123]\.(h|cc)$' offline/packages/globalvertex --exec grep -n -A2 'CloneMe' {}
rg -n -A3 '\->CloneMe\s*\(' offline/packages | rg -i 'globalvertex'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- GlobalVertexv4.h ---'
sed -n '1,220p' offline/packages/globalvertex/GlobalVertexv4.h
echo
echo '--- GlobalVertexv4.cc ---'
sed -n '1,220p' offline/packages/globalvertex/GlobalVertexv4.cc
echo
echo '--- GlobalVertexMapv1.cc ---'
sed -n '1,220p' offline/packages/globalvertex/GlobalVertexMapv1.cc
echo
echo '--- copy/assignment declarations in globalvertex ---'
rg -n 'GlobalVertexv[1234]\s*\(|operator=|CloneMe\(\)' offline/packages/globalvertex/GlobalVertexv*.h offline/packages/globalvertex/GlobalVertexv*.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 12732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [
Path('offline/packages/globalvertex/GlobalVertexv4.h'),
Path('offline/packages/globalvertex/GlobalVertexv4.cc'),
Path('offline/packages/globalvertex/GlobalVertexMapv1.cc'),
]:
print(f'--- {path} ---')
text = path.read_text()
for needle in ['~GlobalVertexv4', 'Reset', 'clear_vtxs', 'delete', 'CloneMe', 'VertexVector', 'std::map<VTXTYPE, VertexVector>']:
if needle in text:
print(f'contains: {needle}')
print()
PYRepository: sPHENIX-Collaboration/coresoftware
Length of output: 605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' offline/packages/globalvertex/GlobalVertexv4.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 5238
CloneMe() returns a shallow copy of owned vertices
_vtxs holds raw Vertex* entries that Reset() and ~GlobalVertexv4() delete. new GlobalVertexv4(*this) copies those pointers instead of cloning the pointees, so the result is not an independent object and can trigger double-delete/use-after-free if it is treated as a true clone.
Build & test reportReport for commit 991104841919cff036d719119729c8fcd9e51b29:
Automatically generated by sPHENIX Jenkins continuous integration |
Add CaloVtxAlgoVit method
Build & test reportReport for commit 9b1f479def0bb6944bfadf83482a11d214d50f53:
Automatically generated by sPHENIX Jenkins continuous integration |
pinkenburg
marked this pull request as draft
July 14, 2026 23:35
Contributor
|
This needs some work - converted to draft to prevent accidental merge |
Build & test reportReport for commit d333b5c395288c0203a36d2e904debb8b91a52d9:
Automatically generated by sPHENIX Jenkins continuous integration |
5 tasks
Calozvertex cnn, possible fix for crashing jobs
Build & test reportReport for commit 18b0172709be4d0c8afc9ca883deb298aa4c9c00:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
|
This code has a long standing problem using some uninitialized variable. Scan build in this jenkins session didn't run (no idea why not), here is the CaloVtxReco output from another PR |
Build & test reportReport for commit a65f6712c68d72e562e777b36c379a0cbab44b1e:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 84b0c746e9c2885b2e38b1601b2d20365288bb3c:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 74484b923f1132e9d532b6b385b97b1b5a34ca5a:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit ee992437358d28c150be4900ef829bcdc90824c1:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit c99c4d42f504ed901634e4e3e5ea72172086b64b:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 043c7c32bc4a6a1f4fd0c2afdf851bc9d73aa18b:
Automatically generated by sPHENIX Jenkins continuous integration |
danjlis
marked this pull request as ready for review
August 25, 2026 21:31
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b392571-7a86-4072-9352-4a0d3e9cf7cb
📒 Files selected for processing (7)
offline/packages/calovtxreco/CaloVtxAlgoCNN.ccoffline/packages/calovtxreco/CaloVtxAlgoCNN.hoffline/packages/calovtxreco/CaloVtxAlgoJetSkew.ccoffline/packages/calovtxreco/CaloVtxAlgoJetSkew.hoffline/packages/calovtxreco/CaloVtxAlgoMLP.ccoffline/packages/calovtxreco/CaloVtxAlgoMLP.hoffline/packages/calovtxreco/CaloVtxReco.cc
💤 Files with no reviewable changes (1)
- offline/packages/calovtxreco/CaloVtxReco.cc
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Comment on lines
+310
to
+313
| if (ijet < 2) | ||
| { | ||
| return Fun4AllReturnCodes::EVENT_OK; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not report a missing MLP vertex as EVENT_OK.
When fewer than two jets qualify, zvtx remains NaN and VertexMLP::PredictVertexZ is not called. This branch still returns EVENT_OK. CaloVtxReco::process_event ignores that status and inserts the NaN value into the vertex map. Events without jetscon also reach this branch.
Return a nonzero failure status, such as Fun4AllReturnCodes::ABORTEVENT, and make CaloVtxReco skip vertex insertion when CalculateVertex fails. Otherwise, these events publish a NaN MLP vertex as a successful reconstruction result.
As per path instructions: “Prioritize correctness, memory safety, error handling, and thread-safety. Only raise Critical or Major findings.”
Source: Path instructions
Contributor
|
scan-build found a memory leak (the new vectors starting in line 76 are not deleted on an early return): |
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
3 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, ...)
This change makes it so new calo-z-vertex algorithms can be added with CaloVtxAlgo inheritance.
Added in this way is the original Jet Skew and Calo Avg Z algorithms. Added is a jet variable neural net model for testing.
Also global vertex v4 is added to accommodate for new vertex types CALO_AVGZ, CALO_JETSEKW, CALO_JETMLP.
VertexDefs.h was added for a CALOALGO enum to pass on information about the call algorithm used in a given CaloVertex object.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation
Refactor calorimeter z-vertex reconstruction so contributors can add algorithms through a common
CaloVtxAlgointerface. Preserve Jet Skew and average-Z reconstruction. Add neural-network options for testing and future development. Record the algorithm that produced each calorimeter vertex.AI-generated summaries can contain errors. Verify details against the implementation and validation results.
Key changes
CaloVtxAlgointerface with initialization, vertex calculation, name, and algorithm-identification methods.CaloVtxRecoto register algorithms and create taggedCaloVertexv1objects for each result.CaloVtxAlgoCaloZfor energy-weighted tower-center z.CaloVtxAlgoJetSkewfor jet-based skew minimization.CaloVtxAlgoMLPwith theVertexMLPROOT-model interface.CaloVtxAlgoCNNandCaloVtxAlgoVitwith ONNX Runtime support.VertexDefs::CALOALGOidentifiers for the calorimeter algorithms.VertexandCaloVertexAPIs to store and retrieve the algorithm identifier.GlobalVertexv4and algorithm-specific calorimeter vertex types.GlobalVertexReco, ROOT dictionaries, installed headers, and build dependencies.z_err,t, andt_errvalues to zero.Potential risk areas
VertexMLPuses shared inline model state, which may limit thread safety.Possible future improvements