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.

[oksuzian]

PR Review Summary — #1936

Reviewed at head 415724542483228793acda0e9e94453e8f925a81. Re-review of c41473e9.

Decision

• 🔴 request changes

Both blockers from the previous pass are fixed, and I verified each rather than taking
the commit messages for it. The delta — 764a2eca "added cmakelists and prologs" and
41572454 "Addressed energy range bug", six files — introduces one new S0, the
NoiseAnalysis prolog cannot be parsed, plus one new S1 in the CMake library lists.
Findings 3, 5 and 6 from the previous pass are untouched; finding 4 is partly addressed.

Fixed since c41473e9

• 🟢 [was S0] The CMake build configures again. With CaloCalibration/CMakeLists.txt,
  CosmicsCalib/CMakeLists.txt and the % gone from NoiseAnalysis/CMakeLists.txt:16,
  a configure over the new tree with the cet_* macros stubbed exits 0 and registers
  all four plugins — CaloCosmicEnecalib, CaloCosmicEnergy, caloT0alig,
  BaselineAnalyzer. Both install_fhicl calls now carry the Offline/ prefix, and
  the missing trailing newline is gone.
• 🟢 [was S0] The energy-band index is bounded. CaloCosmicEnergy_module.cc:409 is now
  if (sipm_mean_e >= 0. && sipm_mean_e < Ebin * Erange) with
  whichband = sipm_mean_e / Erange at :410. With Ebin = 11, Erange = 5
  (:98-99) that admits 0 through 10 against arrays dimensioned [11] (:123,
  :129-130), and the negative case is closed. The literal 5 is gone from all four
  sites.

Findings

1. 🔴 [S0] NoiseAnalysis/fcl/prolog.fcl does not parse — it references a name that
   does not exist

   • Evidence: :8 defines CaloBaselineAna, but :21 reads
     CaloBaselineAna : @local::CaloCaloBaselineAna — Calo doubled. fhicl-dump on a
     file whose only content is an include of this prolog aborts, exit 134:

     ---- Parse error BEGIN
       Local lookup error
       ---- Can't find key BEGIN
         CaloCaloBaselineAna (at part "CaloCaloBaselineAna")
       ---- Can't find key END
       at line 21, character 27, of file ".../NoiseAnalysis/fcl/prolog.fcl"
     

     The sibling CosmicsCalib/fcl/prolog.fcl dumps clean at exit 0, so it is this one
     line.
   • Impact: the prolog is unusable as committed, and nothing in CI includes it, so it
     will fail for the first person who tries to use it.
   • Suggested fix: CaloBaselineAna : @local::CaloBaselineAna.

2. 🟠 [S1] The three new cet_build_plugin blocks carry NoiseAnalysis's library list
   verbatim, which is not the list these modules need

   • Evidence: the LIBRARIES REG block is byte-identical across all four plugin
     declarations in the two new CMakeLists. It names Offline::CaloConditions,
     Offline::CaloVisualizer, Offline::CaloVisualizer_dict, Offline::DAQ and
     Offline::ProditionsService — the packages BaselineAnalyzer includes, none of
     which appears in any CosmicsCalib module. Meanwhile all three CosmicsCalib
     modules construct GeomHandle<Calorimeter> (CaloCosmicEnergy_module.cc:155,
     CaloCosmicEnecalib_module.cc:202, caloT0alig_module.cc:253) and
     Offline::GeometryService is not listed. Offline::CalorimeterGeom does arrive
     transitively via CaloConditions, but GeometryService appears in the PUBLIC list
     of none of the eight entries. The closest sibling, CaloReco's CaloHitMaker —
     the other module in Offline doing GeomHandle<Calorimeter> — declares both
     Offline::CalorimeterGeom and Offline::GeometryService explicitly.
   • Impact: the standard asks for all first-order dependencies to be declared, and CI
     never builds with CMake, so a wrong CMake link list stays green through
     buildtest indefinitely — the same gap that hid the previous blocker.
   • Suggested fix: derive each list from that module's own includes instead of copying
     the donor, and confirm with a real cmake build, not a configure. I traced the
     PUBLIC lists one level only and did not close the dependency graph exhaustively, so
     please let the build be the authority.

