Migrate calo calibration modules from CaloCalibration repository

[giro94]

For now, cosmics and noise modules moved over.
Source and combination will follow later.

The idea is to move here anything that must run at online level or in Pass-1/2/N steps.
Anything else can remain in the separate repo.

[FNALbuild]

Hi @giro94,
You have proposed changes to files in these packages:

• CaloCalibration
• /

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[giro94]

Unsure if we should keep the inner directory structure (CosmicsCalib, NoiseAnalysis, etc), or we should flatten everything into inc/src/fcl directories. @brownd1978

[FNALbuild]

☀️ The build tests passed at c41473e.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged c41473e at 0878d6b
build (prof)	✅	Log file. Build time: 04 min 26 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 (0) FIXME (0) in 4 files
clang-tidy	➡️	8 errors 942 warnings
whitespace check	✅	no whitespace errors found

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

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]

PR Review Summary — #1936

Reviewed at head c41473e9eb9851233e4da5ea8beef3876c2bc38b. First pass.

Decision

• 🔴 request changes

Scope understood

• Migrates four calorimeter calibration modules out of Mu2e/CaloCalibration into Offline/CaloCalibration/: CosmicsCalib (CaloCosmicEnecalib, CaloCosmicEnergy, caloT0alig) and NoiseAnalysis (BaselineAnalyzer plus analyzeBaselines.fcl). Source and combination steps are stated as following later.
• The stated intent is that anything needed at online level or in Pass-1/2/N lives in Offline; the rest stays in the separate repo.
• Note on the prior review: @sophiemiddleton approved at this same head with an empty body, so there are no findings to carry forward. The blocker below is a build-system issue that an approval does not address; the CI green also does not cover it, for the reason given in finding 1.

Findings

