Fixed calo v2 #6
Conversation
📝 WalkthroughWalkthroughMultiple packages updated: new EMCAL v2 calibration loading and centrality→v2 mapping in DetermineTowerBackground; Micromegas hitkey API extended to include sample indexing; assorted QA, clustering, unpacking, const-correctness, formatting, and build-linker updates across detectors and framework modules. Changes
Sequence DiagramsequenceDiagram
participant InitRun as InitRun()
participant CDB as CDBInterface / CDBTTree
participant Store as Local Memory (_CENTRALITY_V2)
participant Event as process_event()
participant Cent as CentralityInfo
Note over InitRun,CDB: Initialization (when _do_flow == 4)
InitRun->>InitRun: detect _do_flow == 4
alt overwrite path provided
InitRun->>InitRun: read `m_overwrite_average_calo_v2_path`
InitRun-->>Store: parse and populate _CENTRALITY_V2
else use CDB
InitRun->>CDB: query `JET_AVERAGE_CALO_V2_SEPD_PSI2`
CDB-->>InitRun: return calibration tree/values
InitRun-->>Store: populate _CENTRALITY_V2
end
Note over Event,Cent: Per-event processing
Event->>Cent: obtain centrality bin for event
Cent-->>Event: return centrality bin index
Event->>Store: lookup _CENTRALITY_V2[bin]
alt bin in-range
Store-->>Event: calibrated v2 value
Event->>Event: set _v2 and Psi2 from value
else out-of-range
Event->>Event: fallback (zero or default behavior)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @offline/packages/jetbackground/DetermineTowerBackground.cc:
- Around line 65-69: The condition around LoadCalibrations() is inverted:
currently it treats success (EVENT_OK/0) as failure because it uses
!LoadCalibrations(); update the check in DetermineTowerBackground::(where
LoadCalibrations() is called) to explicitly compare the return value against
Fun4AllReturnCodes::EVENT_OK (or check for != EVENT_OK) and only print "Load
calibrations failed." and return Fun4AllReturnCodes::ABORTRUN when the call did
not return EVENT_OK.
- Around line 91-95: The message and behavior are inconsistent: when calibdir is
empty the code prints "defaulting to the identity table!" but then calls
exit(-1), which aborts the process; instead, update the block in
DetermineTowerBackground.cc that checks calibdir.empty() to print a clear error
(remove "defaulting..." text), remove exit(-1), and return an error value so the
caller can handle failure (e.g., return false or a non-zero int from
LoadCalibrations/DetermineTowerBackground initialization function); ensure the
returned value matches the function's signature so the framework can return
early rather than terminating the process.
In @offline/packages/jetbackground/DetermineTowerBackground.h:
- Around line 41-45: Change the SetOverwriteCaloV2 signature to accept a const
reference (const std::string& url) instead of a non-const std::string& so
callers can pass string literals and temporaries; update the function
declaration and definition for SetOverwriteCaloV2 and keep the body that sets
m_overwrite_average_calo_v2 = true and assigns m_overwrite_average_calo_v2_path
= url unchanged.
🧹 Nitpick comments (3)
offline/packages/jetbackground/DetermineTowerBackground.h (1)
16-16: Unused include.The
<array>header is included but nostd::arrayis used in this header or the corresponding implementation. The commit message mentions "Convert an array to a vector", suggesting this include may be a leftover.🔎 Proposed fix
#include <string> #include <vector> -#include <array>offline/packages/jetbackground/DetermineTowerBackground.cc (2)
956-962: Consider returningABORTRUNinstead ofexit(-1)for framework consistency.While the
exit(-1)call follows the existing pattern in this file (e.g., lines 147, 157, 165), it bypasses the Fun4All framework's cleanup. The PR objective mentions "Return early if LoadCalibrations fails" suggesting a preference for graceful handling.🔎 Proposed fix (optional)
if (!centinfo) { std::cout << "DetermineTowerBackground::process_event: FATAL, CentralityInfo does not exist, cannot extract centrality with do_flow = " << _do_flow << std::endl; - exit(-1); + return Fun4AllReturnCodes::ABORTRUN; }
603-607: Same suggestion: returnABORTRUNinstead ofexit(-1).For consistency with the framework error handling pattern.
🔎 Proposed fix (optional)
if (!epmap) { std::cout << "DetermineTowerBackground::process_event: FATAL, EventplaneinfoMap does not exist, cannot extract sEPD flow with do_flow = " << _do_flow << std::endl; - exit(-1); + return Fun4AllReturnCodes::ABORTRUN; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
offline/packages/jetbackground/DetermineTowerBackground.ccoffline/packages/jetbackground/DetermineTowerBackground.hoffline/packages/jetbackground/Makefile.am
🧰 Additional context used
🧬 Code graph analysis (1)
offline/packages/jetbackground/DetermineTowerBackground.h (1)
offline/packages/jetbackground/DetermineTowerBackground.cc (2)
LoadCalibrations(76-113)LoadCalibrations(76-76)
🔇 Additional comments (6)
offline/packages/jetbackground/Makefile.am (1)
27-28: LGTM!The new linker dependencies (
-lcentrality_io,-lcdbobjects,-lffamodules) correctly support the new includes forCentralityInfo,CDBTTree, andCDBInterfaceused in the centrality-based calibration workflow.Also applies to: 38-38
offline/packages/jetbackground/DetermineTowerBackground.h (1)
63-68: LGTM!The new private members are properly declared with appropriate default initializers. The
LoadCalibrations()method returnsintmatching the Fun4All return code pattern used inInitRun.offline/packages/jetbackground/DetermineTowerBackground.cc (4)
17-19: LGTM!The new includes align with the new centrality-based calibration workflow requirements.
599-624: LGTM!The
_do_flow == 3 || _do_flow == 4condition correctly extracts Ψ₂ from sEPD for both modes, while the subsequent condition on line 626 (_do_flow >= 1 && _do_flow < 4) appropriately excludesdo_flow == 4from the calorimeter-based v2 calculation—since v2 is instead obtained from the centrality lookup table for that mode.
964-976: Verify centrality bin boundary condition.The condition
centrality_bin > 0 && centrality_bin < 95excludes bin 0 (most central, 0-1%) and bins 95-99. While excluding peripheral bins is reasonable, excluding the most central bin (0) may not be intentional.If bin 0 should be included, consider:
- if (centrality_bin > 0 && centrality_bin < 95) + if (centrality_bin >= 0 && centrality_bin < 95)Also, the
_CENTRALITY_V2vector has 100 elements (indices 0-99), so accessing up to index 94 is safe, but this leaves indices 95-99 unused. Is this the intended behavior?
100-108: CDBTTree::LoadCalibrations() returnsvoid, not an error code.The method signature is
void LoadCalibrations();and does not return a value to check. On failure, the method terminates the program viaexit()orgSystem->Exit(1), so checking a return value is not applicable here.Likely an incorrect or invalid review comment.
| if (calibdir.empty()) | ||
| { | ||
| std::cout << "Could not find and load histograms for EMCAL LUTs! defaulting to the identity table!" << std::endl; | ||
| exit(-1); | ||
| } |
There was a problem hiding this comment.
Misleading error message and inconsistent error handling.
The error message says "defaulting to the identity table!" but then immediately calls exit(-1). This is inconsistent with the PR objective of "Return early if LoadCalibrations fails" since exit(-1) terminates the entire process rather than returning gracefully.
Consider returning an error code instead to allow the framework to handle the failure:
🔎 Proposed fix
if (calibdir.empty())
{
- std::cout << "Could not find and load histograms for EMCAL LUTs! defaulting to the identity table!" << std::endl;
- exit(-1);
+ std::cout << "DetermineTowerBackground::LoadCalibrations: Could not find calibration path for " << m_calibName << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @offline/packages/jetbackground/DetermineTowerBackground.cc around lines
91-95, The message and behavior are inconsistent: when calibdir is empty the
code prints "defaulting to the identity table!" but then calls exit(-1), which
aborts the process; instead, update the block in DetermineTowerBackground.cc
that checks calibdir.empty() to print a clear error (remove "defaulting..."
text), remove exit(-1), and return an error value so the caller can handle
failure (e.g., return false or a non-zero int from
LoadCalibrations/DetermineTowerBackground initialization function); ensure the
returned value matches the function's signature so the framework can return
early rather than terminating the process.
| void SetOverwriteCaloV2(std::string &url) | ||
| { | ||
| m_overwrite_average_calo_v2 = true; | ||
| m_overwrite_average_calo_v2_path = url; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use const std::string& for input parameter.
Taking a non-const reference prevents callers from passing string literals or temporaries (e.g., SetOverwriteCaloV2("/some/path")).
🔎 Proposed fix
- void SetOverwriteCaloV2(std::string &url)
+ void SetOverwriteCaloV2(const std::string &url)
{
m_overwrite_average_calo_v2 = true;
m_overwrite_average_calo_v2_path = url;
}🤖 Prompt for AI Agents
In @offline/packages/jetbackground/DetermineTowerBackground.h around lines
41-45, Change the SetOverwriteCaloV2 signature to accept a const reference
(const std::string& url) instead of a non-const std::string& so callers can pass
string literals and temporaries; update the function declaration and definition
for SetOverwriteCaloV2 and keep the body that sets m_overwrite_average_calo_v2 =
true and assigns m_overwrite_average_calo_v2_path = url unchanged.
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
…idy-57 Clang tidy 57
- consolidated all key setters/getters with masks
Docstrings generation was requested by @blackcathj. * sPHENIX-Collaboration#4095 (comment) The following files were modified: * `offline/packages/CaloReco/PhotonClusterBuilder.cc`
add e22 showershape for PhotonCluster
…ion/coderabbitai/docstrings/34e66d1 📝 Add docstrings to `photonclass`
…s-decoding-update Micromegas decoding update
…ticle Apocalypse silicon pooling: redux
…ion/revert-4096-coderabbitai/docstrings/34e66d1 Revert "📝 Add docstrings to `photonclass`"
Tonight I am going to have fried rabbit... I added the integer interface so we can just use the packet id as nodename without having to convert to strings (that's done inside the code - that's the only thing this interface does). Sorry about that
Fix bugs in CaloValid to select correct MB + 10 cm trigger
…ap_name feat: make cluster map setters for job c
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc (1)
780-797: FillIntt does not erase processed entries from the map, unlike FillTpc and FillMicromegas.The main processing loop (lines 780-797) adds hits to the container but does not erase the BCO entries from
m_InttRawHitMap. The pre-loop cleanup (lines 685-711) only removes entries older than the valid range, leaving processed entries to accumulate in the map.FillTpc (lines 1164-1190) and FillMicromegas (line 1090) both erase entries immediately as they process them, whereas FillIntt retains processed entries until they become old enough for the next call's pre-loop cleanup or are cleared at destruction. This architectural inconsistency allows processed entries to persist across multiple event calls, creating unnecessary memory overhead.
🤖 Fix all issues with AI agents
In @offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc:
- Line 1367: The ternary initializing ref_bco_minus_range is wrong: it returns
m_mvtx_negative_bco when m_RefBCO < m_mvtx_negative_bco, which allows underflow
and is semantically incorrect; change the expression to return 0 in that case
and m_RefBCO - m_mvtx_negative_bco otherwise (i.e., mirror the check used at
line 839 and the if-style pattern used in FillInttPool). Locate the
initialization of ref_bco_minus_range in
Fun4AllStreamingInputManager::(constructor or method where it appears) and
replace the ternary so underflow is prevented by returning 0 when m_RefBCO <
m_mvtx_negative_bco.
- Around line 974-976: The computation of lower_limit in
Fun4AllStreamingInputManager.cc can underflow when m_mvtx_is_triggered is false
because select_crossings - m_mvtx_bco_range - m_mvtx_negative_bco is done on
uint64_t; change the non-triggered branch to clamp at 0: when
m_mvtx_is_triggered is false, check if select_crossings <= (m_mvtx_bco_range +
m_mvtx_negative_bco) and set lower_limit = 0, otherwise set lower_limit =
select_crossings - m_mvtx_bco_range - m_mvtx_negative_bco; leave the triggered
branch and upper_limit logic as-is to avoid wrapping.
In @offline/packages/jetbackground/DetermineTowerBackground.cc:
- Around line 76-108: In DetermineTowerBackground::LoadCalibrations(), remove
the call to exit(-1) and instead return an appropriate Fun4All error code on
failure (e.g., Fun4AllReturnCodes::ABORTEVENT), and ensure CDBTTree is validated
before use: after creating cdbtree_calo_v2 check it is non-null and that its
calibration data is loaded (validate return from
cdbtree_calo_v2->LoadCalibrations or check the number of entries via an
appropriate method), obtain the actual number of centrality bins from the tree
(rather than assuming 100), resize _CENTRALITY_V2 to that count (or take
min(actual_count,100) if 100 is desired), and only call
cdbtree_calo_v2->GetFloatValue(icent, "jet_calo_v2") for valid indices; on any
failure clean up (delete cdbtree_calo_v2) and return the error code instead of
calling exit().
- Around line 537-619: Replace the direct process termination calls in the
flow-extraction branch: where the PHG4TruthInfo check currently does "return -1"
change it to return the appropriate Fun4AllReturnCodes constant (e.g.,
Fun4AllReturnCodes::ABORTEVENT) and where the EventplaneinfoMap check calls
exit(-1) change it to return the same Fun4AllReturnCodes value; keep the rest of
the logic that sets _Psi2 and _is_flow_failure unchanged (symbols to edit:
process_event, _do_flow branches, PHG4TruthInfoContainer check,
EventplaneinfoMap check, _Psi2, _is_flow_failure). Also fix mixed tabs/spaces
indentation in this block (run clang-format) so formatting is consistent.
In @offline/packages/micromegas/MicromegasDefs.cc:
- Around line 69-74: The genHitKey function accepts strip as uint16_t but only
uses 8 bits; change the strip parameter type to uint8_t in the genHitKey
signature to reflect the actual data width and enable type-level validation,
update any call sites that pass larger types if needed, and keep the existing
masking/shift logic using kBitShiftStrip (or remove the explicit 0xFFU mask if
you prefer since uint8_t already constrains the value) while ensuring the
shifts/casts to TrkrDefs::hitkey remain correct.
In @offline/packages/mvtx/MvtxHitPruner.cc:
- Around line 158-165: MvtxHitPruner::process_event currently calls
m_hits->findHitSet(hitsetkey) and dereferences the returned hitset without
checking for nullptr; add a null check right after the call to verify hitset is
non-null before using hitset->size() or copying it, and if null either skip
processing this hitset (continue) or log a warning under Verbosity() instead of
dereferencing; update both the Verbosity() logging and any subsequent use of
hitset (e.g., copying logic) to occur only when hitset is not null.
In @offline/packages/PHGenFitPkg/PHGenFit/Fitter.cc:
- Around line 54-57: The constructor currently does new TGeoManager("Default",
...) and then calls TGeoManager::Import(tgeo_file_name.data()), leaving the
created manager unused; change the constructor to assign the Import result to
_tgeo_manager (i.e. _tgeo_manager = TGeoManager::Import(...)) so the manager
used is the one Import returns, matching the other Fitter constructors that
accept a TGeoManager*; remove the unused new allocation and ensure destructor
cleanup logic targets the manager returned by Import via _tgeo_manager.
In @offline/packages/tpc/LaserClusterizer.cc:
- Around line 210-214: The code accesses regions[0] without checking that
regions contains any elements, which can cause undefined behavior; update the
LaserClusterizer.cc logic around clusHits.clear() so you first verify regions is
non-empty (e.g., if (!regions.empty()) or if (regions.size() > 0)) before
iterating over regions[0], and handle the empty-case explicitly (leave clusHits
empty or apply the intended fallback) to avoid out-of-bounds access.
- Around line 963-969: The pthread_create call can fail but the code only logs
and leaves thread_pair in the container; update the error path for
pthread_create (the rc != 0 branch) so you either remove the corresponding
thread_pair entry from the vector (or set a flag/member on thread_pair to mark
it invalid) and avoid later pthread_join on that entry; ensure any resources
allocated for thread_pair.data are cleaned up and that the error is logged with
context (include rc and which ProcessModule instance) so join/cleanup loops skip
invalid thread_pair entries.
- Around line 69-70: Remove the unused global arrays layerMins and layerMaxes:
locate the declarations of int layerMins[3] and int layerMaxes[3] and delete
them from the file, ensuring no other code references these symbols; if any
future logic needs their values, replace usage with local constants or compute
values where needed rather than keeping unused globals.
In @offline/QA/Calorimeters/CaloValid.cc:
- Around line 338-341: The change gates accumulation of
totalcemc/totalihcal/totalohcal by the tower quality flag isGood, which likely
unintentionally changes these totals from "sum of all tower energies" to "sum of
good tower energies"; revert or make explicit: either remove the isGood
conditional when updating totalcemc/totalihcal/totalohcal so totals always
include offlineenergy, or if the new behavior is intended, update downstream
uses (the correlation plots and the clustering threshold check that reference
these totals) to expect "good-only" sums and rename variables to reflect that
(e.g., goodTotalCEMC) and ensure clustering logic uses the correct total
variable; update references to totalcemc/totalihcal/totalohcal in the
correlation plotting and clustering threshold code accordingly.
🧹 Nitpick comments (17)
offline/packages/tpc/TpcCombinedRawDataUnpackerDebug.cc (2)
537-537: Prefer direct ROOT axis call; also tighten style.
You can drop the extra parentheses/whitespace and maketimebinconst for clarity.Proposed tweak
- double timebin = ( hist2d->GetXaxis())->GetBinCenter(binx); + const double timebin = hist2d->GetXaxis()->GetBinCenter(binx);
695-696: Clamp is correct; consider a cleanerstd::maxform.
Current code works, but the explicit template arg + integer literal is a bit noisy.Proposed tweak
- nuadc = std::max<float>(nuadc, 0); + nuadc = std::max(0.0f, nuadc); hitr->second->setAdc(nuadc);offline/packages/tpc/TpcDistortionCorrection.cc (1)
18-18: LGTM! Good refactor of the function signature.The changes improve the code quality:
constexprfunctions are implicitlyinlinein C++, so removing the redundantinlinekeyword is appropriate. Additionally, changing the parameter fromconst T&toT(pass-by-value) is the preferred approach for template functions operating on fundamental numeric types, as it avoids unnecessary indirection.offline/packages/tpc/TpcLoadDistortionCorrection.cc (1)
61-61: LGTM! Explicit pointer notation improves readability.Adding explicit
*to theautodeclarations makes it immediately clear that these variables are pointers, improving code readability and maintainability. The behavior remains unchanged.Also applies to: 77-77, 82-82, 87-87
offline/packages/tpc/TpcRawDataTree.cc (1)
53-53: Optional: Fix typo in variable name.The variable name
nWaveormInFrameappears to be missing an 'f' and should likely benWaveformInFrame. This typo is used consistently throughout the file (lines 53, 61, 147), so fixing it would require updating all occurrences for consistency.offline/packages/tpc/TpcClusterMover.cc (1)
161-191: LGTM! Const qualifier correctly added.The function appropriately marked
constsince it only reads_verbosityand writes to output parameters without modifying member state.Note: The similar
PHTpcClusterMover::get_circle_circle_intersectioninoffline/packages/trackreco/PHTpcClusterMover.cclacks thisconstqualifier. Consider aligning it for consistency across the codebase.offline/packages/mvtx/SegmentationAlpide.cc (1)
8-10: Use'\n'instead ofstd::endlto avoid unnecessary flushes.The code is consistent with the codebase's existing use of
std::formatelsewhere (widely adopted across multiple packages). The only actionable suggestion is to replacestd::endlwith'\n'on lines 14, 16, and 18 to improve performance by avoiding buffer flushes after each output line.Optional refactor
- << std::endl; + << '\n'; - << std::endl; + << '\n'; - << std::endl; + << '\n';offline/framework/fun4allraw/SingleMicromegasPoolInput_v2.cc (2)
936-936: Remove double semicolon.Minor style issue: there's a double semicolon after the method call.
♻️ Proposed fix
- const auto predicted = bco_matching_information.get_predicted_fee_bco(gtm_bco);; + const auto predicted = bco_matching_information.get_predicted_fee_bco(gtm_bco);
965-965: Fix typo in comment.The comment contains a typo: "waverform" should be "waveform".
♻️ Proposed fix
- // skip the waverform + // skip the waveformoffline/packages/mvtx/MvtxHitPruner.cc (1)
54-68: Make begin() and end() methods const-qualified.The
begin()andend()methods don't modify the object's state and should be callable on const instances ofrange_adaptor. Add theconstqualifier to both methods.♻️ Proposed fix
- const typename T::first_type& begin() { return m_range.first; } - const typename T::second_type& end() { return m_range.second; } + const typename T::first_type& begin() const { return m_range.first; } + const typename T::second_type& end() const { return m_range.second; }offline/packages/tpc/Tpc3DClusterizer.cc (2)
554-559: Consider removing explicit template parameters from std::min/std::max.The explicit template parameters (
<double>,<float>) may be unnecessary and could mask type mismatches:
- Line 554-555:
phiis alreadydouble, andiphiminwas initialized asint(6666)- Line 558-559:
tbinisint(fromcoords[2]), but template specifies<float>The compiler can deduce the correct type, and explicit parameters may hide unintended conversions.
♻️ Proposed simplification
- iphimin = std::min<double>(phi, iphimin); - iphimax = std::max<double>(phi, iphimax); + iphimin = std::min(phi, static_cast<double>(iphimin)); + iphimax = std::max(phi, static_cast<double>(iphimax)); ilaymin = std::min(lay, ilaymin); ilaymax = std::max(lay, ilaymax); - itmin = std::min<float>(tbin, itmin); - itmax = std::max<float>(tbin, itmax); + itmin = std::min(static_cast<float>(tbin), itmin); + itmax = std::max(static_cast<float>(tbin), itmax);Alternatively, initialize the variables with the correct type from the start (lines 523-528).
523-528: Consider correcting variable types to match their usage.The boundary tracking variables have type mismatches with the values they store:
iphimin/iphimax: Declared asintbut will storephivalues (line 554-555), which aredoublefromlayergeom->get_phi()itmin/itmax: Declared asfloatbut will storetbinvalues (line 558-559), which areintfromcoords[2]This relates to the explicit template parameters in the std::min/std::max calls at lines 554-559.
♻️ Proposed fix
- int iphimin = 6666; - int iphimax = -1; + double iphimin = 6666.0; + double iphimax = -1.0; int ilaymin = 6666; int ilaymax = -1; - float itmin = 66666666.6; - float itmax = -6666666666.6; + int itmin = 66666666; + int itmax = -66666666;Then remove the explicit template parameters from std::min/std::max at lines 554-559.
offline/packages/tpc/LaserClusterizer.cc (4)
128-146: Inconsistent const-correctness.
phiFunctionandtimeFunctionstill usedouble *parwhilelayerFunctionwas updated to useconst double *par. All three functions only read fromparand should have consistent const qualifiers.♻️ Suggested fix
- double phiFunction(double *x, double *par) + double phiFunction(double *x, const double *par) { if (par[2] < 0.0) { return 0.0; } return par[0] * TMath::Gaus(x[0], par[1], par[2], false); } - double timeFunction(double *x, double *par) + double timeFunction(double *x, const double *par) { if (par[2] < 0.0) { return 0.0; } double g = TMath::Gaus(x[0], par[1], par[2], true); double cdf = 1 + TMath::Erfc(par[3] * (x[0] - par[1]) / (sqrt(2.0) * par[2])); return par[0] * g * cdf; }
730-731: Nested conditional operator acknowledged but still hard to read.The
NOLINTNEXTLINEcomment suppresses the warning, but consider extracting the module calculation to a helper function for better readability.♻️ Optional: Extract module calculation
// Helper function at namespace scope int getModuleFromLayer(int layer) { if (layer < 23) return 1; if (layer < 39) return 2; return 3; } // Usage at line 731: std::cout << "working on cluster " << my_data->cluster_vector.size() << " side: " << my_data->side << " sector: " << my_data->sector << " module: " << getModuleFromLayer(layer) << std::endl;
567-627: Consider extracting common cluster property assignments.The three conditional branches (lines 567-586, 588-606, 609-627) have significant code duplication. Consider extracting the common property assignments to reduce maintenance burden.
295-338: Inefficient duplicate detection using linear search.The duplicate detection for
usedLayer,usedIPhi, andusedITuses linear search through vectors. For larger clusters, this results in O(n²) complexity. Consider usingstd::setorstd::unordered_setfor O(log n) or O(1) lookup.♻️ Suggested optimization
Replace the vectors with sets at declaration (around line 261-263):
std::set<float> usedLayer; std::set<float> usedIPhi; std::set<float> usedIT;Then simplify the insertion (replacing lines 295-338):
usedLayer.insert(coords[0]); usedIPhi.insert(coords[1]); usedIT.insert(coords[2]);Note: You'll need to adjust the sorting and access patterns later in the function since
std::setis already sorted.offline/packages/micromegas/MicromegasClusterizer.cc (1)
166-166: Remove the stray comment.Line 166 contains
//ma, which appears to be a leftover comment or typo.🧹 Proposed cleanup
- //ma assert(geonode);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
offline/QA/Calorimeters/CaloValid.ccoffline/QA/Calorimeters/CaloValid.hoffline/QA/Tracking/MicromegasClusterQA.ccoffline/QA/Tracking/MicromegasClusterQA.hoffline/framework/fun4allraw/Fun4AllStreamingInputManager.ccoffline/framework/fun4allraw/SingleMicromegasPoolInput_v2.ccoffline/framework/fun4allraw/SingleMicromegasPoolInput_v2.hoffline/framework/fun4allraw/SingleMvtxPoolInput.ccoffline/packages/CaloReco/PhotonClusterBuilder.ccoffline/packages/PHGenFitPkg/PHGenFit/Fitter.ccoffline/packages/PHGenFitPkg/PHGenFit/Track.ccoffline/packages/jetbackground/DetermineTowerBackground.ccoffline/packages/micromegas/MicromegasClusterizer.ccoffline/packages/micromegas/MicromegasCombinedDataDecoder.ccoffline/packages/micromegas/MicromegasCombinedDataDecoder.hoffline/packages/micromegas/MicromegasDefs.ccoffline/packages/micromegas/MicromegasDefs.hoffline/packages/mvtx/CylinderGeom_Mvtx.ccoffline/packages/mvtx/CylinderGeom_Mvtx.hoffline/packages/mvtx/MvtxClusterPruner.ccoffline/packages/mvtx/MvtxClusterizer.ccoffline/packages/mvtx/MvtxClusterizer.hoffline/packages/mvtx/MvtxHitPruner.ccoffline/packages/mvtx/SegmentationAlpide.ccoffline/packages/tpc/LaserClusterizer.ccoffline/packages/tpc/LaserEventIdentifier.ccoffline/packages/tpc/Tpc3DClusterizer.ccoffline/packages/tpc/TpcClusterMover.ccoffline/packages/tpc/TpcClusterMover.hoffline/packages/tpc/TpcClusterizer.ccoffline/packages/tpc/TpcCombinedRawDataUnpacker.ccoffline/packages/tpc/TpcCombinedRawDataUnpackerDebug.ccoffline/packages/tpc/TpcDistortionCorrection.ccoffline/packages/tpc/TpcLoadDistortionCorrection.ccoffline/packages/tpc/TpcRawDataTree.ccoffline/packages/tpc/TpcRawWriter.ccoffline/packages/tpc/TpcSimpleClusterizer.ccoffline/packages/tpc/TrainingHits.ccoffline/packages/trackreco/PHActsTrkFitter.ccoffline/packages/trackreco/PHActsTrkFitter.hoffline/packages/trackreco/PHSiliconTpcTrackMatching.ccoffline/packages/trackreco/PHSiliconTpcTrackMatching.hoffline/packages/trackreco/PHSimpleVertexFinder.hoffline/packages/trackreco/PHTpcDeltaZCorrection.h
✅ Files skipped from review due to trivial changes (2)
- offline/packages/trackreco/PHActsTrkFitter.cc
- offline/packages/tpc/TpcRawWriter.cc
🧰 Additional context used
🧬 Code graph analysis (13)
offline/packages/micromegas/MicromegasDefs.h (1)
offline/packages/micromegas/MicromegasDefs.cc (6)
genHitKey(69-74)genHitKey(69-69)getStrip(77-81)getStrip(77-77)getSample(84-88)getSample(84-84)
offline/packages/trackreco/PHTpcDeltaZCorrection.h (2)
offline/packages/trackreco/PHActsTrkFitter.h (1)
setTrkrClusterContainerName(145-145)offline/packages/trackreco/PHSimpleVertexFinder.h (1)
setTrkrClusterContainerName(57-57)
offline/packages/tpc/TpcClusterMover.h (2)
offline/packages/tpc/TpcClusterMover.cc (2)
get_circle_circle_intersection(161-191)get_circle_circle_intersection(161-161)offline/packages/trackreco/PHTpcClusterMover.cc (2)
get_circle_circle_intersection(351-379)get_circle_circle_intersection(351-351)
offline/packages/tpc/TpcCombinedRawDataUnpackerDebug.cc (1)
offline/framework/fun4allraw/SingleMicromegasPoolInput_v2.h (1)
gtm_bco(240-240)
offline/packages/mvtx/MvtxClusterizer.h (1)
offline/packages/mvtx/MvtxClusterizer.cc (4)
are_adjacent(72-92)are_adjacent(72-74)are_adjacent(94-113)are_adjacent(94-94)
offline/framework/fun4allraw/SingleMicromegasPoolInput_v2.cc (1)
offline/framework/fun4allraw/SingleMicromegasPoolInput_v2.h (4)
m_evaluation_tree(261-264)fee_id(225-225)fee_bco(249-249)gtm_bco(240-240)
offline/packages/trackreco/PHActsTrkFitter.h (7)
offline/packages/trackreco/PHSiliconTpcTrackMatching.h (1)
set_pp_mode(140-140)offline/packages/trackreco/PHSimpleVertexFinder.h (1)
set_pp_mode(58-58)offline/packages/trackreco/MakeSourceLinks.h (2)
set_pp_mode(49-49)ignoreLayer(51-51)offline/packages/trackreco/PHCosmicsTrkFitter.h (1)
ignoreLayer(95-95)offline/packages/trackreco/MakeActsGeometry.h (1)
Acts(41-44)offline/packages/trackreco/PHActsTrkFitter.cc (1)
tracks(728-728)offline/packages/trackreco/PHCosmicsTrkFitter.cc (1)
tracks(543-543)
offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc (2)
offline/framework/fun4allraw/Fun4AllStreamingInputManager.h (3)
m_RefBCO(110-110)m_mvtx_negative_bco(118-118)m_mvtx_bco_range(117-117)offline/packages/mvtx/MvtxCombinedRawDataDecoder.h (1)
m_mvtx_is_triggered(85-86)
offline/packages/tpc/Tpc3DClusterizer.cc (2)
offline/packages/trackbase/LaserClusterv2.h (1)
getLayer(57-58)offline/packages/tpc/LaserClusterizer.h (1)
m_tdriftmax(73-73)
offline/packages/mvtx/CylinderGeom_Mvtx.h (1)
offline/packages/mvtx/CylinderGeom_Mvtx.cc (7)
CylinderGeom_Mvtx(13-72)get_pixel_X_from_pixel_number(195-198)get_pixel_X_from_pixel_number(195-195)get_pixel_Z_from_pixel_number(200-203)get_pixel_Z_from_pixel_number(200-200)get_pixel_number_from_xbin_zbin(205-208)get_pixel_number_from_xbin_zbin(205-205)
offline/packages/micromegas/MicromegasCombinedDataDecoder.cc (1)
offline/packages/micromegas/MicromegasClusterizer.cc (2)
first(228-229)first(228-228)
offline/packages/mvtx/MvtxClusterPruner.cc (1)
offline/packages/mvtx/MvtxHitPruner.cc (4)
m_range(62-62)m_range(63-63)range_adaptor(58-61)range_adaptor(58-58)
offline/packages/mvtx/CylinderGeom_Mvtx.cc (1)
offline/packages/mvtx/CylinderGeom_Mvtx.h (1)
get_NZ(71-86)
🔇 Additional comments (93)
offline/packages/tpc/TpcCombinedRawDataUnpackerDebug.cc (2)
37-37: Good: add<algorithm>forstd::min/std::max.
This keeps the later changes self-contained and avoids relying on indirect includes.
238-239: Nice simplification:std::min/std::maxfor BCO range tracking.
Equivalent behavior with less branching; types (uint64_t) match cleanly.offline/packages/tpc/LaserEventIdentifier.cc (1)
133-133: Good improvement to unsigned-safe bitmask operation.Changing from
(1<<14)to(1U<<14U)is a best practice for bitwise operations. Using unsigned literals prevents potential undefined behavior from signed integer shifts and makes the bitmask intent clearer.offline/packages/tpc/TpcRawDataTree.cc (1)
64-64: LGTM! Good modernization using.data().The change from
&m_adcSamples[0]tom_adcSamples.data()is a best-practice modernization. The.data()method is the C++11 standard way to access a vector's underlying array and is more explicit. Since the vector is pre-sized (line 26) and bounds-checked (line 208), the pointer remains valid throughout the TTree's lifetime.offline/framework/fun4allraw/SingleMvtxPoolInput.cc (5)
4-4: LGTM: Include order adjustment.The repositioning of
MvtxRawDefs.hhas no functional impact.
32-33: LGTM: Formatting improvement.The constructor initializer list formatting is clearer without changing behavior.
165-168: Verify the BCO threshold logic change.The skip condition changed from
strb_bco < minBCO - m_NegativeBcotostrb_bco < minBCO, which means strobes in the range[minBCO - m_NegativeBco, minBCO)will now be processed instead of being skipped. Given thatm_NegativeBcocan be as large as 1000 (line 461) or 120 (line 466), this significantly alters which strobes are included in processing.Confirm this change aligns with the intended BCO windowing behavior and doesn't cause issues with strobe timing or data quality.
210-229: LGTM: Improved readability.The expansion of the compressed ternary expression into an explicit if-else structure significantly improves code readability while preserving the same logic for L1 trigger-to-strobe alignment.
463-467: Verify the m_NegativeBco parameter adjustment.The negative BCO window was reduced from 500 to 120 for strobe widths between 9-11 microseconds. This narrows the backward-looking BCO range by 76%, which could improve performance but risks missing data if the window becomes too tight.
Confirm this parameter adjustment has been validated with data to ensure no hits are lost at the BCO window boundaries.
offline/packages/PHGenFitPkg/PHGenFit/Track.cc (4)
63-63: LGTM: Constructor initialization improvements.Moving initialization to the member initializer list is a C++ best practice that ensures direct initialization rather than default construction followed by assignment.
Also applies to: 80-84
193-194: LGTM: Simplified control flow.Removing redundant
elsebranches afterreturnstatements follows the early-return pattern and improves readability.Also applies to: 240-241, 359-360, 765-765
381-381: LGTM: Idiomatic empty check.Using
.empty()instead of.size() == 0is the preferred C++ idiom for checking empty containers.
575-575: LGTM: Explicit pointer syntax.Using
auto*in range-based for loops makes it explicit that the code is iterating over pointers, improving readability and intent.Also applies to: 594-594
offline/packages/tpc/TpcClusterMover.h (1)
30-30: LGTM! Good const-correctness improvement.The
constqualifier is appropriate since this method only reads_verbosityand writes to output parameters without modifying any member state. The declaration now correctly matches the definition.offline/packages/tpc/TpcClusterMover.cc (2)
18-25: LGTM!The
[[maybe_unused]]attribute appropriately suppresses compiler warnings for this debug utility function.
27-33: Good refactor to use member initializer list.Moving initialization to the member initializer list is a C++ best practice. The initialization order is safe because the radius members (
inner_tpc_min_radius, etc.) are declared before the spacing members in the class, so they're initialized with their default values first via in-class initializers.offline/packages/tpc/TrainingHits.cc (1)
3-15: LGTM! Good refactor to use member initializer list.Moving member initialization from the constructor body to the member initializer list follows C++ best practices and is more efficient. The
v_adc.fill(0)call is correctly kept in the body since it's a method invocation rather than direct initialization.Note: The initializer list order (radius, phi, z, ..., cluskey) differs from the declaration order in the header (v_adc, radius, phi, ..., cluskey), but this does not cause functional issues since
v_adchas an in-class initializer and the remaining members are simple POD types without interdependencies.offline/packages/mvtx/MvtxClusterizer.h (1)
68-69: LGTM: Const-correctness improvement.The addition of
constqualifiers to bothare_adjacentmethod overloads is appropriate, as these methods only check adjacency conditions without modifying object state.offline/packages/mvtx/MvtxClusterizer.cc (5)
66-69: LGTM: Modern C++ improvement with constexpr.Making the
squaretemplate functionconstexprenables compile-time evaluation and is appropriate for this simple arithmetic function.
72-92: LGTM: Const-correctness applied.The
constqualifier correctly reflects that this method does not modify object state. The implementation is consistent with the header declaration.
94-113: LGTM: Const-correctness applied.The
constqualifier is correctly applied to this overload, consistent with the firstare_adjacentoverload.
377-377: LGTM: Safer container access pattern.Using
component.data()instead of&component[0]is the idiomatic C++11+ way to access underlying array data and makes the intent more explicit.
679-679: LGTM: Consistent with line 377.Using
component.data()provides safer and more idiomatic container access.offline/packages/mvtx/MvtxClusterPruner.cc (1)
1-241: LGTM: Formatting and style improvements.The changes in this file are purely formatting and style-related, including consistent spacing, explicit constructor qualification, and improved readability. No functional logic has been altered.
offline/packages/mvtx/CylinderGeom_Mvtx.h (3)
16-16: LGTM: Parameter naming consistency.Renaming
in_Nstavestoin_N_stavesimproves consistency with the member variableN_stavesand follows a more uniform naming convention.
34-34: LGTM: Modern C++ destructor style.Using
= defaultfor the destructor is preferred modern C++ style and allows the compiler to generate optimal code.
56-60: LGTM: Const-correctness for getter methods.Adding
constqualifiers to these getter methods correctly reflects that they do not modify object state and enables their use on const instances.offline/packages/mvtx/CylinderGeom_Mvtx.cc (2)
195-208: LGTM: Const-correctness matching header declarations.The
constqualifiers on these getter methods match the updated header declarations and correctly reflect that these methods do not modify object state.
129-180: LGTM: Explicit namespace qualification.Using explicit
std::qualification instead of relying onusing namespace stdis best practice, avoiding namespace pollution and improving code clarity.offline/packages/tpc/TpcCombinedRawDataUnpacker.cc (2)
78-78: Change is correct:GetSingleFloatValueacceptsstd::stringdirectly.The method signature in
CDBTTree.hisfloat GetSingleFloatValue(const std::string &name, int verbose = 0);, so passingname.str()without.c_str()is appropriate and eliminates an unnecessary type conversion. This follows modern C++ best practices.
60-60: No changes needed—both modifications are correct.Line 60's
auto *cdbexplicitly marks the variable as a pointer, making the type clearer.CDBInterface::instance()returns a pointer, confirmed by line 61's usagecdb->getUrl().Line 78's change from
name.str().c_str()toname.str()is valid becauseGetSingleFloatValue()acceptsconst std::string&(per CDBTTree.h:33), so the temporary string binds correctly. The.c_str()call was unnecessary.offline/framework/fun4allraw/SingleMicromegasPoolInput_v2.h (1)
233-234: LGTM!The addition of the
matchedfield with proper default initialization is clean and follows the existing pattern used foris_heartbeat.offline/framework/fun4allraw/SingleMicromegasPoolInput_v2.cc (3)
610-610: LGTM!The new "matched" branch is correctly added to the evaluation tree, consistent with the Waveform struct modification.
916-922: LGTM!Setting up common waveform fields before the BCO matching attempt is logical and prepares the data structure for subsequent evaluation recording in both matched and unmatched branches.
931-967: Control flow correctly implements matched/unmatched waveform handling.The implementation properly differentiates between matched and unmatched waveforms:
- Matched waveforms are recorded in the evaluation tree and continue through the normal storage pipeline
- Unmatched waveforms are recorded in the evaluation tree and QA counters but are skipped from storage
The logic is sound and aligns with the PR objectives.
offline/packages/PHGenFitPkg/PHGenFit/Fitter.cc (3)
204-249: LGTM!This constructor correctly receives the
TGeoManageras a parameter and uses enum-based fitter selection, avoiding the initialization issue present in the string-based constructor. The logic is clean and straightforward.
290-310: String comparison refactoring approved.The change from
.compare()to==is cleaner and more readable. This constructor properly handles the case where an invalid fitter choice is provided by setting_fittertonullptrand logging an error.
76-95: Duplicate branches appear intentional as default fallback.The string comparison improvements look good. The duplicate
KalmanFitter()instantiation at lines 80-83 and 92-95 is acknowledged by the NOLINT comments and serves as a defensive default: when an unrecognizedfitter_choiceis provided, the code falls back toKalmanFitter. This pattern is consistent across multiple constructors in the file. No additional logic appears to be missing—this is a reasonable safeguard to ensure_fitteris always initialized.offline/packages/CaloReco/PhotonClusterBuilder.cc (1)
564-572: Review comment is incorrect — no out-of-bounds vulnerability exists.
RawClusterv1::get_shower_shapes()has only two return paths: an empty vector (iftowermap.empty() || maxtowerE == 0) or a vector with exactly 13 elements. Indices [8..11] accessed on line 569 are guaranteed to exist whenever the vector is non-empty, and theif (showershape.empty())check at line 278 is sufficient protection. No guard or modification is needed.Likely an incorrect or invalid review comment.
offline/packages/mvtx/MvtxHitPruner.cc (4)
70-78: LGTM: Clean formatting improvements.The constructor and
InitRunmethod formatting updates improve code consistency without changing functionality.
103-121: LGTM: Correct use of range_adaptor.The updated range-based for loop correctly uses the refactored
range_adaptorwith structured bindings. The logic for building the hitset multimap is sound.
126-155: LGTM: Proper hitset consolidation logic.The consolidation of hits from strobed hitsets into the bare hitset (strobe 0) is correctly implemented. The defensive check at Line 143 for
strobe == 0is acceptable, though the multimap should only contain non-zero strobes based on the insertion logic.
196-198: This code is correct and does not have a resource management issue. TheTrkrHitSet::addHitSpecificKeymethod explicitly takes ownership of the pointer, as documented in the header file: "NOTE: This TrkrHitSet takes ownership of the passed TrkrHit pointer and will delete it in the Reset() method." The raw pointer allocation is intentional and safe within this codebase's memory management model.Likely an incorrect or invalid review comment.
offline/packages/tpc/Tpc3DClusterizer.cc (2)
35-35: LGTM: Correct include for std::min/std::max.The addition of
<algorithm>is necessary to support thestd::minandstd::maxfunctions used later in the file.
76-76: LGTM: Improved pointer clarity and brace consistency.These changes improve code readability by:
- Using explicit pointer syntax (
auto *instead ofauto)- Adding braces around single-line control structures for consistency and safety
Also applies to: 116-121, 150-158, 171-175
offline/packages/tpc/TpcSimpleClusterizer.cc (3)
50-53: LGTM: Appropriate template refinement.Removing
inlineis correct sinceconstexpralready implies inline. The const reference parameter is a minor improvement for larger types.
146-149: LGTM: Cleaner boundary tracking with std::min/std::max.Replacing manual if-based min/max updates with standard algorithms improves readability and correctness.
324-324: LGTM: Idiomatic improvements.
- Line 324: Cast removal is correct since
adcis alreadyunsigned short- Line 328: Using
.empty()is the standard idiom for container emptiness checksAlso applies to: 328-328
offline/packages/tpc/TpcClusterizer.cc (5)
52-52: LGTM: Appropriate includes and template refinement.Consistent with changes across other TPC clusterizer files.
Also applies to: 67-70
534-534: LGTM: Explicit rounding improves clarity.The combination of
std::roundandstatic_cast<int>makes the rounding behavior explicit, which is better than implicit float-to-int conversion. The inline comment correctly documents the rounding behavior.
535-538: LGTM: Cleaner boundary tracking.Standard algorithms improve readability over manual if statements.
716-729: LGTM: Efficient in-place construction.Using
emplace_backfor both default construction and in-place construction with arguments is more efficient thanpush_back.
1462-1462: LGTM: Efficient range-based for loops.Using
const auto&avoids unnecessary copies when iterating over container elements.Also applies to: 1466-1466, 1642-1642, 1646-1646
offline/packages/jetbackground/DetermineTowerBackground.cc (4)
17-19: LGTM: Includes added for new calibration functionality.The new includes support the calibration loading and centrality-based v2 extraction introduced in this PR.
59-71: LGTM: Proper initialization for calibration loading.The InitRun modification correctly loads calibrations when do_flow == 4 and handles failures appropriately by returning ABORTRUN.
621-621: LGTM: Correct exclusion of do_flow == 4 from calo-based flow.The modified condition correctly excludes do_flow == 4 from the calorimeter-based flow determination logic, since that mode uses calibration-based v2 values instead.
948-971: Fix error handling and verify centrality bin range.
Line 956: Replace
exit(-1)withreturn Fun4AllReturnCodes::ABORTRUN;to match the error handling pattern used elsewhere in this file (lines 68, 460, 1134).Line 961: The condition
centrality_bin > 0 && centrality_bin < 95only accepts bins 1–94, but the_CENTRALITY_V2vector contains 100 elements (allocated and populated for indices 0–99). Verify whether excluding bin 0 and bins 95–99 is intentional or if this should becentrality_bin >= 0 && centrality_bin < 100.🔧 Proposed fix for error handling
if (!centinfo) { std::cout << "DetermineTowerBackground::process_event: FATAL, CentralityInfo does not exist, cannot extract centrality with do_flow = " << _do_flow << std::endl; - exit(-1); + return Fun4AllReturnCodes::ABORTRUN; }offline/packages/tpc/LaserClusterizer.cc (4)
104-126: LGTM!Good const-correctness improvement by making
paraconst double*. The function only reads from this parameter, so the const qualifier is appropriate.
633-639: LGTM - Resource cleanup improved.The unconditional deletion of
fit3Dis correct since it's always allocated at line 427. The cleanup ofhitHistwith null-check is also appropriate.
440-450: LGTM!The lambda correctly captures by reference to access
my_data->hitHistand the local parameter arrays are properly scoped for the duration of the function calls.
408-409: C++20 support is properly configured. The<format>header is included at line 46, andstd::formatusage at line 409 is consistent with widespread adoption across the codebase (e.g., PHG4ScoringManager.cc, PHG4InttDetector.cc use std::format). The build system already supports C++20, as evidenced by existing std::format usage in simulation modules.offline/packages/trackreco/PHSiliconTpcTrackMatching.cc (1)
448-453: LGTM! Clean configurability improvement.The replacement of the hard-coded "TRKR_CLUSTER" string with the configurable
_cluster_map_namemember variable allows runtime flexibility while maintaining backward compatibility through the default value. The error message correctly reflects the configured name.offline/packages/trackreco/PHSiliconTpcTrackMatching.h (2)
142-145: LGTM! Well-designed configurability setter.The setter correctly uses
const std::string &for the parameter, allowing efficient pass-by-reference while preventing modification and enabling binding to temporaries.
216-216: LGTM! Appropriate default value.The member variable initialization with "TRKR_CLUSTER" maintains backward compatibility while enabling runtime configuration through the setter.
offline/packages/trackreco/PHSimpleVertexFinder.h (1)
57-57: LGTM! Const-correctness improvement.Changing the parameter from
std::string &toconst std::string &is the correct approach since the method only copies the value and doesn't modify the input. This also enables binding to temporaries and aligns with similar improvements across the PR.offline/packages/trackreco/PHTpcDeltaZCorrection.h (1)
35-35: LGTM! Const-correctness improvement.The change from
std::string &toconst std::string &correctly enforces that the parameter is not modified and allows binding to temporaries. This aligns with similar improvements throughout the PR.offline/packages/trackreco/PHActsTrkFitter.h (6)
25-25: LGTM! Include ordering adjustment.Moving
Logger.hppafterHelpers.hppappears to be a deliberate ordering change, likely for consistency or to satisfy include dependencies.
133-137: LGTM! Formatting consistency improvement.The reformatting from single-line to block-style braces improves consistency with the surrounding code style.
145-145: LGTM! Const-correctness improvement.The change to
const std::string&is consistent with similar improvements across multiple files in this PR and follows C++ best practices for setter methods.
158-162: LGTM! Parameter list formatting.The reformatting improves readability of the multi-parameter method signature without changing behavior.
296-312: LGTM! Formatting improvements.The reformatting of the
MaterialSurfaceSelectorstruct improves code readability and consistency without changing behavior.
272-272: Remove this review comment; it does not accurately reflect the code state.The current code shows
"SvtxAlignmentStateMap"without a leading space. Git history confirms this variable was introduced correctly in commit 2efc808 without any leading space bug, and there is no evidence of any recent fix to this line. Similar map name declarations (_track_map_nameand_svtx_seed_map_name) follow the same pattern. The claimed "leading space bug fix" cannot be verified and appears to be incorrect.Likely an incorrect or invalid review comment.
offline/QA/Calorimeters/CaloValid.cc (2)
413-416: Consistent energy gating applied across all calorimeters.The
isGoodgating for total energy accumulation is consistently applied to IHCAL and OHCAL, matching the CEMC implementation. This ensures uniform quality filtering across all calorimeter subsystems.Also applies to: 481-484
347-347: QA trigger bit change from 11 to 12 is correct.The QA gating condition has been updated consistently across all three calorimeters (CEMC line 347, IHCAL line 423, OHCAL line 491) from using
scaledBits[11]toscaledBits[12]. The triggerIndices vector in CaloValid.h explicitly defines{10, 12, 28, 29, 30, 31}, confirming that bit 12 is a valid trigger (MBD NS>=1 and photon triggers). Bit 11 does not appear in the codebase. This change correctly uses the intended trigger bits.offline/QA/Calorimeters/CaloValid.h (1)
58-58: The comment is misleading: trigger 12 is not a photon trigger.Trigger bits 28–31 are confirmed as photon thresholds (from TriggerAnalyzer.cc). Trigger indices 10 and 12 appear to be MBD triggers (bits 0–19 range). If the intent is to select both MBD and photon triggers for π₀ analysis, clarify the comment to reflect this: "MBD NS≥1, Photon Triggers (bits 10, 12, 28–31)" or verify that bits 10 and 12 correspond to the intended MBD trigger conditions.
offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc (1)
977-1011: Pre-loop cleanup already erases MVTX entries—review the timing of this cleanup relative to processing.The code does erase
m_MvtxRawHitMapentries at line 873, but not where the review suggests. Entries are removed in the while loop at lines 850-880 (pre-loop cleanup that runs BEFORE the processing loop at 977-1011). This removes entries withbco < ref_bco_minus_range, but the entries processed in the subsequent for loop (977-1011) persist in the map until the next FillMvtx call.In contrast, FillTpc (line 1180) and FillMicromegas (line 1090) erase entries immediately DURING their processing loops. FillIntt follows the same deferred-cleanup pattern as FillMvtx (entries only erased in pre-loop cleanup of the next call).
The inconsistency in erasure timing means processed entries accumulate in the map across events before being cleaned up, unlike the immediate cleanup in FillTpc/FillMicromegas. Verify whether this deferred cleanup pattern poses a risk in production use or if it's intentional for the event flow.
offline/packages/micromegas/MicromegasCombinedDataDecoder.h (2)
52-56: LGTM: Documentation correctly reflects the signal hit filtering.The updated documentation aligns with the sample-based filtering logic introduced in this PR (e.g., in MicromegasClusterQA.cc).
88-88: Verify that 1024 is the correct maximum sample index for Micromegas hardware.The change increases
m_sample_maxfrom 100 to 1024. While this aligns withMicromegasDefs::m_adc_max(line 115 of MicromegasDefs.h) and matches the default inMicromegasClusterQA.h, these represent different concepts:m_adc_maxis the 10-bit ADC digitization range (0–1023 possible values), whilem_sample_maxis the maximum time sample index used in loops. Confirm that 1024 is the actual hardware specification for maximum time samples and not a conflation of the two constants.offline/QA/Tracking/MicromegasClusterQA.cc (2)
76-78: LGTM: Debug output appropriately logs sample bounds.The added debug prints follow the existing pattern and help verify the sample filtering configuration.
168-174: LGTM: Cluster filtering logic correctly implements "any hit in range" strategy.The filter keeps clusters where at least one hit has a sample within
[m_sample_min, m_sample_max). This is a reasonable approach for QA—it ensures clusters with any valid timing information are retained. Note that the upper bound is exclusive (< m_sample_max).offline/packages/micromegas/MicromegasDefs.h (2)
63-65: LGTM: Backward-compatible signature extension for sample-aware hit keys.The addition of a default
sample = 0parameter maintains backward compatibility while enabling sample indexing. The implementation correctly uses 8 bits for strip (0-255) and 16 bits for sample (0-65535), which aligns with the new defaultm_sample_max = 1024.
68-71: No changes needed to MicromegasDefs.h.The return type change for
getStripfromuint16_ttouint8_tis correct and doesn't break existing code. The implementation properly masks to 8 bits, and all usages in the codebase are compatible with the narrower return type. ThegetSamplefunction correctly returnsuint16_tfor the 16-bit sample field. Both changes appropriately reflect the actual bitfield sizes and improve type safety.offline/QA/Tracking/MicromegasClusterQA.h (1)
54-112: LGTM: Sample bound accessors align with decoder configuration.The new setters and member variables are well-integrated and use defaults consistent with
MicromegasCombinedDataDecoder(0 and 1024). The API follows existing patterns in the class.offline/packages/micromegas/MicromegasClusterizer.cc (5)
159-165: LGTM: Geometry loading refactored for improved readability.The explicit loop with break is clearer than an inline conditional and handles the fallback logic well.
190-190: LGTM: Structured binding improves code clarity.The use of structured bindings is idiomatic and enhances readability.
242-274: LGTM: Clustering logic correctly adapted to use LocalMap.The cluster boundary detection, range storage, and iteration logic have been properly updated to work with the strip-sorted
local_hitmap. The ranges stored remain valid throughout the cluster creation loop.
280-417: Cluster creation correctly uses LocalMap entries.The cluster building loop properly accesses hit keys and hit pointers from the
LocalMapentries. The ADC processing, weight calculations, and error estimates remain logically sound.
223-239: No changes needed. The code and comment are correct.When
hitset->getHits()returns hits from the underlyingstd::map<TrkrDefs::hitkey, TrkrHit*>, they are ordered by numeric hitkey value. Since the hitkey encodes strip at bits 0-7 and sample at bits 8-23, numeric sorting prioritizes sample (time) over strip. This means the hit_range is pre-sorted by sample (time) as the primary key. Whenstd::copyinserts these hits intolocal_hitmap(usingStripSortFtorwhich sorts by strip), thestd::mapinsertion behavior retains only the first occurrence of each strip key, which is the hit with the earliest sample (earliest time). The comment accurately describes the behavior: "only the first one (in time) is kept" for duplicate strips.offline/packages/micromegas/MicromegasCombinedDataDecoder.cc (2)
206-226: LGTM! Clean refactor to pair-based ADC collection with sample index.The changes correctly:
- Store (sample_index, adc) pairs for later sample tracking
- Guard against empty
adc_listbefore callingmax_element(preventing UB)- Use an appropriate comparator to find the maximum by ADC value
258-271: Hit key generation and assignment correctly updated for sample-aware encoding.The hit key now encodes both strip and sample index, which changes deduplication semantics—hits with the same strip but different samples will be stored separately. This aligns with the PR objective of "timed hits" support.
offline/packages/micromegas/MicromegasDefs.cc (3)
28-33: LGTM! Bit shift constants correctly defined for the new hitkey layout.The layout is clear: strip occupies bits 0-7 (8 bits) and sample occupies bits 8-23 (16 bits). Removing
staticfromconstexprin an anonymous namespace is correct sinceconstexpralready implies internal linkage.
41-52: Good defensive masking ingenHitSetKey.The explicit masks ensure that values don't overflow their respective bit fields, preventing subtle corruption of adjacent fields.
83-88: LGTM! NewgetSamplefunction correctly extracts the sample index.The bit manipulation correctly inverts the encoding from
genHitKey, extracting bits 8-23 as the sample value.
| uint64_t lower_limit = m_mvtx_is_triggered ? select_crossings : select_crossings - m_mvtx_bco_range - m_mvtx_negative_bco; | ||
| uint64_t upper_limit = m_mvtx_is_triggered ? select_crossings + m_mvtx_bco_range : select_crossings; | ||
|
|
There was a problem hiding this comment.
Potential underflow in lower_limit calculation for non-triggered mode.
For non-triggered MVTX, lower_limit is computed as select_crossings - m_mvtx_bco_range - m_mvtx_negative_bco. When expanded, this becomes m_RefBCO - m_mvtx_negative_bco. If m_RefBCO < m_mvtx_negative_bco, the subtraction underflows (wrapping to a very large uint64_t), causing the loop at line 977 to skip all entries via the continue at line 981.
While line 839 guards against this scenario by capping ref_bco_minus_range at 0, the lower_limit calculation here does not have equivalent protection.
🛡️ 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
+ : (select_crossings > m_mvtx_bco_range + m_mvtx_negative_bco
+ ? select_crossings - m_mvtx_bco_range - m_mvtx_negative_bco
+ : 0);
uint64_t upper_limit = m_mvtx_is_triggered ? select_crossings + m_mvtx_bco_range : select_crossings;📝 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.
| uint64_t lower_limit = m_mvtx_is_triggered ? select_crossings : select_crossings - m_mvtx_bco_range - m_mvtx_negative_bco; | |
| uint64_t upper_limit = m_mvtx_is_triggered ? select_crossings + m_mvtx_bco_range : select_crossings; | |
| uint64_t lower_limit = m_mvtx_is_triggered | |
| ? select_crossings | |
| : (select_crossings > m_mvtx_bco_range + m_mvtx_negative_bco | |
| ? select_crossings - m_mvtx_bco_range - m_mvtx_negative_bco | |
| : 0); | |
| uint64_t upper_limit = m_mvtx_is_triggered ? select_crossings + m_mvtx_bco_range : select_crossings; |
🤖 Prompt for AI Agents
In @offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc around lines
974 - 976, The computation of lower_limit in Fun4AllStreamingInputManager.cc can
underflow when m_mvtx_is_triggered is false because select_crossings -
m_mvtx_bco_range - m_mvtx_negative_bco is done on uint64_t; change the
non-triggered branch to clamp at 0: when m_mvtx_is_triggered is false, check if
select_crossings <= (m_mvtx_bco_range + m_mvtx_negative_bco) and set lower_limit
= 0, otherwise set lower_limit = select_crossings - m_mvtx_bco_range -
m_mvtx_negative_bco; leave the triggered branch and upper_limit logic as-is to
avoid wrapping.
| int Fun4AllStreamingInputManager::FillMvtxPool() | ||
| { | ||
| uint64_t ref_bco_minus_range = m_RefBCO < m_mvtx_bco_range ? m_mvtx_bco_range : m_RefBCO - m_mvtx_bco_range; | ||
| uint64_t ref_bco_minus_range = m_RefBCO < m_mvtx_negative_bco ? m_mvtx_negative_bco : m_RefBCO - m_mvtx_negative_bco; |
There was a problem hiding this comment.
Inconsistent underflow handling - returns wrong value.
The ternary expression returns m_mvtx_negative_bco when m_RefBCO < m_mvtx_negative_bco, which doesn't prevent underflow and returns a semantically incorrect value. Line 839 correctly returns 0 for the same condition, and FillInttPool (lines 1243-1247) uses a clearer if-statement pattern.
🐛 Fix to match line 839 pattern
- 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;📝 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.
| 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; |
🤖 Prompt for AI Agents
In @offline/framework/fun4allraw/Fun4AllStreamingInputManager.cc at line 1367,
The ternary initializing ref_bco_minus_range is wrong: it returns
m_mvtx_negative_bco when m_RefBCO < m_mvtx_negative_bco, which allows underflow
and is semantically incorrect; change the expression to return 0 in that case
and m_RefBCO - m_mvtx_negative_bco otherwise (i.e., mirror the check used at
line 839 and the if-style pattern used in FillInttPool). Locate the
initialization of ref_bco_minus_range in
Fun4AllStreamingInputManager::(constructor or method where it appears) and
replace the ternary so underflow is prevented by returning 0 when m_RefBCO <
m_mvtx_negative_bco.
| int DetermineTowerBackground::LoadCalibrations() | ||
| { | ||
|
|
||
| CDBTTree *cdbtree_calo_v2 = nullptr; | ||
|
|
||
| std::string calibdir = CDBInterface::instance()->getUrl(m_calibName); | ||
| if (m_overwrite_average_calo_v2) | ||
| { | ||
| calibdir = m_overwrite_average_calo_v2_path; | ||
| } | ||
|
|
||
| if (calibdir.empty()) | ||
| { | ||
| std::cout << "Could not find filename for calo average v2, exiting" << std::endl; | ||
| exit(-1); | ||
| } | ||
|
|
||
| cdbtree_calo_v2 = new CDBTTree(calibdir); | ||
|
|
||
|
|
||
| cdbtree_calo_v2->LoadCalibrations(); | ||
|
|
||
| _CENTRALITY_V2.assign(100,0); | ||
|
|
||
| for (int icent = 0; icent < 100; icent++) | ||
| { | ||
| _CENTRALITY_V2[icent] = cdbtree_calo_v2->GetFloatValue(icent, "jet_calo_v2"); | ||
| } | ||
|
|
||
| delete cdbtree_calo_v2; | ||
|
|
||
| return Fun4AllReturnCodes::EVENT_OK; | ||
| } |
There was a problem hiding this comment.
Replace exit() with proper error return and add bounds checking.
The function has several issues:
- Line 90: Using
exit(-1)violates framework conventions and prevents proper cleanup. Return an error code instead. - Line 98: The hardcoded value of 100 centrality bins is not verified against the actual calibration data size.
- Lines 100-103: No verification that the calibration tree contains valid data for all 100 bins before accessing with
GetFloatValue().
🔧 Proposed fix
int DetermineTowerBackground::LoadCalibrations()
{
CDBTTree *cdbtree_calo_v2 = nullptr;
std::string calibdir = CDBInterface::instance()->getUrl(m_calibName);
if (m_overwrite_average_calo_v2)
{
calibdir = m_overwrite_average_calo_v2_path;
}
if (calibdir.empty())
{
std::cout << "Could not find filename for calo average v2, exiting" << std::endl;
- exit(-1);
+ return Fun4AllReturnCodes::ABORTRUN;
}
cdbtree_calo_v2 = new CDBTTree(calibdir);
cdbtree_calo_v2->LoadCalibrations();
_CENTRALITY_V2.assign(100,0);
for (int icent = 0; icent < 100; icent++)
{
_CENTRALITY_V2[icent] = cdbtree_calo_v2->GetFloatValue(icent, "jet_calo_v2");
}
delete cdbtree_calo_v2;
return Fun4AllReturnCodes::EVENT_OK;
}Additionally, consider verifying that the calibration tree contains the expected number of entries before accessing them.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int DetermineTowerBackground::LoadCalibrations() | |
| { | |
| CDBTTree *cdbtree_calo_v2 = nullptr; | |
| std::string calibdir = CDBInterface::instance()->getUrl(m_calibName); | |
| if (m_overwrite_average_calo_v2) | |
| { | |
| calibdir = m_overwrite_average_calo_v2_path; | |
| } | |
| if (calibdir.empty()) | |
| { | |
| std::cout << "Could not find filename for calo average v2, exiting" << std::endl; | |
| exit(-1); | |
| } | |
| cdbtree_calo_v2 = new CDBTTree(calibdir); | |
| cdbtree_calo_v2->LoadCalibrations(); | |
| _CENTRALITY_V2.assign(100,0); | |
| for (int icent = 0; icent < 100; icent++) | |
| { | |
| _CENTRALITY_V2[icent] = cdbtree_calo_v2->GetFloatValue(icent, "jet_calo_v2"); | |
| } | |
| delete cdbtree_calo_v2; | |
| return Fun4AllReturnCodes::EVENT_OK; | |
| } | |
| int DetermineTowerBackground::LoadCalibrations() | |
| { | |
| CDBTTree *cdbtree_calo_v2 = nullptr; | |
| std::string calibdir = CDBInterface::instance()->getUrl(m_calibName); | |
| if (m_overwrite_average_calo_v2) | |
| { | |
| calibdir = m_overwrite_average_calo_v2_path; | |
| } | |
| if (calibdir.empty()) | |
| { | |
| std::cout << "Could not find filename for calo average v2, exiting" << std::endl; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| cdbtree_calo_v2 = new CDBTTree(calibdir); | |
| cdbtree_calo_v2->LoadCalibrations(); | |
| _CENTRALITY_V2.assign(100,0); | |
| for (int icent = 0; icent < 100; icent++) | |
| { | |
| _CENTRALITY_V2[icent] = cdbtree_calo_v2->GetFloatValue(icent, "jet_calo_v2"); | |
| } | |
| delete cdbtree_calo_v2; | |
| return Fun4AllReturnCodes::EVENT_OK; | |
| } |
🤖 Prompt for AI Agents
In @offline/packages/jetbackground/DetermineTowerBackground.cc around lines 76 -
108, In DetermineTowerBackground::LoadCalibrations(), remove the call to
exit(-1) and instead return an appropriate Fun4All error code on failure (e.g.,
Fun4AllReturnCodes::ABORTEVENT), and ensure CDBTTree is validated before use:
after creating cdbtree_calo_v2 check it is non-null and that its calibration
data is loaded (validate return from cdbtree_calo_v2->LoadCalibrations or check
the number of entries via an appropriate method), obtain the actual number of
centrality bins from the tree (rather than assuming 100), resize _CENTRALITY_V2
to that count (or take min(actual_count,100) if 100 is desired), and only call
cdbtree_calo_v2->GetFloatValue(icent, "jet_calo_v2") for valid indices; on any
failure clean up (delete cdbtree_calo_v2) and return the error code instead of
calling exit().
| // Get psi | ||
| if (_do_flow == 2) | ||
| { // HIJING truth flow extraction | ||
| PHG4TruthInfoContainer *truthinfo = findNode::getClass<PHG4TruthInfoContainer>(topNode, "G4TruthInfo"); | ||
|
|
||
| if (!truthinfo) | ||
| { | ||
| std::cout << "DetermineTowerBackground::process_event: FATAL , G4TruthInfo does not exist , cannot extract truth flow with do_flow = " << _do_flow << std::endl; | ||
| return -1; | ||
| } | ||
|
|
||
| PHG4TruthInfoContainer::Range range = truthinfo->GetPrimaryParticleRange(); | ||
|
|
||
| float Hijing_Qx = 0; | ||
| float Hijing_Qy = 0; | ||
|
|
||
| for (PHG4TruthInfoContainer::ConstIterator iter = range.first; iter != range.second; ++iter) | ||
| { | ||
| PHG4Particle *g4particle = iter->second; | ||
|
|
||
| if (truthinfo->isEmbeded(g4particle->get_track_id()) != 0) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| TLorentzVector t; | ||
| t.SetPxPyPzE(g4particle->get_px(), g4particle->get_py(), g4particle->get_pz(), g4particle->get_e()); | ||
|
|
||
| float truth_pt = t.Pt(); | ||
| if (truth_pt < 0.4) | ||
| { | ||
| continue; | ||
| } | ||
| float truth_eta = t.Eta(); | ||
| if (std::fabs(truth_eta) > 1.1) | ||
| { | ||
| continue; | ||
| } | ||
| float truth_phi = t.Phi(); | ||
| int truth_pid = g4particle->get_pid(); | ||
|
|
||
| if (Verbosity() > 10) | ||
| { | ||
| std::cout << "DetermineTowerBackground::process_event: determining truth flow, using particle w/ pt / eta / phi " << truth_pt << " / " << truth_eta << " / " << truth_phi << " , embed / PID = " << truthinfo->isEmbeded(g4particle->get_track_id()) << " / " << truth_pid << std::endl; | ||
| } | ||
|
|
||
| Hijing_Qx += truth_pt * std::cos(2 * truth_phi); | ||
| Hijing_Qy += truth_pt * std::sin(2 * truth_phi); | ||
| } | ||
|
|
||
| _Psi2 = std::atan2(Hijing_Qy, Hijing_Qx) / 2.0; | ||
|
|
||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "DetermineTowerBackground::process_event: flow extracted from Hijing truth particles, setting Psi2 = " << _Psi2 << " ( " << _Psi2 / M_PI << " * pi ) " << std::endl; | ||
| } | ||
| } | ||
| else if (_do_flow == 3 || _do_flow == 4) | ||
| { // sEPD event plane extraction | ||
| // get event plane map | ||
| EventplaneinfoMap *epmap = findNode::getClass<EventplaneinfoMap>(topNode, "EventplaneinfoMap"); | ||
| if (!epmap) | ||
| { | ||
| std::cout << "DetermineTowerBackground::process_event: FATAL, EventplaneinfoMap does not exist, cannot extract sEPD flow with do_flow = " << _do_flow << std::endl; | ||
| exit(-1); | ||
| } | ||
| if (!(epmap->empty())) | ||
| { | ||
| auto *EPDNS = epmap->get(EventplaneinfoMap::sEPDNS); | ||
| _Psi2 = EPDNS->get_shifted_psi(2); | ||
| } | ||
| else | ||
| { | ||
| _is_flow_failure = true; | ||
| _Psi2 = 0; | ||
| } | ||
|
|
||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "DetermineTowerBackground::process_event: flow extracted from sEPD, setting Psi2 = " << _Psi2 << " ( " << _Psi2 / M_PI << " * pi ) " << std::endl; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Fix error handling in flow extraction logic.
The new flow extraction code has proper logic but several issues:
- Line 545: Returns
-1instead of using a Fun4AllReturnCodes constant. - Line 601: Uses
exit(-1)instead of proper error return. - Multiple lines (e.g., 559, 568, 573, 580): Inconsistent indentation with mixed tabs and spaces.
🔧 Proposed fixes
if (!truthinfo)
{
std::cout << "DetermineTowerBackground::process_event: FATAL , G4TruthInfo does not exist , cannot extract truth flow with do_flow = " << _do_flow << std::endl;
- return -1;
+ return Fun4AllReturnCodes::ABORTRUN;
} if (!epmap)
{
std::cout << "DetermineTowerBackground::process_event: FATAL, EventplaneinfoMap does not exist, cannot extract sEPD flow with do_flow = " << _do_flow << std::endl;
- exit(-1);
+ return Fun4AllReturnCodes::ABORTRUN;
}Run clang-format to fix the indentation inconsistencies throughout this block.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get psi | |
| if (_do_flow == 2) | |
| { // HIJING truth flow extraction | |
| PHG4TruthInfoContainer *truthinfo = findNode::getClass<PHG4TruthInfoContainer>(topNode, "G4TruthInfo"); | |
| if (!truthinfo) | |
| { | |
| std::cout << "DetermineTowerBackground::process_event: FATAL , G4TruthInfo does not exist , cannot extract truth flow with do_flow = " << _do_flow << std::endl; | |
| return -1; | |
| } | |
| PHG4TruthInfoContainer::Range range = truthinfo->GetPrimaryParticleRange(); | |
| float Hijing_Qx = 0; | |
| float Hijing_Qy = 0; | |
| for (PHG4TruthInfoContainer::ConstIterator iter = range.first; iter != range.second; ++iter) | |
| { | |
| PHG4Particle *g4particle = iter->second; | |
| if (truthinfo->isEmbeded(g4particle->get_track_id()) != 0) | |
| { | |
| continue; | |
| } | |
| TLorentzVector t; | |
| t.SetPxPyPzE(g4particle->get_px(), g4particle->get_py(), g4particle->get_pz(), g4particle->get_e()); | |
| float truth_pt = t.Pt(); | |
| if (truth_pt < 0.4) | |
| { | |
| continue; | |
| } | |
| float truth_eta = t.Eta(); | |
| if (std::fabs(truth_eta) > 1.1) | |
| { | |
| continue; | |
| } | |
| float truth_phi = t.Phi(); | |
| int truth_pid = g4particle->get_pid(); | |
| if (Verbosity() > 10) | |
| { | |
| std::cout << "DetermineTowerBackground::process_event: determining truth flow, using particle w/ pt / eta / phi " << truth_pt << " / " << truth_eta << " / " << truth_phi << " , embed / PID = " << truthinfo->isEmbeded(g4particle->get_track_id()) << " / " << truth_pid << std::endl; | |
| } | |
| Hijing_Qx += truth_pt * std::cos(2 * truth_phi); | |
| Hijing_Qy += truth_pt * std::sin(2 * truth_phi); | |
| } | |
| _Psi2 = std::atan2(Hijing_Qy, Hijing_Qx) / 2.0; | |
| if (Verbosity() > 0) | |
| { | |
| std::cout << "DetermineTowerBackground::process_event: flow extracted from Hijing truth particles, setting Psi2 = " << _Psi2 << " ( " << _Psi2 / M_PI << " * pi ) " << std::endl; | |
| } | |
| } | |
| else if (_do_flow == 3 || _do_flow == 4) | |
| { // sEPD event plane extraction | |
| // get event plane map | |
| EventplaneinfoMap *epmap = findNode::getClass<EventplaneinfoMap>(topNode, "EventplaneinfoMap"); | |
| if (!epmap) | |
| { | |
| std::cout << "DetermineTowerBackground::process_event: FATAL, EventplaneinfoMap does not exist, cannot extract sEPD flow with do_flow = " << _do_flow << std::endl; | |
| exit(-1); | |
| } | |
| if (!(epmap->empty())) | |
| { | |
| auto *EPDNS = epmap->get(EventplaneinfoMap::sEPDNS); | |
| _Psi2 = EPDNS->get_shifted_psi(2); | |
| } | |
| else | |
| { | |
| _is_flow_failure = true; | |
| _Psi2 = 0; | |
| } | |
| if (Verbosity() > 0) | |
| { | |
| std::cout << "DetermineTowerBackground::process_event: flow extracted from sEPD, setting Psi2 = " << _Psi2 << " ( " << _Psi2 / M_PI << " * pi ) " << std::endl; | |
| } | |
| } | |
| // Get psi | |
| if (_do_flow == 2) | |
| { // HIJING truth flow extraction | |
| PHG4TruthInfoContainer *truthinfo = findNode::getClass<PHG4TruthInfoContainer>(topNode, "G4TruthInfo"); | |
| if (!truthinfo) | |
| { | |
| std::cout << "DetermineTowerBackground::process_event: FATAL , G4TruthInfo does not exist , cannot extract truth flow with do_flow = " << _do_flow << std::endl; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| PHG4TruthInfoContainer::Range range = truthinfo->GetPrimaryParticleRange(); | |
| float Hijing_Qx = 0; | |
| float Hijing_Qy = 0; | |
| for (PHG4TruthInfoContainer::ConstIterator iter = range.first; iter != range.second; ++iter) | |
| { | |
| PHG4Particle *g4particle = iter->second; | |
| if (truthinfo->isEmbeded(g4particle->get_track_id()) != 0) | |
| { | |
| continue; | |
| } | |
| TLorentzVector t; | |
| t.SetPxPyPzE(g4particle->get_px(), g4particle->get_py(), g4particle->get_pz(), g4particle->get_e()); | |
| float truth_pt = t.Pt(); | |
| if (truth_pt < 0.4) | |
| { | |
| continue; | |
| } | |
| float truth_eta = t.Eta(); | |
| if (std::fabs(truth_eta) > 1.1) | |
| { | |
| continue; | |
| } | |
| float truth_phi = t.Phi(); | |
| int truth_pid = g4particle->get_pid(); | |
| if (Verbosity() > 10) | |
| { | |
| std::cout << "DetermineTowerBackground::process_event: determining truth flow, using particle w/ pt / eta / phi " << truth_pt << " / " << truth_eta << " / " << truth_phi << " , embed / PID = " << truthinfo->isEmbeded(g4particle->get_track_id()) << " / " << truth_pid << std::endl; | |
| } | |
| Hijing_Qx += truth_pt * std::cos(2 * truth_phi); | |
| Hijing_Qy += truth_pt * std::sin(2 * truth_phi); | |
| } | |
| _Psi2 = std::atan2(Hijing_Qy, Hijing_Qx) / 2.0; | |
| if (Verbosity() > 0) | |
| { | |
| std::cout << "DetermineTowerBackground::process_event: flow extracted from Hijing truth particles, setting Psi2 = " << _Psi2 << " ( " << _Psi2 / M_PI << " * pi ) " << std::endl; | |
| } | |
| } | |
| else if (_do_flow == 3 || _do_flow == 4) | |
| { // sEPD event plane extraction | |
| // get event plane map | |
| EventplaneinfoMap *epmap = findNode::getClass<EventplaneinfoMap>(topNode, "EventplaneinfoMap"); | |
| if (!epmap) | |
| { | |
| std::cout << "DetermineTowerBackground::process_event: FATAL, EventplaneinfoMap does not exist, cannot extract sEPD flow with do_flow = " << _do_flow << std::endl; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| if (!(epmap->empty())) | |
| { | |
| auto *EPDNS = epmap->get(EventplaneinfoMap::sEPDNS); | |
| _Psi2 = EPDNS->get_shifted_psi(2); | |
| } | |
| else | |
| { | |
| _is_flow_failure = true; | |
| _Psi2 = 0; | |
| } | |
| if (Verbosity() > 0) | |
| { | |
| std::cout << "DetermineTowerBackground::process_event: flow extracted from sEPD, setting Psi2 = " << _Psi2 << " ( " << _Psi2 / M_PI << " * pi ) " << std::endl; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @offline/packages/jetbackground/DetermineTowerBackground.cc around lines 537
- 619, Replace the direct process termination calls in the flow-extraction
branch: where the PHG4TruthInfo check currently does "return -1" change it to
return the appropriate Fun4AllReturnCodes constant (e.g.,
Fun4AllReturnCodes::ABORTEVENT) and where the EventplaneinfoMap check calls
exit(-1) change it to return the same Fun4AllReturnCodes value; keep the rest of
the logic that sets _Psi2 and _is_flow_failure unchanged (symbols to edit:
process_event, _do_flow branches, PHG4TruthInfoContainer check,
EventplaneinfoMap check, _Psi2, _is_flow_failure). Also fix mixed tabs/spaces
indentation in this block (run clang-format) so formatting is consistent.
| TrkrDefs::hitkey genHitKey(uint16_t strip, uint16_t sample) | ||
| { | ||
| TrkrDefs::hitkey key = strip << kBitShiftStrip; | ||
| return key; | ||
| const TrkrDefs::hitkey key = (strip&0xFFU) << kBitShiftStrip; | ||
| const TrkrDefs::hitkey tmp = (sample&0xFFFFU) << kBitShiftSample; | ||
| return key|tmp; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all calls to genHitKey and related context
rg -n "genHitKey" --type cpp --type h -B2 -A2
# Also search for where strip values come from
rg -n "get_physical_strip|phiStrip|strip.*=" --type cpp --type h | head -50Repository: sPHENIX-Test/coresoftware
Length of output: 22683
🏁 Script executed:
# Check the structure of MicromegasDefs to understand the encoding
rg -n "kBitShift|kBitMask" offline/packages/micromegas/ --type cpp --type h -B1 -A1
# Look for any documentation or comments about strip range
rg -n "strip.*[0-9]|range.*strip|max.*strip" offline/packages/micromegas/ --type cpp --type h -iRepository: sPHENIX-Test/coresoftware
Length of output: 7866
🏁 Script executed:
# Check the full context of MicromegasDefs.cc to understand the bit layout
cat -n offline/packages/micromegas/MicromegasDefs.cc | head -100Repository: sPHENIX-Test/coresoftware
Length of output: 3706
Use uint8_t for the strip parameter to match the actual data width and enable type-level validation.
The strip parameter is declared as uint16_t but only 8 bits are used (masked with 0xFFU). While the code documentation and current usage ensure strip values are constrained to 0–255, changing the parameter type to uint8_t would prevent accidental overflow and make the intent clearer, aligning with the return type of getStrip().
🤖 Prompt for AI Agents
In @offline/packages/micromegas/MicromegasDefs.cc around lines 69 - 74, The
genHitKey function accepts strip as uint16_t but only uses 8 bits; change the
strip parameter type to uint8_t in the genHitKey signature to reflect the actual
data width and enable type-level validation, update any call sites that pass
larger types if needed, and keep the existing masking/shift logic using
kBitShiftStrip (or remove the explicit 0xFFU mask if you prefer since uint8_t
already constrains the value) while ensuring the shifts/casts to
TrkrDefs::hitkey remain correct.
| , _tgeo_manager(new TGeoManager("Default", "Geane geometry")) | ||
| , _doEventDisplay(doEventDisplay) | ||
| { | ||
| _tgeo_manager = new TGeoManager("Default", "Geane geometry"); | ||
| TGeoManager::Import(tgeo_file_name.data()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for _tgeo_manager usage throughout the codebase
rg -n '_tgeo_manager' --type=cpp -A 2 -B 2Repository: sPHENIX-Test/coresoftware
Length of output: 2051
🏁 Script executed:
# Check the Fitter.cc file around the problematic lines
sed -n '47,100p' offline/packages/PHGenFitPkg/PHGenFit/Fitter.cc | cat -nRepository: sPHENIX-Test/coresoftware
Length of output: 1833
🏁 Script executed:
# Check all constructors to see the pattern
sed -n '47,98p' offline/packages/PHGenFitPkg/PHGenFit/Fitter.cc && echo "---" && sed -n '204,249p' offline/packages/PHGenFitPkg/PHGenFit/Fitter.cc && echo "---" && sed -n '267,311p' offline/packages/PHGenFitPkg/PHGenFit/Fitter.ccRepository: sPHENIX-Test/coresoftware
Length of output: 3859
TGeoManager initialization is incorrect and creates unused instance.
Line 54 creates a new TGeoManager("Default", "Geane geometry") and assigns it to _tgeo_manager, but line 57 calls TGeoManager::Import(), which loads a different geometry and returns/sets a different manager as the current one. The "Default" manager created on line 54 becomes unused. The destructor has both delete and Delete() calls commented out (lines 103–104), indicating the developers are aware this manager cannot be properly cleaned up, suggesting a design issue.
The first constructor should instead assign the result of Import() to _tgeo_manager:
_tgeo_manager = TGeoManager::Import(tgeo_file_name.data());This matches the pattern of the other two constructors, which correctly receive and assign a provided TGeoManager* without creating a duplicate instance.
🤖 Prompt for AI Agents
In @offline/packages/PHGenFitPkg/PHGenFit/Fitter.cc around lines 54 - 57, The
constructor currently does new TGeoManager("Default", ...) and then calls
TGeoManager::Import(tgeo_file_name.data()), leaving the created manager unused;
change the constructor to assign the Import result to _tgeo_manager (i.e.
_tgeo_manager = TGeoManager::Import(...)) so the manager used is the one Import
returns, matching the other Fitter constructors that accept a TGeoManager*;
remove the unused new allocation and ensure destructor cleanup logic targets the
manager returned by Import via _tgeo_manager.
| int layerMins[3] = {7, 23, 39}; | ||
| int layerMaxes[3] = {22, 38, 54}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if layerMins or layerMaxes are used anywhere in the codebase
echo "=== Searching for layerMins usage ==="
rg -n '\blayerMins\b' --type=cpp --type=cc --type=h
echo "=== Searching for layerMaxes usage ==="
rg -n '\blayerMaxes\b' --type=cpp --type=cc --type=hRepository: sPHENIX-Test/coresoftware
Length of output: 216
🏁 Script executed:
#!/bin/bash
# Check if layerMins or layerMaxes are used anywhere in the codebase
echo "=== Searching for layerMins usage ==="
rg -n '\blayerMins\b'
echo ""
echo "=== Searching for layerMaxes usage ==="
rg -n '\blayerMaxes\b'Repository: sPHENIX-Test/coresoftware
Length of output: 485
🏁 Script executed:
#!/bin/bash
# Check for actual usage of layerMins and layerMaxes
# Look for array indexing or function calls with these variables
echo "=== Checking LaserClusterizer.cc for layerMins references ==="
rg -n 'layerMins' offline/packages/tpc/LaserClusterizer.cc
echo ""
echo "=== Checking LaserClusterizer.cc for layerMaxes references ==="
rg -n 'layerMaxes' offline/packages/tpc/LaserClusterizer.cc
echo ""
echo "=== Checking TpcCentralMembraneMatching.cc for layerMins references ==="
rg -n 'layerMins' offline/packages/tpccalib/TpcCentralMembraneMatching.cc
echo ""
echo "=== Checking TpcCentralMembraneMatching.cc for layerMaxes references ==="
rg -n 'layerMaxes' offline/packages/tpccalib/TpcCentralMembraneMatching.ccRepository: sPHENIX-Test/coresoftware
Length of output: 486
Remove unused global arrays.
layerMins and layerMaxes are declared on lines 69-70 but never referenced in this file. Remove them.
🤖 Prompt for AI Agents
In @offline/packages/tpc/LaserClusterizer.cc around lines 69 - 70, Remove the
unused global arrays layerMins and layerMaxes: locate the declarations of int
layerMins[3] and int layerMaxes[3] and delete them from the file, ensuring no
other code references these symbols; if any future logic needs their values,
replace usage with local constants or compute values where needed rather than
keeping unused globals.
| clusHits.clear(); | ||
| for(auto hit : regions[0]) | ||
| for (auto hit : regions[0]) | ||
| { | ||
| clusHits.push_back(hit); | ||
| } |
There was a problem hiding this comment.
Potential undefined behavior if regions is empty.
At line 211, regions[0] is accessed without first verifying that regions is non-empty. If the input clusHits is empty or no regions are formed, this will cause undefined behavior.
🐛 Proposed fix
clusHits.clear();
+ if (regions.empty())
+ {
+ return;
+ }
for (auto hit : regions[0])
{
clusHits.push_back(hit);
}🤖 Prompt for AI Agents
In @offline/packages/tpc/LaserClusterizer.cc around lines 210 - 214, The code
accesses regions[0] without checking that regions contains any elements, which
can cause undefined behavior; update the LaserClusterizer.cc logic around
clusHits.clear() so you first verify regions is non-empty (e.g., if
(!regions.empty()) or if (regions.size() > 0)) before iterating over regions[0],
and handle the empty-case explicitly (leave clusHits empty or apply the intended
fallback) to avoid out-of-bounds access.
| int rc; | ||
| rc = pthread_create(&thread_pair.thread, &attr, ProcessModule, (void *) &thread_pair.data); | ||
|
|
||
| if (rc) | ||
| { | ||
| std::cout << "Error:unable to create thread," << rc << std::endl; | ||
| } |
There was a problem hiding this comment.
Insufficient error handling for failed thread creation.
If pthread_create fails, the code logs an error but continues. The thread_pair still remains in the vector with an invalid thread handle, which could cause issues when attempting to join. Consider removing the failed thread_pair from the vector or marking it as invalid.
🐛 Proposed fix
int rc;
rc = pthread_create(&thread_pair.thread, &attr, ProcessModule, (void *) &thread_pair.data);
if (rc)
{
std::cout << "Error:unable to create thread," << rc << std::endl;
+ threads.pop_back(); // Remove the failed thread_pair
+ continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int rc; | |
| rc = pthread_create(&thread_pair.thread, &attr, ProcessModule, (void *) &thread_pair.data); | |
| if (rc) | |
| { | |
| std::cout << "Error:unable to create thread," << rc << std::endl; | |
| } | |
| int rc; | |
| rc = pthread_create(&thread_pair.thread, &attr, ProcessModule, (void *) &thread_pair.data); | |
| if (rc) | |
| { | |
| std::cout << "Error:unable to create thread," << rc << std::endl; | |
| threads.pop_back(); // Remove the failed thread_pair | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In @offline/packages/tpc/LaserClusterizer.cc around lines 963 - 969, The
pthread_create call can fail but the code only logs and leaves thread_pair in
the container; update the error path for pthread_create (the rc != 0 branch) so
you either remove the corresponding thread_pair entry from the vector (or set a
flag/member on thread_pair to mark it invalid) and avoid later pthread_join on
that entry; ensure any resources allocated for thread_pair.data are cleaned up
and that the error is logged with context (include rc and which ProcessModule
instance) so join/cleanup loops skip invalid thread_pair entries.
| if (isGood) | ||
| { | ||
| totalcemc += offlineenergy; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and check its existence
find . -name "CaloValid.cc" -type f 2>/dev/nullRepository: sPHENIX-Test/coresoftware
Length of output: 107
🏁 Script executed:
# Search for uses of totalcemc, totalihcal, and totalohcal
rg -n -C3 '\b(totalcemc|totalihcal|totalohcal)\b' --type=cppRepository: sPHENIX-Test/coresoftware
Length of output: 3381
🏁 Script executed:
# Check git history for recent changes to CaloValid.cc
git log --oneline -20 -- "*CaloValid.cc" 2>/dev/null | head -20Repository: sPHENIX-Test/coresoftware
Length of output: 141
🏁 Script executed:
# Get the diff of the most recent commit
git show --stat c7adbfd77 2>/dev/null | head -50Repository: sPHENIX-Test/coresoftware
Length of output: 3015
🏁 Script executed:
# Check if there are uncommitted changes
git diff -- offline/QA/Calorimeters/CaloValid.cc 2>/dev/null | head -100Repository: sPHENIX-Test/coresoftware
Length of output: 51
🏁 Script executed:
# Look at the broader context around the isGood check to understand the structure
sed -n '310,350p' offline/QA/Calorimeters/CaloValid.ccRepository: sPHENIX-Test/coresoftware
Length of output: 1579
🏁 Script executed:
# Check the parent commits to understand the branching
git log --oneline --graph -10 -- offline/QA/Calorimeters/CaloValid.ccRepository: sPHENIX-Test/coresoftware
Length of output: 143
🏁 Script executed:
# Look for the actual PR diff - check if the isGood gating was recently added
git diff HEAD~1 HEAD -- offline/QA/Calorimeters/CaloValid.cc 2>/dev/null | grep -A 10 -B 10 "isGood" | head -100Repository: sPHENIX-Test/coresoftware
Length of output: 51
🏁 Script executed:
# Check the git log to see if there were changes to CaloValid.cc in recent commits
git log -p -5 --follow -- offline/QA/Calorimeters/CaloValid.cc 2>/dev/null | grep -A 5 -B 5 "isGood.*totalcemc\|totalcemc.*isGood" | head -50Repository: sPHENIX-Test/coresoftware
Length of output: 51
🏁 Script executed:
# Check if there's a branch comparison we can do
git branch -a 2>/dev/nullRepository: sPHENIX-Test/coresoftware
Length of output: 213
🏁 Script executed:
# Look at the file size and structure to understand better
wc -l offline/QA/Calorimeters/CaloValid.ccRepository: sPHENIX-Test/coresoftware
Length of output: 110
🏁 Script executed:
# Check if there are any comments in the code indicating recent changes or TODO items
grep -n "TODO\|FIXME\|NOTE\|isGood" offline/QA/Calorimeters/CaloValid.cc | head -30Repository: sPHENIX-Test/coresoftware
Length of output: 473
🏁 Script executed:
# Compare the current HEAD against master branch to see what changed
git diff master -- offline/QA/Calorimeters/CaloValid.cc 2>/dev/null | grep -A 5 -B 5 "isGood\|totalcemc\|totalihcal\|totalohcal" | head -150Repository: sPHENIX-Test/coresoftware
Length of output: 2192
🏁 Script executed:
# Check the master branch version of the file to see if isGood gating exists there
git show master:offline/QA/Calorimeters/CaloValid.cc 2>/dev/null | sed -n '330,350p'Repository: sPHENIX-Test/coresoftware
Length of output: 761
🏁 Script executed:
# Also check lines around IHCAL and OHCAL in master
git show master:offline/QA/Calorimeters/CaloValid.cc 2>/dev/null | sed -n '405,425p'Repository: sPHENIX-Test/coresoftware
Length of output: 712
Verify intention of gating total energy by tower quality flag.
Total energy accumulation for all three calorimeters (CEMC, IHCAL, OHCAL) is now gated by the isGood flag. This changes the semantics of totalcemc, totalihcal, and totalohcal from accumulating all tower energies to accumulating only "good" tower energies.
This affects the correlation plots at lines 629-632 and the clustering threshold check at line 666. Confirm this behavior change is intentional and verify that downstream analyses depending on these totals remain valid.
🤖 Prompt for AI Agents
In @offline/QA/Calorimeters/CaloValid.cc around lines 338 - 341, The change
gates accumulation of totalcemc/totalihcal/totalohcal by the tower quality flag
isGood, which likely unintentionally changes these totals from "sum of all tower
energies" to "sum of good tower energies"; revert or make explicit: either
remove the isGood conditional when updating totalcemc/totalihcal/totalohcal so
totals always include offlineenergy, or if the new behavior is intended, update
downstream uses (the correlation plots and the clustering threshold check that
reference these totals) to expect "good-only" sums and rename variables to
reflect that (e.g., goodTotalCEMC) and ensure clustering logic uses the correct
total variable; update references to totalcemc/totalihcal/totalohcal in the
correlation plotting and clustering threshold code accordingly.
9 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
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.