Calorimeter alignment condition support - #1916
Conversation
|
Hi @bechenard,
which require these tests: build. @Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main. ⌛ The following tests have been triggered for 2b0f3ed: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 2b0f3ed.
N.B. These results were obtained from a build of this Pull Request at 2b0f3ed after being merged into the base branch at f408474. For more information, please check the job page here. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — "Calorimeter alignment condition support" (#1916)
Reviewed at head 2b0f3eda (2026-08-04). 17 files, +464/−14: new CalAlignDisk/CalAlignCrystal DB tables, an alignedCalorimeter proditions entity (deep-copied DiskCalorimeter with disk shifts/rotations applied), and CaloClusterMaker switched from GeomHandle to ProditionsHandle<Calorimeter>. The core design is sound and the dangerous part — mutating a copy of the geometry through const_cast — is verified safe below. The blockers are all in the build/wiring layer, and two of the three are invisible to the scons CI by construction.
Decision
- 🔴 Request changes. One S0 (CMake configure breaks on merge) and three S1s (fromDb ignores its own params, missing factory registration, CMake link deps not updated). All are small fixes.
Scope understood
- Rigid-body disk alignment (dx,dy,dz,rx,ry,rz) from fcl file or DB, applied to a deep copy of the GeomHandle
DiskCalorimeter, served as proditions entityCalorimeter("alignedCalorimeter" in config). - Crystal-level alignment plumbed but deliberately zeroed ("won't budge without explosives"); only
CaloClusterMakerconsumes the aligned geometry so far.
Findings
1. 🔴 [S0] CMake configure fails: both new configure_file inputs do not exist — the spack/CMake build of Offline breaks on merge, and scons CI cannot see it.
- Evidence:
CaloConditions/CMakeLists.txtaddsconfigure_file(...data/crystal_align.dat ...)andconfigure_file(...data/disk_nominal.dat → data/disk_align.dat).CaloConditions/data/at this head containsSim_crystal.txt, caloDMAP_nominal.dat, disk_align.txt, nominal.txt— neithercrystal_align.datnordisk_nominal.datexists.configure_filewith a missing input is a hard configure-time error. The Jenkins buildtest is scons-only and green. - Impact: identical failure mode to the
crystalPos.txtincident (#1908 → spack break → #1914), caught pre-merge this time. - Suggested fix: delete both lines.
install(DIRECTORY data ...)is already present in this CMakeLists and covers the install; theconfigure_file(${CURRENT_BINARY_DIR})staging idiom is exactly what #1914 removed from CalorimeterGeom — don't extend it. (The mismatched output namedisk_nominal.dat → disk_align.datin the second line is a symptom of the same copy-paste.) The runtime path is safe without them: the fcl nameOffline/CaloConditions/data/disk_align.txtresolves viaConfigFileLookupPolicyfrom the source tree or the installedshare/.
2. 🟠 [S1] fromDb computes validated parameter vectors and then ignores them — raw DB crystal rows get applied, contradicting the PR's own scope.
- Evidence:
AlignedCalorimeterMaker.cc::fromDbbuildsdisk_align_paramsviareadDb(...)(range+count validation) and a zeroedcrystal_align_params, then callsalignCalorimeter(ptr, cad_p->rows(), cac_p->rows())— both locals discarded. - Impact: for disks it's dead work (table
addRowalready enforces sequential indices, so contents are identical). For crystals it's a behavior contradiction:fromFclforces zeros per the stated "crystals won't budge" scope, butfromDbapplies whatevercal.aligncrystalcontains. The zeroed local shows the intended call. - Suggested fix:
alignCalorimeter(ptr, disk_align_params, crystal_align_params);— one line.
3. 🟠 [S1] CalAlignCrystal is not registered in DbTableFactory — the useDb:true path throws on first use.
- Evidence:
DbTableFactory.ccadds onlyCalAlignDisk; the unknown-name fallthrough isthrow cet::exception("DBFILE_BAD_TABLE_NAME").AlignedCalorimeterCache::initialize()createsDbHandle<CalAlignCrystal>wheneveruseDbis true. - Impact: the DB-backed mode is dead on arrival. Latent today (prolog default
useDb:false), first-turn-on failure later. - Suggested fix: add the
CalAlignCrystalcase. Also worth stating in the PR body thatcal.aligndisk/cal.aligncrystalneed the DBA-sideval.tablesregistration beforeuseDbcan ever be exercised.
4. 🟠 [S1] CMake plugin dependencies not updated to mirror the SConscript — CMake is the only link check that matters (--no-undefined).
- Evidence:
CaloCluster/src/SConscriptgainsmu2e_CaloConditions(mainlib+plugins) andmu2e_DbTables(plugins), butCaloCluster/CMakeLists.txtis untouched: theCaloClusterMakerplugin lists neitherOffline::ProditionsServicenorOffline::CaloConditions. The sibling precedent is explicit:StrawHitReco(the canonicalProditionsHandleconsumer) linksOffline::ProditionsService+ its conditions library in its plugin block. - Impact: scons CI green proves nothing here; the CMake link of the module is at risk exactly per the repo's known scons↔CMake drift failure mode.
- Suggested fix: add
Offline::ProditionsService(andOffline::CaloConditionsif the entity headers are reached) to theCaloClusterMakerplugin LIBRARIES, mirroring StrawHitReco.
5. 🟡 [S2] All three new headers have wrong include guards.
AlignedCalorimeterCache.hh:TrackerConditions_AlignedCalorimeterCache_hh(copy-paste from TrackerConditions).AlignedCalorimeterMaker.hh:CalConditions_...(typo for CaloConditions).AlignedCalConfig.hh:CaloConditions_...but the file lives inCaloConfig/.- Suggested fix:
CaloConditions_AlignedCalorimeterCache_hh,CaloConditions_AlignedCalorimeterMaker_hh,CaloConfig_AlignedCalConfig_hh.
6. 🟡 [S2] readFile's EOF loop hangs on a malformed token.
- Evidence:
while (!ordFile.eof()) { ordFile >> ...; if (eof) break; ... }— a non-numeric token setsfailbit, nevereofbit; every subsequent>>is a no-op and the loop appends the stale values forever (unbounded memory, no diagnostic). Potential risk (needs a bad file), but alignment files are exactly the kind of hand-edited input that gets typos. - Suggested fix:
while (ordFile >> index >> dx >> dy >> dz >> rx >> ry >> rz) { ... }and keep the count check after — fails loudly on any malformed line.
7. 🟡 [S2] The aligned/unaligned consumer split is undocumented.
- Evidence: only
CaloClusterMakermigrates to the aligned geometry. Everything downstream that interprets cluster positions —LineFinder's calo matching (diskToMu2e/mu2eToTracker), CalPatRec, TrackCaloMatching, the truth-match cog checks — still usesGeomHandle(unaligned). - Impact: zero today (
disk_align.txtis identity), but the first non-zero DB commit makes cluster cog live in the aligned frame while consumers transform it with unaligned geometry. That rollout plan should be written down. - Suggested fix: a sentence in the PR body (or the maker header) naming which consumers migrate next and the constraint that sim/digitization must never see the aligned entity.
8. ⚪ [S3] Small items.
ProditionsService.ccnow includesDAQConditions/inc/EventTimingCache.hhtwice.- Commented-out
filenameCrystalreads in bothfromFcl/fromDbplus a config that has nofilenameCrystalatom — either wire it or drop the comments (code-history comments). ProditionsHandle<Calorimeter>constructed perproduce()call; sibling modules hold it as a member.- Rotation convention undocumented: units of
rx,ry,rz(radians?), composition orderrotateX·rotateY·rotateZ, pivot = disk origin viasetPose(origin+shift, rotation*rot). One comment inCalAlignParams.hhfixes it. - Title typo: "contition" → "condition".
🟢 Verified correct — no action
- 🟢 The
const_castmutation cannot corrupt the GeomHandle original:DiskCalorimeter's copy ctor copiesdisks_by value (std::vector<Disk>),G4Info_, andtrackerCenter_(the #1908 omission — fixed at main), thenrebuildCrystalPtrs()repoints the crystal-pointer cache into the copy. The maker mutates only its private deep copy. This was the highest-risk item in the PR and it is clean. - 🟢
moveDisk/moveCrystalkeep the object self-consistent: pose updated viasetPose, every crystal's global position recomputed through the new pose, and all frame transforms (mu2eToDisk,diskToMu2e, FF variants) flow through the same pose. Neighbor caches are local-id-based post-#1908 and immune to pose moves. - 🟢 Proditions contract: cache registered under
Calorimeter::cxname("Calorimeter"), matchingProditionsHandle<Calorimeter>;DiskCalorimeter → Calorimeterentity resolution is fine;makeSet/makeIov/makeEntityfollow the standard cache pattern with correctuseDbguards. - 🟢 fcl contract: prolog
alignedCalorimetertable ↔AlignedCalConfigkeys match; registered in both the Proditions prolog and the service config;disk_align.txt= 2 rows × 7 zeros = identity alignment for both disks. - 🟢
rowToCsvmatches theTrkAlignElementsibling convention exactly (including the trailing comma — checked, it's the established pattern, not a defect). - 🟢
addRowenforces strict sequential indices — good defensive parsing. - 🟢 Link transitivity for the maker library:
GeomHandleuse inCaloConditionsresolves viaOffline::ProditionsService's PUBLIC link ofOffline::GeometryService.
Validation check
- Build/tests: 🟡 scons CI green at
2b0f3eda— but findings 1 and 4 live exclusively in the CMake build, which CI does not exercise. A localcmakeconfigure (or spack dev-build) would catch both instantly. - Config contract: 🟢 pass.
- Cross-repo: 🟢 none required in Production/trig-config while the entity has no non-identity data; DB-side
val.tablesregistration needed beforeuseDb(finding 3).
Residual risk
- If merged as-is, the next spack build of Offline fails at CMake configure (finding 1) — the same downstream break as the crystalPos.txt incident.
- No test exercises a non-zero alignment; a one-off job with a shifted disk demonstrating the cog moves accordingly would validate the sign/frame conventions end-to-end.
Author follow-ups
- Delete the two
configure_filelines (finding 1). - Pass the validated locals in
fromDb(finding 2). - Register
CalAlignCrystalin the factory; note theval.tablesprerequisite (finding 3). - Mirror the SConscript deps into
CaloCluster/CMakeLists.txt(finding 4). - Fix the three include guards (finding 5).
- Harden
readFile(finding 6) and document the rotation convention (finding 8). - State the consumer-migration plan for the aligned entity (finding 7).
Support for calorimeter alignment via the condition service. Only disk movement and rotations are supported at this point. Code to move the crystals, should this be necessary (I doubt it because we already use their measured placement in the calorimeter and these crystals won't budge without explosives), is readily available. The resulting object is called alignedCalorimeter, following the tracker.