Added db support for Calo crystal sim

[bechenard]

This is a replacement for the previous pull request, whose history had become all tangled with the changes Paolo made. This one is cleaner.

Summary of changes:

• added condition / database support for crystal/readout simulation parameters
• renamed CaloDAQMap conditions to CalDAQMap to conform to naming scheme
• fixed LRU simulation in crystal

The crystal conditions require the LRU, the number of PE/MeV for each readout, and the ADCperMeV for each readout. Noise will be added later.

[FNALbuild]

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

• ProditionsService
• DbTables
• CaloMC
• CaloConfig
• CaloConditions
• CaloReco
• DAQ

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☔ The build tests failed for ba3258f.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged ba3258f at 915b109
build (prof)	✅	Log file. Build time: 04 min 20 sec
ceSimReco	❌	Log file. Return Code 9.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	❌	Log file. Return Code 9.
cosmicOffSpill	❌	Log file. Return Code 9.
ceSteps	✅	Log file.
ceDigi	❌	Log file. Return Code 9.
muDauSteps	✅	Log file.
ceMix	❌	Log file. Return Code 9.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (6) FIXME (3) in 25 files
clang-tidy	➡️	31 errors 481 warnings
whitespace check	➡️	found whitespace errors

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

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 HEAD of main has changed to 650ced2. Tests are now out of date.

[bechenard]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 5842327.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 5842327 at 650ced2
build (prof)	✅	Log file. Build time: 04 min 46 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 (6) FIXME (3) in 26 files
clang-tidy	➡️	33 errors 510 warnings
whitespace check	➡️	found whitespace errors

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

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 #1887 — Added db support for Calo crystal sim

Core changes

1. New conditions entity CalSimParams (CaloConditions/inc/CalSimParams.hh + .cc, Maker, Cache) reading LRU / pePerMeV / ADCPerMeV per crystal from fcl or DB.
2. New DB table CalSimCrystals (DbTables/inc/CalSimCrystals.hh), registered in DbTableFactory.cc, wired into ProditionsService.
3. Rename CaloDAQMap → CalDAQMap across CaloConditions, CaloConfig, DAQ, CaloReco, ProditionsService.
4. LRU physics fix in CaloShowerROMaker_module.cc: formula changed from a*(Z/HL−1)+1 to 1+a*(Z/L−0.5), and per-crystal PE/MeV now come from conditions instead of hardcoded CsI/LYSO fcl atoms.
5. Data file Sim_crystal.txt (1348 crystals; a few — 582, 609, 610, 637 — use the higher 500/267 values, the rest 30/16).

Issues found

The DB wiring I flagged earlier is now confirmed correct and complete (factory + service + prolog all present). Remaining concerns:

1. Header guard mismatch — CalSimParamsConfig.hh

#ifndef CaloConditions_CaloSimCrystalConfig_hh
#define CaloConditions_CaloSimCrystalConfig_hh
// Initialize CaloSimCrystalConfig from fcl

Guard/comment don't match the struct name and use the wrong package prefix. Should be CaloConfig_CalSimParamsConfig_hh. Cosmetic but a copy-paste leftover.

2. CalSimParamsMaker::fromDb — ineffective size check + no duplicate-id guard

if (pePerMeVs.back().size() != CaloConst::_nSiPMPerCrystal ) { ... }

pePerMeVs.back() only inspects the last array slot, and every row always builds a 2-element vector, so this can never meaningfully fail. Also, unlike fromFcl (which enforces ascending sid == nRead), fromDb indexes by crid.id() and only counts rows — duplicate crystal ids would silently overwrite while still passing nRead == _nCrystal. Add an ordering/uniqueness check.

3. Whitespace errors flagged by CI
The green build still reports found whitespace errors (see whitespace_errs.log). Likely the trailing blank lines added in CaloConditions/fcl/prolog.fcl and CaloMC/fcl/prolog.fcl. Clean these up before merge — Mu2e reviewers routinely ask for this.