3. 🟠 [S1] Carried over, unaddressed — std::string from a possibly-null getenv.
   caloT0alig_module.cc:142 and :446, CaloCosmicEnergy_module.cc:434, all
   unchanged at this head.

4. 🟠 [S1] Carried over, partly addressed — incomplete migration.
   CosmicsCalib/fcl/prolog.fcl now exists, which closes part of this, but it
   configures only CaloCosmicEnecalib; CaloCosmicEnergy and caloT0alig still have
   no configuration in Offline, and CosmicsCalib/data/t0s_allchan_1ns.dat — read at
   caloT0alig_module.cc:142-157 — is still not in the PR, so that read still degrades
   silently to all-zero offsets. When you add caloT0alig: it is an art::EDFilter
   (:57), so it belongs under filters:, not analyzers:.

5. 🟠 [S1] Carried over, unaddressed — unbounded array indices read from a text
   file, caloT0alig_module.cc:150-151 and :174-175.

6. 🟠 [S1] Carried over, unaddressed — CaloCosmicEnergy_module.cc:385 still
   divides by path[iCry] unguarded, while CaloCosmicEnecalib_module.cc:429 still
   carries the path[kk] > 0 guard it needs.

Smaller items

• 🟡 New, on a line this delta touched: the CryALR titles are off by one band.
  CaloCosmicEnergy_module.cc:186-187 labels bin ibin as
  [(ibin+1)*Erange, (ibin+1)*Erange + Erange), so bin 0 is titled "[5, 10)" while it
  is filled from [0, 5). The LR/ALR titles at :205 and :208 get it right with
  i * Erange, (i + 1) * Erange.
• 🟡 The threshold offsets now disagree three ways within this PR: C++ defaults are
  thresholdOffset 100 / thresholdOffsetPin 50 (BaselineAnalyzer_module.cc:73-74),
  analyzeBaselines.fcl:56-57 sets 50 / 100, and NoiseAnalysis/fcl/prolog.fcl:15-16
  sets 100 / 100. Worth settling on one and saying which is right.
• 🟡 Both new prologs set writeCSV : true and writePDF : true while leaving
  CSVfilename and PDFfilename at their empty defaults, so the flag-plus-empty-path
  combination the previous review flagged is now committed rather than hypothetical.
• ⚪ Collapsed: #install_headers(SUBDIRS inc) is commented out rather than deleted, now
  in both new CMakeLists, and neither package has an inc/. Both new prologs #include
minimalMessageService.fcl, standardProducers.fcl and standardServices.fcl, where
  every sibling calo prolog (CaloReco, CaloMC, CaloCluster, CaloDiag) includes
  only other prologs and leaves those to the job fcl — I checked the double include is
  harmless, a job including standardServices.fcl then CosmicsCalib/fcl/prolog.fcl
  dumps clean at exit 0, so this is convention rather than a defect. Every other ⚪ and
  🟡 from the previous review still stands; those files are untouched in this delta.

On the open directory-structure question

@giro94 asked @bechenard on 2026-08-24 whether "50 Calo folders" meant inside
CaloCalibration/ or across Offline, and that is still unanswered — a question between
the two of you, not something I am carrying as a finding. For what it is worth, the
previous review's answer stands: the nested shape has precedent in
ExtinctionMonitorFNAL/, and the CMakeLists just added are exactly that shape.

Validation check

• Build/tests run: mu2e/buildtest is pending at this head, so there is no result
  for the new commits; a run is already queued and I did not trigger a second. The green
  from c41473e9 describes code that has changed, and FNALbuild has marked it out of
  date. My own checks at this head were the stubbed CMake configure and fhicl-dump on
  both new prologs.
