Tpc distortion correction#4246
Conversation
This is the correct way: - 2pi invariance is used for phi axis - identical content is used for r and z axis.
…DistortionCorrectionContainer, to avoid future code duplication.
📝 WalkthroughWalkthroughRefactors TPC histogram I/O into Changes
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6fc37e2c-f9ac-4db7-8bfe-9a41ca56d523
📒 Files selected for processing (6)
offline/packages/tpc/Makefile.amoffline/packages/tpc/TpcDistortionCorrectionContainer.hoffline/packages/tpc/TpcLoadDistortionCorrection.ccoffline/packages/tpccalib/TpcSpaceChargeMatrixInversion.ccoffline/packages/tpccalib/TpcSpaceChargeReconstructionHelper.ccoffline/packages/tpccalib/TpcSpaceChargeReconstructionHelper.h
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
The clang-tidy error is a false positive. |
|
The QA failures aren't from lustre, sometimes for reasons unknown to me the jobs are unable to access a G4 header file. I restarted the build |
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef0474dd-bb5f-4621-8ba5-3bed6242c290
📒 Files selected for processing (2)
offline/packages/tpc/TpcDistortionCorrectionContainer.ccoffline/packages/tpccalib/TpcSpaceChargeReconstructionHelper.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- offline/packages/tpccalib/TpcSpaceChargeReconstructionHelper.cc
| auto *distortion_tfile = TFile::Open(source.c_str()); | ||
| if (!distortion_tfile) | ||
| { | ||
| std::cout << "TpcDistortionCorrectionContainer::load_histograms - cannot open " << source << std::endl; | ||
| exit(1); | ||
| } | ||
|
|
||
| const std::array<const std::string, 2> extension = {{"_negz", "_posz"}}; | ||
| for (int j = 0; j < 2; ++j) | ||
| { | ||
| m_hDPint[j] = dynamic_cast<TH1*>(distortion_tfile->Get((std::string("hIntDistortionP")+extension[j]).c_str())); | ||
| assert(m_hDPint[j]); | ||
| m_hDRint[j] = dynamic_cast<TH1*>(distortion_tfile->Get((std::string("hIntDistortionR")+extension[j]).c_str())); | ||
| assert(m_hDRint[j]); | ||
| m_hDZint[j] = dynamic_cast<TH1*>(distortion_tfile->Get((std::string("hIntDistortionZ")+extension[j]).c_str())); | ||
| assert(m_hDZint[j]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the class definition and inheritance chain
fd 'TpcDistortionCorrectionContainer' --type f | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 190
🏁 Script executed:
# Check the header file to understand the class
cat offline/packages/tpc/TpcDistortionCorrectionContainer.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1634
🏁 Script executed:
# Search for similar TFile::Open patterns in the TPC package
rg 'TFile::Open' offline/packages/tpc/ -A 3 -B 1 | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1610
🏁 Script executed:
# Check if TpcDistortionCorrectionContainer inherits from SubsysReco
rg 'class TpcDistortionCorrectionContainer' offline/ -A 5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3145
🏁 Script executed:
# Verify TFile lifecycle in this specific context - check if histograms survive file closure
rg -A 10 'load_histograms' offline/packages/tpc/TpcDistortionCorrectionContainer.cc | head -25Repository: sPHENIX-Collaboration/coresoftware
Length of output: 884
🏁 Script executed:
# Check how the container is used - does it hold the file open or expect file to close?
rg 'TpcDistortionCorrectionContainer' offline/packages/ --type h -A 3 | grep -E '(load_histograms|m_hD)' | head -30Repository: sPHENIX-Collaboration/coresoftware
Length of output: 190
🏁 Script executed:
# Verify save_histograms implementation for comparison
sed -n '40,55p' offline/packages/tpc/TpcDistortionCorrectionContainer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 675
Fix TFile resource leak in load_histograms to match save_histograms pattern
The raw TFile* opened at line 21 is never closed. This leaks file handles, especially when load_histograms() is called repeatedly. The same class already uses std::unique_ptr<TFile> in save_histograms() (line 46), making this inconsistency a clear correctness gap.
Additionally, histogram validation relies on assert() (lines 32, 34, 36), which silently fails in release builds (NDEBUG), allowing null pointers to propagate.
Proposed fix
void TpcDistortionCorrectionContainer::load_histograms( const std::string& source )
{
std::cout << "TpcDistortionCorrectionContainer::load_histograms - reading corrections from " << source << std::endl;
- auto *distortion_tfile = TFile::Open(source.c_str());
+ std::unique_ptr<TFile> distortion_tfile(TFile::Open(source.c_str()));
if (!distortion_tfile)
{
std::cout << "TpcDistortionCorrectionContainer::load_histograms - cannot open " << source << std::endl;
exit(1);
}Per coding guidelines, prioritize correctness and memory safety in .cc files.
| m_hDPint[j] = dynamic_cast<TH1*>(distortion_tfile->Get((std::string("hIntDistortionP")+extension[j]).c_str())); | ||
| assert(m_hDPint[j]); | ||
| m_hDRint[j] = dynamic_cast<TH1*>(distortion_tfile->Get((std::string("hIntDistortionR")+extension[j]).c_str())); | ||
| assert(m_hDRint[j]); | ||
| m_hDZint[j] = dynamic_cast<TH1*>(distortion_tfile->Get((std::string("hIntDistortionZ")+extension[j]).c_str())); | ||
| assert(m_hDZint[j]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "TpcDistortionCorrectionContainer.cc" -type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 136
🏁 Script executed:
head -50 offline/packages/tpc/TpcDistortionCorrectionContainer.cc | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2309
🏁 Script executed:
sed -n '25,45p' offline/packages/tpc/TpcDistortionCorrectionContainer.cc | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1182
🏁 Script executed:
# Check how m_hDPint, m_hDRint, m_hDZint are used elsewhere in the file
rg "m_hDPint|m_hDRint|m_hDZint" offline/packages/tpc/TpcDistortionCorrectionContainer.cc -A 2 -B 1Repository: sPHENIX-Collaboration/coresoftware
Length of output: 623
🏁 Script executed:
# Check the header file for member variable definitions
find . -name "TpcDistortionCorrectionContainer.h" -type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 135
🏁 Script executed:
# Examine the header file to understand member variables
cat offline/packages/tpc/TpcDistortionCorrectionContainer.h | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1634
Replace assert-only histogram checks with runtime validation
Lines 32/34/36 use assert for histogram validity, which is compiled out in release builds (-DNDEBUG); missing histograms silently flow as null pointers into the m_hDPint, m_hDRint, and m_hDZint arrays, then cause memory safety issues when dereferenced in save_histograms().
Proposed fix
+ auto get_hist_or_throw = [&](const std::string& name) -> TH1*
+ {
+ auto* h = dynamic_cast<TH1*>(distortion_tfile->Get(name.c_str()));
+ if (!h) {
+ throw std::runtime_error(
+ "TpcDistortionCorrectionContainer::load_histograms - missing histogram " + name);
+ }
+ return h;
+ };
+
const std::array<const std::string, 2> extension = {{"_negz", "_posz"}};
for (int j = 0; j < 2; ++j)
{
- m_hDPint[j] = dynamic_cast<TH1*>(distortion_tfile->Get((std::string("hIntDistortionP")+extension[j]).c_str()));
- assert(m_hDPint[j]);
- m_hDRint[j] = dynamic_cast<TH1*>(distortion_tfile->Get((std::string("hIntDistortionR")+extension[j]).c_str()));
- assert(m_hDRint[j]);
- m_hDZint[j] = dynamic_cast<TH1*>(distortion_tfile->Get((std::string("hIntDistortionZ")+extension[j]).c_str()));
- assert(m_hDZint[j]);
+ m_hDPint[j] = get_hist_or_throw(std::string("hIntDistortionP") + extension[j]);
+ m_hDRint[j] = get_hist_or_throw(std::string("hIntDistortionR") + extension[j]);
+ m_hDZint[j] = get_hist_or_throw(std::string("hIntDistortionZ") + extension[j]);
}| std::unique_ptr<TFile> outputfile(TFile::Open(destination.c_str(), "RECREATE")); | ||
| outputfile->cd(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "TpcDistortionCorrectionContainer.cc" -type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 136
🏁 Script executed:
git ls-files | grep -i "tpcdistortion"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 386
🏁 Script executed:
sed -n '40,55p' offline/packages/tpc/TpcDistortionCorrectionContainer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 675
🏁 Script executed:
wc -l offline/packages/tpc/TpcDistortionCorrectionContainer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 137
🏁 Script executed:
cat -n offline/packages/tpc/TpcDistortionCorrectionContainer.cc | head -61Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2506
🌐 Web query:
ROOT TFile::Open() return behavior null zombie failure
💡 Result:
TFile::Open is a static method that returns a TFile pointer (TFile*) on success or NULL (0) on complete failure (e.g., unsupported URL, no plugin handler found). If a TFile* is returned, check IsZombie to determine if the file opened successfully: true means failure (invalid ROOT file, cannot open, etc.), during which MakeZombie sets the kZombie bit. The constructor behaves similarly: creates a zombie object on failure, detectable via IsZombie.
Citations:
- 1: https://root.cern/doc/master/classTFile.html
- 2: https://root.cern.ch/root/html404/src/TFile.cxx.html
- 3: https://root.cern/manual/root_files/
- 4: https://root.cern/root/roottalk/roottalk02/4340.html
- 5: https://root.cern.ch/root/roottalk/roottalk11/0216.html
Guard output-file open failure before dereference
Line 46 dereferences outputfile unconditionally. If TFile::Open() returns null (complete failure) or a "zombie" object (partial failure like permission denied or write error), this causes a null dereference or silent data loss crash.
The same file demonstrates the correct pattern in load_histograms() (lines 21–26), where it checks the result before use. Add a guard:
Proposed fix
std::unique_ptr<TFile> outputfile(TFile::Open(destination.c_str(), "RECREATE"));
+ if (!outputfile || outputfile->IsZombie())
+ {
+ throw std::runtime_error(
+ "TpcDistortionCorrectionContainer::save_histograms - cannot create " + destination);
+ }
outputfile->cd();📝 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.
| std::unique_ptr<TFile> outputfile(TFile::Open(destination.c_str(), "RECREATE")); | |
| outputfile->cd(); | |
| std::unique_ptr<TFile> outputfile(TFile::Open(destination.c_str(), "RECREATE")); | |
| if (!outputfile || outputfile->IsZombie()) | |
| { | |
| throw std::runtime_error( | |
| "TpcDistortionCorrectionContainer::save_histograms - cannot create " + destination); | |
| } | |
| outputfile->cd(); | |
Build & test reportReport for commit f879f900878a9eb927d176185e58d64e76e25e99:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
merged commit 9ada433
into
sPHENIX-Collaboration:master
2 participants
This PR
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)
TPC Distortion Correction: Consolidate Histogram I/O and Extract Guarding Bin Logic
Motivation / Context
Refactor TPC distortion-correction code to remove duplicated ROOT file I/O and to centralize guarding-bin filling logic so macros and reconstruction code reuse the same behavior. This improves maintainability and reduces risk of divergent implementations of histogram loading/saving and guard-bin population.
Key changes
Potential risk areas
Possible future improvements
Note: AI-assisted summary — AI can make mistakes; please validate file-format, error-handling and numeric equivalence of guarding-bin logic when reviewing.