4. Hardcoded magic numbers in CaloMC/fcl/prolog.fcl
readoutPEPerMeV: 30 and MeVToADC: 16 replace the previous @local:: references. These now duplicate what's in the DB/Sim_crystal.txt. Confirm the fcl path (useDb: false) and DB path stay consistent, and that no other fcl still references the removed locals readoutPEPerMeVCsI, readoutPEPerMeVLyso, MeVToADC (dangling @local:: refs would break jobs — the passing CI suggests none remain, but worth a grep).

5. .at() runtime throws on user data
CaloDigiMaker/CaloShowerROMaker call .at(SiPMLocalId()) on the PE/ADC vectors — fine as a hard fail, but a clearer exception message would help since these values come from editable DB/fcl.

6. Removed diagnostic histograms
CaloShowerROMaker drops beginJob() and the hTime_/hEtot_/hECorrtot_/hStot_ histograms entirely. If anyone relied on diagLevel > 1 plots, that capability is gone. Confirm this is intended.

[oksuzian]

Review — "Added db support for Calo crystal sim"

Thanks for this — the DB-migration architecture is sound, the CaloDAQMap → CalDAQMap rename is complete/consistent, and the CAPHRI/CsI values reproduce the old hardcoded constants exactly. Reviewed against a worktree at the PR head (three-dot merge-base diff). One build blocker and two correctness items below; everything else is minor.

🔴 S0 — build blocker