• Config contract check: partial. Every key in both new prologs exists as a
  fhicl::Atom with a default in the matching Config
  (CaloCosmicEnecalib_module.cc:55-74, BaselineAnalyzer_module.cc:64-76), so no
  required key is missing. I could not run mu2e -c, since these modules are not built
  in the release I dumped against, so full art validation is unchecked.
• Cross-repo consistency: needs follow-up, unchanged — see finding 4.

Residual risk

• Unchanged: I have not reviewed the physics of the calibration procedures, and nothing
  in Offline runs the three CosmicsCalib modules, so findings 5 and 6 are reached only
  by particular input values.

Author follow-ups

1. Fix @local::CaloCaloBaselineAna at NoiseAnalysis/fcl/prolog.fcl:21.
2. Give each cet_build_plugin the list its own module needs, and confirm with a local
   CMake build — the configure will not catch it, and neither will CI.
3. Items 3, 5 and 6 are unchanged from the previous review.
4. Add fcl for CaloCosmicEnergy and caloT0alig (the latter as a filter), and bring
   CosmicsCalib/data/ across or state in the PR body that it is staying behind.
5. Settle the threshold-offset values across the code and the two fcl files.

[FNALbuild]

☀️ The build tests passed at fe78246.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged fe78246 at 8ca6473
build (prof)	✅	Log file. Build time: 04 min 25 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 937 warnings
whitespace check	✅	no whitespace errors found

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

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.

[FNALbuild]

☀️ The build tests passed at 137a913.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 137a913 at 8ca6473
build (prof)	✅	Log file. Build time: 08 min 58 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 937 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 137a913 after being merged into the base branch at 8ca6473.

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 fe782469b5b62c99976e23203000c0d740db5bf3. Re-review of 0d1585fa.

Decision

• 🔴 request changes

@giro94 — this round cleared almost everything, including all four of the S1s. The one
blocker left is a single character: the prolog spells the module CaloT0Align while the
build produces CaloT0align. Everything else below is 🟡 or carried.

Delta reviewed: 7662ea46..fe782469, eight files, including the
caloT0alig → CaloT0align rename.

Findings

1. 🔴 [S0] module_type : CaloT0Align does not match the plugin the build produces

   • Evidence: the build side is CaloT0align throughout — cet_build_plugin(CaloT0align art::module) with REG_SOURCE src/CaloT0align_module.cc
     (CosmicsCalib/CMakeLists.txt:25-26), and DEFINE_ART_MODULE(mu2e::CaloT0align)
     (CaloT0align_module.cc:503). The prolog spells it with a capital A at
     CosmicsCalib/fcl/prolog.fcl:30-31 and :45. Running the two side by side on this
     head:

     $ cmake ...                       $ fhicl-dump <includes CosmicsCalib prolog>
     -- plugin: CaloCosmicEnecalib     module_type: "CaloCosmicEnecalib"
     -- plugin: CaloCosmicEnergy       module_type: "CaloCosmicEnergy"
     -- plugin: CaloT0align            module_type: "CaloT0Align"      <-- only mismatch
     

     The other two agree exactly; this one differs by one character. art resolves
     module_type against the plugin/target name, not the C++ class — DAQ's
     CaloDigisFromDTCEvents is the proof in this same PR: its class is
     CaloDigiFromDTCEvents, singular, and the working module_type is the plural target
     name.
   • Impact: any job using @local::CaloT0Align or the CosmicsCalib.filters block fails
     at construction with a plugin-not-found error. Note this is invisible to the checks
     run so far — fhicl-dump exits 0 because the config parses fine, and buildtest
     never instantiates the module.
   • Suggested fix: since the rename was for capitalisation anyway, finish it — make the
     file, target, class and module_type all CaloT0Align, which also gets Align
     into bouncingCapitals. Renaming the prolog key down to CaloT0align works too, but
     leaves the odd spelling.

2. 🟡 [S2] fileTcor is now both the input and the output

   • CaloT0align_module.cc:181 opens _fileTcorName for reading in beginJob, and
     :463 opens the same _fileTcorName for writing in endJob. Before this delta the
     output went to $OUTDIR/tcorr.dat, a different path. The atom is still documented as
     an input — Comment("T0 corrections input file") at :72. Each pass of the
     iteration therefore overwrites the file it read, so there is no history to compare
     across iterations and a crash mid-write destroys the input. If that is deliberate,
     the comment should say so; otherwise a separate fileTcorOut atom keeps the two
     roles apart.

