CaloTowerStatus: Refactor Hot Tower Identification#4267
Merged
pinkenburg merged 5 commits intoMay 15, 2026
Conversation
…reshold check - Added a logical split to handle the default behavior: if the z-score threshold is untouched, the code now relies strictly on the non-zero hotMap values. - Refactored the complex if-statement for custom thresholds into an if/else block using descriptive boolean variables (`is_dead`, `exceeds_zscore_limit`, `is_low_yield_cold`) for better readability. - Replaced the C-style `std::fabs` with the modern C++ standard `std::abs`. - Guarded the entire evaluation block behind `m_doHotMap` to improve clarity.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughInitRun now creates local CDBTTree instances for chi2 and hot-map, passes them into LoadCalib(CDBTTree*, CDBTTree*), deletes them after loading, removes stored CDBTTree members, and refactors process_event hot-map logic to compute is_hot_tower via default-vs-custom branching and set_isHot(true) only when true. ChangesCalibration ownership and Hot-Tower Determination
Possibly Related PRs
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 706f7810-9dc4-45be-a9eb-ac87ae5e6fca
📒 Files selected for processing (1)
offline/packages/CaloReco/CaloTowerStatus.cc
Build & test reportReport for commit 6dd4feed1f9ad156d07c07bc91552ea75adf31a9:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors CaloTowerStatus hot-tower identification to separate default hot-map behavior from custom z-score threshold handling.
Changes:
- Adds an explicit
m_doHotMapguard around hot-map evaluation. - Splits default vs custom threshold behavior.
- Introduces named boolean conditions for custom threshold logic and switches to
std::abs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Using hotMap_val != 0 treats the CDB missing-value as a hot tower: CDBTTree::GetIntValue returns std::numeric_limits<int>::min() when the channel or field is absent, and that value is nonzero. This is a behavior regression from the previous explicit status checks and could mask towers if a calibration entry is missing or malformed; limit this default path to valid positive/known status codes instead of any nonzero value. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
|
Why is the calibration read from the cdbttree in a separate method? The current scheme forces you to keep the cdbttree object around until the class is deleted (at the end of processing) wasting quite some memory. The arrays with the calibrations should be filled when the cdb is read and then the cdbttrees should be deleted. That simplifies the code and will make it more efficient. |
Refactored the calibration loading process to ensure heavy tree objects are freed from memory immediately after use. - Moved m_cdbttree_chi2 and m_cdbttree_hotMap from class members to local variables within InitRun to minimize the memory footprint during the event processing loop. - Updated the LoadCalib method to accept CDBTTree pointers as arguments. - Implemented immediate deletion of the tree objects after populating the m_cdbInfo_vec cache. - Simplified the class destructor as it no longer needs to manage these pointers.
Contributor
Author
|
@pinkenburg Thanks for pointing out the memory waste with the CDBTTree objects. I’ve refactored the class to delete those trees immediately after the CDB data is loaded into the local cache. |
Build & test reportReport for commit 6589011221aabb717be317596a87dae67e0156bb:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit ea5838429ff86f205058cdf062dd1d9f728fc499:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/CaloReco/CaloTowerStatus.cc (1)
79-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse RAII (e.g.,
std::unique_ptr<CDBTTree>) for the local calibration trees to avoid leaks on the exceptional paths.The new ownership model is a nice cleanup, but the
cdbttree_*raw pointers are not exception-safe:
- If
new CDBTTree(calibdir_hotMap)at lines 128 or 135 throws,cdbttree_chi2(already allocated at lines 89/96) is leaked.- The
deletecalls at lines 177–178 sit inside thetryblock afterLoadCalib. IfCreateNodeTree(topNode)throws (it does on a missing DST node, per line 288) orLoadCalibthrows, both trees leak — control jumps to thecatchat line 180 and the deletes are skipped.Wrapping the locals in
std::unique_ptr<CDBTTree>(and passing.get()intoLoadCalib) makes the lifetime correct on every path and removes the rawnew/deletepair entirely.♻️ Suggested RAII refactor
- CDBTTree *cdbttree_chi2 = nullptr; + std::unique_ptr<CDBTTree> cdbttree_chi2; @@ - cdbttree_chi2 = new CDBTTree(calibdir_chi2); + cdbttree_chi2 = std::make_unique<CDBTTree>(calibdir_chi2); @@ - cdbttree_chi2 = new CDBTTree(calibdir_chi2); + cdbttree_chi2 = std::make_unique<CDBTTree>(calibdir_chi2); @@ - CDBTTree *cdbttree_hotMap = nullptr; + std::unique_ptr<CDBTTree> cdbttree_hotMap; @@ - cdbttree_hotMap = new CDBTTree(calibdir_hotMap); + cdbttree_hotMap = std::make_unique<CDBTTree>(calibdir_hotMap); @@ - cdbttree_hotMap = new CDBTTree(calibdir_hotMap); + cdbttree_hotMap = std::make_unique<CDBTTree>(calibdir_hotMap); @@ - LoadCalib(cdbttree_chi2, cdbttree_hotMap); - - delete cdbttree_chi2; - delete cdbttree_hotMap; + LoadCalib(cdbttree_chi2.get(), cdbttree_hotMap.get()); }Add
#include <memory>if not already pulled in transitively.LoadCalib'sCDBTTree*signature stays unchanged, and the trees are released automatically at end of scope, including on thecatchpath.As per coding guidelines ("ownership semantics (RAII), and avoiding raw new/delete").
♻️ Duplicate comments (1)
offline/packages/CaloReco/CaloTowerStatus.cc (1)
242-266:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAsymmetry between default and custom
z_score_thresholdpaths still stands.The default branch (line 249) flags every
hotMap_val > 0as hot, while the custom branch only enumerates codes1and3explicitly and relies on the sigma cut to catch anything else (e.g.hotMap_val == 2). A tower with code2and a modest z-score can therefore be hot in the default path and clean in the custom path — that's a real behavioral fork driven only by whetherz_score_thresholdwas changed. Worth either making the custom branch handle all non-zero codes the default does, or documenting the intended hotMap-code semantics in the header so future readers know this is by design.Also note line 256 uses
z_score_threshold_defaultrather than the user-suppliedz_score_thresholdfor the cold-tail gate on code-3 towers — please double-check that this mixed-threshold behavior is what you want when a physicist sets a custom sigma.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e4fa331-f6b3-4a67-8f05-c073af9f77ed
📒 Files selected for processing (2)
offline/packages/CaloReco/CaloTowerStatus.ccoffline/packages/CaloReco/CaloTowerStatus.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55d10523-4c6e-4dd1-b933-0c93a17829a0
📒 Files selected for processing (2)
offline/packages/CaloReco/CaloTowerStatus.ccoffline/packages/CaloReco/CaloTowerStatus.h
🚧 Files skipped from review as they are similar to previous changes (1)
- offline/packages/CaloReco/CaloTowerStatus.h
Build & test reportReport for commit 1c6b4f480350df179d025c890c8f49aeab264f8a:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 7fff3252e3eb9012bac12cacd4066f1ba7215e91:
Automatically generated by sPHENIX Jenkins continuous integration |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
is_dead,exceeds_zscore_limit,is_low_yield_cold) for better readability.std::fabswith the modern C++ standardstd::abs.m_doHotMapto improve clarity.TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
CaloTowerStatus: Refactor Hot Tower Identification
Motivation / Context
Hot-tower identification previously used a dense conditional and kept CDBTTree objects as class members, which made the logic hard to read and increased memory lifetime of calibration trees. This change clarifies default vs. custom z-score handling, reduces memory usage by loading calibration into local caches, and improves readability and maintainability.
Key changes
Calibration loading and lifetime
Hot-map and hot-tower logic
Minor control-flow change
Potential risk areas
Possible future improvements
Note: This summary was prepared with AI assistance — AI can make mistakes. Review the changes and validate classification behavior before deployment.