Calo mc rev

[bechenard]

Refactoring and modernizing the CaloMC code and fixing a few bugs. The new noise sampler class moved to CaloReco since it will also be needed for reco. The new noise model let you use the waveform taken from histograms, which is what will be needed to include conditions. It keeps the old method to generate noise waveform from scratch if needed. Switched to noise histogram readout to speed up start, which invalidates the RNG sequence so the validation should fail (tested that the validation is ok with the older version and all other changes), but the physics remains the same. Noise file generated with parameters found in fcl file. Check that noise mean/RMS is same before/after (not posting details here)

[FNALbuild]

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

• Mu2eUtilities
• CaloReco
• CaloMC

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☔ The build tests failed for ea62775.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged ea62775 at 1ce31db
build (prof)	✅	Log file. Build time: 08 min 49 sec
ceSimReco	❌	Log file. Return Code 1.
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. Return Code 1.
muDauSteps	✅	Log file.
ceMix	❌	Log file. Return Code 1.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 21 files
clang-tidy	➡️	17 errors 253 warnings
whitespace check	✅	no whitespace errors found

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

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.

[bechenard]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 1bce0f9.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 1bce0f9 at 1ce31db
build (prof)	✅	Log file. Build time: 08 min 52 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 21 files
clang-tidy	➡️	17 errors 253 warnings
whitespace check	✅	no whitespace errors found

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

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

Mu2e/Offline #1919 — "Calo mc rev" (bechenard, branch CaloMCRev, base main)
Reviewed at head 1bce0f9b

Decision

• 🔴 request changes

Scope understood

• Refactor of CaloMC (all five producers), extraction of the calo pulse/noise caches into Mu2eUtilities (CaloPulseShape → CaloPulseUtil, new CaloNoiseUtil), and deletion of CaloNoiseSimGenerator / CaloWFExtractor.
• The noise model changes source: instead of being generated per job from elec/rin/dark rates × MeVToADC/readoutPEPerMeV, it is now read by default (generate: false) from a new checked-in histogram CaloReco/data/ReadoutNoise.root. The "salt and pepper" random-noise path (addRandomNoise) is removed.
• Large amounts of genuinely dead code are removed (diagnostic TH1F/TH2F blocks, PhysicalVolumeMultiHelper/caloMaterial machinery in CaloShowerStepMaker, plotWF). Author states the RNG sequence changes so validation will differ, but "physics remains the same", and explicitly asks for the CMake files to be checked.

---

Findings

1. 🔴 [S0] CaloTemplateWFProcessor drops minPeakAmplitude — every argument to CaloTemplateWFUtil shifts by one

   • Evidence: CaloReco/inc/CaloTemplateWFUtil.hh @ 1bce0f9b

     CaloTemplateWFUtil(const CaloPulseUtil::Config& configPulseCache, double minPeakAmplitude,
                        double minDTPeaks, int printLevel=-1);

     CaloReco/src/CaloTemplateWFProcessor.cc @ 1bce0f9b

     fmutil_ (config.pulseCache(),minDTPeaks_,config.fitPrintLevel()),

     Only three arguments are passed. The pre-PR call site passed six and included minPeakAmplitude_:

     fmutil_ (config.pulseFileName(),config.pulseHistName(),minPeakAmplitude_,
              config.digiSampling(),minDTPeaks_,config.fitPrintLevel()),

     Resulting binding inside CaloTemplateWFUtil, with CaloReco/fcl/prolog.fcl TemplateProcessor values (minPeakAmplitude: 24, minDTPeaks: 20, fitPrintLevel: -1):

     member	intended	actual at head
     minPeakAmplitude_	24	20 (gets minDTPeaks_)
     minDTPeaks_	20	-1 (gets fitPrintLevel)
     printLevel_	-1	-1 (defaulted — masks the shift)
     Both are live in CaloReco/src/CaloTemplateWFUtil.cc::selectComponent:

     if (tempPar[ip] < minPeakAmplitude_)                 return false;   // line 229
     if (dt < minDTPeaks_ && tempPar[ip2] > tempPar[ip])   return false;   // line 238

   • Impact: silent calorimeter waveform-reconstruction regression on every job using processorStrategy: "TemplateFit" (the production default). The fitted-component amplitude cut drops 24 → 20 ADC, and the minimum peak-separation veto becomes dt < -1, i.e. never true — the pile-up peak-merging protection is entirely disabled, so spuriously close fitted components are now all kept. All arguments are double/int, so this compiles clean and CI (return-code-only tests) cannot see it. printLevel_ coincidentally landing on the same value as the prolog's fitPrintLevel removes the one symptom that would have been noticed.
   • Suggested fix: pass all four arguments —
     fmutil_ (config.pulseCache(), minPeakAmplitude_, minDTPeaks_, config.fitPrintLevel()),
     Consider dropping the printLevel=-1 default argument so an under-supplied call cannot compile again.

2. 🔴 [S0] install(DIRECTORY data ...) added to the wrong package: CaloMC has no data/, and the new CaloReco/data/ReadoutNoise.root is never installed

   • Evidence: CaloMC/CMakeLists.txt @ 1bce0f9b (added in 1bce0f9b, "Added missing dir"):

     install(DIRECTORY data DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/Offline/CaloReco)

     CaloMC/ contains only CMakeLists.txt fcl inc src test — there is no CaloMC/data. CaloReco/CMakeLists.txt is not touched by this PR and has no install(DIRECTORY data ...) line at all.
     • Reproduced the CMake behaviour locally (cmake 3.31.8): install(DIRECTORY data ...) on a non-existent directory fails at install time —
       CMake Error at build/cmake_install.cmake:41 (file): file INSTALL cannot find ".../data": No such file or directory, cmake --install exit code 1.
     • Repo idiom check: of the 17 Offline packages that own a data/ directory (CaloConditions, CaloFilters, CalorimeterGeom, CalPatRec, CRVConditions, CRVResponse, DbService, EventGenerator, GlobalConstantsService, Mu2eG4, Mu2eKinKal, Mu2eUtilities, ParticleID, TrackerConditions, TrkHitReco, TrkPatRec, CaloReco), every one except CaloReco carries its own install(DIRECTORY data DESTINATION .../Offline/<Pkg>).
   • Impact: two separate breakages in the CMake half of the dual build. (a) cmake --install of Offline now errors out. (b) Even once (a) is worked around, share/Offline/CaloReco/data/ReadoutNoise.root is absent from the install tree, so CaloNoiseUtil::fillCache()'s ConfigFileLookupPolicy cannot resolve the fcl path "Offline/CaloReco/data/ReadoutNoise.root" and throws cet::exception("NOISEREADER") — i.e. every CaloDigiMaker job with the default addNoise: true dies at first event in a CMake-installed release. CI cannot catch either: mu2e/buildtest builds with scons (scons.log), where ConfigFileLookupPolicy resolves the file straight out of the source tree via MU2E_SEARCH_PATH, which is exactly why ceDigi/ceMix/ceSimReco are green at this head.
   • Suggested fix: delete the line from CaloMC/CMakeLists.txt and add to CaloReco/CMakeLists.txt:

     install(DIRECTORY data DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/Offline/CaloReco)

     Do not also add configure_file(... ${CURRENT_BINARY_DIR} ...) staging lines — that legacy idiom was explicitly dropped alongside install(DIRECTORY data ...) in Offline #1914.