3. 🟡 [S2] Two leftovers from the file-handling rework, both in CaloT0align_module.cc

   • :164-167: the else branch logging "T0 file from previous iteration not found" is
     unreachable — the constructor at :122-126 already throws when that file will not
     open.
   • :199-203: inside the else of if (_fileTcor.is_open()), so the guard
     if (!_fileTcor.is_open()) is always true, and its message says "Cannot open output
     file" for what is an input at that point.

4. 🟠 [S1] Carried over, still open — CosmicsCalib/data/t0s_allchan_1ns.dat is still
   not in the PR. The prolog now ships fileT0 : "fileT0.dat" (:38) as a bare relative
   name with nothing in the repo supplying it, and the constructor throws when it is
   absent, so the entry as committed cannot run anywhere. Either migrate the data file and
   point at it, or say in the PR body that it stays in Mu2e/CaloCalibration and the path
   is the operator's to supply.

Fixed since 0d1585fa — all verified against the code, not the commit messages

• 🟢 [was S1] The zero-path division is guarded. findpath now initialises
  float path = -1 (CaloCosmicEnergy_module.cc:771) instead of 0, and the only
  consumer is wrapped in if (path[iCry] > 0) at :398, around the
  cryDim / path[iCry] at :405.
• 🟢 [was S1] Every getenv is gone from the package — grep -rn getenv over
  CaloCalibration/ at this head returns nothing. CaloT0align::endJob writes through
  the fcl-supplied path and throws when it cannot open it.
• 🟢 [was S1] The first-iteration blocker is resolved. The fileTcor stream is now a
  local opened inside if (_iteration == "middle" || "last") (:180-181), so
  iteration : "first" no longer needs a corrections file to construct.
• 🟢 [was S1] CosmicsCalib is complete and correctly kinded: CaloCosmicEnergy joins
  analyzers, and CaloT0Align goes under a new filters: block — right, since it is an
  art::EDFilter.
• 🟢 [was S2] The four Config().fileX() calls are replaced by the stored _fileT0Name /
  _fileTcorName, so those messages now name the file.
• 🟢 [was S2] The CryALR titles are correct: ibin * Erange, (ibin + 1) * Erange
  (:203-204), matching LR/ALR.
• 🟢 [was S2] The threshold offsets agree in all three places — 100/100 at
  BaselineAnalyzer_module.cc:73-74, analyzeBaselines.fcl:56-57 and
  NoiseAnalysis/fcl/prolog.fcl:12-13.
• 🟢 [was S3] 16100 is now CaloConst::_nDIRAC*100 at both sites (:139, :510); the
  commented-out #install_headers lines are deleted from both CMakeLists; and both
  prologs now include only CaloMC/CaloReco prologs, matching the sibling calo
  convention. Both still dump clean, exit 0, after dropping those includes.
• 🟢 A stubbed CMake configure over the tree at this head still exits 0 and registers all
  four plugins.

Withdrawn

• The previous review carried "both prologs set writeCSV/writePDF true while leaving
  the filenames empty". That was wrong, and I should have checked it then:
  BaselineAnalyzer::beginRun derives a run-numbered name whenever either is empty
  (:193-200), so the shipped combination is fine and evidently intended. The
  OptionalTable point survives only for writeTXT/TXTfoldername, which gets no such
  treatment — :286 builds Form("%s/dirac%03d.baseline", TXTfoldername_.c_str(), board),
  so an empty folder would write to the filesystem root. writeTXT is false everywhere
  in the PR, so nothing ships broken; it is a latent trap, ⚪ at most.

Resolved between reviewers

• @bechenard's directory-structure point is answered and explicitly non-blocking — "I
  meant the latter (avoid having 50 caloXXX in Offline). This is still ok, but we should
  avoid having uncontrolled growth!" I am not carrying it as a finding.

