Small cleaning for PR 1919

[bechenard]

The review of PR1919 flagged a few cosmetic issues to fix - these are the requested changes. This is purely cosmetic so I don;t think there is need for extensive review here.

[FNALbuild]

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

• CaloMC
• Mu2eUtilities

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 9cd4f22.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 9cd4f22 at 5c9ca42
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 4 files
clang-tidy	➡️	6 errors 50 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 9cd4f22 after being merged into the base branch at 5c9ca42.

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 — #1928, "Small cleaning for PR 1919"

Reviewed at head 9cd4f227 (2026-08-10). This PR implements the non-blocking follow-up list from my approval on #1919, so I reviewed it as a fix-for-review: every item on that list is accounted for below, and each claimed fix is verified in the source at this head rather than read off the diff summary.

Decision

• 🟢 Approve. Six of the seven items I asked for are done, and done completely — including the two halves of the AddWFNoise item and the noiseSegment lifetime comment I had raised three times. The seventh (a // TODO on the peToADC tolerance) is a one-liner that can ride along next time.

On "purely cosmetic"

Mostly true, and I agree it does not need an extensive review — but two of the five files change a contract rather than a comment, so I checked those two properly rather than waving them through:

• CaloNoiseUtil::pedestal() changes its return type int → double. Verified value-preserving. Both writers already store std::trunc(...) (CaloNoiseUtil.cc:97, :145), so every value in pedestal_ is integral; the single caller in the whole organisation is CaloDigiMaker_module.cc:207, which assigns into double pedestal(0) (:200) and passes it to buildOutputDigi(..., double pedestal, ...). Org-wide code search finds no consumer of CaloNoiseUtil outside CaloDigiMaker. So the truncation this removes was a second, redundant one — no digitized ADC changes.
• Deleting the digiSampling atom from CaloHitTruthMatch is a FHiCL schema removal, and with fhicl::Table validation an external job that sets a removed key fails at construction, not silently. Verified no such job exists. Org-wide search for digiSampling returns hits only in CaloHitsFromDTCEvents / CaloHitMakerFast blocks (mu2e-trig-config/core/producers/trigCalProducers.fcl:39,58), SourceCalibDigiAna's own atom (CaloCalibration), an ancient CaloShowerStepROFromShowerStep-era block in Tutorial/RunningArt/fcl/CeEndpoint-debug.fcl:116, and otsdaq — none of them a CaloHitTruthMatch block, and nothing in Production.

---

Follow-up accounting (vs review 4890573928 / approval 4890590151 on #1919)

1. 🟢 [was 🟡 S2] The two debug std::cout lines — FIXED, verified. Mu2eUtilities/src/CaloNoiseUtil.cc has no std::cout left outside printCache() (:183-184) and dumpNoise() (:197), which are explicit diagnostic entry points rather than lines that run in every job. This was the one item I actually wanted before merge; ceDigi.log will be clean now.

2. 🟢 [was 🟡 S2] CaloHitTruthMatch.digiSampling required-but-never-read — FIXED, verified, and the removal is safe in both directions. The atom is gone from CaloHitTruthMatch_module.cc:42 and the prolog line from CaloMC/fcl/prolog.fcl. I checked the module still works: digiSampling_ (:80) is initialised from config().pulseCache().digiSampling() (:55), the pulseCache : @local::CaloPulseCache line survives in the prolog block, and digiSampling_ is genuinely read at :180-183. So the member stays live and only the dead knob went — exactly the shape of the CaloDigiMaker fix it mirrors.

3. 🟢 [was ⚪ S3] Typos — FIXED, all three. alredy → already (:105), differnt → different in the user-visible exception string (:110), histogrm → histogram (:85).

4. 🟢 [was ⚪ S3] AddWFNoise — FIXED, both halves. Renamed addWFNoise, so it no longer stands out as the only PascalCase method among nine in CaloDigiMaker; and the declaration/definition parameter-name split is closed too — the definition and the noiseSegment call now both use noiseWFID (:215, :249). The local at the call site is still NoiseWFID (:202), which is fine: it is a local, not the parameter I flagged.

5. 🟢 [was ⚪ S3] pedestal() returning int from a double map — FIXED, verified value-preserving (see above). The definition also moved its brace to its own line, which is now consistent with all seven other CaloNoiseUtil:: definitions in the file — I checked rather than assumed.

6. 🟢 [was ⚪ S3] The noiseSegment span-lifetime comment — FIXED. Present both as a file-level contract in CaloNoiseUtil.hh:6-8 and inline above the function (CaloNoiseUtil.cc:156). Third time I raised it; thanks for taking it.

7. ⚪ [was ⚪ S3] // TODO on the peToADC tolerance — UNADDRESSED. No TODO in the changed files (CI confirms: FIXME/TODO 0/0). The std::abs(it->second - peToADC) < 0.01 at CaloNoiseUtil.cc:109 is still an absolute cut on a dimensionless ratio that happens to sit near 0.53, with nothing recording that it is an engineering tolerance rather than a physics number. Carried forward, still not blocking.

8. 🟡 [was 🟡 S2] Pedestal/RMS before-after and the ReadoutNoise.root recipe — still nowhere, and this PR is not the right home for it. #1919 is merged, so the body cannot be usefully edited now. Noting it so it does not vanish with the follow-up list: the regeneration recipe for a single-histogram binary blob is the kind of thing that is free to write down today and expensive to reconstruct in a year. A comment at the top of CaloNoiseUtil.cc or a line in the calo docs would close it.

---

Findings (none gating)

1. ⚪ [S3] The new header comment introduces a typo of its own, in a PR whose job was fixing typos.

   • Evidence: Mu2eUtilities/inc/CaloNoiseUtil.hh:6 — // Call prepare cache beore using it (beore → before). :7 also reads each peToADC require a __different__ noise histoID (require → requires), and the __different__ markdown emphasis does not do anything in a C++ comment.
   • Impact: none beyond the irony. Worth a second pass while the file is open.

2. ⚪ [S3] The new header contract is stated more broadly than the code implements.

   • Evidence: CaloNoiseUtil.hh:7 says "Throw if histoID is already used for a different peToADC". That guard lives only in generateCache (CaloNoiseUtil.cc:107-111); prepare() routes to fillCache(histoID) when generate_ is false (:44-45), and fillCache never looks at peToADC at all. With the shipped default the throw is unreachable.
   • Impact: a reader takes the header at face value and assumes the invariant holds on every path. It holds only when regenerating.
   • Suggested fix: "when regenerating the cache, throw if ...". This is the same sentence that would carry the // TODO from item 7 — one edit closes both.

3. ⚪ [S3] printCache() and dumpNoise() are the remaining unguarded std::cout in this file, and nothing calls either one.

   • Evidence: CaloNoiseUtil.cc:181-186 and :188-198; org-wide code search for both names returns only the declaration and the definition — zero call sites in Offline or any other Mu2e repo.
   • Impact: none today, which is exactly why I am not asking for it here — it is dead debug scaffolding of the same family as the two lines you just deleted, kept deliberately for the generate : true work. Flagging it so that when the proditions wiring lands, they get a verbosity gate or go, rather than becoming the next Fill Cache noise.

---

Validation check

• Build/tests run by CI: yes, fully green at this exact head. Build 3287, 9cd4f227 merged into base 5c9ca42a: build (prof) 8 min 52 s, all 12 art jobs (ceSimReco, g4test_03MT, transportOnly, POT, g4study, cosmicSimReco, cosmicOffSpill, ceSteps, ceDigi, muDauSteps, ceMix, trigger), rootOverlaps, g4surfaceCheck, check_cmake, FIXME/TODO 0/0, whitespace clean. cosmicOffSpill green matters here specifically — it is the addNoise : false path, the one the #1919 S0 lived on. I did not re-trigger; the head is already green.
• Build/tests run by reviewer: none. Findings traced from the five files at 9cd4f227, plus org-wide code search for the two contract changes.
• Config contract check: pass — the removed CaloHitTruthMatch.digiSampling atom has no external setter anywhere in the organisation, and the module's own digiSampling_ still resolves through pulseCache.
• Cross-repo consistency: pass — no Production or mu2e-trig-config edit needed; @local::HitMakerDigiSampling keeps its other users in CaloReco/fcl/{common,prolog}.fcl and CaloCalibration, so the deleted reference orphans nothing.
• API check: pass — CaloNoiseUtil has exactly one consumer (CaloDigiMaker), so the pedestal() signature change cannot reach anything else.

Residual risk

• Unchanged from #1919 and untouched by this PR: the generate : true path is still exercised by nothing, and the noise-model swap still rests on the trigger-stability argument rather than on recorded pedestal/RMS numbers (item 8).

Author follow-ups (all optional, none blocking)

1. beore → before in the new header comment, and narrow the peToADC sentence to the regeneration path (findings 1 and 2).
2. While you are on that line, the // TODO on the 0.01 tolerance (item 7).
3. Whenever the proditions noise IDs land: write the ReadoutNoise.root recipe down somewhere permanent (item 8), and decide whether printCache/dumpNoise get a verbosity gate or get deleted (finding 3).

[oksuzian]

@FNALbuild run build test

[FNALbuild]

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

[oksuzian]

PR Review Summary — re-review of #1928, "Small cleaning for PR 1919"

Reviewed at head 2d8c85ba (2026-08-10). Second pass. Prior review: mine 🟢 at 9cd4f227 (4900339897, 3 ⚪ findings + one item carried from #1919). The delta is one commit, 2d8c85ba "Fixed AI irony", +4/−2 across two files. Every finding from the last pass is verified in the source at this head.

Decision

• 🟢 Approve, re-affirmed at the new head. All four open items are closed — including the // TODO that had been carried since the #1919 review two rounds ago. One new ⚪ from the delta, and one half of an old finding still open; neither is worth another round.

---

Carry-forward accounting (vs review 4900339897 at 9cd4f227)

1. 🟢 [was ⚪ S3] Typos in the new header comment — FIXED, verified. CaloNoiseUtil.hh:6 is Call prepare cache before using it, and :7 now reads each peToADC requires a __different__ noise histoID. (The __different__ markdown emphasis is still there and still does nothing in a C++ comment. Not worth a commit — noting it only so the item is closed rather than dropped.)

2. 🟢 [was ⚪ S3] The header contract stated more broadly than the code implements — FIXED, verified. :7 now opens with When regenerating, throw if histoID is already used for a different peToADC, which is exactly the scope of the guard: it lives in generateCache (CaloNoiseUtil.cc:107-111) and fillCache never looks at peToADC. The sentence and the code now say the same thing.

3. 🟢 [was ⚪ S3, and carried from the #1919 review] // TODO on the peToADC tolerance — FIXED. CaloNoiseUtil.hh:9: // TODO: fix tolerance on PeToADC ratio. This is the item I have raised in three consecutive reviews; it is now on the record next to the std::abs(...) < 0.01 it refers to, which is all I wanted. CI's FIXME/TODO counter will tick from 0 to 1 — expected, not a regression.

4. 🟡 [was ⚪ S3] printCache / dumpNoise unguarded and uncalled — PARTIAL, and the half you did is the right half. printCache() now has a caller and a verbosity gate: if (diagLevel_>2) noiseSampler_.printCache(); (CaloDigiMaker_module.cc:205). That satisfies the "protect production prints with a verbosity flag" rule and turns dead scaffolding into a usable diagnostic. dumpNoise still has zero callers org-wide — searched again at this head, only the declaration and definition. Leaving it for the generate : true work, as before. See finding 1 for the placement.

5. 🟡 Pedestal/RMS evidence and the ReadoutNoise.root recipe — still open, still not this PR's to fix (#1919 is merged). Restating so it does not vanish with the follow-up list.

---

Findings (none gating)

1. ⚪ [S3] The new printCache() call prints job-constant state once per readout channel.

   • Evidence: CaloDigiMaker_module.cc:205 sits inside the for (int iRO=0;iRO<nWaveforms;++iRO) loop (:194), where nWaveforms = nCrystals × nSiPMPerCrystal = 2696 for the current calorimeter. The cache it prints does not change after the first call: prepare() is always invoked with the same NoiseWFID(0), fillCache early-returns when the ID is present, and generateCache returns when the peToADC matches. So at diagLevel > 2 the same block is printed for every non-empty readout of every event, and printCache itself emits one line per cached histogram on top of a header line.
   • Impact: none in production — diagLevel : 0 in CaloMC/fcl/prolog.fcl:58 and nothing overrides it, and the gate is inside if (addNoise_) so the addNoise : false path never reaches it. Purely about whether the diagnostic is usable when someone does turn it on: a few thousand identical blocks per event is harder to read than one.
   • Suggested fix: hoist it out of the loop, or guard it with a bool printed so it fires once per job. Whenever you next touch this.

2. ⚪ [S3] dumpNoise remains the file's one unguarded std::cout with no caller (carried, see item 4 above). Same disposition as last time: fine to leave until the proditions noise IDs land, then gate it or delete it.

---

Validation check

• Build/tests run by CI: not yet at this head. mu2e/buildtest reported "This test has not been triggered yet" at 2d8c85ba, so I triggered one — the build I referenced in my previous review (3287) was at 9cd4f227 and is stale. Result pending as of this review; please check it before merging. The 9cd4f227 build was fully green, and this delta is two comment lines plus one diagLevel-gated call, so I expect green.
• Build/tests run by reviewer: none. Findings traced from the two changed files at 2d8c85ba, the enclosing loop in CaloDigiMaker_module.cc, and an org-wide code search for dumpNoise.
• Config contract check: pass — unchanged from the last pass; no FHiCL schema or default is touched by this delta.
• Cross-repo consistency: pass — comment-only plus a gated debug call; nothing external to update.
• API check: pass — printCache() is public and const on CaloNoiseUtil (CaloNoiseUtil.hh:49), called on a non-const member from a non-const method. No signature change in this delta.

Residual risk

• Unchanged: the generate : true path is still exercised by nothing, and the physics evidence for the #1919 noise-model swap still rests on the trigger-stability argument rather than on recorded pedestal/RMS numbers.

Author follow-ups (optional, none blocking)

1. Confirm mu2e/buildtest is green at 2d8c85ba before merging — I triggered it, but it had not reported when this review was written.
2. Whenever you next touch these files: hoist the printCache() call out of the readout loop (finding 1), and decide dumpNoise's fate along with the proditions work (finding 2).

[FNALbuild]

☀️ The build tests passed at 2d8c85b.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 2d8c85b at 0e06c26
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 (1) FIXME (0) in 4 files
clang-tidy	➡️	6 errors 50 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 2d8c85b after being merged into the base branch at 0e06c26.

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.