1. 🔴 [S0] The CMake build of Offline no longer configures.

   • Evidence: CMakeLists.txt:126 adds add_subdirectory(CaloCalibration), but at this head CaloCalibration/ contains only CosmicsCalib/ and NoiseAnalysis/ — there is no CaloCalibration/CMakeLists.txt
     (gh api repos/Mu2e/Offline/contents/CaloCalibration?ref=c41473e9 returns exactly those two entries). Reproduced against the real cmake:

     CMake Error at CMakeLists.txt:3 (add_subdirectory):
       The source directory .../CaloCalibration does not contain a CMakeLists.txt file.
     

     Adding the missing intermediate file then exposes a second, independent error, because CaloCalibration/NoiseAnalysis/CMakeLists.txt:16 begins with a stray %:

     CMake Error at CaloCalibration/NoiseAnalysis/CMakeLists.txt:1:
       Parse error.  Expected a command name, got unquoted argument with text "%install_headers".
     

     CaloCalibration/CosmicsCalib/ has no CMakeLists.txt at all, so its three modules would not be built by CMake even once configuration succeeds.
   • Why CI is green anyway, and why this is not caught: mu2e/buildtest is the scons-via-Muse build, and it did compile and link all four modules (scons.log lines 713-780, including -Wl,--no-undefined). The check_cmake job iterates for dir in $PWD/* and only descends where $dir/src exists (bin/check_cmake.sh:33, guarded by the -d $dir/src test at :8), so a two-level package is invisible to it — it reported success without ever looking at CaloCalibration.
   • Suggested fix: add CaloCalibration/CMakeLists.txt with add_subdirectory(CosmicsCalib) and add_subdirectory(NoiseAnalysis); add CaloCalibration/CosmicsCalib/CMakeLists.txt with a cet_build_plugin block per module; and in NoiseAnalysis/CMakeLists.txt drop the % and use the spelling the rest of the repo uses, install_headers(USE_PROJECT_NAME SUBDIRS inc) — or drop that line entirely, since there is no inc/ directory here. Two smaller items in the same file: install_fhicl(SUBDIRS fcl SUBDIRNAME CaloCalibration/NoiseAnalysis/fcl) is missing the Offline/ prefix that every other install_fhicl in the repo carries, and the file has no trailing newline.

2. 🔴 [S0] Out-of-bounds writes in CaloCosmicEnergy when a hit lands exactly on the top of the energy range.

   • Evidence: CaloCosmicEnergy_module.cc:408-416

     if (sipm_mean_e <= 55.) {
       int whichband = sipm_mean_e / 5;
       Energy_band[whichband] += sipm_mean_e;
       counter_energy_band[whichband]++;
       LR[whichband]->Fill(...);
       ALR[whichband]->Fill(...);
       CryALR[crystal_id][whichband]->Fill(...);
       Cry_Energy_band[crystal_id][whichband] += sipm_mean_e;
       Cry_counter_energy_band[crystal_id][whichband]++;
     }

     Ebin is 11 (:98) and all five of those arrays are dimensioned [Ebin] (:120-122, :128-129). A hit with energyDep() of exactly 55.0 MeV gives whichband == 11, one past the end of every one of them. LR, ALR and CryALR are arrays of TH1F*, so the write is preceded by a read of an out-of-range pointer which is then dereferenced through ->Fill(). A negative energyDep() — which the reconstruction can produce on a noise-dominated channel — indexes at -1 by the same path, since the guard has no lower bound.
   • Impact: heap corruption or a segfault in a calibration job, dependent on input values, so it will not show up reliably in a short test.
   • Suggested fix: make the guard exclusive and two-sided — if (sipm_mean_e >= 0. && sipm_mean_e < Ebin * 5.) — and derive the band width from a named constant rather than the literal 5 repeated at :186, :204, :207 and :409.

3. 🟠 [S1] std::string constructed from a possibly-null getenv, with the emptiness check placed after the fact.

   • Evidence: three sites. caloT0alig_module.cc:142 std::string _fileT0 = getenv("MUSE_WORK_DIR");, caloT0alig_module.cc:446-450 and CaloCosmicEnergy_module.cc:433-437:

     std::string outDir = std::getenv("OUTDIR");
     if (outDir.length() == 0) {
       mf::LogError("OUTDIR-NOT-SET") << "Environmental variable for calib output file not set ";
     }

     Constructing std::string from a null pointer is undefined behaviour, so the LogError below it can never run for the case it is written for; and when it does run, execution continues and the job writes to /tcorr.dat and /calib_parameters.dat at the filesystem root. MUSE_WORK_DIR in particular is set by Muse and will not be present in a CMake/spack-installed release, which is the environment this migration is meant to serve.
   • Impact: a segfault, or output silently written outside the intended directory, from an unset environment variable.
   • Suggested fix: CaloCosmicEnecalib in this same PR already shows the pattern to follow — an OutCalibFile fhicl atom, opened in the constructor, with throw cet::exception(...) when the open fails (CaloCosmicEnecalib_module.cc:73-75, :187-190). Give the other two modules the same treatment and drop the getenv calls; the T0 data file path should likewise come from fhicl rather than from MUSE_WORK_DIR.

4. 🟠 [S1] The migration is incomplete: caloT0alig reads a data file that was not moved, and neither was any fcl for CosmicsCalib.

   • Evidence: caloT0alig_module.cc:142-157 reads $MUSE_WORK_DIR/CaloCalibration/CosmicsCalib/data/t0s_allchan_1ns.dat. That file exists in the source repo (Mu2e/CaloCalibration → CosmicsCalib/data/t0s_allchan_1ns.dat) but is not in this PR — CaloCalibration/CosmicsCalib/ contains only src. CosmicsCalib/vst/ was not migrated either, and NoiseAnalysis is the only one of the two packages that brings its fcl/ directory across.
   • Impact: if (T0File.is_open()) simply fails and the job proceeds with Toff[] all zeros — no warning, no error, and a plausible-looking set of residuals out the far end. The three CosmicsCalib modules also have no runnable configuration in Offline, so nothing in the repo exercises them.
   • Suggested fix: bring data/ and a driver fcl across with the modules, and treat a missing T0 file as fatal rather than as a silent zero. Worth stating explicitly in the PR body if vst/ is deliberately staying behind.

5. 🟠 [S1] caloT0alig writes into fixed arrays using an index read straight out of a text file.

   • Evidence: caloT0alig_module.cc:150-151 while (T0File >> iChanT0 >> TvalT0) { Toff[iChanT0] = TvalT0; ... } and :174-175 while (inpFile >> iChan >> Tval >> ...) { Tcor[iChan] = Tval; ... }. Toff and Tcor are float[nROchan] with nROchan == 2696 (:99-102), and neither loop bounds-checks the index. The count is only checked afterwards, at :181, and only for the second file.
   • Impact: a stale or corrupted calibration file — exactly the class of input this iterative procedure regenerates each pass — overwrites arbitrary memory.
   • Suggested fix: reject iChan < 0 || iChan >= nROchan inside both loops with a cet::exception.

6. 🟠 [S1] CaloCosmicEnergy divides by a path length that its own helper can return as zero.

   • Evidence: CaloCosmicEnergy_module.cc:759-849 — findpath initialises float path = 0; and has an else branch (:840-844) that assigns nothing when no crystal face is crossed; it also leaves xup/xlow/yleft/yright at zero when m == 0 (:769). The result is used unguarded at :384: ... ->energyDep() * cryDim / path[iCry].
   • Impact: an infinity is filled into hSiPMfp, where it lands in the overflow bin and quietly biases the normalized-track MPV.
   • Suggested fix: the newer CaloCosmicEnecalib already guards this — else if ((chi2norm < CutChi2Norm) && (path[kk] > 0)) at CaloCosmicEnecalib_module.cc:429. Apply the same guard here, and have findpath signal "no path" explicitly rather than returning a value that reads as a real length.

Smaller items

• 🟡 Dead code, several kinds: caloT0alig_module.cc:424-426 is unreachable after return retval; at :422; int diag = 0; with a dozen if (diag == 1) blocks that can never run appears in both CaloCosmicEnergy_module.cc:766 and CaloCosmicEnecalib_module.cc:602; commented-out code at caloT0alig_module.cc:468-470, CaloCosmicEnergy_module.cc:213-215 and :236-241, and analyzeBaselines.fcl:18 (#@local::Services.Reco); TFitResultPtr fitresult at CaloCosmicEnergy_module.cc:315 is never read; _nProcessed/_nFiltered are counted in caloT0alig and never reported anywhere.
• 🟡 CaloCosmicEnergy_module.cc:292 assigns max_y = PosX[h]; inside the loop that is scanning PosY. Dy is only ever printed at _diagLevel > 0 (:303), so nothing downstream is wrong today, but the variable is both mis-computed and otherwise unused — either fix it or drop it. The two loops at :273 and :289 are also labelled "bubble sort" when they are min/max scans.
• 🟡 caloT0alig_module.cc:326-333 mixes an SiPM-local id with a vector position: idx is CaloSiPMId::SiPMLocalId(), which is _id % 2 (DataProducts/inc/CaloSiPMId.hh:25), but it is then used to subscript hit.recoCaloDigis().at(idx) while the loop itself runs over iCha. On a crystal with a single surviving readout whose local id is 1, .at(1) throws std::out_of_range; where both are present but not stored in local-id order, one digi is read twice and the other never. Index with iCha and use the local id only where a local id is meant.
• 🟡 Silent degradation in BaselineAnalyzer: a CSV that will not open produces std::cout << "Warning! ..." and sets writeCSV_ = false (:272-278), so the job exits 0 having produced no thresholds; and channels with no data are given a fabricated baseline of 2048 that is then written into the threshold CSV alongside the measured ones (:401-412), with nothing in the file marking them as defaults. Both should be errors.
• 🟡 Numbers that already have a home elsewhere. BaselineAnalyzer_module.cc hardcodes 16100 (:139, :510) where CaloConst::_nDIRAC is 161, 20 (:403, :418, :424, :567) where it uses CaloConst::_nChPerDIRAC correctly at :182, board < 80 for the disk split (:382), and 2048 as the pedestal (:136-137, :408). CaloCosmicEnergy_module.cc fills the position error with 9.81f and a comment deriving it from a 34 mm crystal (:231) while reading the real crystal dimension from the geometry twelve lines earlier (:156), and carries three different vertical-track thresholds — Dx < 33 (:325), Dx < 35 (:368) and MaxDxVertical = cryDim * 1.1 (:158, :340). caloT0alig_module.cc:310 uses 3.1416 for π and :94 redefines the speed of light as cvel = 299.792458 rather than CLHEP::c_light.
• 🟡 CaloCosmicEnergy and CaloCosmicEnecalib carry verbatim copies of findpath (~70 lines), of the Landau-Gauss convolution (langaufun / langaus), and of the 9.81 / 144. / cryDim * 1.1 constants. Since the header of CaloCosmicEnecalib_module.cc:5-6 describes it as the successor to CaloCosmicEnergy, it is worth saying in the PR body whether both are meant to live in Offline long-term; if they are, the shared pieces belong in one place.
• 🟡 BaselineAnalyzer's writeTXT/TXTfoldername, writeCSV/CSVfilename and writePDF/PDFfilename are the flag-plus-loose-atoms shape that fhicl::OptionalTable<Config> exists for; with writeTXT: true and the default empty folder the module writes to /dirac000.baseline. Separately, the C++ defaults are thresholdOffset = 100, thresholdOffsetPin = 50 (:73-74) while analyzeBaselines.fcl:56-57 sets 50 and 100 — the two are swapped relative to each other, which is worth confirming is deliberate.
• ⚪ Collapsed nits: BaselineAnalyzer_module.cc:12-13 and :19-22 include five artdaq headers (Fragment, ContainerFragment, EventHeader, DTCEventFragment, CalorimeterDataDecoder, FragmentType) that the module never uses, and those are what pull the four artdaq-core* entries into its link list; TFile, TEllipse, TTree and <sys/stat.h> are unused there too, as are GlobalConstantsHandle.hh, TDirectory.h, Selector.h and Sequence.h in caloT0alig_module.cc. Class caloT0alig starts lowercase where the repo capitalises type names; its beginJob/endJob/filter are virtual without override (:78-80). Prints go to std::cout rather than message-facility throughout, several of them unguarded by any verbosity flag (BaselineAnalyzer_module.cc:275, :291, :331, :434). The "*** TO BE IMPLEMENTED ***" markers at caloT0alig_module.cc:132 and :244 are real TODOs that the FIXME/TODO CI counter does not match on.

On the directory-structure question

Keeping CosmicsCalib/NoiseAnalysis as subdirectories is fine and has precedent: ExtinctionMonitorFNAL/ is a two-level package whose top-level CMakeLists.txt is nothing but seven add_subdirectory lines plus its own install_fhicl, and each leaf carries the usual cet_build_plugin / install_source / install_headers block. Copying that shape is exactly what finding 1 asks for. The one real cost is the check_cmake.sh blindness described above, which ExtinctionMonitorFNAL shares — that is a pre-existing gap in the CI script, not something this PR introduced, but it does mean a nested package gets less automatic protection than a flat one.

Validation check

• Build/tests run: partial. mu2e/buildtest is green at this head and genuinely compiled and linked all four modules under -Werror (scons.log:713-780); whitespace clean; FIXME/TODO 0 in 4 files; clang-tidy reported 8 errors / 942 warnings, which I did not attribute. I reproduced the two CMake errors in finding 1 against a minimal tree with the same file layout.
• Config contract check: pass for the one new fcl. fhicl-dump Offline/CaloCalibration/NoiseAnalysis/fcl/analyzeBaselines.fcl under SimJob/MDC2025av with an appended shim exits 0 and resolves to 1216 lines; CaloDigisFromDTCEvents (DAQ/src/) and CaloVisualizer/inc/THMu2eCaloDisk.hh both exist in Offline at this head. CosmicsCalib has no fcl to check.
• Cross-repo consistency: needs follow-up — see finding 4 for what stayed behind in Mu2e/CaloCalibration.

Residual risk

• I did not review the physics of the calibration procedures themselves (langaus fitting strategy, the asymmetry-to-Npe inversion, the T0 iteration scheme), only their implementation.
• Nothing in Offline runs the three CosmicsCalib modules, so none of them has runtime coverage here; the failure modes in findings 2, 5 and 6 are reached by particular input values and would not surface in a short smoke test.

Author follow-ups

1. Add CaloCalibration/CMakeLists.txt and CaloCalibration/CosmicsCalib/CMakeLists.txt, and fix the %install_headers line, the missing Offline/ prefix on install_fhicl, and the missing trailing newline in NoiseAnalysis/CMakeLists.txt. Please confirm with a local CMake configure, since buildtest will not catch it.
2. Bound the energy-band index in CaloCosmicEnergy and the file-read indices in caloT0alig.
3. Replace the three getenv calls with fhicl parameters that throw when the target cannot be opened, following CaloCosmicEnecalib.
4. Migrate CosmicsCalib/data/ and a driver fcl, or say in the PR body what is deliberately staying in Mu2e/CaloCalibration.
5. Guard the cryDim / path division in CaloCosmicEnergy.
6. Say whether CaloCosmicEnergy and CaloCosmicEnecalib are both intended to live here long-term; if so, the duplicated findpath and langaus code should get a single home.

[bechenard]

Ok, but I would like to avoid creating 50 Calo folders in the future

[giro94]

Ok, but I would like to avoid creating 50 Calo folders in the future

Do you mean within this CaloCalibration folder or the number of CaloXXX folders in Offline? @bechenard
If the former, I would be happy to flatten out if preferred.
If the latter, the balance with STM / CRV / Tracker is not so unbalanced...

[FNALbuild]

📝 The HEAD of main has changed to eba23bd. Tests are now out of date.