Validation check

• Build/tests run: the commit status for mu2e/buildtest at this head is success
  (09:30:33). Worth noting that FNALbuild's own result comment names 137a9130, three
  commits back, so the table and the status disagree about which commit was tested; the
  three commits in between are def8cc5b, b8c2b4cf and fe782469. I did not trigger
  another run. My own checks at this head: the stubbed CMake configure, fhicl-dump on
  both prologs and on the CosmicsCalib block, and the plugin-name comparison in
  finding 1.
• Config contract check: partial. Prolog keys match the Config atoms for all three
  CosmicsCalib modules and for BaselineAnalyzer; module_type is where it breaks, and
  that is finding 1. Still no mu2e -c run, since these modules are not built in the
  release I dumped against.
• Cross-repo consistency: needs follow-up — see finding 4.

Residual risk

• Unchanged: I have not reviewed the physics of the calibration procedures. The remaining
  🟡 items in BaselineAnalyzer and CaloCosmicEnecalib from the first review still
  stand — those files are untouched apart from the _nDIRAC change.

Author follow-ups

1. Make the module name agree across file, cet_build_plugin target, class and
   module_type. CaloT0Align everywhere is the version I would pick.
2. Say whether fileTcor is meant to be read and rewritten in place, or split the output
   onto its own atom.
3. Drop the unreachable else at :164-167 and the always-true guard at :199-203.
4. Migrate CosmicsCalib/data/, or state in the PR body that the fileT0 path is
   supplied by the operator.

[oksuzian]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 657b9fa.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 657b9fa at 3f6e789
build (prof)	✅	Log file. Build time: 04 min 28 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 937 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 657b9fa after being merged into the base branch at 3f6e789.

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 657b9fa4ccec1ad55d78b4e212579f0b01696b48. Re-review of fe782469.

Decision

• 🟡 comment only

@giro94 — the blocker is fixed. All four names now agree, and I verified it the same way
I found the mismatch. Nothing left that gates the merge; what remains is one carried S1
about migration completeness and two small cleanups in CaloT0Align_module.cc.

Delta reviewed: fe782469..657b9fa4, one commit ("fix name typo"), two files — the
CaloT0align → CaloT0Align rename of the file, the cet_build_plugin target, the class
and every cet::exception category and std::cout prefix inside it.

Fixed since fe782469

• 🟢 [was S0] The module name agrees across all four places. Running the same two commands
  side by side at this head, the three CosmicsCalib plugins now match exactly:

  $ cmake ... (stubbed macros)      $ fhicl-dump <CosmicsCalib prolog>
  -- plugin: CaloCosmicEnecalib     module_type: "CaloCosmicEnecalib"
  -- plugin: CaloCosmicEnergy       module_type: "CaloCosmicEnergy"
  -- plugin: CaloT0Align            module_type: "CaloT0Align"
  

  Source of each: CosmicsCalib/CMakeLists.txt:25-26 (cet_build_plugin(CaloT0Align ...),
  REG_SOURCE src/CaloT0Align_module.cc), DEFINE_ART_MODULE(mu2e::CaloT0Align) at
  CaloT0Align_module.cc:503, and module_type : CaloT0Align at
  CosmicsCalib/fcl/prolog.fcl:31. grep -rn T0align over the whole tree at this head
  returns nothing, so no reference to the old spelling survives anywhere — including the
  scons path, which derives the plugin name from the same file base name.
• 🟢 The case-only rename landed cleanly in git. That is worth checking rather than
  assuming: a rename differing only in case is the one git records as a rename on Linux
  but can silently drop on a case-insensitive checkout, leaving both paths. The tree at
  this head contains exactly one file, CosmicsCalib/src/CaloT0Align_module.cc, and the
  compare API reports it as renamed with previous_filename set.
• 🟢 Align in bouncingCapitals also resolves the last of the naming nits from the first
  review, where the class was caloT0alig.

Findings still open