3. 🟠 [S1] CaloNoiseUtil batch selection divides by base twice, and prepare() makes the per-readout peToADC a no-op after the first call

   • Evidence: Mu2eUtilities/src/CaloNoiseUtil.cc @ 1bce0f9b

     void CaloNoiseUtil::prepare(int histoID, double peToADC)
     {
         int baseID = histoID / base;              // already divided
         if (baseID == histoBaseID_) return;       // early-out
         if (generate_) generateCache(baseID, peToADC);
         else           fillCache(baseID);
     }
     ...
     void CaloNoiseUtil::fillCache(int histoBaseID)
     {
         ...
         if (hid/base != histoBaseID/base) continue;   // divides the already-divided value again

     noiseSegment() has the same double-division (int baseID = histoID/base; ... fillCache(baseID)), and histoBaseID_ is stored as the already-divided value.
   • Impact:
     • Wrong batch, silently. fillCache(baseID) selects histograms with hid/base == baseID/base. That is only correct for baseID == 0. For any baseID >= 1 (i.e. histoID >= 10000) it evaluates to hid/10000 == 0, so the cache is filled with batch 0 and the requested batch is silently never loaded — noiseSegment then throws CALONOISEUTIL histoID ... is invalid, or worse, returns batch-0 noise. Latent today only because CaloDigiMaker hardcodes const int NoiseWFID(0); it becomes live the moment per-readout noise IDs arrive, which is the stated purpose of the class.
     • peToADC is dead after the first call. CaloDigiMaker::generateSpotNoise() recomputes readoutScaleFactor(iRO, conds) per readout and passes it to prepare(0, scaleFactor), but prepare() early-returns for every readout after the first, so with generate: true the whole detector is scaled by the first SiPM's ADCPerMeV/pePerMeV; with the prolog default generate: false the argument is ignored outright. The per-readout readoutScaleFactor call in the noise path is therefore pure cost with no effect.
   • Suggested fix: keep one convention — either pass the raw histoID into fillCache/generateCache and divide once inside, or pass the already-divided baseID and compare hid/base != histoBaseID. Then either drop the scaleFactor argument from generateSpotNoise/prepare (documenting that noise amplitude is absolute ADC), or key the cache on (baseID, peToADC) so a changed scale actually regenerates.

4. 🟠 [S1] No physics evidence offered for a change that replaces the noise model and its pedestal definition

   • Evidence: PR body — "the new noise sampler invalidates the RNG sequence so the validation should fail ..., but the physics remains the same" — with no supporting numbers. The substantive changes are: (a) noise waveform sourced from CaloReco/data/ReadoutNoise.root instead of being generated from elecNphotPerNs/rinNphotPerNs/darkNphotPerNs × MeVToADC/readoutPEPerMeV; (b) pedestal changes from the closed-form trunc(noiseRinDark*digiSampling*Σpulse*scaleFactor) (old CaloNoiseSimGenerator::generateWF) to trunc(mean of histogram bin contents) (CaloNoiseUtil::fillCache); (c) readoutPEPerMeVCsI/readoutPEPerMeVLyso deleted from CaloReco/fcl/common.fcl, so the previous absolute normalisation is gone; (d) the addRandomNoise salt-and-pepper path is deleted outright.
   • Impact: the pedestal is written into every CaloDigi and subtracted before peak extraction, and the noise RMS drives the minPeakADC/minPeakAmplitude efficiency turn-on. Because the RNG re-sequencing makes the standard validation diff uninformative, there is currently no way for a reviewer to distinguish "same physics, different seeds" from "shifted pedestal / different noise RMS". Combined with finding 1, the reco side is changing at the same time, so a single post-fix comparison is needed.
   • Suggested fix: after fixing findings 1–3, post the before/after distributions that the RNG change does not invalidate: pedestal value, digitized-waveform RMS in empty samples, CaloHit multiplicity and energy spectrum, and reconstructed-peak multiplicity per crystal. Also state how ReadoutNoise.root was produced (which generator settings, doc-db reference) so it can be regenerated.

5. 🟡 [S2] CaloHitTruthMatch swaps a tolerant operator[] for .at() on the shower map

   • Evidence: CaloMC/src/CaloHitTruthMatch_module.cc @ 1bce0f9b

     const auto& sortedSims = caloShowerSimsMap.at(hit.crystalID());

     previously caloShowerSimsMap[hit.crystalID()] on a std::map, which default-constructed an empty vector and let the hit fall through to the existing "hit not matched" diagnostic path.
   • Impact: a CaloHit in a crystal with no surviving CaloShowerSim now raises an uncaught std::out_of_range (art aborts the job) instead of producing an empty CaloHitMC. Today the invariant crystals(CaloHit) ⊆ crystals(CaloShowerSim) happens to hold because CaloShowerROMaker emits CaloShowerRO and CaloShowerSim from the same step loop, and CompressDigiMCs's calo path copies every CaloShowerSim. But nothing enforces it: caloShowerSimCollection is externally repointable and is repointed in production — Production/JobConfig/recoMC/epilog.fcl sets it to "compressDigiMCs" while caloHitCollection stays CaloHitMaker, so the two collections are no longer produced by the same module in the same job. Any future pruning (e.g. the caloClusterMCTag compression path) turns an unmatched hit into a crash. caloHitMap.at() on the next line is safe — that map is built from caloHits itself.
   • Suggested fix: use find() and fall through to the unmatched path, or keep .at() but wrap it in an explicit cet::exception with a message naming the two input tags, so the failure is diagnosable rather than a bare std::out_of_range.

6. 🟡 [S2] Configuration knobs silently demoted to hardcoded constants

   • Evidence: CaloMC/src/CaloDigiMaker_module.cc @ 1bce0f9b

     const int NoiseWFID(0);      // will get this from proditions later;
     const double minAmplitude(2);

     minNoiseAmplitude was a fhicl::Atom<float> with minNoiseAmplitude : 2 in CaloMC/fcl/prolog.fcl; both the Config entry and the prolog line are deleted in this PR.
   • Impact: the noise-window threshold can no longer be tuned per campaign without a recompile. The value is unchanged (2), so there is no behaviour change today — this is purely a loss of configurability in the module whose noise model is being reworked.
   • Suggested fix: keep minNoiseAmplitude as a fhicl::Atom (default 2) and restore the prolog line; leave the NoiseWFID TODO as-is but note it in the PR body so it is not lost.

7. 🟡 [S2] digiSampling now has two independent homes per module

   • Evidence: CaloReco/inc/CaloTemplateWFProcessor.hh still declares fhicl::Atom<double> digiSampling and CaloReco/fcl/prolog.fcl still sets digiSampling : @local::HitMakerDigiSampling under TemplateProcessor, but after this PR the processor no longer reads it — the value that actually reaches CaloPulseUtil comes from pulseCache.digiSampling. The same duplication now exists in CaloDigiMaker and CaloHitTruthMatch (a top-level digiSampling used for the digitisation/time window, plus pulseCache.digiSampling used to build the pulse cache).
   • Impact: FHiCL validation still requires the dead TemplateProcessor.digiSampling, and the two copies can be overridden independently and silently disagree, giving a pulse cache sampled at one rate and a time window computed at another. The prolog sets both from @local::HitMakerDigiSampling today, so nothing is broken now.
   • Suggested fix: delete the unused digiSampling atom from CaloTemplateWFProcessor::Config and its prolog line; for the modules that legitimately need both, either derive the top-level value from pulseCache.digiSampling or add a constructor-time consistency check that throws when they differ.

8. 🟡 [S2] CaloNoiseUtil dead / order-dependent state

   • Evidence: Mu2eUtilities/inc/CaloNoiseUtil.hh declares double minPeakADC_; — it is absent from the constructor initialiser list in CaloNoiseUtil.cc and never read anywhere in the class (it was carried over from CaloNoiseSimGenerator, where it was used by the now-deleted addSaltAndPepper). In fillCache, pedestal_ is recomputed inside the while over TFile keys, so with more than one histogram in a batch the surviving value is whichever key TIter yielded last.
   • Impact: an uninitialised member that a future edit could read; and a pedestal that is order-dependent on ROOT's key iteration rather than on a defined rule. Neither bites today (one histogram, minPeakADC_ never read).
   • Suggested fix: delete minPeakADC_; compute the pedestal from a named histogram (or average across the batch) rather than from whichever key comes last, and say so in the comment.
   • Related, same file: noiseSegment() returns a std::span<float> into noiseMap_, which fillCache() clears — safe at the single call site in CaloDigiMaker (used immediately), but worth a comment stating that the span must not outlive the next prepare()/noiseSegment() call.

9. 🟡 [S2] Include guards on the new/renamed Mu2eUtilities headers do not follow the repo convention

   • Evidence: Mu2eUtilities/inc/CaloNoiseUtil.hh uses CaloNoiseUtil_HH; Mu2eUtilities/inc/CaloPulseUtil.hh uses CaloPulseUtil_HH. The convention is path-qualified without the repo prefix, i.e. Mu2eUtilities_CaloNoiseUtil_hh / Mu2eUtilities_CaloPulseUtil_hh. This PR does fix CaloMC/inc/CaloPhotonPropagation.hh to CaloMC_CaloPhotonPropagation_hh, so the two new headers are inconsistent with the PR's own cleanup.
   • Impact: collision risk with any future CaloNoiseUtil/CaloPulseUtil elsewhere in the tree; inconsistency in a package being touched.
   • Suggested fix: rename both guards to the path-qualified form.

10. ⚪ [S3] Missing and speculative includes in the new files

    • Evidence: Mu2eUtilities/src/CaloNoiseUtil.cc uses std::istringstream without <sstream> and std::trunc without <cmath> (both currently arrive transitively via <iostream>/ROOT headers on this toolchain). Mu2eUtilities/inc/CaloNoiseUtil.hh includes art/Framework/Services/Optional/RandomNumberGenerator.h, Offline/SeedService/inc/SeedService.hh and fhiclcpp/types/Sequence.h, none of which the header needs — the class only holds CLHEP distributions and a fhicl::Atom-based Config. These were inherited verbatim from CaloNoiseSimGenerator.hh.
    • Impact: violates "include only headers actually needed; avoid speculative includes"; the transitive includes are a toolchain accident, not a guarantee.
    • Suggested fix: add <sstream> and <cmath> to the .cc; drop the three unused includes from the .hh.

11. ⚪ [S3] Small consistency items carried through the refactor

    • cet::exception categories: the PR improves CaloDigiMaker to "CALODIGIMAKER" and CaloPhotonPropagation to "CaloPhotonPropagation", but leaves "Rethrow" in all four ShowerStepUtil.cc throws and "CATEGORY" in CaloPulseUtil.cc; the new CaloNoiseUtil.cc mixes "NOISEREADER", "CALONOISEUTIL" and "CaloNoiseUtil" for the same class.
    • Typo "Hitsogram" is copied into the renamed CaloPulseUtil.cc and again into new code as "Hitsogram "<<name.c_str()<<" is invalid" in CaloNoiseUtil::fillCache.
    • Prolog style: pulseCache : @local::CaloPulseCache (CaloDigiMaker) vs pulseCache : { @table::CaloPulseCache } (CaloHitTruthMatch) vs {@table::CaloPulseCache} (TemplateProcessor) — three spellings of the same thing.
    • Mu2eUtilities/CMakeLists.txt: src/CaloPulseUtil.cc is listed before src/CaloNoiseUtil.cc, breaking the otherwise-alphabetical source list.
    • CaloNoiseUtil::pedestal() and printCache() can be const.

12. ⚪ [S3] PR hygiene

    • Best practice reminder: this PR mixes functional changes (noise model, PEStatCorrection activation, .at(), CaloTemplateWFUtil signature) with a large rename + reformat + diagnostic-histogram removal across 27 files, which is what let finding 1 hide in the diff. Splitting the pure rename/cleanup from the noise-model change would make the physics-relevant delta reviewable on its own.
    • Best practice reminder: the PR description gives intent but no validation evidence and no acceptance criteria; see finding 4.

---

Verified — no action needed

• 🟢 No data-product change. Nothing under MCDataProducts/ or RecoDataProducts/ is touched; CaloClusterMC, CaloHitMC, CaloEDepMC, CaloShowerSim, CaloShowerRO, CaloShowerStep keep their shapes. Consequently no classes_def.xml / classes.h update and no schema-evolution consideration is required for existing files. Checked explicitly because the title suggested otherwise.
• 🟢 MC-truth ordering/pairing contracts preserved. CaloClusterTruthMatch still sorts its CaloHitMC vector by descending totalEnergyDep() before constructing CaloClusterMC, and CaloHitTruthMatch still sorts CaloEDepMC by descending energyDep(). The rewrite replaces pointer arithmetic (thisCaloCluster - caloClusterBase) with an index loop for the art::Ptr<CaloCluster> — equivalent and safer. No positional cluster-hit ↔ MC-hit pairing is introduced anywhere, so the "CaloClusterMC hits are energy-sorted, not positionally aligned" contract still holds.
• 🟢 CaloDigiMaker::extract() is behaviour-identical to the deleted CaloWFExtractor::extract(). The old call site constructed CaloWFExtractor(bufferDigi, nBinsPeak, minPeakADC, bufferDigi), i.e. startOffset_ == bufferDigi_, so the inlined size_t timeSample(nBinsPeak_+bufferDigi_) reproduces the old nBinsPeak_+startOffset_ exactly; the rest of the body is a verbatim copy.
• 🟢 CaloPhotonPropagation::propTimeSimu rewrite is equivalent. std::lower_bound over the per-depth CDF returns the same "first bin whose cumulative probability reaches the draw" as the old linear while (cdf_[ibin]<test && ibin<iend) scan, with the same clamp to nTimeDiv_-1. The added iz clamp for negative z is a genuine fix (the old unsigned iz = z/dzTime_ was UB for z < 0). Taking ownership of the histogram via std::unique_ptr fixes a leak.
• 🟢 CaloShowerStepMaker ancestor logic is preserved and the removed machinery was genuinely dead. Moving the alreadyInspected lookup ahead of the isInsideAnyCrystal/isInsideSameDisk tests yields the same ancestor assignment: sims that now enter inspectedSims before the break map to themselves, which is what the old code's fall-through produced. PhysicalVolumeMultiHelper vi, mapPhysVol_, caloMaterial_ and physVolInfoInput_ were write-only — vi was passed into collectStepBySimAncestor and never referenced in its body, and mapPhysVol_ was only ever inserted into. This is a correct dead-state removal.
• 🟢 PEStatCorrection was a dead knob and is now honoured. At base 1ce31dbc, CaloShowerROMaker_module.cc declared, initialised and stored PEStatCorrection_ at lines 97/111/148 and never read it — Poisson smearing was unconditional. It is now wired into the NPE draw. CaloMC/fcl/prolog.fcl sets PEStatCorrection : true and no fcl in Offline, Production or mu2e-trig-config overrides it, so default behaviour is unchanged; any config setting it false would now behave differently.
• 🟢 deltaTimeMinus promotion is a genuine fix, value-preserving. The old module had an uninitialised double deltaTimeMinus_ member plus a local double deltaTimeMin = 100.; next to a // FIXME. The new fhicl::Atom<double> deltaTimeMinus defaults to 100.0, exactly reproducing the old cut, and the uninitialised member is gone.
• 🟢 Cross-repo FHiCL contract is clean. Grepped Offline @ 1bce0f9b, Production @ 1e15fc2f and mu2e-trig-config @ 638f3a5, plus an org-wide gh search code: nothing outside the files this PR edits sets NoiseGenerator, addRandomNoise, minNoiseAmplitude, noiseWFSize, nMaxFragment, calo pulseFileName/pulseHistName, readoutPEPerMeVCsI/readoutPEPerMeVLyso, or CaloShowerStepMaker.physVolInfoInput / .caloMaterial. Production's physVolInfoInput hits are all on StoppedParticlesFinder, a different module. No CaloPulseShape / CaloNoiseSimGenerator / CaloWFExtractor reference exists outside Offline. EventNtuple, Production and mu2e-trig-config consume only module labels (CaloDigiMaker, CaloHitTruthMatch, CaloClusterTruthMatch, CaloShowerStepMaker, CaloShowerROMaker) and product types, none of which are renamed or removed.
  • One pre-existing stale reference, not caused by this PR: CaloMC/test/RunCaloCalibGun.fcl:95 sets physics.producers.CaloShowerStepFromStepPt.physVolInfoInput, and CaloShowerStepFromStepPt no longer exists as a module. Worth deleting opportunistically.
• 🟢 Dual build — scons side is correct without edits. CaloMC/src/SConscript, CaloReco/src/SConscript and Mu2eUtilities/src/SConscript all exist (93 SConscript files remain in the tree) and use helper.make_mainlib(...) / helper.make_plugins(...), which glob rather than list sources. No *_module.cc is added or removed by this PR — only non-module sources (CaloNoiseSimGenerator.cc, CaloWFExtractor.cc deleted; CaloPulseShape.cc → CaloPulseUtil.cc; CaloNoiseUtil.cc added) — so the "new module needs an explicit cet_build_plugin entry that scons picks up for free" asymmetry does not apply here. The CMake source lists in CaloMC/CMakeLists.txt and Mu2eUtilities/CMakeLists.txt were updated correctly for all five files.
• 🟢 CMake link dependencies are right. Offline::ConfigTools is correctly added to the CaloMC mainlib for CaloPhotonPropagation.cc's ConfigFileLookupPolicy; Mu2eUtilities already lists Offline::ConfigTools and Offline::SeedService, which is what the new CaloNoiseUtil.cc needs. The CaloMC/CaloReco plugin LIBRARIES REG blocks need no additions.
• 🟢 CI is green at this exact head. gh api repos/Mu2e/Offline/commits/1bce0f9b/statuses shows mu2e/buildtest success plus all 12 art tests success; the FNALbuild comment for 1bce0f9b (build 3271, merged into base 1ce31dbc) shows ✅ for build (prof), ceSimReco, g4test_03MT, transportOnly, POT, g4study, cosmicSimReco, cosmicOffSpill, ceSteps, ceDigi, muDauSteps, ceMix, rootOverlaps, g4surfaceCheck, trigger, check_cmake, FIXME/TODO (0/0), whitespace. The three failures at the previous head ea62775d (ceSimReco, ceDigi, ceMix) are resolved. clang-tidy reports 17 errors / 253 warnings, unchanged in character from main. Note that check_cmake passing does not cover finding 2 — it validates source lists, not install rules, and buildtest compiles with scons.

---

Re-review / carry-forward

• gh api repos/Mu2e/Offline/pulls/1919/reviews → empty; .../pulls/1919/comments (inline threads) → empty. The only issue comments are FNALbuild CI reports and the author's @FNALbuild run build test. No prior findings from any reviewer to carry forward; this is a first review of the PR.

Validation check

• Build/tests run by CI: yes — scons prof build + 12 art validation jobs + rootOverlaps/g4surfaceCheck/trigger/check_cmake, all green at 1bce0f9b.
• Build/tests run by reviewer: partial — reproduced the CMake install(DIRECTORY) failure standalone (cmake 3.31.8, exit 1). Did not build Offline.
• Config contract check: partial — the FHiCL schema is internally coherent and no external repo references a removed key, but TemplateProcessor.digiSampling is now a required-but-unused parameter and digiSampling is duplicated in three module configs (finding 7).
• Cross-repo consistency: pass — no Production, mu2e-trig-config or EventNtuple change is required.
• CMake / dual-build check: fail — finding 2.

Residual risk

• Finding 1 changes reconstructed calorimeter hit content on every TemplateFit job and is invisible to return-code CI; anything already produced on this branch should be treated as suspect.
• Finding 2 means a CMake-installed release cannot even be produced, and would fail at first event if it were.
• The noise-model swap is only exercised through its generate: false histogram path in CI; the generate: true path (findings 3) has no test coverage at all.
• CaloNoiseUtil is written for many noise waveforms but only ever driven with NoiseWFID = 0; the multi-batch code path is untested and currently incorrect.

Author follow-ups

1. Fix the CaloTemplateWFUtil construction in CaloReco/src/CaloTemplateWFProcessor.cc to pass minPeakAmplitude_ (finding 1), and consider removing the printLevel default argument so the arity can't silently under-supply again.
2. Move install(DIRECTORY data DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/Offline/CaloReco) out of CaloMC/CMakeLists.txt and into CaloReco/CMakeLists.txt; verify with cmake --install (not just scons) that share/Offline/CaloReco/data/ReadoutNoise.root lands in the install tree (finding 2).
3. Fix the double /base division in CaloNoiseUtil::prepare/fillCache/noiseSegment, and decide whether peToADC should key the cache or be removed from the interface (finding 3).
4. Post before/after pedestal, empty-sample waveform RMS, CaloHit multiplicity/energy and reconstructed-peak multiplicity — quantities the RNG re-sequencing does not invalidate — and state how ReadoutNoise.root was generated (finding 4).
5. Restore tolerance (or a diagnosable cet::exception) for a CaloHit whose crystal has no CaloShowerSim, given Production/JobConfig/recoMC/epilog.fcl repoints caloShowerSimCollection at compressDigiMCs (finding 5).
6. Restore minNoiseAmplitude as a FHiCL parameter (finding 6); remove the now-unused TemplateProcessor.digiSampling and decide on a single home for digiSampling (finding 7).
7. Drop the unused CaloNoiseUtil::minPeakADC_, make the pedestal estimate order-independent, and fix the include guards and includes on the two new Mu2eUtilities headers (findings 8–10).
8. Confirm whether the removal of the addRandomNoise salt-and-pepper path is intentional and permanent, or whether it needs to come back in a follow-up — the prolog default was false, so nothing regresses today, but the capability is gone.

[michaelmackenzie]

This is a large PR, with both logic and code-cleaning changes. Overall it looks fine to me, but I don't think I'll be able to do a detailed review as I'm not a calo reco/MC expert. The trigger results remained stable, which means the reconstruction logic seems stable (there are no random numbers in that CI test, so it shouldn't be affected by changes of that form).

