Fixed calo v2 #4069
Fixed calo v2 #4069
Conversation
Build & test reportReport for commit 35a6b31e4b87d1ce0fe8e10fb3450750dae4538a:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 2e9ccc7bd05638acfab1959cc62d9e2bbca8515d:
Automatically generated by sPHENIX Jenkins continuous integration |
| cdbtree_calo_v2 = new CDBTTree(calibdir); | ||
| } | ||
|
|
||
| if (!cdbtree_calo_v2) |
There was a problem hiding this comment.
the ctor of cdbttree will always return an object, checking for null will never trigger. Errors are caught in the LoadCalibrations method and the code will do a hard exit() if an error is encountered:
Build & test reportReport for commit ce0d3af05d26ad9aaf3275d50dce0afd8d54b2ec:
Automatically generated by sPHENIX Jenkins continuous integration |
📝 WalkthroughWalkthroughThis PR adds EMCAL v2 calibration loading functionality to DetermineTowerBackground, introducing a new do_flow == 4 pathway that loads calibrations via CDBInterface/CDBTTree and uses centrality-based v2 assignment with optional path overwrite capability. Changes
✨ Finishing touches
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: 4
📜 Review details
Configuration used: Repository 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
📓 Path-based instructions (2)
**/*.{h,hpp,hxx,hh}
⚙️ CodeRabbit configuration file
**/*.{h,hpp,hxx,hh}: Focus on API clarity/stability, ownership semantics (RAII), and avoiding raw new/delete.
If interfaces change, ask for compatibility notes and any needed downstream updates.Only raise Critical or Major findings. Do not post minor style, formatting, naming, or “nice-to-have” refactors.
Files:
offline/packages/jetbackground/DetermineTowerBackground.h
**/*.{cc,cpp,cxx,c}
⚙️ CodeRabbit configuration file
**/*.{cc,cpp,cxx,c}: Prioritize correctness, memory safety, error handling, and thread-safety.
Flag hidden global state, non-const singletons, and unclear lifetime assumptions.Only raise Critical or Major findings. Do not post minor style, formatting, naming, or “nice-to-have” refactors.
Files:
offline/packages/jetbackground/DetermineTowerBackground.cc
🧬 Code graph analysis (2)
offline/packages/jetbackground/DetermineTowerBackground.h (1)
offline/packages/jetbackground/DetermineTowerBackground.cc (2)
LoadCalibrations(76-113)LoadCalibrations(76-76)
offline/packages/jetbackground/DetermineTowerBackground.cc (2)
offline/packages/jetbackground/DetermineTowerBackground.h (3)
_do_flow(70-70)_Psi2(72-72)_v2(71-71)offline/packages/jetbackground/TowerBackgroundv1.h (2)
_Psi2(36-36)_v2(35-35)
🔇 Additional comments (3)
offline/packages/jetbackground/Makefile.am (1)
27-28: LGTM - Library dependencies added for calibration loading.The new dependencies (-lcentrality_io, -lcdbobjects, -lffamodules) are correctly added to support the EMCAL v2 calibration functionality.
Also applies to: 38-38
offline/packages/jetbackground/DetermineTowerBackground.cc (1)
17-19: LGTM - Required includes added.offline/packages/jetbackground/DetermineTowerBackground.h (1)
41-45: LGTM - New API and data members for calibration loading.The additions support the new EMCAL v2 calibration pathway. The data members are appropriately declared and the LoadCalibrations method is correctly positioned as a private helper.
Also applies to: 63-68
| if (_do_flow == 4) | ||
| { | ||
| if (Verbosity()) | ||
| { | ||
| std::cout << "Loading the average calo v2" << std::endl; | ||
| } | ||
| if (!LoadCalibrations()) | ||
| { | ||
| std::cout << "Load calibrations failed." << std::endl; | ||
| return Fun4AllReturnCodes::ABORTRUN; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Critical: Inverted logic on LoadCalibrations() return value.
LoadCalibrations() returns Fun4AllReturnCodes::EVENT_OK (which is 0) on success. Line 65 treats this as a boolean: if (!LoadCalibrations()). This inverts the logic—success (returning 0) would trigger !0 == true, entering the failure branch and aborting the run.
🔧 Fix the return value check
- if (!LoadCalibrations())
+ if (LoadCalibrations() != Fun4AllReturnCodes::EVENT_OK)
{
std::cout << "Load calibrations failed." << std::endl;
return Fun4AllReturnCodes::ABORTRUN;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (_do_flow == 4) | |
| { | |
| if (Verbosity()) | |
| { | |
| std::cout << "Loading the average calo v2" << std::endl; | |
| } | |
| if (!LoadCalibrations()) | |
| { | |
| std::cout << "Load calibrations failed." << std::endl; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| } | |
| if (_do_flow == 4) | |
| { | |
| if (Verbosity()) | |
| { | |
| std::cout << "Loading the average calo v2" << std::endl; | |
| } | |
| if (LoadCalibrations() != Fun4AllReturnCodes::EVENT_OK) | |
| { | |
| std::cout << "Load calibrations failed." << std::endl; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| } |
| int DetermineTowerBackground::LoadCalibrations() | ||
| { | ||
|
|
||
| CDBTTree *cdbtree_calo_v2 = nullptr; | ||
|
|
||
| std::string calibdir; | ||
| if (m_overwrite_average_calo_v2) | ||
| { | ||
| calibdir = m_overwrite_average_calo_v2_path; | ||
| } | ||
| else | ||
| { | ||
| calibdir = CDBInterface::instance()->getUrl(m_calibName); | ||
| } | ||
|
|
||
| if (calibdir.empty()) | ||
| { | ||
| std::cout << "Could not find and load histograms for EMCAL LUTs! defaulting to the identity table!" << std::endl; | ||
| exit(-1); | ||
| } | ||
| else | ||
| { | ||
| 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.
Major: Memory safety and error handling issues in LoadCalibrations().
Several issues requiring attention:
-
Memory leak risk: Raw pointer with
new/delete(lines 98, 110). IfLoadCalibrations()orGetFloatValue()throws an exception, the CDBTTree is never deleted. Usestd::unique_ptr<CDBTTree>for RAII. -
No error checking: Line 101 calls
cdbtree_calo_v2->LoadCalibrations()without checking return value or catching exceptions. If this fails silently, subsequentGetFloatValue()calls may return garbage data. -
Inconsistent error handling: Line 94 calls
exit(-1)instead of returningFun4AllReturnCodes::ABORTRUN. This bypasses Fun4All's error handling and makes the module harder to test and debug. The caller (InitRun) expects to handle ABORTRUN returns. -
Misleading error message: Line 93 says "defaulting to the identity table" but then calls
exit(-1)instead of providing a default.
🔧 Recommended fixes
int DetermineTowerBackground::LoadCalibrations()
{
- CDBTTree *cdbtree_calo_v2 = nullptr;
-
std::string calibdir;
if (m_overwrite_average_calo_v2)
{
calibdir = m_overwrite_average_calo_v2_path;
}
else
{
calibdir = CDBInterface::instance()->getUrl(m_calibName);
}
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 << "Could not find calibration URL for " << m_calibName << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
}
- else
- {
- cdbtree_calo_v2 = new CDBTTree(calibdir);
- }
+
+ std::unique_ptr<CDBTTree> cdbtree_calo_v2 = std::make_unique<CDBTTree>(calibdir);
- cdbtree_calo_v2->LoadCalibrations();
+ if (cdbtree_calo_v2->LoadCalibrations() != 0) // Check actual return semantics
+ {
+ std::cout << "Failed to load calibrations from " << calibdir << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+ }
_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;
}| 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.
Major: exit(-1) in process_event bypasses error handling.
Line 606 calls exit(-1) when EventplaneinfoMap is missing. This immediately terminates the process, bypassing Fun4All's error handling infrastructure. Return Fun4AllReturnCodes::ABORTRUN instead to allow graceful shutdown and proper cleanup.
🔧 Fix error handling
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;
}📝 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.
| 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; | |
| } | |
| } | |
| 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; | |
| } | |
| } |
| else if (_do_flow == 4) | ||
| { | ||
|
|
||
| CentralityInfo *centinfo = findNode::getClass<CentralityInfo>(topNode, "CentralityInfo"); | ||
|
|
||
| if (!centinfo) | ||
| { | ||
| std::cout << "DetermineTowerBackground::process_event: FATAL, CentralityInfo does not exist, cannot extract centrality with do_flow = " << _do_flow << std::endl; | ||
| exit(-1); | ||
| } | ||
|
|
||
| int centrality_bin = centinfo->get_centrality_bin(CentralityInfo::PROP::mbd_NS); | ||
|
|
||
| if (centrality_bin > 0 && centrality_bin < 95) | ||
| { | ||
| _v2 = _CENTRALITY_V2[centrality_bin]; | ||
| } | ||
| else | ||
| { | ||
| _v2 = 0; | ||
| _is_flow_failure = true; | ||
| _Psi2 = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find _CENTRALITY_V2 definition and initialization
rg -n -B5 -A5 '_CENTRALITY_V2\s*=' --type=cc --type=h
# Look for _CENTRALITY_V2 size/allocation
rg -n -B3 -A3 '_CENTRALITY_V2.*\[|new.*_CENTRALITY_V2|_CENTRALITY_V2.*new' --type=cc --type=hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Find LoadCalibrations function mentioned in scratchpad
rg -n -B5 -A10 'LoadCalibrations' offline/packages/jetbackground/DetermineTowerBackground.cc | head -60
# Search for centrality bin range limits/constants
rg -n 'centrality.*100|CENT.*100|bin.*100' --type=cc --type=h -g '*DetermineTowerBackground*'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1080
🏁 Script executed:
#!/bin/bash
# Check for get_centrality_bin implementation and documentation
rg -n -B2 -A8 'int.*get_centrality_bin|get_centrality_bin.*PROP' --type=h -g '*Centrality*'
# Look at the actual file around the problem lines
cat -n offline/packages/jetbackground/DetermineTowerBackground.cc | sed -n '950,980p'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 4747
🏁 Script executed:
#!/bin/bash
# Check other exit(-1) instances in the file to understand pattern
rg -n 'exit\(-1\)' offline/packages/jetbackground/DetermineTowerBackground.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 159
Major: Array bounds check incomplete and exit(-1) usage.
Two issues in the do_flow == 4 pathway:
-
exit(-1) (Line 961): Should return
Fun4AllReturnCodes::ABORTRUNinstead. Line 68 elsewhere in this file shows the correct error handling pattern. -
Incomplete bounds protection (Line 966): The condition
centrality_bin > 0 && centrality_bin < 95allows access to_CENTRALITY_V2[centrality_bin]without explicitly protecting againstcentrality_bin >= 100. The array is initialized with size 100 (indices 0-99) at line 103. Whileget_centrality_bin()likely returns values in [0, 99] or error indicators like -99/NaN, explicit bounds checking is safer. The current range also intentionally excludes bins 0 and 95-99—verify whether this is a physics requirement or an oversight.
Suggested fixes
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;
}
int centrality_bin = centinfo->get_centrality_bin(CentralityInfo::PROP::mbd_NS);
- if (centrality_bin > 0 && centrality_bin < 95)
+ if (centrality_bin >= 0 && centrality_bin < 100)
{
_v2 = _CENTRALITY_V2[centrality_bin];
}Note: If excluding bins 0 and 95-99 is intentional, add an explicit check with a comment explaining the physics constraint.
2 participants
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
Adds option for determine tower background to use average call which now has a default in the CDB, but can be loaded as a CDBTTree. Code to generate this is to be added to the calibrations repository.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation
This PR extends the tower background determination module to support a new flow option (
do_flow == 4) that uses average calorimeter v2 values from a centrality-dependent calibration database. This provides an alternative to existing flow determination methods (calorimeter event plane, HIJING truth, and sEPD) that can be calibrated and optimized offline.Key Changes
LoadCalibrations()method that loads EMCAL v2 values from CDBInterface/CDBTTree at initialization (activated whendo_flow == 4)process_event()to extract centrality from CentralityInfo and map to v2 via_CENTRALITY_V2lookup table for the new flow optionSetOverwriteCaloV2()setter to allow runtime path override of calibration source (enables custom calibration directories)_CENTRALITY_V2vector to store centrality-indexed v2 values"JET_AVERAGE_CALO_V2_SEPD_PSI2"-lcentrality_io,-lcdbobjects, and-lffamodulesto Makefile.am for CDB and centrality infrastructurePotential Risk Areas
exit(-1)for missing CentralityInfo/EPD map (non-standard error propagation); compare with existing code patternsTesting & Validation
SetFlow(4)and confirm v2 values are correctly populated from centrality binsdo_flow=3(sEPD reference) anddo_flow=4in same event