1. 🟠 [S1] Carried over, unchanged — migration completeness for the fileT0 input.

   • Evidence: CosmicsCalib/fcl/prolog.fcl:38 ships fileT0 : "fileT0.dat", a bare
     relative name; there is no CosmicsCalib/data/ in the PR, and nothing in the tree
     provides that file. The constructor throws when it cannot be opened
     (CaloT0Align_module.cc:122-126), so the entry as committed cannot run in the
     directory it is launched from.
   • Impact: bounded. No fcl anywhere in Offline includes the CosmicsCalib prolog
     (grep -rn CosmicsCalib --include=*.fcl finds only the prolog itself), so nothing
     breaks at merge and no CI job touches it — which is why this is a comment and not a
     change request. The cost is that the shipped default is a value nobody can use, and
     the next person to pick this up has no way to tell whether the file was forgotten or
     is meant to come from outside.
   • Suggested fix: either migrate the data file and point fileT0 at a repo-relative
     path, or say in the PR body that it stays in Mu2e/CaloCalibration and the path is
     the operator's to supply. One sentence closes this.

2. 🟡 [S2] Carried over — fileTcor is both the input and the output.
   CaloT0Align_module.cc:181 opens _fileTcorName for reading in beginJob; :463
   opens the same name for writing in endJob. The atom is documented as an input only —
   Comment("T0 corrections input file") at :72. Each iteration overwrites the file it
   read, so nothing survives to compare across iterations and a crash mid-write destroys
   the input. If in-place is deliberate, say so in the comment; otherwise a separate
   fileTcorOut atom keeps the two roles apart.

3. 🟡 [S2] Carried over — two leftovers from the file-handling rework, both still at
   the same lines after the rename. :164-167: the else logging "T0 file from previous
   iteration not found" is unreachable, since the constructor at :122-126 already threw.
   :199-203: if (!_fileTcor.is_open()) sits inside the else of
   if (_fileTcor.is_open()), so it is always true, and its message says "Cannot open
   output file" for what is an input at that point.

4. ⚪ On lines this delta touched: :156 and :186 both read << "from file " with no
   leading space, so each message renders as ... invalid channel 5from file .... At
   :155-156 the rename also left the continuation two columns short of aligning under
   the << above it — clang-format fixes that one.

Validation check

• Build/tests run: green, and this time it covers the delta. There was no result at
  657b9fa4 when I started — mu2e/buildtest read "This test has not been triggered
  yet", so the earlier green described the pre-rename code — so I triggered a run and
  waited for it. It passed at 15:41 (build 3309, prof, 4 min 28 sec), with the full job
  list green including ceSimReco, ceMix, ceDigi and g4test_03MT. That is what
  matters here, since the whole delta is a rename of a class, a target and a source file.
  My own checks at this head: the stubbed CMake configure (exit 0, all four plugins
  registered), fhicl-dump on both prologs (exit 0), and the plugin-name comparison
  quoted above.
• Config contract check: pass. Prolog keys match the Config atoms for all three
  CosmicsCalib modules and for BaselineAnalyzer, and module_type now matches the
  built plugin in all four cases — this was the one item outstanding last round.
• Cross-repo consistency: needs follow-up — see finding 1.

Residual risk

• Unchanged: I have not reviewed the physics of the calibration procedures. The remaining
  🟡 items in BaselineAnalyzer and CaloCosmicEnecalib from the first review still
  stand — those files are untouched by this delta.

Author follow-ups

1. Say whether fileT0 ships with the PR or is supplied by the operator.
2. Say whether fileTcor is meant to be read and rewritten in place, or split the output
   onto its own atom.
3. Drop the unreachable else at :164-167 and the always-true guard at :199-203.

[rlcee]

looks good to me

[giro94]

The "fileT0.dat" will be provided by the user and shouldn't be in the repo

[giro94]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at a37fd7c.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged a37fd7c at a432e8d
build (prof)	✅	Log file. Build time: 04 min 31 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 936 warnings
whitespace check	✅	no whitespace errors found

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

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.