Crv Calibration Updates

[ehrlich-uva]

No description provided.

[FNALbuild]

Hi @ehrlich-uva,
You have proposed changes to files in these packages:

• CRVReco

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

⌛ The following tests have been triggered for 564af28: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 564af28.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 564af28 at 5d5efb9
build (prof)	✅	Log file. Build time: 04 min 34 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	🔶	TODO (1) FIXME (0) in 4 files
clang-tidy	🔶	6 errors 188 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 564af28 after being merged into the base branch at 5d5efb9.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

[oksuzian]

@FNALbuild run build test

[oksuzian]

@FNALbuild run build test

[FNALbuild]

⌛ The following tests have been triggered for abd80ca: build (already triggered: build) (Build queue - API unavailable)

[FNALbuild]

☀️ The build tests passed at abd80ca.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged abd80ca at f311ac2
build (prof)	✅	Log file. Build time: 04 min 34 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (1) FIXME (0) in 5 files
clang-tidy	➡️	8 errors 187 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at abd80ca after being merged into the base branch at f311ac2.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

[oksuzian]

I'll review this pull request by first loading the PR details and its changes.Here's my review of PR #1679 – Crv Calibration Updates. This PR modifies 9 files (4,234 additions, 60 deletions) across the CRV calibration, reconstruction, timing, and event display subsystems. Below are the issues I found, organized by severity.

---

🔴 Critical Issues

1. Thread-unsafe static variable in CrvTimingStudies_module.cc

static bool firstEvent=true;
if(firstEvent)
{
  firstEvent=false;
  // ... one-time initialization ...
}

Using a function-local static bool in an art::EDAnalyzer::analyze() method is not thread-safe in a multi-threaded art framework scheduling context. If art ever runs multiple module instances or uses shared-memory threading, this is a data race. Even in single-threaded mode, static local variables persist across all module instances if two CrvTimingStudies analyzers are configured — the second instance would skip the initialization entirely.

Recommendation: Use beginRun() or beginJob() for one-time initialization, or use a member variable bool _firstEvent{true}; instead of a static local.

2. Inconsistent behavior between CrvRecoPulsesFinder_module.cc and MakeCrvRecoPulses.cc

if(conf().pulseAreaThreshold()>conf().minADCdifference())
  throw cet::exception("CRVRECO_BAD_CONFIG")
  << "CrvRecoPulseFinder pulseAreaThreshold " << conf().pulseAreaThreshold()
  << "  larger than minADCdifference " << conf().minADCdifference();

if(_pulseAreaThreshold>_minADCdifference) _pulseAreaThreshold=_minADCdifference;

The module throws an exception when pulseAreaThreshold > minADCdifference, but the underlying MakeCrvRecoPulses constructor silently clamps the value. This is contradictory:

• If the module always throws first, the clamp in MakeCrvRecoPulses is dead code.
• If MakeCrvRecoPulses is constructed from other call sites that skip the module check, the behavior silently changes without warning.

Recommendation: Pick one consistent strategy. Either always throw (remove the clamp) or always clamp with a warning (remove the throw). The current combination is confusing.

---

🟡 Medium Issues

3. Missing default value for removeTimeOffsets fhicl parameter

fhicl::Atom<bool> removeTimeOffsets{ Name("removeTimeOffsets"), Comment("remove time offsets added by reco")};

Unlike other new parameters in this PR (e.g., minPeakPulseArea has default 250.0), removeTimeOffsets has no default value. This means every existing fcl configuration file that uses CrvTimingStudies will fail at validation time until updated. This is a breaking change for existing workflows.

Recommendation: Add a sensible default: fhicl::Atom<bool> removeTimeOffsets{ Name("removeTimeOffsets"), Comment("..."), false};

4. spectrumNPeaks default changed from 6 to 100 — potential memory/performance concern

fhicl::Atom<int> spectrumNPeaks{Name("spectrumNPeaks"), Comment("maximum number of peaks searched by TSpectrum"), 100};

TSpectrum allocates internal buffers proportional to nPeaks. Increasing from 6 → 100 is a 16× increase. While TSpectrum can handle this, it's a significant change that should be justified. The old comment mentioned that values ≤ 3 cause warnings — with 100, TSpectrum may find many spurious peaks in noisy histograms, and only the first 2 are checked in FindSPEpeak.