[bechenard]

@michaelmackenzie If you are not sensitive to the random number sequence, then all changes are cleaning + bug fixes. Since the trigger doesn't change, it means we're in business.

[bechenard]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☔ The build tests failed for be443da.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged be443da at 1ce31db
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. Return Code 1.
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 21 files
clang-tidy	➡️	17 errors 247 warnings
whitespace check	✅	no whitespace errors found

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

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 — second pass on "Calo mc rev" (#1919)

Reviewed at head be443da3 (2026-08-07). Prior review: 🔴 at 1bce0f9b (review 4884162084, 12 findings). The delta is one commit, be443da3 "Fixed issues and improve structure of noise sampler", +109/−108 across 15 files. Every prior finding is accounted for below and verified in the code at this head rather than taken from the commit message.

Decision

• 🔴 Request changes — one new blocker, and it is the direct cause of the CI failure at this head. Both prior S0s are properly fixed and eight of the twelve findings are closed, several going beyond what I asked. But the fix for my own finding 8 turned pedestal() from a stored value into a map lookup that throws, and the map is only ever populated as a side effect of a call that addNoise : false jobs never make. Production/JobConfig/digitize/OffSpill.fcl and NoField.fcl both set exactly that.

Scope understood (delta only)

• Fixes to findings 1, 2, 5, 7, 8, 9, 10, 11; CaloNoiseUtil restructured so the pedestal and the waveform cache are keyed per histogram ID; float → double for the noise waveform; prepare() reduced to the generate-only path with fillCache() moved to a lazy call inside noiseSegment().

---

Findings

1. 🔴 [S0] CaloNoiseUtil::pedestal() throws on every addNoise : false job — this is the cosmicOffSpill CI failure.

   • Evidence, the failure: mu2e/cosmicOffSpill at be443da3 is failure — mu2e -c Production/Validation/cosmicOffSpill.fcl -n 10 finished with return code 1. The same test at 1bce0f9b was success, return code 0, and it is green right now on four other open Offline PRs (#1918, #1916, #1911, #1901), so this is not a broken main. The rest of the suite is green at this head; mu2e/buildtest was re-triggered at 23:15 and is still running.
   • Evidence, the mechanism — pedestal_ has exactly two writers and one of them is unreachable in this configuration:
     • Mu2eUtilities/src/CaloNoiseUtil.cc:93 — pedestal_[hid] = ... inside fillCache()
     • Mu2eUtilities/src/CaloNoiseUtil.cc:134 — pedestal_[histoID] = ... inside generateCache()
     • fillCache() is called from exactly one place, :152, which is inside noiseSegment(). prepare() is now just if (generate_) generateCache(histoID, peToADC); — with the shipped default generate : false (CaloReco/fcl/common.fcl, CaloNoiseCache) it does nothing at all.
     • Mu2eUtilities/src/CaloNoiseUtil.cc:168-170 — pedestal(histoID) now throws cet::exception("CALONOISEUTIL") when the key is absent, where it previously returned an int member initialised to 0.
   • Evidence, the trigger — CaloMC/src/CaloDigiMaker_module.cc:199-204:

     if (isEmpty) continue;
     if (addNoise_) {
       const double scaleFactor = readoutScaleFactor(iRO, calCrystalConds);
       generateSpotNoise(waveform,scaleFactor);          // the only path to noiseSegment()
     }
     buildOutputDigi(iRO, waveform, noiseSampler_.pedestal(NoiseWFID), caloDigiColl);

     Production/JobConfig/digitize/OffSpill.fcl:24 sets physics.producers.CaloDigiMaker.addNoise : false, and Production/Validation/cosmicOffSpill.fcl is a two-line wrapper around that file. With noise off, generateSpotNoise never runs, noiseSegment never runs, fillCache never runs, and the first non-empty readout of the first event throws phistoID 0 is invalid for pedestal.
   • Impact: deterministic, not stochastic — every off-spill digitisation job dies at the first calorimeter readout. Production/JobConfig/digitize/NoField.fcl:21 sets the same key, so no-field digitisation breaks identically; that one has no validation job, so CI would not have told you. There is also a second, data-dependent path with addNoise : true: generateSpotNoise only calls noiseSegment for waveform ranges above its hardcoded minAmplitude = 2, so a job whose first non-empty readout is entirely sub-threshold throws the same way.
   • Suggested fix: restore eager filling so the cache is valid after prepare() regardless of source, and call prepare() once per event before the readout loop rather than from inside the noise path —

     void CaloNoiseUtil::prepare(int histoID, double peToADC)
     {
        if (generate_) generateCache(histoID, peToADC);
        else           fillCache(histoID);
     }

     fillCache() clears noiseMap_ on entry, so calling it once at the top is correct. Guarding the call site (addNoise_ ? noiseSampler_.pedestal(NoiseWFID) : 0.0) fixes cosmicOffSpill but leaves the sub-threshold path in finding 1's second paragraph open, so it is not sufficient on its own.
   • I could not read cosmicOffSpill.log to confirm the exception text — build 3272 is still running and its artifacts are not published yet (/artifact/cosmicOffSpill.log returns Jenkins "Not Found"). The code path above is traced from the source at this head and the config that selects it, so I am confident in the diagnosis, but the log will name the exception if you want it confirmed before patching.

2. 🟠 [S1] CaloNoiseUtil generate path: the two cache paths key the map differently, and emplace silently drops the regenerated waveform — carried over from finding 3, now partial.

   • The double /base division is genuinely fixed: fillCache computes histoBaseID = histoID/base once and compares hid/base != histoBaseID (:49, :85). Good.
   • What remains, all in generateCache:
     • Mu2eUtilities/src/CaloNoiseUtil.cc:131 — noiseMap_.emplace(histoBaseID, waveform); keys by the batch ID, while noiseSegment looks up by the full histoID (:145) and fillCache keys by the full hid (:88). The three disagree. For histoID = 0 they coincide, which is why nothing shows today; for any histoID ≥ 1 the generate path emplaces at key 0, noiseSegment misses, and it throws "noiseSegment called before prepare()".
     • std::map::emplace does not overwrite an existing key. CaloDigiMaker::generateSpotNoise calls prepare(NoiseWFID, scaleFactor) per readout with a per-readout scaleFactor, so from the second readout on the freshly generated waveform is discarded and the first readout's is reused — the original "peToADC is dead after the first call" problem, in a new form. Worse, pedestal_[histoID] at :134 is updated each time, so pedestal and waveform now disagree about which scale factor they were built with.
     • The discarded regeneration is not free: each call burns one randPoisson_, nPh × randFlat_, and 10 000 × randGauss_ draws before throwing the result away.
   • Impact: latent — generate : false is the shipped default, so none of this executes in production or CI. It becomes live the moment per-readout noise IDs arrive from proditions, which is the stated purpose of the class.
   • Suggested fix: pick one key convention (full histoID everywhere reads simplest, since that is what callers pass) and use noiseMap_[key] = std::move(waveform) so a regeneration actually replaces. Then either key the cache on (histoID, peToADC) or drop peToADC from the interface and document that noise amplitude is absolute ADC.

3. 🟡 [S2] The .at() fix copies the shower-sim vector on every hit.

   • Evidence: CaloMC/src/CaloHitTruthMatch_module.cc:167-169

     const auto& sortedSimsIt = caloShowerSimsMap.find(hit.crystalID());
     const auto& sortedSims   = sortedSimsIt == caloShowerSimsMap.end() ?
                                std::vector<const CaloShowerSim*>{} : sortedSimsIt->second;

     caloShowerSimsMap is std::unordered_map<int, std::vector<const CaloShowerSim*>> (:143). In cond ? A : B the second operand is a prvalue and the third an lvalue of the same type, so the result is a prvalue: sortedSimsIt->second is copy-initialised into a temporary. const auto& then binds to that temporary and extends its lifetime — so this is safe, not dangling, but it is a full heap-allocating copy of the vector for every CaloHit, where the old .at() returned a reference.
   • Impact: no correctness problem and the crash I flagged is genuinely gone. It is an allocation per hit in the per-event matching loop that reads like a reference, which is the kind of cost that never gets found again.
   • Suggested fix: make both operands lvalues so the result stays an lvalue —

     static const std::vector<const CaloShowerSim*> noSims;
     const auto it = caloShowerSimsMap.find(hit.crystalID());
     const auto& sortedSims = (it == caloShowerSimsMap.end()) ? noSims : it->second;

     (const auto& on the iterator at :167 can also just be const auto.)

4. 🟡 [S2] digiSampling now has two sources inside one constructor — carried over from finding 7, partial.

   • The CaloTemplateWFProcessor half is properly fixed: the fhicl::Atom<double> digiSampling is gone from its Config and the digiSampling : @local::HitMakerDigiSampling line is gone from CaloReco/fcl/prolog.fcl.
   • But CaloMC/src/CaloDigiMaker_module.cc still declares fhicl::Atom<float> digiSampling (:59) and now reads the two differently in adjacent initialisers:

     digiSampling_    (config().pulseCache().digiSampling()),   // :74
     startTimeBuffer_ (config().digiSampling()*config().bufferDigi()),  // :76

     Before this delta both came from config().digiSampling() and could not disagree. CaloPulseCache.digiSampling and the module's own digiSampling are both @local::HitMakerDigiSampling in the prolog, so they agree today — override one and startTimeBuffer_ is computed at one sampling rate while waveformSize (:184) uses the other.
   • Suggested fix: read config().pulseCache().digiSampling() in both places and delete the top-level atom, or keep the atom and add a constructor check that throws when the two differ.

5. 🟡 [S2] Hardcoded knobs — carried over from finding 6, unaddressed, and now duplicated.

   • CaloMC/src/CaloDigiMaker_module.cc:213 still has const double minAmplitude(2);; the minNoiseAmplitude fhicl atom and its prolog line remain deleted. Note this constant is not cosmetic after finding 1 — it is what decides whether noiseSegment is called at all.
   • const int NoiseWFID(0); is now defined twice, at :181 and :211. The :181 copy is new in this delta.
   • Suggested fix: restore minNoiseAmplitude as a fhicl::Atom (default 2) with its prolog line, and make NoiseWFID a single class-scope constant until proditions supplies it.

6. 🟡 [S2] No recorded physics evidence — carried over from finding 4, partially addressed; downgraded from 🟠.

   • What changed: @michaelmackenzie approved at this exact head, and your comment states that with the trigger unchanged the remaining delta is "cleaning + bug fixes". Sign-off from a reviewer who owns that side is real evidence and I am not going to keep treating this as a major finding.
   • What is still missing: nothing quantitative in the PR body. The pedestal definition changed from a closed-form expression to trunc(mean of histogram bins), and readoutPEPerMeVCsI/readoutPEPerMeVLyso were deleted, so pedestal value and empty-sample RMS are the two numbers that pin down "same physics" and neither is invalidated by the RNG re-sequencing.
   • Suggested fix: paste before/after pedestal and empty-sample waveform RMS (and, if cheap, CaloHit multiplicity) into the PR body once finding 1 is fixed, plus one line on how ReadoutNoise.root was produced so it can be regenerated. The PR body is the permanent record; the comment thread is not.

7. ⚪ [S3] Small residuals, all new in this delta:

   • Mu2eUtilities/src/CaloNoiseUtil.cc:172 — return iter->second;; (stray second semicolon).
   • :169 — the new exception message says "phistoID " for what is a histoID.
   • CaloMC/src/CaloHitTruthMatch_module.cc:171-176 — the for inside the new if (diagLevel_ > 2) block is indented as though it were inside the if (sortedSims.empty()) above it; it is not. Behaviour is fine, the indentation is not.
   • noiseSegment() still returns a std::span<double> into noiseMap_, and fillCache() — now reachable from inside noiseSegment() — clears that map. Still safe at the single call site, which consumes the span immediately, but the lifetime rule now spans two functions and is worth the one-line comment I suggested last time.
   • Pre-existing, not from this PR, still there: CaloMC/test/RunCaloCalibGun.fcl:95 sets physics.producers.CaloShowerStepFromStepPt.physVolInfoInput for a module that no longer exists.

---

Carry-forward accounting (vs review 4884162084 at 1bce0f9b)

1. 🟢 [was S0] CaloTemplateWFUtil argument shift — FIXED, verified. CaloReco/src/CaloTemplateWFProcessor.cc:29 now passes all four arguments, fmutil_(config.pulseCache(),minPeakAmplitude_,minDTPeaks_,config.fitPrintLevel()), and you took the second half of the suggestion too — CaloTemplateWFUtil.hh:15 drops the printLevel=-1 default, so an under-supplied call can no longer compile. I also checked the thing that would have made this fix a fresh bug: minPeakAmplitude_ (:82) and minDTPeaks_ (:84) are both declared before fmutil_ (:89), so they are initialised before being read.
2. 🟢 [was S0] install(DIRECTORY data ...) — FIXED, verified. Removed from CaloMC/CMakeLists.txt, added to CaloReco/CMakeLists.txt:48, and — as prescribed — no configure_file(... ${CURRENT_BINARY_DIR} ...) staging line came with it.
3. 🟠 [was S1] Noise cache keying — PARTIAL, now finding 2.
4. 🟡 [was S1] Physics evidence — PARTIAL, downgraded, now finding 6.
5. 🟢 [was S2] .at() on the shower map — FIXED. No longer throws on a CaloHit whose crystal has no CaloShowerSim; the unmatched case falls through to an empty vector and a diagLevel_ > 2 message. The efficiency of the replacement is finding 3, not a regression of this finding.
6. 🟡 [was S2] Hardcoded knobs — UNADDRESSED, now finding 5.
7. 🟡 [was S2] digiSampling homes — PARTIAL, now finding 4.
8. 🟢 [was S2] CaloNoiseUtil dead / order-dependent state — FIXED. minPeakADC_ is gone from the header; the pedestal is now a std::map<int,double> filled per histogram inside the loop, so it no longer depends on which TIter key came last. This fix is also what created finding 1 — the value became unavailable rather than merely wrong.
9. 🟢 [was S2] Include guards — FIXED. Mu2eUtilities_CaloNoiseUtil_hh and Mu2eUtilities_CaloPulseUtil_hh.
10. 🟢 [was S3] Includes — FIXED. <cmath> and <sstream> added to the .cc; Sequence.h, RandomNumberGenerator.h and SeedService.hh dropped from the .hh. art_root_io/TFileService.h and TFileDirectory.h also came out of CaloDigiMaker_module.cc.
11. 🟢 [was S3] Consistency batch — DONE. "Rethrow" → "SHOWERSTEPUTIL" at all four ShowerStepUtil throws; "CATEGORY" → "CALOPULSEUTIL"; "NOISEREADER"/"CaloNoiseUtil" unified to "CALONOISEUTIL"; both "Hitsogram" typos corrected; Mu2eUtilities/CMakeLists.txt back in alphabetical order; printCache() and pedestal() now const; pulseCache : @local::CaloPulseCache spelled the same way in all three places. New nits are in finding 7.
12. ⚪ [was S3] PR hygiene — partially addressed. The body still carries no validation evidence (finding 6); the single-topic point is water under the bridge at this stage.

---

Verified 🟢 — no action needed

• 🟢 The float → double change for the noise waveform is harmless. histo->GetArray() returns const float* from a TH1F, so the stored values are unchanged, just widened; the cache is 10 000 elements, so the extra 40 kB per waveform is not worth a comment. CaloDigiMaker's waveform was already std::vector<double>, so this removes a narrowing at the summation site rather than adding one.
• 🟢 Everything cleared in the first review is still clear at this head. No data product changed; MC-truth ordering contracts in CaloClusterTruthMatch/CaloHitTruthMatch are untouched by this delta; extract(), propTimeSimu, the CaloShowerStepMaker ancestor logic, PEStatCorrection and deltaTimeMinus are all unmodified since 1bce0f9b.
• 🟢 Cross-repo FHiCL contract still holds. The only new external coupling this delta creates is the addNoise : false interaction in finding 1, which is a code bug rather than a config-contract break — no Production or mu2e-trig-config file needs editing once prepare() is fixed. CaloPulseCache.digiSampling and CaloDigiMaker.digiSampling both resolve to @local::HitMakerDigiSampling, so finding 4 is latent.
• 🟢 Dual build still fine. No module added or removed, so the scons glob is unaffected; the CMake source list reorder in Mu2eUtilities/CMakeLists.txt is cosmetic; CaloReco/CMakeLists.txt gains only the install rule.

Validation check

• Build/tests run by CI: failing at this head — mu2e/cosmicOffSpill returns 1 (finding 1). Green: ceDigi, ceMix, ceSimReco, ceSteps, cosmicSimReco, g4study, g4test_03MT, muDauSteps, POT, transportOnly. mu2e/buildtest was re-triggered at 23:15 and is still pending, so the compile of this head is not yet confirmed by CI.
• Build/tests run by reviewer: none this pass. Findings are traced from the source at be443da3, the CI status API, and Production @ main. I could not retrieve cosmicOffSpill.log (build still running).
• Config contract check: fail — finding 1 (addNoise : false no longer works).
• Cross-repo consistency: pass — no Production / mu2e-trig-config edit is needed; the OffSpill.fcl and NoField.fcl settings are correct and it is Offline that must accommodate them.
• CMake / dual-build check: pass — the previous S0 is fixed correctly.

Residual risk

• NoField.fcl sets addNoise : false too and has no validation job, so finding 1 breaks no-field digitisation silently — nothing in CI would have caught it if cosmicOffSpill had not existed.
• The generate : true path remains completely untested and, per finding 2, still incorrect for any noise ID other than 0. It is dead code today; it will not be when proditions supplies real IDs.
• The pedestal is now a per-histogram map with no default, which is the right model, but every consumer of it is one missed prepare() away from an exception rather than a wrong number. Worth deciding deliberately whether "no noise configured" should mean pedestal 0 or a hard error.

Author follow-ups

1. Fix CaloNoiseUtil::prepare() to fill the cache in the non-generate case, and call it once per event outside the addNoise_ guard — then confirm cosmicOffSpill goes green (finding 1, blocking).
2. Make generateCache key noiseMap_ the same way fillCache and noiseSegment do, and replace rather than emplace (finding 2).
3. Avoid the per-hit vector copy in CaloHitTruthMatch (finding 3).
4. Settle digiSampling on one source inside CaloDigiMaker (finding 4); restore minNoiseAmplitude and de-duplicate NoiseWFID (finding 5).
5. Put the pedestal / empty-sample-RMS before-and-after in the PR body, plus one line on how ReadoutNoise.root was made (finding 6).
6. The nits in finding 7 whenever you next touch these files.

[bechenard]

There was a stupid typo that made it through, dammit... Will fix this later.

[bechenard]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 8d9ef25.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 8d9ef25 at 99536c1
build (prof)	✅	Log file. Build time: 04 min 20 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 21 files
clang-tidy	➡️	17 errors 249 warnings
whitespace check	✅	no whitespace errors found

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

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 — third pass on "Calo mc rev" (#1919)

Reviewed at head 8d9ef253. Prior reviews: 🔴 at 1bce0f9b (4884162084, 12 findings), 🔴 at be443da3 (4887293882, 7 findings). The delta is one commit, 8d9ef253 "Further bug fixes", +48/−41 across 6 files. Every prior finding is accounted for below and verified in the source at this head, not taken from the commit message.

Decision

• 🟡 Comment only. Upgraded from 🔴.

The S0 is properly fixed — both halves of it, including the sub-threshold sub-case that CI would not have caught — and the S1 keying bug is fixed. What is left is one debug std::cout that is running in production CI logs right now, and a handful of carried-forward minors. None of it gates.

Scope understood (delta only)

• CaloNoiseUtil::prepare() regains the else fillCache(histoID) branch; noiseSegment() loses its lazy-fill fallback; generateCache keys noiseMap_ by full histoID and assigns rather than emplaces; a new peToADC_ map guards regeneration at a different scale.
• CaloDigiMaker: minNoiseAmp restored as a FHiCL atom, digiSampling atom removed, NoiseWFID de-duplicated, generateSpotNoise renamed AddWFNoise with prepare() hoisted to the call site, and the pedestal defaulted to 0 when addNoise_ is false.

---

Carry-forward accounting (vs review 4887293882 at be443da3)

1. 🟢 [was S0] pedestal() throws on every addNoise : false job — FIXED in 8d9ef253, verified.

You took both halves of the prescription, and the second half is what actually matters.

• prepare() (Mu2eUtilities/src/CaloNoiseUtil.cc:42-46) now fills the cache in the non-generate case, exactly as suggested.
• prepare() is hoisted out of the noise-adding function to the call site (CaloMC/src/CaloDigiMaker_module.cc:204), where it runs unconditionally inside the addNoise_ block before AddWFNoise. This closes the second, data-dependent failure I flagged — the addNoise : true job whose first non-empty readout is entirely sub-threshold. Under the old structure prepare() sat behind the minAmplitude range scan and could be skipped; it no longer can. That path has no CI coverage, so a green suite would not have told you it was fixed.
• The addNoise : false pedestal is now a literal 0 (:200), and I checked this against the pre-PR behaviour rather than assuming: CaloNoiseSimGenerator initialised pedestal_(0.0) in its constructor (CaloMC/src/CaloNoiseSimGenerator.cc:26 at base 1ce31dbc) and initialize() was only called under if (addNoise_). So base also digitized with pedestal 0 when noise was off. Value-preserving, not merely non-throwing.

Verified downstream: mu2e/cosmicOffSpill is green at this head (build 3279), as are all 12 art jobs, buildtest, rootOverlaps, g4surfaceCheck, trigger and check_cmake. Production/JobConfig/digitize/NoField.fcl:23 sets the same addNoise : false and has no validation job; it is fixed by the same change.

2. 🟢 [was S1] generateCache keying and emplace — FIXED, verified.
All three sites now agree on the full histoID: noiseMap_[histoID] = std::move(waveform) (:145), noiseMap_[hid].assign(...) (:92), noiseMap_.find(histoID) (:161). The batch ID survives only where it belongs, as the std::erase_if filter (:118) and the fillCache batch test (:89). emplace → assignment means a regeneration now actually replaces. The noiseSegment-before-prepare throw is also gone, correctly — with prepare() hoisted it is now unreachable by construction.

The peToADC half became a design decision rather than the fix I suggested; see finding 2 below. Not a regression, so I am closing this one.

3. 🟢 [was S2] Per-hit vector copy in CaloHitTruthMatch — FIXED, verified.
CaloMC/src/CaloHitTruthMatch_module.cc:167-169 is now the static const + lvalue-ternary form, so sortedSims binds to a reference and the per-hit heap allocation is gone. The stray indentation in the adjacent diagLevel_ > 2 block is fixed too.

4. 🟡 [was S2] digiSampling two homes — PARTIAL. CaloDigiMaker is fully fixed: the atom is deleted, and both digiSampling_ (:74) and startTimeBuffer_ (:76) now read config().pulseCache().digiSampling(), so they cannot disagree. CaloHitTruthMatch still has the problem; see finding 3.

5. 🟢 [was S2] Hardcoded knobs — FIXED, verified. minNoiseAmp is back as fhicl::Atom<float> (:59) with minNoiseAmp : 2 in CaloMC/fcl/prolog.fcl:56, and it is read at both comparison sites in AddWFNoise. NoiseWFID is now defined once (:202) and passed as a parameter. I confirmed no file in Offline, Production or mu2e-trig-config overrides the removed CaloDigiMaker.digiSampling, so deleting a required atom breaks no external config.

6. 🟡 [was S2] Physics evidence — UNADDRESSED, carried over. Still nothing quantitative in the PR body. See finding 4.

7. ⚪ [was S3] Small residuals — MOSTLY FIXED. The return iter->second;; double semicolon and the "phistoID" message are corrected. CaloMC/test/RunCaloCalibGun.fcl drops the stale CaloShowerStepFromStepPt.physVolInfoInput line — thanks for taking that one, it was pre-existing and not yours. The noiseSegment span-lifetime comment did not appear; new nits in finding 5.

---

Findings

1. 🟡 [S2] Two debug std::cout statements ship enabled — they are in the CI logs at this head.

• Evidence: Mu2eUtilities/src/CaloNoiseUtil.cc:54 and :115, both unindented at column 0:

  std::cout<<"Fill Cache noise\n";
  std::cout<<"Generate Cache noise\n";

  Not speculative — I pulled the CI artifact for this head and grepped it. ceDigi.log from build 3279 contains Fill Cache noise at line 52, interleaved with the art record banners:

  Begin processing the 3rd record. run: 1430 subRun: 0 event: 6 ...
  Fill Cache noise
  Begin processing the 5th record. run: 1430 subRun: 0 event: 9 ...
  

• Impact: bounded but real. It is one line per job, not per event — which is itself useful confirmation that your new fillCache early-return works — so this is log noise, not a performance problem. It violates "protect production prints with a verbosity flag or message facility", and the column-0 indentation makes clear these were temporary. cosmicOffSpill.log has zero occurrences, which is the addNoise : false path correctly never filling the cache.
• Suggested fix: delete both, or gate them behind the existing diagnostic level.

2. 🟡 [S2] The new peToADC guard turns a silent-wrong-answer into a job abort, and the call site can still trigger it.

• Evidence: Mu2eUtilities/src/CaloNoiseUtil.cc:109-113 throws when the same histoID is prepared with a peToADC differing by ≥ 0.01. The only caller passes a per-SiPM scale factor against a fixed histoID = 0 (CaloDigiMaker_module.cc:202-204), and readoutScaleFactor returns ADCPerMeV/pePerMeV for that specific SiPM (:360-367).
• Whether it fires, checked rather than assumed: CaloConditions/data/Sim_crystal.txt has 1348 crystals with exactly two distinct ratios — 0.533333 (1344 crystals) and 0.534000 (4 crystals, e.g. crystal 582 with 500 500 267 267). Max spread is 6.7e-4, 15× below the 0.01 tolerance. So with generate : false shipped as the default this is dead code, and even with generate : true it does not throw on the current conditions file.
• Impact: latent, and I want to be precise about what changed. Previously a differing scale factor was silently ignored (the emplace no-op). Now it is a hard error. That is the better failure mode, but the tolerance is an absolute cut on a dimensionless ratio whose value happens to be ~0.53; a future conditions file with genuinely per-crystal calibration would abort the job rather than produce per-crystal noise. The guard encodes "one noise waveform for the whole detector" as an invariant while the surrounding code is being built for per-readout IDs.
• Suggested fix: no action needed for this PR. When proditions supplies real noise IDs, either key the cache on (histoID, peToADC) so different scales coexist, or drop peToADC from the interface and document noise as absolute ADC. Worth a // TODO next to the throw so the tolerance is not mistaken for a physics number.

3. 🟡 [S2] CaloHitTruthMatch.digiSampling is now required-but-never-read — carried over from finding 4, partial.

• Evidence: CaloMC/src/CaloHitTruthMatch_module.cc:42 still declares fhicl::Atom<double> digiSampling, and CaloMC/fcl/prolog.fcl:68 still sets it. But :56 initialises digiSampling_(config().pulseCache().digiSampling()) — I grepped for config().digiSampling() in that file and there are zero reads. FHiCL validation still rejects a config that omits the key, and overriding it silently does nothing.
• Impact: no behaviour change today. It is the same trap you just closed in CaloDigiMaker, left open in the sibling module: a knob that looks live and is not.
• Suggested fix: delete the atom and the prolog line, exactly as you did for CaloTemplateWFProcessor in the previous round.

4. 🟡 [S2] Still no recorded physics evidence — carried over.

• The approval from @michaelmackenzie plus stable trigger results remains the strongest evidence on this PR, and I am not re-raising the severity. But the PR body is the permanent record and it still says only "the physics remains the same" with no numbers, for a change that replaced the noise source and redefined the pedestal from a closed-form expression to trunc(mean of histogram bins).
• Suggested fix: paste before/after pedestal and empty-sample waveform RMS into the body — neither is invalidated by the RNG re-sequencing — plus one line on how ReadoutNoise.root was produced. The file contains a single histo_0, so the regeneration recipe is the only way anyone reproduces it later.

5. ⚪ [S3] Nits, all new in this delta.

• Typos in new comments/messages: "alredy" (:107), "differnt" in a user-visible exception string (:112), "histogrm" (:87).
• AddWFNoise is the only PascalCase method among nine in CaloDigiMaker (makeDigitization, fillROHits, buildOutputDigi, extract, readoutScaleFactor, diag0, diag1). Its parameter is noiseWFID in the declaration (:105) and NoiseWFID in the definition (:215).
• CaloNoiseUtil::pedestal() returns int while pedestal_ is std::map<int,double> (CaloNoiseUtil.hh:43, :63) — the double is truncated a second time on return. Harmless because both writers already std::trunc, but the signature says the map should be int, or the return should be double.
• The noiseSegment span-lifetime comment I suggested twice is still absent. Lower stakes now that fillCache is no longer reachable from inside noiseSegment, so this is the last time I raise it.

---

🟢 Verified — no action needed

• 🟢 addNoise : false is value-preserving, not just non-throwing — checked against CaloNoiseSimGenerator's constructor at base rather than inferred from the green CI.
• 🟢 The S0 sub-threshold sub-case is closed by construction, not by luck — prepare() now precedes the range scan.
• 🟢 Removing the CaloDigiMaker.digiSampling atom breaks no external config. Searched Offline, Production @ 20c93edc and mu2e-trig-config @ 51b30e69, plus an org-wide code search: the only external CaloDigiMaker overrides anywhere are the two addNoise : false lines in Production/JobConfig/digitize/{OffSpill,NoField}.fcl. Removing a required FHiCL atom is the one refactor that breaks callers silently at job start, so this was worth confirming explicitly.
• 🟢 fillCache's new early-return is correct in all three cases — same ID cached (returns), different ID same batch (reloads batch), different batch (clears, loads new batch). The one-print-per-job in ceDigi.log is direct evidence of the first case working.
• 🟢 CI fully green at this exact head (8d9ef253, build 3279, merged into base 99536c11): buildtest, all 12 art jobs including the previously-failing cosmicOffSpill, rootOverlaps, g4surfaceCheck, trigger, check_cmake, FIXME/TODO 0/0, whitespace clean. clang-tidy 17 errors / 249 warnings, unchanged in character from main. I did not re-trigger — the head is already green.
• 🟢 Everything cleared in passes one and two still holds. No data product changed; MC-truth ordering contracts intact; extract(), propTimeSimu, CaloShowerStepMaker ancestor logic, PEStatCorrection, deltaTimeMinus untouched since 1bce0f9b. The CaloReco/CMakeLists.txt install rule from pass two is unchanged, so the CMake S0 stays fixed.

Validation check

• Build/tests run by CI: yes, all green at 8d9ef253.
• Build/tests run by reviewer: none. Findings traced from source at this head, the shipped Sim_crystal.txt, the base revision 1ce31dbc, and the fetched ceDigi.log/cosmicOffSpill.log artifacts from build 3279.
• Config contract check: pass — addNoise : false works again; no external override of any removed atom. One dead-but-required key remains (finding 3).
• Cross-repo consistency: pass — no Production or mu2e-trig-config edit needed.
• CMake / dual-build check: pass — unchanged since the pass-two fix.

Residual risk

• The generate : true path is still entirely untested. It is closer to correct than it has been — the keying is fixed and the scale-factor collision is now loud — but nothing exercises it.
• The pedestal remains a per-histogram map with no default. Correct model, and the call site now guarantees prepare() runs first, but any future caller that forgets gets an exception rather than a wrong number. That is the right trade; worth keeping in mind when proditions wiring lands.
• Physics evidence for the noise-model swap still rests on the trigger-stability argument rather than on pedestal/RMS numbers.

Author follow-ups (none blocking)

1. Delete the two std::cout debug lines in CaloNoiseUtil.cc:54,115 — they are in the CI logs at this head (finding 1).
2. Drop the unread digiSampling atom from CaloHitTruthMatch and its prolog line, mirroring the CaloDigiMaker fix you already made (finding 3).
3. Put pedestal and empty-sample-RMS before/after in the PR body, plus one line on how ReadoutNoise.root was generated (finding 4).
4. The nits in finding 5 whenever you next touch these files; a // TODO on the peToADC tolerance (finding 2) would be worth adding while the reasoning is fresh.

Both blockers I raised across two passes are now genuinely fixed, and in each case you implemented the part that mattered rather than the part that would have made CI green — hoisting prepare() closed a sub-threshold path that no test covers. My remaining findings are all things that can ride along with the next touch of these files.

[oksuzian]

PR Review Summary — approval on #1919, "Calo mc rev"

Reviewed at head 8d9ef253. This supersedes the decision on my third-pass review (4890573928) posted minutes ago at this same head — the findings there stand exactly as written, and I am not repeating them here.

Decision

• 🟢 Approve. Supersedes my 🔴 at 1bce0f9b (4884162084) and 🔴 at be443da3 (4887293882).

Why this is a separate post

I posted the third pass as a comment-only review. That was the wrong event: a COMMENTED review does not clear a prior CHANGES_REQUESTED, so my two earlier blocking reviews were still gating this PR while the review body said nothing remaining blocks. That is contradictory, and only I can lift it. Correcting it here.

What this approval rests on — all verified at 8d9ef253 in the third-pass review:

• 🟢 [was S0] pedestal() throwing on every addNoise : false job — fixed. prepare() regained its non-generate branch and was hoisted to the call site ahead of the range scan, which also closes the sub-threshold addNoise : true path that no CI test covers. The addNoise : false pedestal of 0 is value-preserving against base 1ce31dbc, not merely non-throwing.
• 🟢 [was S0] install(DIRECTORY data ...) in the wrong package — fixed in the previous round and unchanged since.
• 🟢 [was S1] CaloTemplateWFUtil argument shift — fixed in the previous round, with the printLevel default dropped so the arity cannot silently under-supply again.
• 🟢 [was S1] generateCache keying and emplace — fixed. All three sites key on the full histoID; regeneration now replaces.
• 🟢 CI fully green at this head (build 3279), including the previously-failing cosmicOffSpill.

Non-blocking follow-ups — carried from review 4890573928, none gating:

1. Delete the two debug std::cout lines in Mu2eUtilities/src/CaloNoiseUtil.cc:54,115. These are live in the CI logs at this head (one line per job in ceDigi.log), so they will ship into production logs. This is the one I would genuinely like to see gone, and it is a two-line deletion.
2. Drop the unread digiSampling atom from CaloHitTruthMatch and its prolog line, mirroring the fix you already made in CaloDigiMaker.
3. Add pedestal and empty-sample-RMS before/after to the PR body, plus one line on how ReadoutNoise.root was generated.
4. Typos (alredy, differnt, histogrm), AddWFNoise casing, pedestal() returning int from a double map, and a // TODO on the peToADC tolerance.

Approving rather than holding the PR for these: the two real blockers are fixed and verified, @michaelmackenzie has approved the reconstruction side, and none of the above changes behaviour. Item 1 is worth doing before merge if it is convenient, but I do not think it warrants blocking.

[bechenard]

I'll make another PR to address the last nits, I like when this is clean