Conversation
Fill ntuple for both matched and unmatched waveforms
…s_pool_evaluation_update Small update on TPOT event combining evaluation
if multiple hits are found on the same strip only the first one, timewise, is kept.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
add quality flag to total energy calc and correlations
fix: clang-format track fitter
…e into KFParticle
- Updated downscale factors for the calorimeters and mbd by the ratio of nucleons: OO / AuAu - Add "OO" as a m_species option - Updated RunnumberRange to place temporary first and last for OO as well as update the last runnumber for proton+proton.
…nstead of unsigned int
added mbd_status calib, overlapping waveforms (pileup) fit
…id-OO-update CaloValid - O+O Update
Docstrings generation was requested by @pinkenburg. * sPHENIX-Collaboration#4145 (comment) The following files were modified: * `offline/QA/Calorimeters/CaloValid.cc` * `offline/framework/phool/RunnumberRange.h`
…ion/coderabbitai/docstrings/373f714 📝 Add docstrings to `CaloValid-OO-update`
…_fix Use short int for vertex beam crossing
do not abort for missing trms calib
…transforms Minor updates to PHActsSiliconSeeder, TrackResiduals, PHSimpleVertexFinder
does not try to download sampmax calibs etc for sims
Undo checks for existing calibs
…plot fix: expand crossing range
Improvements to Cluster State Residuals QA
…izer_fix Make TrkrNtuplizer do_hit_eval do hit eval (and also fix PID)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
offline/packages/trackreco/PHSiliconSeedMerger.cc (3)
98-98:⚠️ Potential issue | 🔴 Critical
std::numeric_limits<int>::quiet_NaN()is invalid for integer types.
quiet_NaN()is only meaningful for floating-point types. For integers, this returns 0 (or is undefined behavior in some implementations). The comparisontrack1Strobe == track2Strobeat line 188/202 will incorrectly match when both are uninitialized.Consider using a sentinel value like
-1orstd::optional<int>to represent "not set".🐛 Proposed fix using sentinel value
- int track1Strobe = std::numeric_limits<int>::quiet_NaN(); + int track1Strobe = -1; // sentinel for "no MVTX cluster found"And similarly for line 134:
- int track2Strobe = std::numeric_limits<int>::quiet_NaN(); + int track2Strobe = -1; // sentinel for "no MVTX cluster found"Then update the comparison logic to check for valid strobes:
- if (track1Strobe == track2Strobe && m_mergeSeeds) + if (track1Strobe >= 0 && track1Strobe == track2Strobe && m_mergeSeeds)
134-134:⚠️ Potential issue | 🔴 CriticalSame
quiet_NaN()issue fortrack2Strobe.See comment above regarding line 98. This also needs to be fixed.
183-225:⚠️ Potential issue | 🔴 CriticalLogic error:
keysToKeepis computed but never used, and duplicate insertions occur.There are two issues in this block:
keysToKeep(lines 184, 187-191, 201, 203-205) accumulates the merged keys but is never used afterward. The intended merge is not being applied.Lines 223-225 unconditionally insert
track1IDintomatchesandtrack2IDintoseedsToDelete, duplicating the logic already handled in the if/else branches above (lines 192-193, 206-207). This causes:
- When
mvtx1Keys.size() >= mvtx2Keys.size(): correct behavior (track1ID kept, track2ID deleted)- When
mvtx2Keys.size() > mvtx1Keys.size(): wrong behavior - track2ID should be kept but lines 223-225 override to keep track1ID and delete track2ID🐛 Proposed fix to remove duplicate insertions and use keysToKeep
if (mvtx1Keys.size() >= mvtx2Keys.size()) { keysToKeep = mvtx1Keys; if (track1Strobe == track2Strobe && m_mergeSeeds) { keysToKeep.insert(mvtx2Keys.begin(), mvtx2Keys.end()); } matches.insert(std::make_pair(track1ID, keysToKeep)); seedsToDelete.insert(track2ID); if (Verbosity() > 2) { std::cout << " will delete seed " << track2ID << std::endl; } } else { keysToKeep = mvtx2Keys; if (track1Strobe == track2Strobe && m_mergeSeeds) { keysToKeep.insert(mvtx1Keys.begin(), mvtx1Keys.end()); } matches.insert(std::make_pair(track2ID, keysToKeep)); seedsToDelete.insert(track1ID); if (Verbosity() > 2) { std::cout << " will delete seed " << track1ID << std::endl; } } if (Verbosity() > 2) { std::cout << "Match IDed" << std::endl; - for (const auto& key : mvtx1Keys) + for (const auto& key : keysToKeep) { std::cout << " total track keys " << key << std::endl; } } - matches.insert(std::make_pair(track1ID, mvtx1Keys)); - seedsToDelete.insert(track2ID); break; }offline/packages/trackreco/PHSimpleVertexFinder.cc (2)
282-286:⚠️ Potential issue | 🟠 MajorPotential null pointer dereference on
siseed.At line 286,
siseed->get_crossing()is called without a null check. Whiletrackis verified non-null at line 283,get_silicon_seed()at line 285 can returnnullptr, leading to a crash.🐛 Proposed fix
SvtxTrack *track = _track_map->get(trid); if (track) { auto *siseed = track->get_silicon_seed(); + if (!siseed) + { + continue; + } short int intt_crossing = siseed->get_crossing();
399-400:⚠️ Potential issue | 🟠 MajorPotential null pointer dereference on
siseedin verbose output loop.Same issue as above:
siseed->get_crossing()at line 400 is called without checking ifsiseedis null.🐛 Proposed fix
auto *siseed = track->get_silicon_seed(); + if (!siseed) + { + continue; + } short int intt_crossing = siseed->get_crossing();
🤖 Fix all issues with AI agents
In `@generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h`:
- Line 88: The default value for the member _thePtLow is inconsistent: the
header HepMCParticleTrigger.h sets float _thePtLow{-999.9} while the constructor
initializer in the .cc sets it to 0; align them by choosing one canonical
default and making both places match (preferably set the header and the
constructor initializer list for HepMCParticleTrigger to the same value, e.g.,
-999.9 or 0) so the declaration of _thePtLow and the constructor initializer
list in the HepMCParticleTrigger class are identical.
In `@offline/packages/globalvertex/GlobalVertexv3.h`:
- Around line 22-26: CloneMe currently returns a shallow copy via the default
copy constructor which duplicates raw pointers in _vtxs and leads to
double-free/UAF when Reset()/destructor run; change CloneMe in class
GlobalVertexv3 to perform a deep clone by creating a new GlobalVertexv3 instance
and for each pointer in the original's _vtxs allocate/clone a fresh vertex
object (using the vertex type's copy constructor or its Clone/CloneMe method if
available) and push those new pointers into the new object's _vtxs, ensuring
ownership semantics match Reset()/destructor and avoiding shared raw pointers
between original and clone.
In `@offline/packages/globalvertex/SvtxVertex_v2.h`:
- Around line 61-71: set_beam_crossing currently casts rollover_short(cross) to
unsigned which maps negative values (e.g. -32768) to large unsigned sentinel
values; change set_beam_crossing to validate the result of rollover_short(cross)
before casting: call rollover_short(cross) into a short int (cross_ro), and if
cross_ro is negative set _beamcrossing to a defined safe value (e.g. 0u or
another agreed clamp) instead of static_cast<unsigned int>(cross_ro); keep the
existing special-case when cross == short_int_max that sets _beamcrossing to
std::numeric_limits<unsigned int>::max(), and update any callers/contract if a
different sentinel is chosen.
- Around line 91-98: rollover_short currently maps cross == -32768 into the same
sentinel slot as short_int_max causing collisions; update rollover_short to
explicitly handle the minimum short value (use numeric_limits<short>::min() or
the defined short_int_min) and either (a) reject it (throw std::out_of_range or
assert) so set_beam_crossing can reject/document out-of-range inputs, or (b) map
it to a distinct reserved value (not short_int_max) so serialization is
bijective; ensure callers set_beam_crossing/get_beam_crossing handle the new
behavior and update any docs/comments accordingly.
In `@offline/packages/globalvertex/SvtxVertex_v3.cc`:
- Around line 106-112: The covar_index method lacks bounds checking and can
produce out-of-range indexes into the _err[6] covariance storage; update
SvtxVertex_v3::covar_index to validate that both i and j are within [0,2] before
computing the packed index (keep the existing std::swap(i,j) behavior), and if
an index is invalid, fail fast by throwing a std::out_of_range (or using an
assert) with a clear message mentioning the invalid i/j values and the object,
so callers cannot index _err out-of-bounds.
In `@offline/packages/jetbackground/DetermineTowerBackground.cc`:
- Around line 542-602: The code in DetermineTowerBackground::process_event
currently uses a literal return -1 when G4TruthInfo is missing and exit(-1) when
EventplaneinfoMap is missing; replace both with the framework-safe return value
Fun4AllReturnCodes::ABORTRUN so the framework can handle the abort: change the
early return after the truthinfo null check (where it currently does return -1)
to return Fun4AllReturnCodes::ABORTRUN, and replace the exit(-1) in the
EventplaneinfoMap null check with return Fun4AllReturnCodes::ABORTRUN (ensure
the correct header/namespace for Fun4AllReturnCodes is included where
DetermineTowerBackground::process_event is defined).
- Around line 970-976: The range check for centrality_bin in
DetermineTowerBackground.cc wrongly excludes valid indices (0 and 95–99); update
the conditional in the block that assigns _v2 (inside the code path using
centinfo->get_centrality_bin) to accept the full populated range—use
centrality_bin >= 0 && centrality_bin < 100 or better yet centrality_bin >= 0 &&
centrality_bin < static_cast<int>(_CENTRALITY_V2.size())—so _v2 is set from
_CENTRALITY_V2[centrality_bin] for valid bins and avoid incorrectly marking flow
failure for those bins.
In `@offline/packages/jetbase/TowerJetInput.cc`:
- Line 13: Remove the unused include directive for GlobalVertexv3.h: locate the
top-of-file include block where `#include` <globalvertex/GlobalVertexv3.h> is
present and delete that line; the file only uses GlobalVertexMap and calls
GlobalVertex::get_z(), so no references to GlobalVertexv3 exist and removing the
include will clean up the headers.
In `@offline/packages/mbd/MbdCalib.cc`:
- Around line 89-99: Download_Status currently treats a missing status file/URL
as an error and leaves the member _mbdstatus at -1, causing Download_All and
repeated runs to fail or retain stale values; change the logic so that a missing
status is non-fatal and defaults to "good" by setting _mbdstatus = 0 (either in
the MbdCalib constructor/initializer or at start of Download_Status), treat a
not-found / empty response as success (no error) while still parsing the content
and returning errors for malformed content, and update any checks in
Download_All that assume a non-negative _mbdstatus so they do not fail when the
status was simply absent; key symbols to modify: Download_Status, Download_All,
and the _mbdstatus initialization in the MbdCalib class.
- Around line 1421-1427: Download_TimeRMS currently returns distinct codes (1
success, 0 missing, negative errors) but callers ignore its return so they
cannot distinguish missing TimeRMS from success; update every caller that
invokes Download_TimeRMS to capture its return value, check for ==1 (proceed),
==0 (log a clear warning that _trms_y was empty and handle missing calibration
path), and <0 (treat as error: log with the returned code and propagate or fail
as appropriate). Use get_trms() only for safe access to _trms_y_interp but do
not rely on it to infer load status; if desired, convert non-1 results into
appropriate control flow (early return, exception, or status propagation) so
callers can debug and respond to missing or error states.
In `@offline/packages/TrackingDiagnostics/TrkrNtuplizer.cc`:
- Around line 1542-1549: The range check for INTT layers incorrectly uses
"layer_local < _nlayers_intt", causing most INTT layers to be skipped; update
the condition in the block that handles INTT hits (where variables like
layer_local, _nlayers_maps, _nlayers_intt, InttDefs, and CylinderGeomIntt
appear) so the upper bound is _nlayers_maps + _nlayers_intt (i.e., use
layer_local >= _nlayers_maps && layer_local < _nlayers_maps + _nlayers_intt) to
correctly detect all INTT layers before calling find_strip_center_localcoords.
In `@offline/packages/trackreco/PHSiliconSeedMerger.cc`:
- Line 92: The build fails because code (e.g., PHSiliconSeedMerger.cc using
seedsToDelete.contains(track1ID)) relies on C++20; update the build
configuration to enable C++20 by modifying configure.ac to add the C++20
standard flag to the compiler flags (either append -std=c++20 to
CXXFLAGS/CXXFLAGS_DEFAULT) or, preferably, use Autoconf’s AX_CHECK_COMPILE_FLAG
(or similar) to detect and add a supported C++20 flag; ensure this change covers
all trackreco sources that call std::set::contains and that the configure check
emits a clear error if no C++20-supporting flag is available.
In `@offline/QA/Tracking/StateClusterResidualsQA.cc`:
- Around line 114-142: The loop that pushes dynamic_cast results from
hm->getHisto into vectors (e.g., m_histograms_x, m_histograms_y,
m_histograms_layer_x, etc.) can store null pointers if getHisto() returns
nullptr or the cast fails, leading to crashes later in process_event when Fill()
is called; update the loop that iterates over m_pending (inside the
m_use_local_coords branches) to check each returned pointer before push_back (or
assert/throw with a clear message), and only push valid TH1/TH2 pointers (or
substitute a safe no-op placeholder) so that subsequent process_event accesses
of those vectors are guaranteed non-null. Ensure checks reference the same calls
shown (hm->getHisto(std::string(cfg.name + "...")) and the dynamic_cast results)
and adjust downstream logic in process_event to assume non-null or handle the
placeholder consistently.
- Around line 184-262: The loop over m_pending uses a separate counter h that is
skipped when early continue is taken (in the charge filter), causing histogram
indexing to desynchronize; fix by ensuring h is incremented before every
continue in the for (const auto& cfg : m_pending) loop (i.e., place ++h just
before each continue in the charge checks) or refactor the loop to an
index-based iteration (for (size_t h=0; h<m_pending.size(); ++h) using
m_pending[h]) so the histogram index always matches the config; update
references to cfg accordingly (m_pending, h, charge filter block, and the
histogram-fill section).
In `@offline/QA/Tracking/StateClusterResidualsQA.h`:
- Line 181: The header guard closing comment is incorrect: the guard used is
STATECLUSTERRESIDUALSQA_H but the closing comment reads TRACKFITTINGQA_H; update
the trailing comment to match the guard identifier (STATECLUSTERRESIDUALSQA_H)
so the opening and closing guard identifiers are consistent with the
`#ifndef/`#define for STATECLUSTERRESIDUALSQA_H.
🧹 Nitpick comments (26)
simulation/g4simulation/g4main/PHG4TruthTrackingAction.h (1)
22-83: Attach the Doxygen blocks directly to the declarations they document.
These comment blocks sit outside the class and may be treated as orphaned by doc tooling; move them adjacent to each member (or consolidate into class-level docs) so they’re reliably picked up.Example of moving one block (apply the same pattern to others)
-class PHG4TruthTrackingAction : public PHG4TrackingAction -{ - public: - //! constructor - PHG4TruthTrackingAction(PHG4TruthEventAction*); +class PHG4TruthTrackingAction : public PHG4TrackingAction +{ + public: + /** + * Construct a PHG4TruthTrackingAction associated with an event action. + * `@param` eventAction Pointer to the owning PHG4TruthEventAction used to record per-event truth information. + */ + PHG4TruthTrackingAction(PHG4TruthEventAction*);offline/packages/trackreco/PHSiliconSeedMerger.h (3)
28-50: Inconsistent indentation for method definitions.The method bodies (
void trackMapName(...),void clusterOverlap(...),void searchIntt(),void mergeSeeds()) start at column 0 but should be indented to match the class member indentation level. This appears to be a formatting issue where the docblocks are indented but the method signatures are not.🔧 Suggested indentation fix
/** * Set the name of the track seed container to use when retrieving silicon tracks. * * `@param` name Name of the TrackSeedContainer node (defaults to "SiliconTrackSeedContainer"). */ -void trackMapName(const std::string &name) { m_trackMapName = name; } + void trackMapName(const std::string &name) { m_trackMapName = name; } /** * Set the maximum number of overlapping clusters considered during seed merging. * `@param` nclusters Maximum number of clusters that may overlap (overlap threshold). */ -void clusterOverlap(const unsigned int nclusters) { m_clusterOverlap = nclusters; } + void clusterOverlap(const unsigned int nclusters) { m_clusterOverlap = nclusters; } /** * `@brief` Allow merging searches to include the INTT detector. * * Configure the merger to include INTT clusters in subsequent processing by disabling the MVTX-only restriction. */ -void searchIntt() { m_mvtxOnly = false; } + void searchIntt() { m_mvtxOnly = false; } /** * Enable merging of silicon seed tracks during event processing. * * When enabled, the module will merge overlapping silicon seed tracks where applicable. */ -void mergeSeeds() { m_mergeSeeds = true; } + void mergeSeeds() { m_mergeSeeds = true; }
57-73: Same indentation issue for member variable declarations.The member variables and their documentation blocks have inconsistent indentation -
unsigned int m_clusterOverlap{1};andbool m_mvtxOnly{false};are at column 0 instead of being indented within the class.🔧 Suggested indentation fix
/** * Minimum number of clusters that must be shared between two silicon track seeds * for them to be considered overlapping. * * Defaults to 1. */ -unsigned int m_clusterOverlap{1}; + unsigned int m_clusterOverlap{1}; bool m_mergeSeeds{false}; /** * Restrict seed processing to the MVTX detector only. * * When set to `true`, operations that iterate or merge silicon seed tracks * will be limited to clusters originating from the MVTX vertex detector. * When `false`, clusters from other silicon detectors are included. */ -bool m_mvtxOnly{false}; + bool m_mvtxOnly{false}; + bool m_mergeSeeds{false};
75-77: Extra blank lines at end of file.There are unnecessary trailing blank lines after the
#endifguard. Consider removing them for cleaner formatting.offline/packages/jetbackground/DetermineTowerBackground.cc (1)
79-106: Use RAII forCDBTTreeto avoid manual delete paths.
[RAII + exception safety]♻️ Suggested change
- CDBTTree *cdbtree_calo_v2 = nullptr; + std::unique_ptr<CDBTTree> cdbtree_calo_v2; ... - cdbtree_calo_v2 = new CDBTTree(calibdir); + cdbtree_calo_v2 = std::make_unique<CDBTTree>(calibdir); ... - cdbtree_calo_v2->LoadCalibrations(); + cdbtree_calo_v2->LoadCalibrations(); ... - _CENTRALITY_V2[icent] = cdbtree_calo_v2->GetFloatValue(icent, "jet_calo_v2"); + _CENTRALITY_V2[icent] = cdbtree_calo_v2->GetFloatValue(icent, "jet_calo_v2"); - - delete cdbtree_calo_v2;offline/packages/globalvertex/SvtxVertex_v2.h (1)
100-118: Add documentation explaining the rollover conversion logic.The conversion between unsigned int and short int involves complex rollover semantics that are not immediately obvious. Consider adding comments explaining:
- The expected value ranges for
_beamcrossing- Why values > 32767 might exist (legacy data, serialization compatibility)
- The intended behavior of the double rollover at lines 114-117
simulation/g4simulation/g4main/PHG4TruthTrackingAction.cc (2)
360-427: Ancestry traversal lacks cycle detection—relies on Geant4 invariant.The
while (parent)loop at line 402 relies on the assumption that Geant4 always processes parents before children, ensuring no cycles. The comment at line 401 acknowledges this risk. While this invariant should hold in practice, a defensive check could prevent an infinite loop if the truth container is ever corrupted.🛡️ Optional: Add visited-set guard for defensive programming
PHG4Particle* parent = truth.GetParticle(particle->get_parent_id()); - // if there is a loop of the parent, then we have a problem lol with this while loop btw + std::unordered_set<int> visited; while (parent) { + if (!visited.insert(parent->get_track_id()).second) + { + std::cerr << "PHG4TruthTrackingAction::issPHENIXPrimary - cycle detected in ancestry for track id " << particle->get_track_id() << std::endl; + return false; + } if (isLongLived(parent->get_pid())) {
379-386: Remove or guard commented-out debug code before merging.Leaving commented debug prints in production code adds clutter. Consider removing this block entirely or guarding it behind a verbosity flag if it's needed for future debugging.
generators/Herwig/HepMCTrigger/HepMCParticleTrigger.cc (5)
12-16: Remove unused FastJet includes.
fastjet/JetDefinition.hhandfastjet/PseudoJet.hhare included but not used in this particle trigger implementation. These are likely copied fromHepMCJetTriggerand should be removed.♻️ Proposed fix
`#include` <phool/PHCompositeNode.h> `#include` <phool/getClass.h> -#include <fastjet/JetDefinition.hh> - `#include` <HepMC/GenEvent.h> -#include <fastjet/PseudoJet.hh> `#include` <map>
85-108: Early abort on first bad event in multi-event map loses information.In
process_event, when iterating overPHHepMCGenEventMap, an earlyreturn Fun4AllReturnCodes::ABORTEVENTon the first failing event (lines 90, 95, 100) discards any information about which specific event failed. If the map typically contains only one event, this is fine, but if multiple events exist, consider logging which one failed before aborting.
114-121:AddParticlescan useinsertfor efficiency.The loop can be replaced with a single
insertcall for better clarity and potential performance.♻️ Proposed fix
void HepMCParticleTrigger::AddParticles(const std::vector<int>& particles) { - for (auto p : particles) - { - _theParticles.push_back(p); - } - return; + _theParticles.insert(_theParticles.end(), particles.begin(), particles.end()); }
344-351: Usemap::try_emplaceoroperator[]increment for cleaner map update.The contains-then-increment pattern can be simplified.
♻️ Proposed fix
- if (particle_types.contains(pid)) - { - particle_types[pid]++; - } - else - { - particle_types[pid] = 1; - } + particle_types[pid]++; // operator[] default-initializes to 0 if key absent
302-302: The API cannot distinguish particles from antiparticles—this is an intentional design choice that affects usability.Line 302 uses
std::abs((*iter)->pdg_id()), which combines particles with their antiparticles. The absolute value normalization is applied consistently throughout the matching pipeline: users callingAddParticle(11)orAddParticle(-11)both match viastd::abs(trigger_particle)inparticleAboveThreshold()(line 363), and all event particles are stored using their absolute pdg_id in theparticle_typesmap. This means electrons (11) and positrons (-11) cannot be distinguished.If distinguishing particles from antiparticles is important for your trigger use cases, the API would need to preserve the sign of pdg_id throughout the matching logic.
generators/Herwig/HepMCTrigger/HepMCJetTrigger.h (1)
50-51: Addconstqualifier to getter methods.These getters don't modify state and should be marked
constfor correctness and to allow calling on const references.♻️ Proposed fix
- int getNevts(){return this->n_evts;} - int getNgood(){return this->n_good;} + int getNevts() const { return n_evts; } + int getNgood() const { return n_good; }generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h (2)
8-8: Remove unused FastJet include.
<fastjet/PseudoJet.hh>is not used in this header. This appears to be copied fromHepMCJetTrigger.h.♻️ Proposed fix
`#include` <fun4all/SubsysReco.h> -#include <fastjet/PseudoJet.hh> - `#include` <cmath>
69-70: Addconstqualifier to getter methods.These getters don't modify state and should be marked
const. This matches the recommended change forHepMCJetTrigger.h.♻️ Proposed fix
- int getNevts(){return this->n_evts;} - int getNgood(){return this->n_good;} + int getNevts() const { return n_evts; } + int getNgood() const { return n_good; }offline/QA/Tracking/StateClusterResidualsQA.cc (3)
33-34: Unusedsquaretemplate function.This helper is defined but never used in the file. Consider removing it to avoid dead code.
🧹 Proposed fix
namespace { - template <typename T> - inline T square (T const& t) { return t * t; } - template <class T> class range_adaptor
215-235: Unused variablesstate_zandcluster_zin local coordinates branch.In the
m_use_local_coords == truebranch,state_zandcluster_zare declared but never assigned or used, leaving them uninitialized.🧹 Proposed fix: Move declarations inside branches
- float state_x; - float state_y; - float state_z; - float cluster_x; - float cluster_y; - float cluster_z; if (m_use_local_coords == true) { - state_x = state->get_localX(); - state_y = state->get_localY(); + float state_x = state->get_localX(); + float state_y = state->get_localY(); Acts::Vector2 loc = geometry->getLocalCoords(state->get_cluskey(), cluster); - cluster_x = loc.x(); - cluster_y = loc.y(); + float cluster_x = loc.x(); + float cluster_y = loc.y(); // ... rest unchanged } else { - state_x = state->get_x(); + float state_x = state->get_x(); // ... rest unchanged }
394-400: Unused histogram manager inEndRun.The histogram manager is retrieved and asserted but not used. If no cleanup is needed, consider simplifying:
int StateClusterResidualsQA::EndRun(const int /*unused*/) { - auto *hm = QAHistManagerDef::getHistoManager(); - assert(hm); - return Fun4AllReturnCodes::EVENT_OK; }offline/QA/Tracking/StateClusterResidualsQA.h (1)
9-9: Unused include<limits>.
FLT_MAXcomes from<cfloat>(line 11);std::numeric_limitsis not used.-#include <limits>offline/packages/trackreco/PHActsSiliconSeeding.h (1)
179-188: Clarify units for MVTX/beam-spot setters.
Defaults are documented in cm, but the setters accept raw floats. A short Doxygen note (or x_cm/y_cm naming) will reduce accidental mm inputs.💡 Suggested doc tweak
- void set_mvtxCenterXY(const float X, const float Y) + /// MVTX center in cm (sPHENIX coordinates) + void set_mvtxCenterXY(const float X, const float Y) { m_mvtx_x0 = X; m_mvtx_y0 = Y; } - void set_beamSpotXY(const float X, const float Y) + /// Beam spot in cm (sPHENIX coordinates) + void set_beamSpotXY(const float X, const float Y) { m_beamSpotx = X; m_beamSpoty = Y; }offline/packages/trackreco/PHActsSiliconSeeding.cc (1)
1174-1207: Simplify MVTX-center offset math initerateLayers(INTT-only).
All current call sites passstartLayer >= 3, so thelayer < 3branch is dead and the offset math collapses to a no-op. Consider removing it here (or add a note if MVTX layers will be added later).♻️ Optional simplification
- // move the fitted circle center the negative of the MVTX center position - float x0 = 0.0; // cm - float y0 = 0.0; for (int layer = startLayer; layer < endLayer; ++layer) { float layerradius = m_geomContainerIntt->GetLayerGeom(layer)->get_radius(); - if(layer < 3) - { - x0 = m_mvtx_x0; - y0 = m_mvtx_y0; - } - - float xfitradius_moved = fitpars[1] - x0; - float yfitradius_moved = fitpars[2] - y0; - - const auto [xplus, yplus, xminus, yminus] = TrackFitUtils::circle_circle_intersection(layerradius, fitpars[0], xfitradius_moved, yfitradius_moved); - - float xp = xplus + x0; - float xm = xminus + x0; - float yp = yplus + y0; - float ym = yminus + y0; + const auto [xplus, yplus, xminus, yminus] = + TrackFitUtils::circle_circle_intersection(layerradius, fitpars[0], fitpars[1], fitpars[2]); + + float xp = xplus; + float xm = xminus; + float yp = yplus; + float ym = yminus;offline/packages/TrackingDiagnostics/TrkrNtuplizer.cc (2)
1480-1484: Use_nlayers_mapsinstead of hard-coded3.
Keeps MVTX layer selection consistent with configurable layer counts.♻️ Proposed fix
- if (layer_local < 3) + if (layer_local < _nlayers_maps)
1506-1507: Remove commented-out assignments to reduce clutter.🧹 Cleanup
- //fx_hit[n_hit::nhitphielem] = TpcDefs::getSectorId(hitset_key); - //fx_hit[n_hit::nhitzelem] = TpcDefs::getSide(hitset_key);offline/packages/globalvertex/GlobalVertexv2.h (1)
82-99: Consider consolidating duplicated rollover utilities.The
rollover_shortandrollover_from_unsignedintfunctions are identically implemented inGlobalVertexv2.h,MbdVertexv2.h, andSvtxVertex_v2.h. Consider extracting these to a shared utility header to reduce code duplication.offline/packages/globalvertex/GlobalVertexv3.cc (1)
120-151: Avoid copying the SVTX vertex vector in hot-path getters.
These getters copysvtxit->secondinto a local vector; a const reference avoids repeated allocations and copies.♻️ Suggested change
- GlobalVertex::VertexVector trackvertices = svtxit->second; + const auto& trackvertices = svtxit->second;- GlobalVertex::VertexVector trackvertices = svtxit->second; + const auto& trackvertices = svtxit->second;- GlobalVertex::VertexVector trackvertices = svtxit->second; + const auto& trackvertices = svtxit->second;- GlobalVertex::VertexVector trackvertices = svtxit->second; + const auto& trackvertices = svtxit->second;Also applies to: 153-174, 176-197, 199-231
| float _theEtaHigh{1.1}; | ||
| float _theEtaLow{-1.1}; | ||
| float _thePtHigh{999.9}; | ||
| float _thePtLow{-999.9}; |
There was a problem hiding this comment.
Inconsistent default for _thePtLow between header and constructor.
The header initializes _thePtLow to -999.9, but the constructor initializer list in the .cc file sets it to 0. The constructor value wins at runtime, but this inconsistency is confusing and error-prone if the constructor initialization is ever removed.
🐛 Proposed fix (align header with .cc)
float _thePtHigh{999.9};
- float _thePtLow{-999.9};
+ float _thePtLow{0};📝 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.
| float _thePtLow{-999.9}; | |
| float _thePtHigh{999.9}; | |
| float _thePtLow{0}; |
🤖 Prompt for AI Agents
In `@generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h` at line 88, The
default value for the member _thePtLow is inconsistent: the header
HepMCParticleTrigger.h sets float _thePtLow{-999.9} while the constructor
initializer in the .cc sets it to 0; align them by choosing one canonical
default and making both places match (preferably set the header and the
constructor initializer list for HepMCParticleTrigger to the same value, e.g.,
-999.9 or 0) so the declaration of _thePtLow and the constructor initializer
list in the HepMCParticleTrigger class are identical.
| // PHObject virtual overloads | ||
| void identify(std::ostream& os = std::cout) const override; | ||
| void Reset() override; | ||
| int isValid() const override; | ||
| PHObject* CloneMe() const override { return new GlobalVertexv3(*this); } |
There was a problem hiding this comment.
CloneMe performs a shallow copy of owned vertices.
Line 26 uses the default copy constructor, which shallow-copies _vtxs raw pointers. Because Reset()/destructor delete these pointers, CloneMe() can lead to double-free or UAF when the clone and original are destroyed.
🛠️ Proposed deep-clone implementation
- PHObject* CloneMe() const override { return new GlobalVertexv3(*this); }
+ PHObject* CloneMe() const override;+PHObject* GlobalVertexv3::CloneMe() const
+{
+ auto* clone = new GlobalVertexv3();
+ clone->_id = _id;
+ clone->_bco = _bco;
+ for (const auto& [type, vertices] : _vtxs)
+ {
+ for (const auto* vertex : vertices)
+ {
+ clone->clone_insert_vtx(type, vertex);
+ }
+ }
+ return clone;
+}🤖 Prompt for AI Agents
In `@offline/packages/globalvertex/GlobalVertexv3.h` around lines 22 - 26, CloneMe
currently returns a shallow copy via the default copy constructor which
duplicates raw pointers in _vtxs and leads to double-free/UAF when
Reset()/destructor run; change CloneMe in class GlobalVertexv3 to perform a deep
clone by creating a new GlobalVertexv3 instance and for each pointer in the
original's _vtxs allocate/clone a fresh vertex object (using the vertex type's
copy constructor or its Clone/CloneMe method if available) and push those new
pointers into the new object's _vtxs, ensuring ownership semantics match
Reset()/destructor and avoiding shared raw pointers between original and clone.
| void set_beam_crossing(short int cross) override | ||
| { | ||
| if (cross == short_int_max) | ||
| { | ||
| _beamcrossing = std::numeric_limits<unsigned int>::max(); | ||
| return; | ||
| } | ||
|
|
||
| const short int cross_ro = rollover_short(cross); | ||
| _beamcrossing = static_cast<unsigned int>(cross_ro); | ||
| } |
There was a problem hiding this comment.
Unsafe cast: negative cross_ro converts to large unsigned value.
At line 70, static_cast<unsigned int>(cross_ro) with a negative cross_ro (possible when input is -32768) produces UINT_MAX or near-max values, colliding with the sentinel. Consider adding a guard:
Proposed fix with validation
void set_beam_crossing(short int cross) override
{
if (cross == short_int_max)
{
_beamcrossing = std::numeric_limits<unsigned int>::max();
return;
}
const short int cross_ro = rollover_short(cross);
+ if (cross_ro < 0)
+ {
+ // Handle edge case - either reject or map to a valid value
+ _beamcrossing = std::numeric_limits<unsigned int>::max();
+ return;
+ }
_beamcrossing = static_cast<unsigned int>(cross_ro);
}📝 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 set_beam_crossing(short int cross) override | |
| { | |
| if (cross == short_int_max) | |
| { | |
| _beamcrossing = std::numeric_limits<unsigned int>::max(); | |
| return; | |
| } | |
| const short int cross_ro = rollover_short(cross); | |
| _beamcrossing = static_cast<unsigned int>(cross_ro); | |
| } | |
| void set_beam_crossing(short int cross) override | |
| { | |
| if (cross == short_int_max) | |
| { | |
| _beamcrossing = std::numeric_limits<unsigned int>::max(); | |
| return; | |
| } | |
| const short int cross_ro = rollover_short(cross); | |
| if (cross_ro < 0) | |
| { | |
| // Handle edge case - either reject or map to a valid value | |
| _beamcrossing = std::numeric_limits<unsigned int>::max(); | |
| return; | |
| } | |
| _beamcrossing = static_cast<unsigned int>(cross_ro); | |
| } |
🤖 Prompt for AI Agents
In `@offline/packages/globalvertex/SvtxVertex_v2.h` around lines 61 - 71,
set_beam_crossing currently casts rollover_short(cross) to unsigned which maps
negative values (e.g. -32768) to large unsigned sentinel values; change
set_beam_crossing to validate the result of rollover_short(cross) before
casting: call rollover_short(cross) into a short int (cross_ro), and if cross_ro
is negative set _beamcrossing to a defined safe value (e.g. 0u or another agreed
clamp) instead of static_cast<unsigned int>(cross_ro); keep the existing
special-case when cross == short_int_max that sets _beamcrossing to
std::numeric_limits<unsigned int>::max(), and update any callers/contract if a
different sentinel is chosen.
| static short int rollover_short(short int cross) | ||
| { | ||
| if (cross == short_int_max) return short_int_max; | ||
| if (cross >= 0) return cross; | ||
|
|
||
| const int cross_ro = static_cast<int>(short_int_max) + static_cast<int>(cross); // cross negative | ||
| return static_cast<short int>(cross_ro); | ||
| } |
There was a problem hiding this comment.
Critical: rollover_short(-32768) returns -1, causing sentinel collision.
When cross = -32768:
cross_ro = 32767 + (-32768) = -1- In
set_beam_crossing,static_cast<unsigned int>(-1)yieldsUINT_MAX - This conflicts with the sentinel value for
short_int_max(32767)
Both set_beam_crossing(32767) and set_beam_crossing(-32768) result in _beamcrossing = UINT_MAX, making them indistinguishable.
Additionally, the conversion is non-bijective: negative values in [-32767, -1] map via rollover to [0, 32766], the same range as non-negative inputs [0, 32766]. Round-trip set_beam_crossing(x) → get_beam_crossing() will not preserve negative input values.
Proposed fix to handle -32768 edge case
static short int rollover_short(short int cross)
{
if (cross == short_int_max) return short_int_max;
if (cross >= 0) return cross;
const int cross_ro = static_cast<int>(short_int_max) + static_cast<int>(cross); // cross negative
+ // Ensure result is non-negative to avoid UINT_MAX collision when cast
+ if (cross_ro < 0)
+ {
+ return 0; // or handle as special case / error
+ }
return static_cast<short int>(cross_ro);
}Consider documenting the valid input range and expected behavior for negative values, or rejecting out-of-range inputs explicitly.
📝 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.
| static short int rollover_short(short int cross) | |
| { | |
| if (cross == short_int_max) return short_int_max; | |
| if (cross >= 0) return cross; | |
| const int cross_ro = static_cast<int>(short_int_max) + static_cast<int>(cross); // cross negative | |
| return static_cast<short int>(cross_ro); | |
| } | |
| static short int rollover_short(short int cross) | |
| { | |
| if (cross == short_int_max) return short_int_max; | |
| if (cross >= 0) return cross; | |
| const int cross_ro = static_cast<int>(short_int_max) + static_cast<int>(cross); // cross negative | |
| // Ensure result is non-negative to avoid UINT_MAX collision when cast | |
| if (cross_ro < 0) | |
| { | |
| return 0; // or handle as special case / error | |
| } | |
| return static_cast<short int>(cross_ro); | |
| } |
🤖 Prompt for AI Agents
In `@offline/packages/globalvertex/SvtxVertex_v2.h` around lines 91 - 98,
rollover_short currently maps cross == -32768 into the same sentinel slot as
short_int_max causing collisions; update rollover_short to explicitly handle the
minimum short value (use numeric_limits<short>::min() or the defined
short_int_min) and either (a) reject it (throw std::out_of_range or assert) so
set_beam_crossing can reject/document out-of-range inputs, or (b) map it to a
distinct reserved value (not short_int_max) so serialization is bijective;
ensure callers set_beam_crossing/get_beam_crossing handle the new behavior and
update any docs/comments accordingly.
| unsigned int SvtxVertex_v3::covar_index(unsigned int i, unsigned int j) const | ||
| { | ||
| if (i > j) | ||
| { | ||
| std::swap(i, j); | ||
| } | ||
| return i + 1 + (j + 1) * (j) / 2 - 1; |
There was a problem hiding this comment.
Missing bounds validation for covariance matrix indices.
The covar_index function doesn't validate that i and j are within bounds (0-2). If called with indices >= 3, it will compute an out-of-bounds index into _err[6], causing undefined behavior.
🛡️ Proposed defensive fix
unsigned int SvtxVertex_v3::covar_index(unsigned int i, unsigned int j) const
{
+ if (i > 2 || j > 2)
+ {
+ return 0; // or throw/assert
+ }
if (i > j)
{
std::swap(i, j);
}
return i + 1 + (j + 1) * (j) / 2 - 1;
}🤖 Prompt for AI Agents
In `@offline/packages/globalvertex/SvtxVertex_v3.cc` around lines 106 - 112, The
covar_index method lacks bounds checking and can produce out-of-range indexes
into the _err[6] covariance storage; update SvtxVertex_v3::covar_index to
validate that both i and j are within [0,2] before computing the packed index
(keep the existing std::swap(i,j) behavior), and if an index is invalid, fail
fast by throwing a std::out_of_range (or using an assert) with a clear message
mentioning the invalid i/j values and the object, so callers cannot index _err
out-of-bounds.
| if (layer_local >= _nlayers_maps && layer_local < _nlayers_intt) | ||
| { | ||
| int row = InttDefs::getRow(hit_key); | ||
| int col = InttDefs::getCol(hit_key); | ||
|
|
||
| CylinderGeomIntt* intt_cylinder = dynamic_cast<CylinderGeomIntt*>(_inttGeom->GetLayerGeom(layer_local)); | ||
| double localcoords[3]; | ||
| intt_cylinder->find_strip_center_localcoords(InttDefs::getLadderZId(hitset_key),row,col,localcoords); |
There was a problem hiding this comment.
Fix INTT layer range check (currently skips most INTT layers).
layer_local < _nlayers_intt only handles the first INTT layer when using global layer indices. Use _nlayers_maps + _nlayers_intt as the upper bound.
🐛 Proposed fix
- if (layer_local >= _nlayers_maps && layer_local < _nlayers_intt)
+ if (layer_local >= _nlayers_maps && layer_local < (_nlayers_maps + _nlayers_intt))📝 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.
| if (layer_local >= _nlayers_maps && layer_local < _nlayers_intt) | |
| { | |
| int row = InttDefs::getRow(hit_key); | |
| int col = InttDefs::getCol(hit_key); | |
| CylinderGeomIntt* intt_cylinder = dynamic_cast<CylinderGeomIntt*>(_inttGeom->GetLayerGeom(layer_local)); | |
| double localcoords[3]; | |
| intt_cylinder->find_strip_center_localcoords(InttDefs::getLadderZId(hitset_key),row,col,localcoords); | |
| if (layer_local >= _nlayers_maps && layer_local < (_nlayers_maps + _nlayers_intt)) | |
| { | |
| int row = InttDefs::getRow(hit_key); | |
| int col = InttDefs::getCol(hit_key); | |
| CylinderGeomIntt* intt_cylinder = dynamic_cast<CylinderGeomIntt*>(_inttGeom->GetLayerGeom(layer_local)); | |
| double localcoords[3]; | |
| intt_cylinder->find_strip_center_localcoords(InttDefs::getLadderZId(hitset_key),row,col,localcoords); |
🤖 Prompt for AI Agents
In `@offline/packages/TrackingDiagnostics/TrkrNtuplizer.cc` around lines 1542 -
1549, The range check for INTT layers incorrectly uses "layer_local <
_nlayers_intt", causing most INTT layers to be skipped; update the condition in
the block that handles INTT hits (where variables like layer_local,
_nlayers_maps, _nlayers_intt, InttDefs, and CylinderGeomIntt appear) so the
upper bound is _nlayers_maps + _nlayers_intt (i.e., use layer_local >=
_nlayers_maps && layer_local < _nlayers_maps + _nlayers_intt) to correctly
detect all INTT layers before calling find_strip_center_localcoords.
| TrackSeed* track1 = m_siliconTracks->get(track1ID); | ||
|
|
||
| if (seedsToDelete.find(track1ID) != seedsToDelete.end()) | ||
| if (seedsToDelete.contains(track1ID)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for C++20 or C++17 configuration in build files
echo "=== Checking for C++ standard in CMake files ==="
rg -n "CMAKE_CXX_STANDARD|CXX_STANDARD|c\+\+20|c\+\+2a|std=c\+\+20" --type cmake -A 2
echo -e "\n=== Searching CMakeLists.txt files ==="
fd "CMakeLists.txt" | head -20
echo -e "\n=== Checking root CMakeLists.txt ==="
if [ -f "CMakeLists.txt" ]; then
head -50 CMakeLists.txt
fi
echo -e "\n=== Checking offline package CMakeLists.txt ==="
if [ -f "offline/CMakeLists.txt" ]; then
grep -n "CMAKE_CXX_STANDARD\|CXX_STANDARD" offline/CMakeLists.txt || echo "Not found in offline/CMakeLists.txt"
fi
echo -e "\n=== Checking trackreco package CMakeLists.txt ==="
if [ -f "offline/packages/trackreco/CMakeLists.txt" ]; then
cat offline/packages/trackreco/CMakeLists.txt
fiRepository: sPHENIX-Test/coresoftware
Length of output: 461
🏁 Script executed:
#!/bin/bash
# Find all CMakeLists.txt files
echo "=== Finding CMakeLists.txt files ==="
find . -name "CMakeLists.txt" -type f 2>/dev/null | head -30
echo -e "\n=== Searching for C++ standard settings (broad search) ==="
rg "CMAKE_CXX_STANDARD|set.*CXX_STANDARD|c\+\+20|c\+\+17|c\+\+2a" -i 2>/dev/null | head -40
echo -e "\n=== Checking root directory structure ==="
ls -la | head -20
echo -e "\n=== Checking offline directory ==="
ls -la offline/ | head -20
echo -e "\n=== Checking trackreco directory ==="
ls -la offline/packages/trackreco/ 2>/dev/null | head -20Repository: sPHENIX-Test/coresoftware
Length of output: 3120
🏁 Script executed:
#!/bin/bash
# Find the specific file
echo "=== Locating PHSiliconSeedMerger.cc ==="
find . -name "PHSiliconSeedMerger.cc" -type f
echo -e "\n=== Checking if contains() is used elsewhere in codebase ==="
rg "\.contains\(" --type cc --type cpp -l | head -20
echo -e "\n=== Checking line 92 of PHSiliconSeedMerger.cc ==="
if [ -f "offline/packages/trackreco/PHSiliconSeedMerger.cc" ]; then
sed -n '85,99p' offline/packages/trackreco/PHSiliconSeedMerger.cc | cat -n
else
echo "File not found at expected location"
fi
echo -e "\n=== Looking for build configuration files ==="
find . -name "*.cmake" -o -name "Makefile*" -o -name "setup.py" -o -name "setup.cfg" 2>/dev/null | head -20
echo -e "\n=== Checking .clang-tidy for any C++ standard hints ==="
head -50 .clang-tidyRepository: sPHENIX-Test/coresoftware
Length of output: 3795
🏁 Script executed:
#!/bin/bash
# Look for configure.ac or Makefile.am in trackreco
echo "=== Looking for build files in trackreco ==="
ls -la offline/packages/trackreco/ | grep -E "Makefile|configure"
echo -e "\n=== Searching for configure.ac in repo ==="
find . -name "configure.ac" -o -name "configure.in" 2>/dev/null
echo -e "\n=== Checking trackreco Makefile.am if it exists ==="
if [ -f "offline/packages/trackreco/Makefile.am" ]; then
cat offline/packages/trackreco/Makefile.am
fi
echo -e "\n=== Searching for CXX flags in any Makefile.am ==="
rg "CXXFLAGS|CXX_STANDARD|c\+\+20" 2>/dev/null | head -20
echo -e "\n=== Looking for any contains() usage in .cc files ==="
rg "contains\(" offline/packages/trackreco/ 2>/dev/null
echo -e "\n=== Check if std::set is used elsewhere with contains ==="
rg "std::set" offline/packages/trackreco/ --context 2 2>/dev/null | head -30Repository: sPHENIX-Test/coresoftware
Length of output: 16578
🏁 Script executed:
#!/bin/bash
# Check the full trackreco configure.ac file
echo "=== Full trackreco/configure.ac ==="
cat offline/packages/trackreco/configure.ac
echo -e "\n=== Check for parent configure.ac or build system defaults ==="
cat configure.ac 2>/dev/null || echo "No root configure.ac"
echo -e "\n=== Check if any compiler version is enforced ==="
rg "AC_PROG_CXX|AX_CXX" offline/packages/trackreco/ 2>/dev/null
echo -e "\n=== Count of contains() usages by file ==="
rg "\.contains\(" offline/packages/trackreco/ -l 2>/dev/null | xargs -I {} sh -c 'echo "{}:"; rg "\.contains\(" "{}" | wc -l'Repository: sPHENIX-Test/coresoftware
Length of output: 1179
Add C++20 standard flag to build configuration.
The code uses std::set::contains(), a C++20 feature, but offline/packages/trackreco/configure.ac does not explicitly enable C++20 (no -std=c++20 or equivalent flag). This will fail on compilers defaulting to C++17 or earlier. The issue is widespread—six other trackreco source files also use .contains(). Update configure.ac to add the C++20 flag to CXXFLAGS or use AX_CHECK_COMPILE_FLAG to ensure C++20 support.
🤖 Prompt for AI Agents
In `@offline/packages/trackreco/PHSiliconSeedMerger.cc` at line 92, The build
fails because code (e.g., PHSiliconSeedMerger.cc using
seedsToDelete.contains(track1ID)) relies on C++20; update the build
configuration to enable C++20 by modifying configure.ac to add the C++20
standard flag to the compiler flags (either append -std=c++20 to
CXXFLAGS/CXXFLAGS_DEFAULT) or, preferably, use Autoconf’s AX_CHECK_COMPILE_FLAG
(or similar) to detect and add a supported C++20 flag; ensure this change covers
all trackreco sources that call std::set::contains and that the configure check
emits a clear error if no C++20-supporting flag is available.
| for (const auto& cfg : m_pending) | ||
| { | ||
| if (m_use_local_coords) | ||
| { | ||
| m_histograms_x.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_local_rphi")))); | ||
| m_histograms_y.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_local_z")))); | ||
| m_histograms_layer_x.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_local_layer_rphi")))); | ||
| m_histograms_layer_y.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_local_layer_z")))); | ||
| m_histograms_phi_x.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_local_phi_rphi")))); | ||
| m_histograms_phi_y.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_local_phi_z")))); | ||
| m_histograms_eta_x.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_local_eta_rphi")))); | ||
| m_histograms_eta_y.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_local_eta_z")))); | ||
| } | ||
| else | ||
| { | ||
| 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")))); | ||
| m_histograms_layer_x.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_layer_x")))); | ||
| m_histograms_layer_y.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_layer_y")))); | ||
| m_histograms_layer_z.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_layer_z")))); | ||
| m_histograms_phi_x.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_phi_x")))); | ||
| m_histograms_phi_y.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_phi_y")))); | ||
| m_histograms_phi_z.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_phi_z")))); | ||
| m_histograms_eta_x.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_eta_x")))); | ||
| m_histograms_eta_y.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_eta_y")))); | ||
| m_histograms_eta_z.push_back(dynamic_cast<TH2 *>(hm->getHisto(std::string(cfg.name + "_eta_z")))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing null-checks after dynamic_cast may cause crashes in process_event.
If hm->getHisto() returns nullptr or the cast fails, null pointers are stored in the histogram vectors. Later calls to Fill() (lines 228-257) will dereference these nulls.
🐛 Proposed fix: Add null-checks or assertions
for (const auto& cfg : m_pending)
{
if (m_use_local_coords)
{
- m_histograms_x.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_local_rphi"))));
- m_histograms_y.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_local_z"))));
+ auto* h_x = dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_local_rphi")));
+ auto* h_y = dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_local_z")));
+ if (!h_x || !h_y)
+ {
+ std::cout << PHWHERE << " Failed to retrieve histograms for config: " << cfg.name << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+ }
+ m_histograms_x.push_back(h_x);
+ m_histograms_y.push_back(h_y);
// ... similar checks for other histograms🤖 Prompt for AI Agents
In `@offline/QA/Tracking/StateClusterResidualsQA.cc` around lines 114 - 142, The
loop that pushes dynamic_cast results from hm->getHisto into vectors (e.g.,
m_histograms_x, m_histograms_y, m_histograms_layer_x, etc.) can store null
pointers if getHisto() returns nullptr or the cast fails, leading to crashes
later in process_event when Fill() is called; update the loop that iterates over
m_pending (inside the m_use_local_coords branches) to check each returned
pointer before push_back (or assert/throw with a clear message), and only push
valid TH1/TH2 pointers (or substitute a safe no-op placeholder) so that
subsequent process_event accesses of those vectors are guaranteed non-null.
Ensure checks reference the same calls shown (hm->getHisto(std::string(cfg.name
+ "...")) and the dynamic_cast results) and adjust downstream logic in
process_event to assume non-null or handle the placeholder consistently.
| 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()); | ||
| if (!cluster) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| float state_x; | ||
| float state_y; | ||
| float state_z; | ||
| float cluster_x; | ||
| float cluster_y; | ||
| float cluster_z; | ||
| if (m_use_local_coords == true) | ||
| { | ||
| state_x = state->get_localX(); | ||
| state_y = state->get_localY(); | ||
| Acts::Vector2 loc = geometry->getLocalCoords(state->get_cluskey(), cluster); | ||
| cluster_x = loc.x(); | ||
| cluster_y = loc.y(); | ||
| m_histograms_x[h]->Fill(state_x - cluster_x); | ||
| m_histograms_y[h]->Fill(state_y - cluster_y); | ||
| m_histograms_layer_x[h]->Fill(TrkrDefs::getLayer(state->get_cluskey()), state_x - cluster_x); | ||
| m_histograms_layer_y[h]->Fill(TrkrDefs::getLayer(state->get_cluskey()), state_y - cluster_y); | ||
| m_histograms_phi_x[h]->Fill(state->get_phi(), state_x - cluster_x); | ||
| m_histograms_phi_y[h]->Fill(state->get_phi(), state_y - cluster_y); | ||
| m_histograms_eta_x[h]->Fill(state->get_eta(), state_x - cluster_x); | ||
| m_histograms_eta_y[h]->Fill(state->get_eta(), state_y - cluster_y); | ||
| } | ||
| else | ||
| { | ||
| state_x = state->get_x(); | ||
| state_y = state->get_y(); | ||
| state_z = state->get_z(); | ||
| Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster); | ||
| cluster_x = glob.x(); | ||
| cluster_y = glob.y(); | ||
| 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); | ||
| m_histograms_layer_x[h]->Fill(TrkrDefs::getLayer(state->get_cluskey()), state_x - cluster_x); | ||
| m_histograms_layer_y[h]->Fill(TrkrDefs::getLayer(state->get_cluskey()), state_y - cluster_y); | ||
| m_histograms_layer_z[h]->Fill(TrkrDefs::getLayer(state->get_cluskey()), state_z - cluster_z); | ||
| m_histograms_phi_x[h]->Fill(state->get_phi(), state_x - cluster_x); | ||
| m_histograms_phi_y[h]->Fill(state->get_phi(), state_y - cluster_y); | ||
| m_histograms_phi_z[h]->Fill(state->get_phi(), state_z - cluster_z); | ||
| m_histograms_eta_x[h]->Fill(state->get_eta(), state_x - cluster_x); | ||
| m_histograms_eta_y[h]->Fill(state->get_eta(), state_y - cluster_y); | ||
| m_histograms_eta_z[h]->Fill(state->get_eta(), state_z - cluster_z); | ||
| } | ||
| } | ||
| } | ||
| ++h; | ||
| } |
There was a problem hiding this comment.
Index desynchronization: continue skips ++h, causing wrong histograms to be filled.
When the charge filter at lines 189-196 triggers continue, the index h is not incremented, but the loop advances to the next config. This causes subsequent configs to fill the wrong histograms.
Example: If m_pending has 3 configs and the first config's charge check fails, h remains 0 while processing the second config, filling m_histograms_x[0] instead of m_histograms_x[1].
🐛 Proposed fix: Increment `h` before `continue`
if (cfg.charge != 0)
{
if ((cfg.charge < 0) && track->get_positive_charge())
{
+ ++h;
continue;
}
if ((cfg.charge > 0) && !(track->get_positive_charge()))
{
+ ++h;
continue;
}
}Alternatively, use index-based iteration:
- int h = 0;
- for (const auto& cfg : m_pending)
+ for (size_t h = 0; h < m_pending.size(); ++h)
{
+ const auto& cfg = m_pending[h];
if (cfg.charge != 0)
{
if ((cfg.charge < 0) && track->get_positive_charge())
{
continue;
}
...
}
...
- ++h;
}🤖 Prompt for AI Agents
In `@offline/QA/Tracking/StateClusterResidualsQA.cc` around lines 184 - 262, The
loop over m_pending uses a separate counter h that is skipped when early
continue is taken (in the charge filter), causing histogram indexing to
desynchronize; fix by ensuring h is incremented before every continue in the for
(const auto& cfg : m_pending) loop (i.e., place ++h just before each continue in
the charge checks) or refactor the loop to an index-based iteration (for (size_t
h=0; h<m_pending.size(); ++h) using m_pending[h]) so the histogram index always
matches the config; update references to cfg accordingly (m_pending, h, charge
filter block, and the histogram-fill section).
| std::vector<TH2*> m_histograms_eta_z{}; | ||
| }; | ||
|
|
||
| #endif // TRACKFITTINGQA_H |
There was a problem hiding this comment.
Header guard comment mismatch.
The closing comment says TRACKFITTINGQA_H but the guard is STATECLUSTERRESIDUALSQA_H (line 3).
-#endif // TRACKFITTINGQA_H
+#endif // STATECLUSTERRESIDUALSQA_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.
| #endif // TRACKFITTINGQA_H | |
| `#endif` // STATECLUSTERRESIDUALSQA_H |
🤖 Prompt for AI Agents
In `@offline/QA/Tracking/StateClusterResidualsQA.h` at line 181, The header guard
closing comment is incorrect: the guard used is STATECLUSTERRESIDUALSQA_H but
the closing comment reads TRACKFITTINGQA_H; update the trailing comment to match
the guard identifier (STATECLUSTERRESIDUALSQA_H) so the opening and closing
guard identifiers are consistent with the `#ifndef/`#define for
STATECLUSTERRESIDUALSQA_H.
15 participants
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 by CodeRabbit
New Features
Improvements
Bug Fixes