Recommendation: Document why 100 is the right default, or consider a smaller value (e.g., 10–20) that avoids the old warning while not being excessive.

5. FindSPEpeak only checks the first 2 peaks (by Y) — fragile logic

double x=peaksX[0];
if(x<minPeak)
{
  if(nPeaks==1) return false;
  x=peaksX[1];
  if(x<minPeak) return false;
}

The comments say "SPE peak is either the highest peak or second highest peak." But if TSpectrum finds 100 peaks (the new default), the logic only ever looks at peaks [0] and [1] (sorted by Y, i.e., the tallest two). If both the tallest and second-tallest peaks are below minPeak (e.g., due to noise), the function returns false even though a valid SPE peak may exist at index [2] or later. Given the increase to 100 allowed peaks, it would be more robust to iterate until the first peak above minPeak is found.

6. Fit option "QR" vs old "0QR" — fitting now draws to the current pad

In the old code the fit used "0QR" (the "0" suppresses drawing). The new code in FindSPEpeak uses "QR":

hist->Fit(&function, "QR");

Without the "0" option, ROOT will attempt to draw the fit result on the active canvas/pad. In batch mode this is harmless but can slow things down; in interactive mode it modifies histograms visually. This appears to be an accidental omission of the "0" flag.

Recommendation: Use "0QR" to suppress drawing, consistent with batch processing.

---

🟢 Minor Issues / Nits

7. Comment typo: "outcomment" is not standard English

#outcomment this line, if displaying simulated events

Suggestion: Use "comment out" → #comment out this line if displaying simulated events

8. maxFitDifference comment has inconsistent capitalization

fhicl::Atom<double> maxFitDifference{Name("maxFitDifference"), Comment("maximum Difference between the TSpectrum peak and the fitted peak. Indicates fit problems."), 100.0};

"Difference" is capitalized mid-sentence. Minor formatting nit.

9. Typo "FGPAs" in histogram title

Form("Time Diffs between FGPAs %i and %i;time difference [ns];Counts",fpga1->first,fpga2->first),

"FGPAs" should be "FPGAs". (This typo appears to be pre-existing but is in a line being modified — good opportunity to fix it.)

10. No outputFile error checking in CrvCalibration_module.cc

std::ofstream outputFile;
outputFile.open(_tmpDBfileName);
outputFile<<"TABLE CRVSiPM"<<std::endl;

There is no check that the file was opened successfully. If the path is invalid or permissions are wrong, all writes will silently fail. Consider adding if(!outputFile.is_open()) throw ...;.

11. Run/SubRun info removed from output table header

The old code wrote TABLE CRVSiPM <run:subrun>-<run:subrun> to the output file, providing traceability. The new code just writes TABLE CRVSiPM with no provenance information. This is a loss of metadata that may make it harder to identify which data the calibration file corresponds to.

---

Summary

Severity	Count	Key Items
🔴 Critical	2	Thread-unsafe static; contradictory throw vs. clamp
🟡 Medium	4	Missing default, excessive nPeaks, fragile peak selection, fit draw option
🟢 Minor	5	Typos, missing error checks, lost provenance

The most impactful items to address before merge are the thread-safety of the static firstEvent variable and the contradictory error handling between the module and MakeCrvRecoPulses. The missing default for removeTimeOffsets will also break existing configurations.

[ehrlich-uva]

1. Updated
2. Updated
3. This was done intentionally so that it needs to be decided for every fcl file.
4. 100 is ROOT's default value. The code was checked on real data multiple times and was found to work fine.
5. This logic has been tested and was found to work fine. It is based on a long experience with the analysis of Wideband data. Yes, if the first two peaks are below minPeak, then it will return false, because the calibration constant is not expected to be below minPeak.
6. This was done to get histograms with a drawn fit function - even when they are produced in batch mode.
7. Updated
8. Updated
9. Updated
10. Updated
11. Removed intentionally. The correct run/subrun range will be added when the table gets added to the database.

[oksuzian]

@FNALbuild run build test

[FNALbuild]

⌛ The following tests have been triggered for 3f3c4c5: build (Build queue - API unavailable)

[FNALbuild]

☀️ The build tests passed at 3f3c4c5.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 3f3c4c5 at 10120d9
build (prof)	✅	Log file. Build time: 08 min 51 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (1) FIXME (0) in 6 files
clang-tidy	➡️	8 errors 194 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 3f3c4c5 after being merged into the base branch at 10120d9.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.