1. CaloDigiMaker plugin is missing its Offline::CaloConditions link dependency.
CaloMC/src/CaloDigiMaker_module.cc now includes CalSimParams.hh, holds a ProditionsHandle<CalSimParams>, and calls out-of-line CalSimParams::pePerMeVs/ADCPerMeVs (lines 18, 122, 236-237) — but cet_build_plugin(CaloDigiMaker …) in CaloMC/CMakeLists.txt does not list Offline::CaloConditions. The PR adds it to CaloShowerROMaker only; CaloDigiMaker was missed, and there's no transitive path (Offline::CaloMC PUBLIC = CalorimeterGeom/GeometryService/Mu2eUtilities/SeedService). Because the top-level build sets NO_UNDEFINED / -Wl,--no-undefined, this is a hard link failure (undefined reference to mu2e::CalSimParams::pePerMeVs/ADCPerMeVs) under the cmake build — currently unverified since mu2e/buildtest hasn't run. (CaloMC/src/SConscript already links mu2e_CaloConditions, so it's a CMake↔SConscript drift.)
Fix: add Offline::CaloConditions to the cet_build_plugin(CaloDigiMaker …) LIBRARIES REG list.

🟠 S1 — major

2. CalSimParamsMaker::fromFcl() — out-of-bounds std::array write on an oversized data file.
CaloConditions/src/CalSimParamsMaker.cc:54-68 checks only ordering (sid != nRead) and positivity, then writes crystalIds[sid]/LRUs[sid]/… into std::array<…,_nCrystal> (1348). There's no sid >= _nCrystal guard inside the loop; the nRead != _nCrystal check runs only after. A 1349th ordered row writes index 1348 (UB) before the count check fires. The DB path is safe because CalSimCrystals::addRow() already guards index >= _nCrystal.
Fix: add if (sid >= CaloConst::_nCrystal) throw cet::exception(...) before the writes, mirroring addRow().

3. The LRU recentring shifts the simulated (and reconstructed) calo energy scale by ~+2.6%.
In CaloShowerROMaker::LRUCorrection, OLD factor = 0.05·(z/L − 1) + 1 (range 0.95–1.00, mean 0.975) → NEW factor = 0.05·(z/L − 0.5) + 1 (range 0.975–1.025, mean 1.000). The coefficient is unchanged (lru = 0.05 for every row), so this is a pure +0.025 recentring ≈ +2.56% on the mean. LRUCorrection is on by default, and the CsI digi→reco chain is unity (MeVToADC=16, ADC2MeVCsI=1/16), so the shift flows straight into reconstructed crystal energy. This looks like the intended LRU fix (the old form wasn't zero-mean), but it's an absolute-scale physics change that isn't called out and makes pre-/post-MC differ ~2.5%.
Ask: please note it in the description and attach a calo energy-scale validation (Ce / source peak); confirm ADC2MeVCsI/ADC2MeVlyso are still valid.

4. CaloHitTruthMatch — units inconsistency collapses the MC-truth match window.
The PR removes for (auto& v : wf_) v *= MeVToADC_; but leaves minAmplitude = 32. Since CaloPulseShape normalizes the pulse to peak = 1.0, wf_[nbin]*hit.energyDep() (line 204) is now MeV-valued, not ADC — so the forward-window crossover moves from 32/16 = 2 MeV to 32 MeV. Per-crystal deposits are almost always < 32 MeV, so deltaTimePlus collapses to its floor and the energy-dependent widening is effectively disabled (MC-truth only: CaloHitMCTruthAssn, consumed by CaloClusterTruthMatch).
Fix: set minAmplitude : 2 (= old 32/16) or restore the ×16 scaling — keep both sides of the comparison in the same units. Is the collapse intended, or should minAmplitude drop to 2?

🟡 S2 — minor

• Accessors copy in hot loops — CalSimParams::pePerMeVs()/ADCPerMeVs() (CalSimParams.hh:39-40) return std::vector<float> by value (heap alloc + copy per RO/step in CaloDigiMaker/CaloShowerROMaker). Storage is std::array<std::vector<float>,N>, so return const std::vector<float>&.
• fromFcl vs fromDb validate differently (CalSimParamsMaker.cc:60 vs :105) — fromFcl rejects negative lru/npe (not adc); fromDb does no positivity check. Mirror them (and add adc), or drop for parity and document.

🔵 S3 — nits

• CAPHRI rows (582/609/610/637) use ADCPerMeV = 267 vs exact 1/ADC2MeVlyso = 266.67 → +0.125% lyso sim↔reco mismatch; use 266.67.
• Dead art_root_io/TFileService.h + TFileDirectory.h includes remain after histogram removal (CaloShowerROMaker_module.cc:9-10).
• DbTables/inc/CalSimCrystals.hh: add explicit includes for cetlib_except/exception.h and DataProducts/inc/CaloConst.hh (currently transitive); orderBy() missing override.
• Carried through the rename: commented-out //CalDAQMap():…, ~CalDAQMap(){} → = default, stale CALODAQMAP_* exception category strings.

Notes

• useDb:true is an ops prerequisite — the CalSimCrystals (cal.simcrystals) table must be committed and attached to a GLOBAL tag before any campaign flips it on. Default useDb:false (reads Sim_crystal.txt) is unaffected.
• The public service key caloDAQConditions → calDAQConditions doesn't break in-tree or Production main, but personal/analysis fcl overriding it should track the rename.
• FYI, not a change request: the deltaTimeMin 100→10ns line some diffs show in CaloHitTruthMatch is not from this PR — the three-dot merge-base diff confirms it came from a later main commit.
• Minor hygiene: this bundles DB support + a repo-wide rename + physics changes; splitting the pure rename would ease review next time.

Verified correct (no action): CAPHRI id mapping is exact (_caphriId = {582,609,610,637} = the four pePerMeV=500 rows, 1344 CsI + 4 = 1348); PE/ADC ratios reproduce the old constants; _nSiPMPerCrystal(2) is authoritative; CSV round-trip is safe; DbTableFactory registration matches cxname; the fcl↔config↔cache rename is consistent across all three layers; the DAQ ×4 + CaloReco edits are pure renames.

[giro94]

Everything looks great, thanks!

Unsure if we'll need the pePerMeV for both SiPMs, but it won't hurt having 2 columns just in case.

[giro94]

Just a note: the latest AI review notes some things to be fixed, in particular 4. and S3.1

[bechenard]

Yes, the AI monster is always attacking you when you expect it the least. I just rolled a 19 and launches a fireball to kill it. Will fix the code too when I have time - going on vacation for a few days today