updated CRV calibration macro

[ehrlich-uva]

No description provided.

[oksuzian]

I'll review this pull request by first loading the relevant details and changes.Here's my review of PR #5 — "updated CRV calibration macro". The PR modifies 4 existing files and adds 2 new files across 6 commits. Below are the issues I found:

---

Issues Found

1. 🔴 Potential Null Pointer Dereference in CrvCalibration.C

        if(i==1) hist=(TH1F*)gDirectory->FindObjectAny(Form("crvCalibrationHistPulseArea_%zu",channel));
        else hist=(TH1F*)gDirectory->FindObjectAny(Form("crvCalibrationHistPulseHeight_%zu",channel));
        hist->GetListOfFunctions()->Delete();

The new line hist->GetListOfFunctions()->Delete() is called immediately after FindObjectAny, which can return NULL if the histogram doesn't exist. This would cause a segfault. The old code didn't dereference hist until inside FindSPEpeak, which checked hist->GetEntries() — but even that was not null-safe. The new code makes this worse by unconditionally dereferencing hist before any checks. A null check should be added:

if(!hist) { calibValue[i]=-1; continue; }
hist->GetListOfFunctions()->Delete();

2. 🔴 Potential Null Pointer Dereference in CrvTimeOffsets_extracted.C

  TFile *file = new TFile(rootFileName.c_str());
  file->cd("CrvTimingStudies");

There's no check on whether the file was opened successfully. TFile constructor won't return null, but the file may be a zombie. Should add:

if(file->IsZombie()) { std::cerr << "Cannot open " << rootFileName << std::endl; return; }

3. 🟡 std::map::at() Can Throw If Key Not Found in CrvTimeOffsets_extracted.C

    timeOffsetChannelMap[channel]=timeOffsets.at(globalfpga);

If globalfpga does not exist in the timeOffsets map (e.g., because the propagation loop didn't reach all FPGAs), std::map::at() will throw std::out_of_range. This could happen if some FPGA time differences couldn't be connected to the reference chain. Consider adding a check or using find() with a fallback/warning.

4. 🟡 Typo: "opposide" instead of "opposite" in CrvTimeOffsets_extracted.C

    TText *t1 = new TText(.1,.85,"Module 1 (FEBs on opposide sides)");
    TText *t2 = new TText(.1,.35,"Module 5 (FEBs on opposide sides)");

"opposide" → "opposite" (appears twice).

5. 🟡 Memory Leaks in CrvTimeOffsets_extracted.C — new Without delete

Throughout the function, TCanvas* and TText* objects are allocated with new inside loops but never deleted:

    TCanvas *c = new TCanvas(Form("FEB %i",feb+1),Form("FEB %i",feb+1),800,800);
    TText *t = new TText(.1,.8,Form("FEB %i",(feb!=1?feb+1:29)));

While ROOT's garbage collector often handles this, it's still best practice to either use stack-allocated objects or explicitly delete them. In the loop that iterates over 28 FEBs + 8 modules + neighboring modules, this accumulates many orphaned objects.

6. 🟡 Hardcoded Date-Stamped File Paths in .fcl Files

All three modified .fcl files now reference date-stamped calibration files:

services.DbService.textFile : ["Offline/CRVConditions/data/status_extracted_20260214.txt","Offline/CRVConditions/data/calib_extracted_20260214.txt"]

This appears in CrvCalibration_extracted.fcl, CrvPass1_extracted.fcl, CrvPedestal_extracted.fcl, and CrvTimeOffsets_extracted.fcl. This means every time calibration constants are updated, all four .fcl files need to be modified. Consider using a symlink or a variable/include mechanism so there's a single place to update.

7. 🟡 maxFitDifference = 100.0 Uses Absolute Comparison

const double maxFitDifference=100.0;
...
    if(fabs(SPEpeak-x)>maxFitDifference) return false;

The same absolute threshold of 100.0 is applied to both pulse height and pulse area fits, but these two quantities may have very different scales (pulse heights are ~10+ while pulse areas are ~250+). A relative threshold might be more appropriate, or separate thresholds for each quantity.

8. 🟢 spectrumNPeaks Increased from 6 to 100

const int    spectrumNPeaks=100;

This is a large increase. The new FindSPEpeak logic only ever looks at the first two peaks (sorted by Y), so allocating a TSpectrum with capacity 100 is unnecessary overhead. A smaller value (e.g., 10–20) would be sufficient.

9. 🟢 Verbose Level Left at 2 in CrvPass1_extracted.fcl

services.DbService.verbose : 2

The other .fcl files set verbose : 0. This looks like a debugging leftover and should probably be set to 0 for consistency.

---

Summary

Severity	Count	Issues
🔴 High	2	Null pointer dereferences (CrvCalibration.C hist, CrvTimeOffsets_extracted.C file)
🟡 Medium	5	Uncaught map::at() exception, typo "opposide", memory leaks, hardcoded dates, absolute fit threshold for different scales
🟢 Low	2	Oversized spectrumNPeaks, inconsistent verbose level

The most critical issues are the null pointer dereferences — particularly the hist pointer in CrvCalibration.C which can realistically be null for missing channels.

[ehrlich-uva]

Updated the code to address the issues from above.

[oksuzian]

Memory leaks: TCanvas and TText objects are created with new inside loops but never deleted. In ROOT interactive sessions this is typical (ROOT manages garbage collection), but if this macro is ever run in batch or compiled mode, it would leak. Consider adding a comment or using RAII patterns.

[ehrlich-uva]

All TCanvas and TText objects are now added to gROOT for automatic memory clean up.

[oksuzian]

Ralf says it's ready to be merged

[rlcee]

This style of referring to database text in the Offline will only work in limited cases for a limited time. So OK for today, but I hope we can move on soon.