Test #11
Test #11
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
…transforms New tpc transforms
…e-cout Preserve cout
abort events with any mbd packet empty
New QA module to make cluster-state residual plots
…ticle Stop KFP from aborting events
…runer feat: flag to prune all seeds
Additional calo embedding features
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive update across 60+ files spanning event triggers, QA systems, detector infrastructure, and tracking. Key additions include HepMCParticleTrigger for particle-level event filtering, StateClusterResidualsQA for tracking residuals, and data-merging utilities (CombineTowerInfo, CopyIODataNodes). Numerous const-correctness enhancements, pointer semantics refinements, and algorithmic improvements are applied throughout TPC, MVTX, and micromegas subsystems, along with framework changes for I/O state management and calibration handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HepMCParticleTrigger
participant HepMCEvent
participant ParticleList
User->>HepMCParticleTrigger: process_event(PHCompositeNode)
HepMCParticleTrigger->>HepMCParticleTrigger: Validate PHHepMCGenEventMap
HepMCParticleTrigger->>HepMCEvent: Iterate events
loop For each event
HepMCParticleTrigger->>HepMCParticleTrigger: isGoodEvent(HepMC::GenEvent)
HepMCParticleTrigger->>ParticleList: getParticles(HepMC::GenEvent)
ParticleList->>ParticleList: Apply kinematic cuts (Pt, P, Eta, Pz)
ParticleList-->>HepMCParticleTrigger: Return particle counts
HepMCParticleTrigger->>HepMCParticleTrigger: Check trigger thresholds
alt Event passes trigger
HepMCParticleTrigger->>HepMCParticleTrigger: Increment good event counter
end
end
HepMCParticleTrigger-->>User: Return EVENT_OK
sequenceDiagram
participant InputFileHandler
participant Script
participant FileSystem
participant FileStream
InputFileHandler->>InputFileHandler: OpenNextFile()
alt RunBeforeOpeningScript configured
InputFileHandler->>Script: Validate script exists & is executable
Script-->>InputFileHandler: Script state (valid/invalid)
alt Script valid
InputFileHandler->>Script: Execute with current file info
Script->>FileSystem: Perform pre-open operations
FileSystem-->>Script: Operation results
Script-->>InputFileHandler: Return exit code
end
end
InputFileHandler->>FileStream: Open next file
FileStream-->>InputFileHandler: File handle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Pull request overview
This pull request contains extensive code quality improvements and modernizations across the sPHENIX codebase. The changes include formatting improvements, const correctness additions, modernization of C++ patterns, and several bug fixes and new features.
Changes:
- Code style and formatting improvements (spacing, indentation, const correctness)
- Modernization to C++20/modern C++ patterns (std::format, std::max/std::min, range-based loops)
- Bug fixes (return codes, error handling, BCO handling improvements)
- New features (CombineTowerInfo, StateClusterResidualsQA, calibration loading for jets)
Reviewed changes
Copilot reviewed 87 out of 87 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| trackreco/*.h/cc | Added const correctness to setters, formatting improvements, code cleanup |
| trackbase/AlignmentTransformation.h/cc | Added use_module_tilt flag logic, reorganized transform calculation |
| tpc/*.cc | Replaced if-else min/max with std::max/std::min, improved const correctness |
| mvtx/*.h/cc | Added const to methods, improved formatting, replaced C-style casts |
| micromegas/MicromegasDefs.* | Added sample field to hit key, improved bit masking |
| micromegas/MicromegasCombinedDataDecoder.* | Store sample with hit, improved max ADC finding |
| mbd/*.cc | Added error handling for empty packets, improved diagnostics |
| jetbackground/* | Added centrality-based v2 loading from calibration database |
| Skimmers/Trigger/* | Added max accept events feature, fixed GL1 packet lookup |
| fun4allraw/* | Major BCO handling improvements, GL1 skip logic fixes |
| fun4all/* | Added script execution before file opening, cout state preservation |
| ffamodules/* | Removed unnecessary includes, modernized destructors |
| QA/Tracking/* | Added new StateClusterResidualsQA module, sample range filtering |
| CaloEmbedding/* | Added new CombineTowerInfo module for combining tower energies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @blackcathj. * #11 (comment) The following files were modified: * `generators/Herwig/HepMCTrigger/HepMCJetTrigger.cc` * `generators/Herwig/HepMCTrigger/HepMCParticleTrigger.cc` * `generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h` * `generators/PHPythia8/PHPythia8.cc` * `generators/phhepmc/PHHepMCGenEventv1.cc` * `offline/QA/Calorimeters/CaloValid.cc` * `offline/QA/Tracking/MicromegasClusterQA.cc` * `offline/QA/Tracking/MicromegasClusterQA.h` * `offline/QA/Tracking/StateClusterResidualsQA.cc` * `offline/QA/Tracking/StateClusterResidualsQA.h` * `offline/framework/ffamodules/CDBInterface.cc` * `offline/framework/ffamodules/FlagHandler.h` * `offline/framework/ffamodules/HeadReco.cc` * `offline/framework/ffamodules/HeadReco.h` * `offline/framework/ffamodules/SyncReco.h` * `offline/framework/ffamodules/Timing.cc` * `offline/framework/ffamodules/Timing.h` * `offline/framework/fun4all/Fun4AllServer.cc` * `offline/framework/fun4all/Fun4AllServer.h` * `offline/framework/fun4all/InputFileHandler.cc` * `offline/framework/fun4all/InputFileHandler.h` * `offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc` * `offline/framework/fun4allraw/SingleMicromegasPoolInput_v2.cc` * `offline/framework/fun4allraw/SingleMicromegasPoolInput_v2.h` * `offline/framework/fun4allraw/SingleMvtxPoolInput.cc` * `offline/framework/fun4allraw/SingleTriggeredInput.cc` * `offline/framework/fun4allraw/SingleTriggeredInput.h` * `offline/packages/CaloEmbedding/CombineTowerInfo.cc` * `offline/packages/CaloEmbedding/CombineTowerInfo.h` * `offline/packages/CaloEmbedding/CopyIODataNodes.cc` * `offline/packages/CaloEmbedding/CopyIODataNodes.h` * `offline/packages/CaloReco/PhotonClusterBuilder.cc` * `offline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.cc` * `offline/packages/PHGenFitPkg/PHGenFit/Fitter.cc` * `offline/packages/PHGenFitPkg/PHGenFit/Track.cc` * `offline/packages/Skimmers/Trigger/TriggerDSTSkimmer.cc` * `offline/packages/Skimmers/Trigger/TriggerDSTSkimmer.h` * `offline/packages/jetbackground/DetermineTowerBackground.cc` * `offline/packages/jetbackground/DetermineTowerBackground.h` * `offline/packages/mbd/MbdCalib.cc` * `offline/packages/mbd/MbdEvent.cc` * `offline/packages/mbd/MbdReco.cc` * `offline/packages/mbd/MbdSig.h` * `offline/packages/micromegas/MicromegasClusterizer.cc` * `offline/packages/micromegas/MicromegasCombinedDataDecoder.cc` * `offline/packages/micromegas/MicromegasCombinedDataDecoder.h` * `offline/packages/micromegas/MicromegasDefs.cc` * `offline/packages/micromegas/MicromegasDefs.h` * `offline/packages/mvtx/CylinderGeom_Mvtx.cc` * `offline/packages/mvtx/CylinderGeom_Mvtx.h` * `offline/packages/mvtx/MvtxClusterPruner.cc` * `offline/packages/mvtx/MvtxClusterizer.cc` * `offline/packages/mvtx/MvtxHitPruner.cc` * `offline/packages/mvtx/SegmentationAlpide.cc` * `offline/packages/tpc/LaserClusterizer.cc` * `offline/packages/tpc/LaserEventIdentifier.cc` * `offline/packages/tpc/Tpc3DClusterizer.cc` * `offline/packages/tpc/TpcClusterMover.cc` * `offline/packages/tpc/TpcClusterMover.h` * `offline/packages/tpc/TpcClusterizer.cc` * `offline/packages/tpc/TpcCombinedRawDataUnpacker.cc` * `offline/packages/tpc/TpcCombinedRawDataUnpackerDebug.cc` * `offline/packages/tpc/TpcDistortionCorrection.cc` * `offline/packages/tpc/TpcLoadDistortionCorrection.cc` * `offline/packages/tpc/TpcRawDataTree.cc` * `offline/packages/tpc/TpcRawWriter.cc` * `offline/packages/tpc/TpcSimpleClusterizer.cc` * `offline/packages/tpc/TrainingHits.cc` * `offline/packages/trackbase/AlignmentTransformation.cc` * `offline/packages/trackbase/AlignmentTransformation.h` * `offline/packages/trackreco/DSTClusterPruning.cc` * `offline/packages/trackreco/DSTClusterPruning.h` * `offline/packages/trackreco/PHActsTrkFitter.cc` * `offline/packages/trackreco/PHActsTrkFitter.h` * `offline/packages/trackreco/PHSiliconTpcTrackMatching.cc` * `offline/packages/trackreco/PHSiliconTpcTrackMatching.h` * `offline/packages/trackreco/PHSimpleVertexFinder.h` * `offline/packages/trackreco/PHTpcDeltaZCorrection.h`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
offline/framework/fun4allraw/SingleMicromegasPoolInput_v2.cc (1)
914-963: Reset evaluation fields per waveform to avoid stale QA data.
m_waveform.packet_idisn’t set here andfee_bco_predicted_matchedcan retain a prior value when no prediction exists. This can corrupt QA output, especially in the non‑match path.🔧 Proposed fix (reset defaults and set packet_id)
- if( m_do_evaluation ) - { - m_waveform.is_heartbeat = is_heartbeat; - m_waveform.fee_id = fee_id; - m_waveform.channel = payload.channel; - m_waveform.fee_bco = fee_bco; - } + if( m_do_evaluation ) + { + m_waveform.packet_id = packet_id; + m_waveform.is_heartbeat = is_heartbeat; + m_waveform.fee_id = fee_id; + m_waveform.channel = payload.channel; + m_waveform.fee_bco = fee_bco; + // reset per-waveform defaults to avoid stale values + m_waveform.matched = false; + m_waveform.gtm_bco_matched = 0; + m_waveform.fee_bco_predicted_matched = 0; + } ... - if( m_do_evaluation ) - { - m_waveform.matched = true; - m_waveform.gtm_bco_matched = gtm_bco; - { - const auto predicted = bco_matching_information.get_predicted_fee_bco(gtm_bco);; - if( predicted ) - { - m_waveform.fee_bco_predicted_matched = predicted.value(); - } - } - m_evaluation_tree->Fill(); - } + if( m_do_evaluation ) + { + m_waveform.matched = true; + m_waveform.gtm_bco_matched = gtm_bco; + const auto predicted = bco_matching_information.get_predicted_fee_bco(gtm_bco); + m_waveform.fee_bco_predicted_matched = predicted ? predicted.value() : 0; + m_evaluation_tree->Fill(); + }offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc (1)
779-797: Critical: Missing cleanup logic after processing hits.The refactored loop iterates over
m_InttRawHitMapand adds hits to the container, but unlike the previous pattern (visible inFillTpcat lines 1174-1180 and the cleanup loop at lines 696-704), processed entries are never cleaned up. This will cause:
- Memory leak: Hits remain in the map and are never deleted
- Double processing: Same hits will be added to the container on subsequent
FillIntt()calls- State accumulation:
CleanupUsedPackets,clearPacketBClkStackMap, andclearFeeGTML1BCOMapare not calledCompare with the pre-BCO cleanup loop (lines 696-710) which correctly calls all cleanup functions and erases entries.
Proposed fix to add cleanup after hit processing
for (auto& [bco, hitinfo] : m_InttRawHitMap) { if (bco > select_crossings) { break; } for (auto *intthititer : hitinfo.InttRawHitVector) { if (Verbosity() > 1) { std::cout << "Adding intt hit with bco 0x" << std::hex << intthititer->get_bco() << std::dec << std::endl; } inttcont->AddHit(intthititer); } } + + // Cleanup processed entries + auto it = m_InttRawHitMap.begin(); + while (it != m_InttRawHitMap.end() && it->first <= select_crossings) + { + for (auto *iter : m_InttInputVector) + { + iter->CleanupUsedPackets(it->first); + iter->clearPacketBClkStackMap(it->first); + iter->clearFeeGTML1BCOMap(it->first); + } + it->second.InttRawHitVector.clear(); + it = m_InttRawHitMap.erase(it); + } return 0;offline/packages/mbd/MbdEvent.cc (1)
453-499: Validate_nsamplesupper bound to prevent OOB writes and stale processing.
m_adc/m_sampare sized byMbdDefs::MAX_SAMPLES(seeoffline/packages/mbd/MbdEvent.haround lines 140–141). If a corrupted packet reports_nsamples > MAX_SAMPLES, the loop will write out of bounds; additionally,_nsamples > 30currently skipsSetXYbut still leavesSetNSamples()> 0, so later stages may process stale/uninitialized state. Please reject invalid_nsamplesand explicitly skip channels when you skipSetXY.🛠️ Suggested guard and explicit skip
- _nsamples = dstp[ipkt]->iValue(0, "SAMPLES"); + _nsamples = dstp[ipkt]->iValue(0, "SAMPLES"); + if (_nsamples <= 0 || _nsamples > MbdDefs::MAX_SAMPLES) + { + std::cout << PHWHERE << " ERROR, evt " << m_evt + << " invalid nsamples (" << _nsamples + << ") in Packet " << pktid << std::endl; + return Fun4AllReturnCodes::ABORTEVENT; + } @@ - _mbdsig[feech].SetNSamples( _nsamples ); - - if ( _nsamples > 0 && _nsamples <= 30 ) - { - _mbdsig[feech].SetXY(m_samp[feech], m_adc[feech]); - } + _mbdsig[feech].SetNSamples(_nsamples); + if (_nsamples <= 30) + { + _mbdsig[feech].SetXY(m_samp[feech], m_adc[feech]); + } + else + { + _mbdsig[feech].SetNSamples(0); // ensure later stages skip this channel + }offline/packages/tpc/Tpc3DClusterizer.cc (1)
554-633: Cluster size usesphiradians instead of pad index.
iphimin/iphimaxare integer pad bins, but you now update them withphi(radians). This yields incorrectphisizeand silently truncates doubles to ints. Use the pad index (coords[1]) or rename variables tophiMin/phiMaxand keep them as doubles.🛠️ Proposed fix (use iphi bins)
- iphimin = std::min<double>(phi, iphimin); - iphimax = std::max<double>(phi, iphimax); + const int iphi = static_cast<int>(coords[1]); + iphimin = std::min(iphi, iphimin); + iphimax = std::max(iphi, iphimax);offline/framework/fun4all/InputFileHandler.h (1)
7-40: Harden pre-open script execution against shell injection.
RunBeforeOpeningconcatenates script arguments and filenames into a shell command string (lines 183–187 of InputFileHandler.cc) and executes it viagSystem->Exec(). SincegSystem->Exec()invokes the shell via::system(), this pattern is vulnerable to shell injection if filenames or arguments contain shell metacharacters.To mitigate: (1) pass arguments as a vector to an exec variant that doesn't invoke a shell (e.g.,
posix_spawnor fork+execve), or (2) if a shell is required, apply strict whitelisting of allowed filenames/arguments and validate/escape all user-controlled inputs.
🤖 Fix all issues with AI agents
In `@generators/PHPythia8/PHPythia8.cc`:
- Around line 36-49: The constructor may leave m_Pythia8 null when PYTHIA8 is
unset, so add null guards at the start of Init, process_event, and End to avoid
dereferencing it: in Init check if (!m_Pythia8) return ABORTRUN; in
process_event check if (!m_Pythia8) return ABORTEVENT; and in End check if
(!m_Pythia8) return EVENT_OK; ensure these checks occur before any use of
m_Pythia8 (referencing the methods Init, process_event, End and the member
m_Pythia8) so the methods exit safely when the Pythia8 instance was not created.
In `@offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc`:
- Around line 977-1011: The loop iterating m_MvtxRawHitMap lacks the cleanup
performed in the FillIntt path; after finishing with each hitinfo you must
mirror that cleanup: call hitinfo.CleanupUsedPackets() and
hitinfo.clearFeeGTML1BCOMap(), delete any heap-allocated MvtxFeeIdInfo objects
if ownership was not transferred to mvtxEvtHeader (or skip deletion if
mvtxEvtHeader took ownership), clear hitinfo.MvtxFeeIdInfoVector and
hitinfo.MvtxRawHitVector, and finally erase the processed entry from
m_MvtxRawHitMap so memory and state are not leaked (perform these steps in the
same scope as the existing mvtxEvtHeader/mvtxcont handling in
Fun4AllStreamingInputManager::... loop).
In `@offline/packages/micromegas/MicromegasDefs.h`:
- Around line 61-71: genHitKey currently packs strip into 8 bits and getStrip
returns uint8_t, which truncates actual strip indices >255; change the hitkey
bit-packing to allocate enough bits for strip (use at least 10–11 bits to cover
~1084 strips or simply 16 bits for safety), update genHitKey to mask/shift strip
with the new width and getStrip to return a matching wider unsigned type (e.g.,
uint16_t), and adjust getSample/its masking/return type as needed so
TrkrDefs::hitkey encodes both fields without overflow or silent truncation;
alternatively, if you want to keep 8-bit encoding, add a validation in genHitKey
to throw/log and refuse strip>=256, but prefer expanding the strip field to
avoid mis-association.
In `@offline/packages/mvtx/SegmentationAlpide.cc`:
- Line 9: The build fails because SegmentationAlpide.cc uses C++20 <format>;
update the build configuration to enable C++20: either add AM_CXXFLAGS =
-std=c++20 (or append -std=c++20 to existing AM_CXXFLAGS) in
offline/packages/mvtx/Makefile.am or set the project-wide standard in
configure.ac (e.g., via AC_PROG_CXX and appropriate checks or AC_ARG_ENABLE for
CXXFLAGS) so files like SegmentationAlpide.cc that include <format> compile with
C++20 enabled.
In `@offline/packages/tpc/Tpc3DClusterizer.cc`:
- Around line 660-681: Guard the NTuple fill so it only runs when output is
enabled and the NTuple exists: wrap the m_clusterNT->Fill(fX) call in a
conditional check (e.g., if (m_output && m_clusterNT) or if (m_clusterNT)
depending on class invariants) in Tpc3DClusterizer so that m_clusterNT->Fill is
not invoked when m_output is false and m_clusterNT was not created; update the
surrounding block accordingly to avoid null dereference.
♻️ Duplicate comments (1)
offline/packages/mbd/MbdEvent.cc (1)
583-601: Apply the same_nsamplesrange validation here.Same concern as above: this overload still lacks an upper-bound guard for
_nsamples, which risks overrunningm_adc/m_samp(seeoffline/packages/mbd/MbdEvent.haround lines 140–141) if a corrupted packet reports too many samples.
🟠 Major comments (16)
offline/packages/CaloReco/PhotonClusterBuilder.cc-568-570 (1)
568-570: Guard against out-of-range access when computinge22.
showershapeis only checked for empty, bute22reads indices 8–11; ifget_shower_shapesreturns fewer than 12 elements this will be undefined behavior. Add a size check or default to 0 when the vector is too short.🔧 Proposed fix
- photon->set_shower_shape_parameter("e22", showershape[8] + showershape[9] + showershape[10] + showershape[11]); + float e22 = 0.0F; + if (showershape.size() > 11) + { + e22 = showershape[8] + showershape[9] + showershape[10] + showershape[11]; + } + photon->set_shower_shape_parameter("e22", e22);offline/packages/PHGenFitPkg/PHGenFit/Fitter.cc-54-57 (1)
54-57: Potential dangling pointer afterTGeoManager::Import.In ROOT, creating a new
TGeoManager(whichImportdoes internally) deletes the existinggGeoManager. The sequence here:
- Line 54: Creates
TGeoManager,_tgeo_managerpoints to it- Line 57:
Importcreates a newTGeoManagerfrom the file, deleting the one from line 54This leaves
_tgeo_manageras a dangling pointer. Compare withgetInstanceat line 192 which correctly captures the return value:TGeoManager* tgeo_manager = TGeoManager::Import(tgeo_file_name.data(), "Default");Proposed fix
Fitter::Fitter( const std::string& tgeo_file_name, const PHField* field, const std::string& fitter_choice, const std::string& /*track_rep_choice*/, const bool doEventDisplay) : verbosity(1000) - , _tgeo_manager(new TGeoManager("Default", "Geane geometry")) + , _tgeo_manager(nullptr) , _doEventDisplay(doEventDisplay) { - TGeoManager::Import(tgeo_file_name.data()); + _tgeo_manager = TGeoManager::Import(tgeo_file_name.data(), "Default");offline/framework/fun4allraw/SingleTriggeredInput.cc-492-497 (1)
492-497: Memory leak:evtnot deleted when packet is missing in retry loop.Same issue as above - when
pktis null, the code continues but the newly fetchedevt(from lines 481-491) is leaked.Proposed fix
pkt = evt->getPacket(representative_pid); if (!pkt) { std::cout << "representative packet invalid inside skiptrace.. continuing.." << std::endl; + delete evt; + evt = nullptr; continue; }offline/framework/fun4allraw/SingleTriggeredInput.cc-460-465 (1)
460-465: Memory leak:evtnot deleted when packet is missing.When
pktis null, the code callscontinuebutevt(allocated at line 439-448) is never deleted, causing a memory leak.Proposed fix
Packet* pkt = evt->getPacket(representative_pid); if (!pkt) { std::cout << "representative packet invalid inside skiptrace.. continuing.." << std::endl; + delete evt; + evt = nullptr; continue; }offline/framework/fun4allraw/SingleTriggeredInput.cc-509-511 (1)
509-511: Memory leak:pktnot deleted at the end of the skiptrace block.The
Packet* pktallocated at line 460 (and potentially reassigned at line 492) is never deleted when exiting the skiptrace block. This leaks memory in two scenarios:
- When
seb_diff == gl1_diffinitially (the if-block at line 472 is skipped)- When the retry while-loop exits after recovering or reaching max count
Proposed fix
clockconstcount++; } } + delete pkt; }offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc-1367-1367 (1)
1367-1367: Inconsistent underflow fallback value.When
m_RefBCO < m_mvtx_negative_bco, this line returnsm_mvtx_negative_bco, but line 839 in the same class returns0for the identical condition:// Line 839 (FillMvtx): uint64_t ref_bco_minus_range = m_RefBCO < m_mvtx_negative_bco ? 0 : ... // Line 1367 (FillMvtxPool): uint64_t ref_bco_minus_range = m_RefBCO < m_mvtx_negative_bco ? m_mvtx_negative_bco : ...This is also inconsistent with
FillInttPool(lines 1243-1247) which uses0as the fallback.Proposed fix for consistency
- uint64_t ref_bco_minus_range = m_RefBCO < m_mvtx_negative_bco ? m_mvtx_negative_bco : m_RefBCO - m_mvtx_negative_bco; + uint64_t ref_bco_minus_range = m_RefBCO < m_mvtx_negative_bco ? 0 : m_RefBCO - m_mvtx_negative_bco;offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc-974-976 (1)
974-976: Potential unsigned integer underflow inlower_limitcalculation.When
m_mvtx_is_triggeredis false,lower_limitis computed as:select_crossings - m_mvtx_bco_range - m_mvtx_negative_bcoSince
select_crossings = m_RefBCO + m_mvtx_bco_range, this simplifies tom_RefBCO - m_mvtx_negative_bco. However, unlike line 839 which has underflow protection, this line does not guard againstm_RefBCO < m_mvtx_negative_bco, causing wraparound to a very large value.Proposed fix to add underflow protection
- uint64_t lower_limit = m_mvtx_is_triggered ? select_crossings : select_crossings - m_mvtx_bco_range - m_mvtx_negative_bco; + uint64_t lower_limit = m_mvtx_is_triggered ? select_crossings : ref_bco_minus_range; uint64_t upper_limit = m_mvtx_is_triggered ? select_crossings + m_mvtx_bco_range : select_crossings;This reuses the already-computed
ref_bco_minus_rangefrom line 839 which has proper underflow protection.offline/framework/fun4all/InputFileHandler.cc-95-107 (1)
95-107: Handle RunBeforeOpening() failures before proceeding to open the file.When the pre-opening script fails (returns non-zero), the code currently logs the failure but then immediately attempts to open the file anyway. If the script is intended to stage, validate, or prepare the file for processing, this behavior will cause confusing cascading failures or partial processing. Skip the file and move to the next one when RunBeforeOpening() fails:
Suggested fix
if (RunBeforeOpening(stringvec)) { std::cout << PHWHERE << " RunBeforeOpening() failed" << std::endl; + m_FileList.pop_front(); + continue; }offline/framework/fun4all/InputFileHandler.cc-165-199 (1)
165-199: Shell injection vulnerability and incomplete exit-status fix.The command construction via string concatenation is genuinely risky:
m_OpeningArgsand elements fromstringvec(filenames) are user-controlled and not escaped, so spaces, quotes, and metacharacters can break execution or enable injection. The exit-status handling is also problematic: storinggSystem->Exec()(which returnswait()status, including -1 on error) inunsigned intloses negative error codes; only the exit code should be extracted via right-shift by 8 after validating success.The suggested diff addresses only the exit-status issue. For a complete fix, adopt either an argv-based execution method or robust shell-escaping of all parameters before concatenation.
🔧 Exit-status correction (partial)
- unsigned int iret = gSystem->Exec(fullcmd.c_str()); - - if (iret) - { - iret = iret >> 8U; - } - return static_cast<int> (iret); + int iret = gSystem->Exec(fullcmd.c_str()); + if (iret < 0) + { + return iret; + } + if (iret) + { + iret = iret >> 8; + } + return iret;offline/packages/micromegas/MicromegasClusterizer.cc-223-275 (1)
223-275: Confirm that the code actually keeps all samples per strip, not just the earliest.The comment claims "only the first one (in time) is kept," but the implementation contradicts this. The
local_hitmapstores all hits fromhit_rangeas separate entries (since each hitkey is unique). Whenhit_rangeis ordered numerically by hitkey (strip in bits 0-7, sample in bits 8-23), hits come in order: all sample=0 hits, then sample=1, etc. During insertion intolocal_hitmap, all hits are retained—nothing filters them to one per strip. Iteration yields all samples per strip, just ordered by strip due toStripSortFtor.If the intent is to keep only the earliest sample per strip, explicitly track the minimum sample per strip as shown in the suggested fix. Otherwise, clarify the comment to reflect that all samples are retained.
offline/packages/CaloEmbedding/CopyIODataNodes.cc-307-327 (1)
307-327: Add null/size checks inCopyTowerInfoto avoid crashes.
from_towerInfoandto_towerInfoare dereferenced without validation, and size mismatches are unchecked. Either can produce a segfault or out-of-range access.🛠️ Safer copy loop
void CopyIODataNodes::CopyTowerInfo(PHCompositeNode *from_topNode, PHCompositeNode *to_topNode) { TowerInfoContainer *from_towerInfo = findNode::getClass<TowerInfoContainer>(from_topNode, from_towerInfo_name); TowerInfoContainer *to_towerInfo = findNode::getClass<TowerInfoContainer>( to_topNode, to_towerInfo_name); - unsigned int ntowers = from_towerInfo->size(); + if (!from_towerInfo || !to_towerInfo) + { + std::cout << "TowerInfoContainer missing on source or destination" << std::endl; + m_CopyTowerInfoFlag = false; + return; + } + const unsigned int ntowers = from_towerInfo->size(); + if (to_towerInfo->size() != ntowers) + { + std::cout << "TowerInfoContainer size mismatch: from=" + << ntowers << " to=" << to_towerInfo->size() << std::endl; + } for (unsigned int ch = 0; ch < ntowers; ++ch) { TowerInfo *from_tow = from_towerInfo->get_tower_at_channel(ch); - to_towerInfo->get_tower_at_channel(ch)->copy_tower(from_tow); + TowerInfo *to_tow = to_towerInfo->get_tower_at_channel(ch); + if (!from_tow || !to_tow) { continue; } + to_tow->copy_tower(from_tow); }offline/packages/CaloEmbedding/CombineTowerInfo.cc-35-72 (1)
35-72: Guard against missing detector node before dereference.
DetNodecan be null (e.g.,m_detectorunset or node missing), which will crash onDetNode->addNode(node). Please validate or create the node.🔧 Proposed fix
void CombineTowerInfo::CreateNodes(PHCompositeNode* topNode) { PHNodeIterator iter(topNode); PHCompositeNode* dstNode = dynamic_cast<PHCompositeNode*>(iter.findFirst("PHCompositeNode", "DST")); if (!dstNode) { throw std::runtime_error("CombineTowerInfo: DST node not found"); } - PHCompositeNode *DetNode = dynamic_cast<PHCompositeNode *>(iter.findFirst("PHCompositeNode", m_detector)); + if (m_detector.empty()) + { + throw std::runtime_error("CombineTowerInfo: detector node name not set"); + } + PHCompositeNode *DetNode = + dynamic_cast<PHCompositeNode *>(iter.findFirst("PHCompositeNode", m_detector)); + if (!DetNode) + { + DetNode = new PHCompositeNode(m_detector); + dstNode->addNode(DetNode); + }generators/Herwig/HepMCTrigger/HepMCJetTrigger.cc-99-103 (1)
99-103: PDG filter range 12–18 includes muons and taus, not just neutrinos.The range
12–18spans electron neutrinos (12), muons (13), muon neutrinos (14), taus (15), tau neutrinos (16), and hypothetical heavy leptons (17–18). If the intent is to exclude only neutrinos from jet clustering, use explicit IDs:pd == 12 || pd == 14 || pd == 16(and optionally 18). Please clarify the physics requirement.Also, the typo in the comment: "behavioro" should be "behavior".
offline/QA/Tracking/StateClusterResidualsQA.cc-185-198 (1)
185-198: Potential null pointer usage before null check.
clustercould benullptriffindClusterfails, butgeometry->getGlobalPosition()is called with it at line 189 before the null check at line 193. IfgetGlobalPositiondereferences the cluster pointer, this will cause undefined behavior.Suggested fix
auto *cluster = cluster_map->findCluster(state->get_cluskey()); + if (!cluster) + { + continue; + } float state_x = state->get_x(); float state_y = state->get_y(); float state_z = state->get_z(); Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster); float cluster_x = glob.x(); float cluster_y = glob.y(); float cluster_z = glob.z(); - if (cluster) - { - m_histograms_x[h]->Fill(state_x - cluster_x); - m_histograms_y[h]->Fill(state_y - cluster_y); - m_histograms_z[h]->Fill(state_z - cluster_z); - } + m_histograms_x[h]->Fill(state_x - cluster_x); + m_histograms_y[h]->Fill(state_y - cluster_y); + m_histograms_z[h]->Fill(state_z - cluster_z);generators/Herwig/HepMCTrigger/HepMCParticleTrigger.cc-239-267 (1)
239-267:SetAbsEta*andSetEta*share the same threshold variables.Both
SetAbsEtaHigh/SetAbsEtaLowandSetEtaHigh/SetEtaLowmodify_theEtaHighand_theEtaLow. If a user calls bothSetEtaHigh(1.5)andSetAbsEtaHigh(1.0), the second call overwrites the first. Consider using separate storage for absolute eta thresholds (e.g.,_theAbsEtaHigh,_theAbsEtaLow) to allow both cuts to be active simultaneously with independent values.offline/QA/Tracking/StateClusterResidualsQA.h-56-101 (1)
56-101: Undefined behavior if setter called beforeaddHistogram().All setter methods (e.g.,
setNMvtx,setPhiRange, etc.) callm_pending.back()without checking ifm_pendingis empty. If a user calls any setter before callingaddHistogram(), this results in undefined behavior.Suggested fix: Add assertion or guard
StateClusterResidualsQA& setNMvtx(int min, int max) { + assert(!m_pending.empty() && "Must call addHistogram() before setting properties"); m_pending.back().min_mvtx_clusters = min; m_pending.back().max_mvtx_clusters = max; return *this; }Apply similar checks to all setter methods, or document clearly that
addHistogram()must be called first.
🟡 Minor comments (4)
offline/packages/mvtx/MvtxClusterPruner.cc-229-238 (1)
229-238: Guard against zero total when computing fraction.
Line 237 can emit inf/NaN whenm_cluster_counter_total == 0. A small guard keeps logs sane and avoids floating divide‑by‑zero edge cases.🔧 Suggested guard
- std::cout << "MvtxClusterPruner::End -" - << " m_cluster_counter_deleted: " << m_cluster_counter_deleted - << " fraction: " << double(m_cluster_counter_deleted) / m_cluster_counter_total - << std::endl; + std::cout << "MvtxClusterPruner::End -" + << " m_cluster_counter_deleted: " << m_cluster_counter_deleted; + if (m_cluster_counter_total > 0) + { + std::cout << " fraction: " + << double(m_cluster_counter_deleted) / m_cluster_counter_total; + } + else + { + std::cout << " fraction: n/a"; + } + std::cout << std::endl;offline/packages/tpc/Tpc3DClusterizer.cc-597-601 (1)
597-601: Early return leaksclus.
clusis allocated before thenHits == 0check, so the early return leaks memory.🛠️ Proposed fix
if (nHits == 0) { std::cout << "no hits" << std::endl; + delete clus; return; }offline/packages/Skimmers/Trigger/TriggerDSTSkimmer.cc-24-27 (1)
24-27: Guardmax_acceptto prevent all events being rejected if set to zero.If
set_accept_max(0)is explicitly called, the conditionaccepted_events >= max_acceptwill be true from the start (0 >= 0), rejecting all events. Add a guard:max_accept > 0to prevent this edge case.🔧 Suggested fix
- if ((accepted_events >= max_accept) && use_max_accept) + if (use_max_accept && max_accept > 0 && accepted_events >= max_accept) { return Fun4AllReturnCodes::ABORTEVENT; }offline/QA/Tracking/StateClusterResidualsQA.h-127-127 (1)
127-127: Include guard macro name mismatch in closing comment.The closing comment references
TRACKFITTINGQA_Hbut the actual include guard defined at the top isSTATECLUSTERRESIDUALSQA_H.Suggested fix
-#endif // TRACKFITTINGQA_H +#endif // STATECLUSTERRESIDUALSQA_H
🧹 Nitpick comments (28)
offline/packages/mbd/MbdSig.h (1)
35-35: Make the accessor const (and optionally noexcept).
This getter should not mutate state; marking itconstimproves API usability for const instances.♻️ Suggested tweak
- int GetNSamples() { return _nsamples; } + int GetNSamples() const noexcept { return _nsamples; }offline/framework/fun4allraw/SingleTriggeredInput.h (1)
115-115: Consider consistent naming convention.The new member
eventcounterdoesn't follow them_prefix convention used by most other private members in this class (e.g.,m_EventNumber,m_AllDone,m_FilesDone). While there are a few other members without the prefix (firstcall,firstclockcheck), aligning with the dominant convention improves consistency.Suggested naming
- long long eventcounter{0}; + long long m_EventCounter{0};offline/packages/tpc/TpcDistortionCorrection.cc (1)
17-21: LGTM!Good modernization. Removing redundant
inline(already implied byconstexpr) and switching fromconst T&to by-value is appropriate for a function primarily used with small primitive types likedouble, where passing by value avoids indirection overhead.Minor optional note: the
conston a by-value parameter (const T x) only prevents modification of the local copy inside the function and is typically omitted in this style. ConsiderT xif you prefer consistency with common idioms, but this is purely a style preference.offline/packages/trackreco/DSTClusterPruning.h (1)
77-80: Minor: Inconsistent comment indentation and orphaned Doxygen tag.The Doxygen comment on lines 77-78 has inconsistent indentation compared to other comments in the file. Also, line 80 has an orphaned
//@}closing tag without a corresponding//!@{opening tag.Suggested fix
-//! set to true if you want to dump out all clusters on all silicon -//! and all tpc seeds individually + //! set to true if you want to dump out all clusters on all silicon + //! and all tpc seeds individually bool m_pruneAllSeeds = false; - //@}offline/packages/trackreco/DSTClusterPruning.cc (2)
22-27: Remove duplicate includes.
SvtxTrack.handSvtxTrackMap.hare included twice (lines 22-23 and 26-27).Suggested fix
`#include` <trackbase_historic/SvtxTrack.h> `#include` <trackbase_historic/SvtxTrackMap.h> `#include` <trackbase_historic/TrackSeed.h> -#include <trackbase_historic/SvtxTrack.h> -#include <trackbase_historic/SvtxTrackMap.h> -
192-206: Consider extracting duplicate cluster-copying logic to a helper method.The cluster lookup, null-check, duplicate-check, copy, and insert pattern appears three times in this file (here, and in the original path for TPC and Silicon seeds around lines 241-255 and 266-280). Extracting this to a private helper method would reduce maintenance burden.
Example helper method
// In the private section of the class: void copyClusterToReducedMap(TrkrDefs::cluskey cluster_key); // Implementation: void DSTClusterPruning::copyClusterToReducedMap(TrkrDefs::cluskey cluster_key) { auto cluster = m_cluster_map->findCluster(cluster_key); if (!cluster) { std::cout << "DSTClusterPruning::evaluate_tracks - unable to find cluster for key " << cluster_key << std::endl; return; } if (!m_reduced_cluster_map->findCluster(cluster_key)) { m_cluster = new TrkrClusterv5(); m_cluster->CopyFrom(cluster); m_reduced_cluster_map->addClusterSpecifyKey(cluster_key, m_cluster); } }offline/packages/mvtx/MvtxHitPruner.cc (1)
158-165: Consider adding null check forfindHitSetresult.
findHitSetcould potentially returnnullptrif the hitset was removed or doesn't exist. While the logic should ensure the key exists (it was added to the multimap from existing hitsets), a defensive null check would prevent crashes if the data structure is modified unexpectedly.🔧 Suggested defensive check
// copy all hits to the hitset with strobe 0 auto* hitset = m_hits->findHitSet(hitsetkey); + if (!hitset) + { + continue; + } if (Verbosity()) {offline/packages/tpc/TpcClusterizer.cc (1)
368-380: LGTM!Using
std::maxandstd::minfor boundary clamping is cleaner and more idiomatic than the if-else patterns. Note that lines 370-373 and 377-380 still use if-statements for the upper bounds - consider usingstd::minfor consistency.♻️ Optional: Apply std::min consistently for upper bounds
int isophimax = iphi + 1; - if (!(isophimax < NPhiBinsMax)) - { - isophimax = NPhiBinsMax - 1; - } + isophimax = std::min(isophimax, NPhiBinsMax - 1); int isotmin = it - 1; isotmin = std::max(isotmin, 0); int isotmax = it + 1; - if (!(isotmax < NTBinsMax)) - { - isotmax = NTBinsMax - 1; - } + isotmax = std::min(isotmax, NTBinsMax - 1);offline/packages/trackreco/PHActsTrkFitter.cc (1)
136-187: Simplify redundant evaluator check (minor cleanup).Inside the
if (m_actsEvaluator)block, the nestedif (m_actsEvaluator && !m_simActsEvaluator)is redundant and can be simplified toif (!m_simActsEvaluator)for clarity. Optional cleanup.offline/framework/fun4allraw/SingleTriggeredInput.cc (1)
355-359: Debug output is unconditional and uses function-static variable.The
static bool firstclockarraypersists across all instances ofSingleTriggeredInput, so this message only prints once globally rather than once per instance. If per-instance behavior is intended, consider using a member variable. Additionally, this output isn't gated byVerbosity(), unlike other debug statements in this file.Suggested improvement
- static bool firstclockarray=true; - if(firstclockarray){ - std::cout << "first clock call pid " << pid << " m_bclkarray_map[pid][0] : " << m_bclkarray_map[pid][0] << std::endl; - firstclockarray=false; - } + if (Verbosity() > 0 && !m_firstClockPrinted) + { + std::cout << "first clock call pid " << pid << " m_bclkarray_map[pid][0] : " << m_bclkarray_map[pid][0] << std::endl; + m_firstClockPrinted = true; + }Add
bool m_firstClockPrinted{false};as a member variable in the header if per-instance behavior is desired.offline/packages/Skimmers/Trigger/TriggerDSTSkimmer.cc (1)
54-54: Avoid hard-coded class ID forGl1Packet.Consider using a named constant (or a class-provided ID) to avoid silent drift if the ID changes. Please also confirm
14001is the correct class ID in the current data model.💡 Example
+ constexpr int kGl1PacketClassId = 14001; - Gl1Packet *_gl1PacketInfo = findNode::getClass<Gl1Packet>(topNode, 14001); + Gl1Packet *_gl1PacketInfo = findNode::getClass<Gl1Packet>(topNode, kGl1PacketClassId);offline/packages/Skimmers/Trigger/TriggerDSTSkimmer.h (1)
25-30: New max-accept feature looks good; minor style note.The feature correctly implements the enable/configure pattern for limiting accepted events. The
return;statement on line 29 is redundant in a void function.♻️ Optional: Remove redundant return statement
void set_accept_max(int max_events) { use_max_accept = true; max_accept = max_events; - return; }offline/packages/micromegas/MicromegasClusterizer.cc (1)
159-167: Geometry-node fallback loop looks good; consider removing the stray comment.
//maappears to be leftover noise—worth dropping for clarity.offline/packages/micromegas/MicromegasDefs.cc (1)
69-87: Validate strip/sample ranges and call sites after the API change.
genHitKeynow truncatesstripto 8 bits and returnsuint8_tfromgetStrip. If any upstream path still uses strip values >255 (or relied on the oldgenHitKey(strip)overload), keys will collide silently. Please confirm range guarantees and update/verify all call sites.If range safety isn’t guaranteed, consider explicit validation to avoid silent truncation.
🛠️ Possible guard (example)
TrkrDefs::hitkey genHitKey(uint16_t strip, uint16_t sample) { + // guard against silent truncation + assert(strip <= 0xFFU); const TrkrDefs::hitkey key = (strip&0xFFU) << kBitShiftStrip; const TrkrDefs::hitkey tmp = (sample&0xFFFFU) << kBitShiftSample; return key|tmp; }offline/QA/Tracking/MicromegasClusterQA.h (2)
54-58: Guard against invalid sample ranges in setters.If
m_sample_min > m_sample_max, all clusters will be filtered out. Consider enforcingmin <= maxat set time.♻️ Possible invariant handling
- void set_sample_min(uint16_t value) { m_sample_min = value; } + void set_sample_min(uint16_t value) + { + m_sample_min = value; + if (m_sample_min > m_sample_max) { m_sample_max = m_sample_min; } + } - void set_sample_max(uint16_t value) { m_sample_max = value; } + void set_sample_max(uint16_t value) + { + m_sample_max = value; + if (m_sample_max < m_sample_min) { m_sample_min = m_sample_max; } + }
108-112: Consider documenting inclusive/exclusive semantics for defaults.The filter uses
[min, max)in the.cc. A brief comment here would prevent ambiguity.offline/QA/Tracking/MicromegasClusterQA.cc (1)
168-173: Avoid duplicategetSamplecalls and handle invalid ranges.The lambda calls
getSampletwice per hit, and an invalid range (min >= max) will silently drop all clusters. Consider a tiny refactor to compute once and short‑circuit on invalid configuration.♻️ Possible refactor
- if( std::none_of( hit_range.first, hit_range.second, - [this]( const TrkrClusterHitAssoc::Map::value_type& pair ) - { return MicromegasDefs::getSample( pair.second ) >= m_sample_min && MicromegasDefs::getSample( pair.second ) < m_sample_max; } ) ) + if (m_sample_min >= m_sample_max) + { continue; } + const auto in_range = [this](const TrkrClusterHitAssoc::Map::value_type& pair) + { + const auto sample = MicromegasDefs::getSample(pair.second); + return sample >= m_sample_min && sample < m_sample_max; + }; + if (std::none_of(hit_range.first, hit_range.second, in_range)) { continue; }offline/packages/CaloEmbedding/CopyIODataNodes.cc (1)
367-391: Gate logging on verbosity and validate names.Unconditional
std::coutinCreateTowerInfocan be noisy; also consider guarding against emptyfrom_towerInfo_name/to_towerInfo_nameto avoid confusing node lookups.♻️ Example tweak
void CopyIODataNodes::CreateTowerInfo(PHCompositeNode *from_topNode, PHCompositeNode *to_topNode) { - std::cout << "copying tower info" << std::endl; + if (Verbosity() > 0) { std::cout << "copying tower info" << std::endl; } + if (from_towerInfo_name.empty() || to_towerInfo_name.empty()) + { + std::cout << "TowerInfo names not configured" << std::endl; + m_CopyTowerInfoFlag = false; + return; + }offline/packages/CaloEmbedding/CopyIODataNodes.h (1)
38-79: Validate TowerInfo names before enabling copying.If either name is empty,
findNode::getClasswill search for an empty node name. Consider guarding and keeping the flag off unless both names are set.♻️ Example guard
void set_CopyTowerInfo(const std::string& set_from_towerInfo_name,const std::string& set_to_towerInfo_name) { - from_towerInfo_name = set_from_towerInfo_name; - to_towerInfo_name = set_to_towerInfo_name; - m_CopyTowerInfoFlag = true; + from_towerInfo_name = set_from_towerInfo_name; + to_towerInfo_name = set_to_towerInfo_name; + m_CopyTowerInfoFlag = !(from_towerInfo_name.empty() || to_towerInfo_name.empty()); return; }offline/packages/tpc/LaserClusterizer.cc (1)
408-410: Confirmstd::formatis supported in all build targets.
std::formatrequires C++20 and a libstdc++/libc++ version with full format support; older toolchains will fail to compile. If any target is < C++20, considerfmt::formator ROOTForm.offline/packages/jetbackground/DetermineTowerBackground.cc (1)
76-108: Consider returning error code instead of callingexit(-1).At line 90, calling
exit(-1)whencalibdiris empty is abrupt. Consider returning an error code (e.g.,Fun4AllReturnCodes::ABORTRUN) to allow the caller to handle the failure gracefully, consistent with how the caller already handlesLoadCalibrations()failures at lines 65-69.Suggested change
if (calibdir.empty()) { std::cout << "Could not find filename for calo average v2, exiting" << std::endl; - exit(-1); + return Fun4AllReturnCodes::ABORTRUN; }offline/packages/jetbackground/DetermineTowerBackground.h (2)
16-16: Unnecessary include.
<array>is included but not used—the code usesstd::vectorfor_CENTRALITY_V2. Consider removing this include.
41-45: Parameter should beconst std::string&.The
urlparameter is not modified, so it should be a const reference to allow passing temporaries and const strings:- void SetOverwriteCaloV2(std::string &url) + void SetOverwriteCaloV2(const std::string &url)offline/QA/Tracking/StateClusterResidualsQA.cc (2)
32-33: Unused template function.The
squaretemplate is defined but never used in this file. Consider removing it unless it's intended for future use.
239-245: EndRun retrieves histogram manager but doesn't use it.The
EndRunmethod gets the histogram manager and asserts it exists, but doesn't actually do anything with it. If no action is needed, consider removing the unused code or adding a TODO comment if functionality is planned.offline/QA/Tracking/StateClusterResidualsQA.h (1)
8-12: Consider grouping standard library headers consistently.The standard headers are not in a consistent order. Consider grouping them alphabetically or by category (containers, utilities, etc.) for readability.
Suggested header ordering
`#include` <string> -#include <limits> -#include <cmath> -#include <cfloat> +#include <cfloat> +#include <cmath> `#include` <vector>Note:
<limits>appears unused and can be removed sinceFLT_MAXfrom<cfloat>is used instead ofstd::numeric_limits.generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h (1)
8-8: Unused include:fastjet/PseudoJet.hh.This header includes
fastjet/PseudoJet.hhbut the class declaration doesn't usePseudoJetanywhere. Consider removing it to reduce compilation dependencies.Suggested fix
`#include` <fun4all/SubsysReco.h> -#include <fastjet/PseudoJet.hh> - `#include` <cmath>generators/Herwig/HepMCTrigger/HepMCParticleTrigger.cc (1)
12-16: Remove unused FastJet includes.The implementation includes
fastjet/JetDefinition.hhandfastjet/PseudoJet.hhbut doesn't use any FastJet functionality. These appear to be copied from the relatedHepMCJetTriggerimplementation.Suggested fix
`#include` <phool/getClass.h> -#include <fastjet/JetDefinition.hh> - `#include` <HepMC/GenEvent.h> -#include <fastjet/PseudoJet.hh> `#include` <map>
| /** | ||
| * @brief Construct a PHPythia8 generator instance and configure HepMC conversion. | ||
| * | ||
| * Initializes the Pythia8 engine using the path from the environment variable | ||
| * `PYTHIA8`, configures a HepMC::Pythia8ToHepMC converter to store process, | ||
| * PDF, and cross-section information, and sets the default embedding ID to 1. | ||
| * The constructor preserves and restores std::cout formatting around Pythia8 | ||
| * construction to avoid altering global stream state. | ||
| * | ||
| * If `PYTHIA8` is not set, an error message is printed and the Pythia8 instance | ||
| * remains uninitialized. | ||
| * | ||
| * @param name Name forwarded to the SubsysReco base class (module instance name). | ||
| */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the PHPythia8.cc file around the constructor
cat -n generators/PHPythia8/PHPythia8.cc | head -80Repository: sPHENIX-Test/coresoftware
Length of output: 3345
🏁 Script executed:
# Let's also get the header file to understand the member variable declaration
fd "PHPythia8.h" generators/Repository: sPHENIX-Test/coresoftware
Length of output: 101
🏁 Script executed:
# Search for the Init, process_event, and End method implementations
rg "int PHPythia8::Init|int PHPythia8::process_event|int PHPythia8::End" -A 15 generators/PHPythia8/PHPythia8.ccRepository: sPHENIX-Test/coresoftware
Length of output: 1485
🏁 Script executed:
# Search for m_Pythia8 usage to see if it's dereferenced without guards
rg "m_Pythia8->" generators/PHPythia8/PHPythia8.cc | head -20Repository: sPHENIX-Test/coresoftware
Length of output: 916
Add guards to prevent null-pointer dereference in Init, process_event, and End.
The constructor leaves m_Pythia8 uninitialized when PYTHIA8 is unset and returns early without error handling. However, Init, process_event, and End immediately dereference m_Pythia8 without any null checks, causing a crash in production if the environment variable is missing.
Add guard checks in all three methods to return appropriate error codes (ABORTRUN for Init, ABORTEVENT for process_event, EVENT_OK for End) before dereferencing m_Pythia8:
🛡️ Guard pattern (apply in Init/process_event/End)
int PHPythia8::Init(PHCompositeNode *topNode)
{
+ if (!m_Pythia8)
+ {
+ std::cout << PHWHERE << " PYTHIA8 not configured; aborting run." << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+ }
...
}
int PHPythia8::process_event(PHCompositeNode * /*topNode*/)
{
+ if (!m_Pythia8)
+ {
+ std::cout << PHWHERE << " PYTHIA8 not configured; aborting event." << std::endl;
+ return Fun4AllReturnCodes::ABORTEVENT;
+ }
...
}
int PHPythia8::End(PHCompositeNode * /*topNode*/)
{
+ if (!m_Pythia8) return Fun4AllReturnCodes::EVENT_OK;
...
}🤖 Prompt for AI Agents
In `@generators/PHPythia8/PHPythia8.cc` around lines 36 - 49, The constructor may
leave m_Pythia8 null when PYTHIA8 is unset, so add null guards at the start of
Init, process_event, and End to avoid dereferencing it: in Init check if
(!m_Pythia8) return ABORTRUN; in process_event check if (!m_Pythia8) return
ABORTEVENT; and in End check if (!m_Pythia8) return EVENT_OK; ensure these
checks occur before any use of m_Pythia8 (referencing the methods Init,
process_event, End and the member m_Pythia8) so the methods exit safely when the
Pythia8 instance was not created.
| for (auto& [bco, hitinfo] : m_MvtxRawHitMap) | ||
| { | ||
| while (select_crossings <= m_MvtxRawHitMap.begin()->first && m_MvtxRawHitMap.begin()->first <= select_crossings + m_mvtx_bco_range) // triggered | ||
| if (bco < lower_limit) | ||
| { | ||
| if (Verbosity() > 2) | ||
| { | ||
| std::cout << "Adding 0x" << std::hex << m_MvtxRawHitMap.begin()->first | ||
| << " ref: 0x" << select_crossings << std::dec << std::endl; | ||
| } | ||
| for (auto *mvtxFeeIdInfo : m_MvtxRawHitMap.begin()->second.MvtxFeeIdInfoVector) | ||
| { | ||
| if (Verbosity() > 1) | ||
| { | ||
| mvtxFeeIdInfo->identify(); | ||
| } | ||
| mvtxEvtHeader->AddFeeIdInfo(mvtxFeeIdInfo); | ||
| delete mvtxFeeIdInfo; | ||
| } | ||
| m_MvtxRawHitMap.begin()->second.MvtxFeeIdInfoVector.clear(); | ||
| mvtxEvtHeader->AddL1Trg(m_MvtxRawHitMap.begin()->second.MvtxL1TrgBco); | ||
| continue; | ||
| } | ||
| if (bco > upper_limit) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| for (auto *mvtxhititer : m_MvtxRawHitMap.begin()->second.MvtxRawHitVector) | ||
| { | ||
| if (Verbosity() > 1) | ||
| { | ||
| mvtxhititer->identify(); | ||
| } | ||
| mvtxcont->AddHit(mvtxhititer); | ||
| } | ||
| for (auto *iter : m_MvtxInputVector) | ||
| { | ||
| iter->CleanupUsedPackets(m_MvtxRawHitMap.begin()->first); | ||
| } | ||
| m_MvtxRawHitMap.begin()->second.MvtxRawHitVector.clear(); | ||
| m_MvtxRawHitMap.begin()->second.MvtxL1TrgBco.clear(); | ||
| m_MvtxRawHitMap.erase(m_MvtxRawHitMap.begin()); | ||
| // m_MvtxRawHitMap.empty() need to be checked here since we do not call FillPoolMvtx() | ||
| if (m_MvtxRawHitMap.empty()) | ||
| { | ||
| break; | ||
| } | ||
| if (Verbosity() > 2) | ||
| { | ||
| std::cout << "Adding 0x" << std::hex << bco | ||
| << " ref: 0x" << select_crossings << std::dec << std::endl; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| while (select_crossings - m_mvtx_bco_range - m_mvtx_negative_bco <= m_MvtxRawHitMap.begin()->first && m_MvtxRawHitMap.begin()->first <= select_crossings) // streamed | ||
| for (auto *mvtxFeeIdInfo : hitinfo.MvtxFeeIdInfoVector) | ||
| { | ||
| if (Verbosity() > 2) | ||
| { | ||
| std::cout << "Adding 0x" << std::hex << m_MvtxRawHitMap.begin()->first | ||
| << " ref: 0x" << select_crossings << std::dec << std::endl; | ||
| } | ||
| for (auto *mvtxFeeIdInfo : m_MvtxRawHitMap.begin()->second.MvtxFeeIdInfoVector) | ||
| if (Verbosity() > 1) | ||
| { | ||
| if (Verbosity() > 1) | ||
| { | ||
| mvtxFeeIdInfo->identify(); | ||
| } | ||
| mvtxEvtHeader->AddFeeIdInfo(mvtxFeeIdInfo); | ||
| delete mvtxFeeIdInfo; | ||
| mvtxFeeIdInfo->identify(); | ||
| } | ||
| m_MvtxRawHitMap.begin()->second.MvtxFeeIdInfoVector.clear(); | ||
| mvtxEvtHeader->AddL1Trg(m_MvtxRawHitMap.begin()->second.MvtxL1TrgBco); | ||
| mvtxEvtHeader->AddFeeIdInfo(mvtxFeeIdInfo); | ||
| } | ||
| mvtxEvtHeader->AddL1Trg(hitinfo.MvtxL1TrgBco); | ||
|
|
||
| for (auto *mvtxhititer : m_MvtxRawHitMap.begin()->second.MvtxRawHitVector) | ||
| { | ||
| if (Verbosity() > 1) | ||
| { | ||
| mvtxhititer->identify(); | ||
| } | ||
| mvtxcont->AddHit(mvtxhititer); | ||
| } | ||
| for (auto *iter : m_MvtxInputVector) | ||
| { | ||
| iter->CleanupUsedPackets(m_MvtxRawHitMap.begin()->first); | ||
| } | ||
| m_MvtxRawHitMap.begin()->second.MvtxRawHitVector.clear(); | ||
| m_MvtxRawHitMap.begin()->second.MvtxL1TrgBco.clear(); | ||
| m_MvtxRawHitMap.erase(m_MvtxRawHitMap.begin()); | ||
| // m_MvtxRawHitMap.empty() need to be checked here since we do not call FillPoolMvtx() | ||
| if (m_MvtxRawHitMap.empty()) | ||
| for (auto *mvtxhititer : hitinfo.MvtxRawHitVector) | ||
| { | ||
| if (Verbosity() > 1) | ||
| { | ||
| break; | ||
| mvtxhititer->identify(); | ||
| } | ||
| mvtxcont->AddHit(mvtxhititer); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Missing cleanup logic after processing MVTX hits.
Same issue as FillIntt. The loop processes entries but never:
- Calls
CleanupUsedPackets()on input vectors - Calls
clearFeeGTML1BCOMap()on input vectors - Deletes
MvtxFeeIdInfoobjects (ownership transferred tomvtxEvtHeader?) - Clears
MvtxFeeIdInfoVector,MvtxL1TrgBco,MvtxRawHitVector - Erases processed entries from
m_MvtxRawHitMap
Compare with the cleanup loop at lines 856-873 which properly handles all cleanup steps.
Proposed fix to add cleanup after hit processing
for (auto *mvtxhititer : hitinfo.MvtxRawHitVector)
{
if (Verbosity() > 1)
{
mvtxhititer->identify();
}
mvtxcont->AddHit(mvtxhititer);
}
}
+ // Cleanup processed entries
+ auto it = m_MvtxRawHitMap.begin();
+ while (it != m_MvtxRawHitMap.end() && it->first <= upper_limit)
+ {
+ if (it->first < lower_limit)
+ {
+ ++it;
+ continue;
+ }
+ for (auto *iter : m_MvtxInputVector)
+ {
+ iter->CleanupUsedPackets(it->first);
+ iter->clearFeeGTML1BCOMap(it->first);
+ }
+ it->second.MvtxFeeIdInfoVector.clear();
+ it->second.MvtxL1TrgBco.clear();
+ it->second.MvtxRawHitVector.clear();
+ it = m_MvtxRawHitMap.erase(it);
+ }
return 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.
| for (auto& [bco, hitinfo] : m_MvtxRawHitMap) | |
| { | |
| while (select_crossings <= m_MvtxRawHitMap.begin()->first && m_MvtxRawHitMap.begin()->first <= select_crossings + m_mvtx_bco_range) // triggered | |
| if (bco < lower_limit) | |
| { | |
| if (Verbosity() > 2) | |
| { | |
| std::cout << "Adding 0x" << std::hex << m_MvtxRawHitMap.begin()->first | |
| << " ref: 0x" << select_crossings << std::dec << std::endl; | |
| } | |
| for (auto *mvtxFeeIdInfo : m_MvtxRawHitMap.begin()->second.MvtxFeeIdInfoVector) | |
| { | |
| if (Verbosity() > 1) | |
| { | |
| mvtxFeeIdInfo->identify(); | |
| } | |
| mvtxEvtHeader->AddFeeIdInfo(mvtxFeeIdInfo); | |
| delete mvtxFeeIdInfo; | |
| } | |
| m_MvtxRawHitMap.begin()->second.MvtxFeeIdInfoVector.clear(); | |
| mvtxEvtHeader->AddL1Trg(m_MvtxRawHitMap.begin()->second.MvtxL1TrgBco); | |
| continue; | |
| } | |
| if (bco > upper_limit) | |
| { | |
| break; | |
| } | |
| for (auto *mvtxhititer : m_MvtxRawHitMap.begin()->second.MvtxRawHitVector) | |
| { | |
| if (Verbosity() > 1) | |
| { | |
| mvtxhititer->identify(); | |
| } | |
| mvtxcont->AddHit(mvtxhititer); | |
| } | |
| for (auto *iter : m_MvtxInputVector) | |
| { | |
| iter->CleanupUsedPackets(m_MvtxRawHitMap.begin()->first); | |
| } | |
| m_MvtxRawHitMap.begin()->second.MvtxRawHitVector.clear(); | |
| m_MvtxRawHitMap.begin()->second.MvtxL1TrgBco.clear(); | |
| m_MvtxRawHitMap.erase(m_MvtxRawHitMap.begin()); | |
| // m_MvtxRawHitMap.empty() need to be checked here since we do not call FillPoolMvtx() | |
| if (m_MvtxRawHitMap.empty()) | |
| { | |
| break; | |
| } | |
| if (Verbosity() > 2) | |
| { | |
| std::cout << "Adding 0x" << std::hex << bco | |
| << " ref: 0x" << select_crossings << std::dec << std::endl; | |
| } | |
| } | |
| else | |
| { | |
| while (select_crossings - m_mvtx_bco_range - m_mvtx_negative_bco <= m_MvtxRawHitMap.begin()->first && m_MvtxRawHitMap.begin()->first <= select_crossings) // streamed | |
| for (auto *mvtxFeeIdInfo : hitinfo.MvtxFeeIdInfoVector) | |
| { | |
| if (Verbosity() > 2) | |
| { | |
| std::cout << "Adding 0x" << std::hex << m_MvtxRawHitMap.begin()->first | |
| << " ref: 0x" << select_crossings << std::dec << std::endl; | |
| } | |
| for (auto *mvtxFeeIdInfo : m_MvtxRawHitMap.begin()->second.MvtxFeeIdInfoVector) | |
| if (Verbosity() > 1) | |
| { | |
| if (Verbosity() > 1) | |
| { | |
| mvtxFeeIdInfo->identify(); | |
| } | |
| mvtxEvtHeader->AddFeeIdInfo(mvtxFeeIdInfo); | |
| delete mvtxFeeIdInfo; | |
| mvtxFeeIdInfo->identify(); | |
| } | |
| m_MvtxRawHitMap.begin()->second.MvtxFeeIdInfoVector.clear(); | |
| mvtxEvtHeader->AddL1Trg(m_MvtxRawHitMap.begin()->second.MvtxL1TrgBco); | |
| mvtxEvtHeader->AddFeeIdInfo(mvtxFeeIdInfo); | |
| } | |
| mvtxEvtHeader->AddL1Trg(hitinfo.MvtxL1TrgBco); | |
| for (auto *mvtxhititer : m_MvtxRawHitMap.begin()->second.MvtxRawHitVector) | |
| { | |
| if (Verbosity() > 1) | |
| { | |
| mvtxhititer->identify(); | |
| } | |
| mvtxcont->AddHit(mvtxhititer); | |
| } | |
| for (auto *iter : m_MvtxInputVector) | |
| { | |
| iter->CleanupUsedPackets(m_MvtxRawHitMap.begin()->first); | |
| } | |
| m_MvtxRawHitMap.begin()->second.MvtxRawHitVector.clear(); | |
| m_MvtxRawHitMap.begin()->second.MvtxL1TrgBco.clear(); | |
| m_MvtxRawHitMap.erase(m_MvtxRawHitMap.begin()); | |
| // m_MvtxRawHitMap.empty() need to be checked here since we do not call FillPoolMvtx() | |
| if (m_MvtxRawHitMap.empty()) | |
| for (auto *mvtxhititer : hitinfo.MvtxRawHitVector) | |
| { | |
| if (Verbosity() > 1) | |
| { | |
| break; | |
| mvtxhititer->identify(); | |
| } | |
| mvtxcont->AddHit(mvtxhititer); | |
| } | |
| } | |
| for (auto& [bco, hitinfo] : m_MvtxRawHitMap) | |
| { | |
| if (bco < lower_limit) | |
| { | |
| continue; | |
| } | |
| if (bco > upper_limit) | |
| { | |
| break; | |
| } | |
| if (Verbosity() > 2) | |
| { | |
| std::cout << "Adding 0x" << std::hex << bco | |
| << " ref: 0x" << select_crossings << std::dec << std::endl; | |
| } | |
| for (auto *mvtxFeeIdInfo : hitinfo.MvtxFeeIdInfoVector) | |
| { | |
| if (Verbosity() > 1) | |
| { | |
| mvtxFeeIdInfo->identify(); | |
| } | |
| mvtxEvtHeader->AddFeeIdInfo(mvtxFeeIdInfo); | |
| } | |
| mvtxEvtHeader->AddL1Trg(hitinfo.MvtxL1TrgBco); | |
| for (auto *mvtxhititer : hitinfo.MvtxRawHitVector) | |
| { | |
| if (Verbosity() > 1) | |
| { | |
| mvtxhititer->identify(); | |
| } | |
| mvtxcont->AddHit(mvtxhititer); | |
| } | |
| } | |
| // Cleanup processed entries | |
| auto it = m_MvtxRawHitMap.begin(); | |
| while (it != m_MvtxRawHitMap.end() && it->first <= upper_limit) | |
| { | |
| if (it->first < lower_limit) | |
| { | |
| +it; | |
| continue; | |
| } | |
| for (auto *iter : m_MvtxInputVector) | |
| { | |
| iter->CleanupUsedPackets(it->first); | |
| iter->clearFeeGTML1BCOMap(it->first); | |
| } | |
| it->second.MvtxFeeIdInfoVector.clear(); | |
| it->second.MvtxL1TrgBco.clear(); | |
| it->second.MvtxRawHitVector.clear(); | |
| it = m_MvtxRawHitMap.erase(it); | |
| } | |
| return 0; |
🤖 Prompt for AI Agents
In `@offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc` around lines
977 - 1011, The loop iterating m_MvtxRawHitMap lacks the cleanup performed in
the FillIntt path; after finishing with each hitinfo you must mirror that
cleanup: call hitinfo.CleanupUsedPackets() and hitinfo.clearFeeGTML1BCOMap(),
delete any heap-allocated MvtxFeeIdInfo objects if ownership was not transferred
to mvtxEvtHeader (or skip deletion if mvtxEvtHeader took ownership), clear
hitinfo.MvtxFeeIdInfoVector and hitinfo.MvtxRawHitVector, and finally erase the
processed entry from m_MvtxRawHitMap so memory and state are not leaked (perform
these steps in the same scope as the existing mvtxEvtHeader/mvtxcont handling in
Fun4AllStreamingInputManager::... loop).
| * @brief Generate a hitkey from strip index inside tile | ||
| * @param[in] strip strip index | ||
| * @param[in] sample sample index | ||
| */ | ||
| TrkrDefs::hitkey genHitKey(uint16_t strip ); | ||
| TrkrDefs::hitkey genHitKey(uint16_t strip, uint16_t sample = 0 ); | ||
|
|
||
| //! get strip from hit key | ||
| uint16_t getStrip(TrkrDefs::hitkey); | ||
| uint8_t getStrip(TrkrDefs::hitkey); | ||
|
|
||
| //! get sample from hit key | ||
| uint16_t getSample(TrkrDefs::hitkey); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the MicromegasDefs.h file
find . -name "MicromegasDefs.h" -type fRepository: sPHENIX-Test/coresoftware
Length of output: 115
🏁 Script executed:
# Get the full context of the file around lines 61-71
cat -n offline/packages/micromegas/MicromegasDefs.h | head -100Repository: sPHENIX-Test/coresoftware
Length of output: 3516
🏁 Script executed:
# Search for genHitKey implementation and strip-related constants
rg -n "genHitKey|MMPADSPERFIBER|MMSTRIPCOUNT|STRIPS_PER" offline/packages/micromegas/Repository: sPHENIX-Test/coresoftware
Length of output: 445
🏁 Script executed:
# Search for any documentation on strip count/range
rg -n "strip.*count|num.*strip|MAX.*STRIP" offline/packages/micromegas/ -iRepository: sPHENIX-Test/coresoftware
Length of output: 2148
🏁 Script executed:
# View the implementation of genHitKey and getStrip in MicromegasDefs.cc
sed -n '69,150p' offline/packages/micromegas/MicromegasDefs.ccRepository: sPHENIX-Test/coresoftware
Length of output: 1177
🏁 Script executed:
# Also check for any constants related to strip range
rg -n "constexpr|static.*const" offline/packages/micromegas/MicromegasDefs.h | head -30Repository: sPHENIX-Test/coresoftware
Length of output: 604
🏁 Script executed:
# Check the actual strip counts used in geometry
sed -n '235,250p' offline/packages/micromegas/CylinderGeomMicromegas.ccRepository: sPHENIX-Test/coresoftware
Length of output: 935
🏁 Script executed:
# Find the pitch value
rg -n "m_pitch|pitch" offline/packages/micromegas/CylinderGeomMicromegas.h | head -20Repository: sPHENIX-Test/coresoftware
Length of output: 235
🏁 Script executed:
# Look for pitch definition
rg -n "m_pitch.*=" offline/packages/micromegas/CylinderGeomMicromegas.cc | head -10Repository: sPHENIX-Test/coresoftware
Length of output: 51
🏁 Script executed:
# Check for any test data or documentation showing actual strip counts
rg -n "strip.*count|get_strip_count" offline/packages/micromegas/ -A 5 | head -40Repository: sPHENIX-Test/coresoftware
Length of output: 4446
🏁 Script executed:
# Search for tile geometry setup and dimensions
rg -n "halfLength|RectangleBounds|set_tile|m_tiles" offline/packages/micromegas/CylinderGeomMicromegas.cc | head -30Repository: sPHENIX-Test/coresoftware
Length of output: 1719
🏁 Script executed:
# Look for any initialization with dimensions or constants
rg -n "10|20|30|40|50.*cm|mm.*pitch|set_pitch" offline/packages/micromegas/ | grep -v ".git" | head -20Repository: sPHENIX-Test/coresoftware
Length of output: 2949
🏁 Script executed:
# Check for test or initialization files
fd -e cc -e h offline/packages/micromegas/ | xargs rg -l "CylinderGeomMicromegas" | head -10Repository: sPHENIX-Test/coresoftware
Length of output: 1034
🏁 Script executed:
# Check the Geant4 detector definition for micromegas
sed -n '1,100p' simulation/g4simulation/g4micromegas/PHG4MicromegasDetector.ccRepository: sPHENIX-Test/coresoftware
Length of output: 2898
🏁 Script executed:
# Search for geometry or detector size specifications
rg -n "halfLength|width|height|size" simulation/g4simulation/g4micromegas/PHG4MicromegasDetector.cc | head -30Repository: sPHENIX-Test/coresoftware
Length of output: 621
🏁 Script executed:
# Look for GDML files or geometry definitions
fd --full-path '.*micromegas.*' offline/packages/micromegas/ --type f | xargs basename -a | sort | uniqRepository: sPHENIX-Test/coresoftware
Length of output: 722
🏁 Script executed:
# Get the tile_length definition and surrounding context
sed -n '130,170p' simulation/g4simulation/g4micromegas/PHG4MicromegasDetector.ccRepository: sPHENIX-Test/coresoftware
Length of output: 1547
🏁 Script executed:
# Also verify the strip calculation with actual values
python3 << 'EOF'
# Tile dimensions from the code
tile_width = 31.6 # cm
pitch = 0.1 # cm (1 mm)
# Calculate strip count for tile width
strip_count_width = int(tile_width / pitch)
print(f"For tile_width = {tile_width} cm:")
print(f" Strip count = floor((2 * {tile_width/2}) / {pitch}) = {strip_count_width}")
print(f" Max uint8_t = 255")
print(f" Truncation risk: {strip_count_width > 255}")
EOFRepository: sPHENIX-Test/coresoftware
Length of output: 155
Fix bit-packing scheme: 8-bit strip encoding truncates detectors with 600+ strips.
The micromegas detectors have tile_width = 31.6 cm and tile_length = 54.2 cm with pitch = 0.1 cm, yielding strip counts of ~632 and ~1084 depending on segmentation direction. However, genHitKey only encodes 8 bits for strip ((strip & 0xFFU)), and getStrip returns uint8_t (max 255). Any strip index ≥ 256 silently truncates, causing hit mis-association. The current encoding scheme is incompatible with the detector geometry. Either expand the bit allocation for strip in the hitkey format, or validate and enforce a 256-strip maximum.
🤖 Prompt for AI Agents
In `@offline/packages/micromegas/MicromegasDefs.h` around lines 61 - 71, genHitKey
currently packs strip into 8 bits and getStrip returns uint8_t, which truncates
actual strip indices >255; change the hitkey bit-packing to allocate enough bits
for strip (use at least 10–11 bits to cover ~1084 strips or simply 16 bits for
safety), update genHitKey to mask/shift strip with the new width and getStrip to
return a matching wider unsigned type (e.g., uint16_t), and adjust getSample/its
masking/return type as needed so TrkrDefs::hitkey encodes both fields without
overflow or silent truncation; alternatively, if you want to keep 8-bit
encoding, add a validation in genHitKey to throw/log and refuse strip>=256, but
prefer expanding the strip field to avoid mis-association.
| #include <boost/format.hpp> | ||
|
|
||
| #include <iostream> | ||
| #include <format> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and check build configuration files
echo "=== Finding CMakeLists.txt files ==="
find . -name "CMakeLists.txt" -type f | head -20
echo ""
echo "=== Checking CMakeLists.txt content for C++ standard ==="
find . -name "CMakeLists.txt" -type f -exec grep -n -E 'CMAKE_CXX_STANDARD|CXX_STANDARD|set\(.*_CXX' {} + | head -50
echo ""
echo "=== Looking for Makefiles ==="
find . -name "Makefile*" -o -name "*.mk" | head -20
echo ""
echo "=== Checking Makefiles for C++ standard settings ==="
find . \( -name "Makefile*" -o -name "*.mk" \) -type f -exec grep -n -E 'std=c\+\+|CXXFLAGS|CPPFLAGS' {} + 2>/dev/null | head -50Repository: sPHENIX-Test/coresoftware
Length of output: 4568
🏁 Script executed:
#!/bin/bash
# Look for configure.ac or configure files
echo "=== Finding configure.ac files ==="
find . -name "configure.ac" -o -name "configure.in" | head -10
echo ""
echo "=== Checking for C++ standard in configure.ac files ==="
find . -name "configure.ac" -o -name "configure.in" | xargs grep -n -E 'std=c\+\+|AC_PROG_CXX|AX_CXX_COMPILE_STDCXX' 2>/dev/null | head -50
echo ""
echo "=== Checking mvtx specific Makefile.am ==="
find . -path "*/mvtx/*" -name "Makefile.am" | xargs cat
echo ""
echo "=== Looking for any -std=c++ in all Makefiles ==="
find . -name "Makefile.am" -o -name "Makefile" | xargs grep -h "std=c++" 2>/dev/null | sort -uRepository: sPHENIX-Test/coresoftware
Length of output: 7348
🏁 Script executed:
#!/bin/bash
# Look for root configure.ac
echo "=== Checking root and parent directories ==="
ls -la configure.ac 2>/dev/null || echo "No root configure.ac"
find . -maxdepth 2 -name "configure.ac" -o -name "configure.in" | head -5
echo ""
echo "=== Looking for top-level build configuration ==="
ls -la | grep -E "configure|Makefile|CMake"
echo ""
echo "=== Checking for any global C++ standard flags ==="
find . -name "Makefile.am" | xargs grep -h "AM_CXXFLAGS\|CXXFLAGS" | sort -u | grep -i "std"
echo ""
echo "=== Looking at the SegmentationAlpide.cc file location ==="
find . -name "SegmentationAlpide.cc" -exec ls -la {} \;
echo ""
echo "=== Checking if the file includes format ==="
grep -n "include.*format" offline/packages/mvtx/SegmentationAlpide.ccRepository: sPHENIX-Test/coresoftware
Length of output: 514
Build system must be updated to support C++20 for <format> usage.
The code uses #include <format> (C++20), but the build system does not specify a C++ standard. Update offline/packages/mvtx/Makefile.am to add AM_CXXFLAGS = -std=c++20 (or ensure a higher standard is set project-wide in the configure.ac). Without this, compilation will fail.
🤖 Prompt for AI Agents
In `@offline/packages/mvtx/SegmentationAlpide.cc` at line 9, The build fails
because SegmentationAlpide.cc uses C++20 <format>; update the build
configuration to enable C++20: either add AM_CXXFLAGS = -std=c++20 (or append
-std=c++20 to existing AM_CXXFLAGS) in offline/packages/mvtx/Makefile.am or set
the project-wide standard in configure.ac (e.g., via AC_PROG_CXX and appropriate
checks or AC_ARG_ENABLE for CXXFLAGS) so files like SegmentationAlpide.cc that
include <format> compile with C++20 enabled.
| float fX[20] = {0}; | ||
| int n = 0; | ||
| fX[n++] = m_event; | ||
| fX[n++] = m_seed; | ||
| fX[n++] = clusX; | ||
| fX[n++] = clusY; | ||
| fX[n++] = clusZ; | ||
| fX[n++] = clusR; | ||
| fX[n++] = clusPhi; | ||
| fX[n++] = clusiPhi; | ||
| fX[n++] = clusT; | ||
| fX[n++] = adcSum; | ||
| fX[n++] = maxAdc; | ||
| fX[n++] = (layerSum / adcSum); | ||
| fX[n++] = maxsector; | ||
| fX[n++] = maxside; | ||
| fX[n++] = nHits; | ||
| fX[n++] = phisize; | ||
| fX[n++] = tsize; | ||
| fX[n++] = lsize; | ||
| m_clusterNT->Fill(fX); | ||
| // } |
There was a problem hiding this comment.
Guard NTuple fill when m_output is false.
m_clusterNT is only created when m_output is true. Unconditionally calling m_clusterNT->Fill will crash when output is disabled.
🛠️ Proposed fix
- float fX[20] = {0};
- int n = 0;
- fX[n++] = m_event;
- fX[n++] = m_seed;
- fX[n++] = clusX;
- fX[n++] = clusY;
- fX[n++] = clusZ;
- fX[n++] = clusR;
- fX[n++] = clusPhi;
- fX[n++] = clusiPhi;
- fX[n++] = clusT;
- fX[n++] = adcSum;
- fX[n++] = maxAdc;
- fX[n++] = (layerSum / adcSum);
- fX[n++] = maxsector;
- fX[n++] = maxside;
- fX[n++] = nHits;
- fX[n++] = phisize;
- fX[n++] = tsize;
- fX[n++] = lsize;
- m_clusterNT->Fill(fX);
+ if (m_output && m_clusterNT)
+ {
+ float fX[20] = {0};
+ int n = 0;
+ fX[n++] = m_event;
+ fX[n++] = m_seed;
+ fX[n++] = clusX;
+ fX[n++] = clusY;
+ fX[n++] = clusZ;
+ fX[n++] = clusR;
+ fX[n++] = clusPhi;
+ fX[n++] = clusiPhi;
+ fX[n++] = clusT;
+ fX[n++] = adcSum;
+ fX[n++] = maxAdc;
+ fX[n++] = (layerSum / adcSum);
+ fX[n++] = maxsector;
+ fX[n++] = maxside;
+ fX[n++] = nHits;
+ fX[n++] = phisize;
+ fX[n++] = tsize;
+ fX[n++] = lsize;
+ m_clusterNT->Fill(fX);
+ }📝 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 fX[20] = {0}; | |
| int n = 0; | |
| fX[n++] = m_event; | |
| fX[n++] = m_seed; | |
| fX[n++] = clusX; | |
| fX[n++] = clusY; | |
| fX[n++] = clusZ; | |
| fX[n++] = clusR; | |
| fX[n++] = clusPhi; | |
| fX[n++] = clusiPhi; | |
| fX[n++] = clusT; | |
| fX[n++] = adcSum; | |
| fX[n++] = maxAdc; | |
| fX[n++] = (layerSum / adcSum); | |
| fX[n++] = maxsector; | |
| fX[n++] = maxside; | |
| fX[n++] = nHits; | |
| fX[n++] = phisize; | |
| fX[n++] = tsize; | |
| fX[n++] = lsize; | |
| m_clusterNT->Fill(fX); | |
| // } | |
| if (m_output && m_clusterNT) | |
| { | |
| float fX[20] = {0}; | |
| int n = 0; | |
| fX[n++] = m_event; | |
| fX[n++] = m_seed; | |
| fX[n++] = clusX; | |
| fX[n++] = clusY; | |
| fX[n++] = clusZ; | |
| fX[n++] = clusR; | |
| fX[n++] = clusPhi; | |
| fX[n++] = clusiPhi; | |
| fX[n++] = clusT; | |
| fX[n++] = adcSum; | |
| fX[n++] = maxAdc; | |
| fX[n++] = (layerSum / adcSum); | |
| fX[n++] = maxsector; | |
| fX[n++] = maxside; | |
| fX[n++] = nHits; | |
| fX[n++] = phisize; | |
| fX[n++] = tsize; | |
| fX[n++] = lsize; | |
| m_clusterNT->Fill(fX); | |
| } |
🤖 Prompt for AI Agents
In `@offline/packages/tpc/Tpc3DClusterizer.cc` around lines 660 - 681, Guard the
NTuple fill so it only runs when output is enabled and the NTuple exists: wrap
the m_clusterNT->Fill(fX) call in a conditional check (e.g., if (m_output &&
m_clusterNT) or if (m_clusterNT) depending on class invariants) in
Tpc3DClusterizer so that m_clusterNT->Fill is not invoked when m_output is false
and m_clusterNT was not created; update the surrounding block accordingly to
avoid null dereference.
13 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
Release Notes
New Features
Improvements
Code Quality
✏️ Tip: You can customize this high-level summary in your review settings.