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).
|
What is the plan for calaligncrystal? I wasn't expecting it to be a dbtable since it is a constant. Would this table be in the db or read from a text file? |
|
📝 The HEAD of |
|
Let me scrub the crystal stuff (I left it for just in case but I really doubt this will be needed), this will make the code cleaner |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — "Calorimeter alignment condition support" (#1916)
Reviewed at head b7f10044 (re-review; previous review was at 2b0f3eda).
18 files, +439/−18. Delta under review: 2b0f3eda..b7f10044 ("Fixed AI findings").
Decision
- 🟡 Comment only. All four blocking items from the
2b0f3edareview (one S0, three S1) are fixed and verified at this head — plus @rlcee's request to scrub the crystal table, which the author implemented by deletingCalAlignCrystaloutright. What remains is two 🟡 S2s and a handful of ⚪ S3s, none of which gate. Flip to 🟢 approve oncemu2e/buildtestis green atb7f10044(currently pending — this head has never been built).
Scope understood
- Rigid-body disk alignment (dx,dy,dz,rx,ry,rz) from an fcl-named text file or from
cal.aligndisk, applied to a deep copy of theGeomHandleDiskCalorimeter, served as proditions entityCalorimeter(config keyalignedCalorimeter). - Crystal-level alignment removed entirely in this revision (
CalAlignCrystal, the crystal loop inalignCalorimeter, the crystalDbHandle, the commented-outfilenameCrystalreads). Repo-wide code search forCalAlignCrystalat this head returns 0 hits — no dangling references. - Only
CaloClusterMakerconsumes the aligned geometry so far; everything downstream still usesGeomHandle.
Carry-forward from the 2b0f3eda review
| # | Prior finding | Status at b7f10044 |
|---|---|---|
| 1 | 🔴 S0 — configure_file inputs data/crystal_align.dat / data/disk_nominal.dat do not exist → CMake configure error |
🟢 FIXED, verified |
| 2 | 🟠 S1 — fromDb computes validated params then passes cad_p->rows() / cac_p->rows() |
🟡 PARTIAL — crystal half gone, disk half unchanged; downgraded to S2 (finding A) |
| 3 | 🟠 S1 — CalAlignCrystal missing from DbTableFactory |
🟢 FIXED, verified (resolved by deleting the class) |
| 4 | 🟠 S1 — CaloCluster/CMakeLists.txt not mirrored to the SConscript |
🟢 FIXED, verified |
| 5 | 🟡 S2 — wrong include guards in all three new headers | 🟢 FIXED, verified (all three) |
| 6 | 🟡 S2 — readFile while(!eof) infinite loop on a malformed token |
🟢 FIXED, verified (residual nits in finding C) |
| 7 | 🟡 S2 — aligned/unaligned consumer split undocumented | 🟠 UNADDRESSED — carried over (finding B) |
| 8 | ⚪ S3 — five small items | 🟡 PARTIAL — 3 of 5 fixed; see finding D |
Verification detail for the four cleared blockers:
- 1 (S0) —
CaloConditions/CMakeLists.txtat head has noconfigure_filelines at all;install(DIRECTORY data DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/Offline/CaloConditions)remains. The author went one step further than prescribed and also removed the three pre-existing lines (nominal.txt,caloDMAP_nominal.dat,Sim_crystal.txt). 🟢 That is correct and matches the #1914 precedent exactly:CalorimeterGeom/CMakeLists.txtat this same head hasinstall(DIRECTORY data ...)and zeroconfigure_file. (Mechanically confirmed why the original lines were even parseable:CURRENT_BINARY_DIRis undefined anywhere in the tree — onlyCMAKE_CURRENT_BINARY_DIRexists — so the unquoted${CURRENT_BINARY_DIR}expanded to zero arguments and the third argument became the output path. The missing-input error was therefore real, and the fix is real.) - 3 (S1) —
DbTables/src/DbTableFactory.ccat head registersCalAlignDiskonly (line 133), andCalAlignCrystalno longer exists (DbTables/inc/CalAlignElement.hhnow definesCalAlignElement+CalAlignDisk). Nothing references the removed class. This also answers @rlcee's question directly. - 4 (S1) —
CaloCluster/CMakeLists.txtCaloClusterMakerplugin now listsOffline::CaloConditionsandOffline::ProditionsService. Matches theStrawHitRecoprecedent (TrkHitReco/CMakeLists.txt:Offline::ProditionsService+Offline::TrackerConditionsin the pluginLIBRARIES REG). - 5 (S2) —
CaloConditions_AlignedCalorimeterCache_hh,CaloConditions_AlignedCalorimeterMaker_hh,CaloConfig_AlignedCalConfig_hh. All three now match the repo convention (path words +_hh, no repo prefix inOffline). - 6 (S2) —
readFileis nowwhile (std::getline(...))+ per-linestd::istringstream, throwingcet::exceptionon a failed extraction and on a trailing extra token. The unbounded-append failure mode is gone.
Findings
A. 🟡 [S2] (carried over from finding 2, downgraded) fromDb still computes disk_align_params and then discards it, passing the raw cad_p->rows() instead.
- Evidence:
CaloConditions/src/AlignedCalorimeterMaker.cc:151-152auto disk_align_params = readDb(cad_p, CaloConst::_nDisk); alignCalorimeter(ptr, cad_p->rows());
disk_align_paramsis never read. The crystal half of the original finding is resolved by deletion, so this is no longer a behavior contradiction:readDbis still called, so its range and count checks still fire, and givenCalAlignElement::addRow's strictindex == _rows.size()invariant the vector it builds is element-wise identical tocad_p->rows(). Hence S2, not S1. - Impact: dead work plus a misleading read — a maintainer sees a validated vector built and reasonably assumes it is what gets applied. It will not trip
-Wunused-variable(non-trivial destructor), so no compiler will ever point at it. - Suggested fix:
alignCalorimeter(ptr, disk_align_params);— the same one-line change the original review asked for, and it makesfromDbstructurally identical tofromFcl(which does it correctly at line 137).
B. 🟠 [S2, carried over] The aligned/unaligned consumer split is still undocumented.
- Evidence: PR body is unchanged from
2b0f3eda("Deployment on consumer modules will be done in a later stage by the corresponding experts").CaloClusterMaker_module.cc:86-87takesProditionsHandle<Calorimeter>; every other calorimeter consumer (LineFindercalo matching viadiskToMu2e/mu2eToTracker, CalPatRec, TrackCaloMatching, the truth-match cog checks) still usesGeomHandle<DiskCalorimeter>. - Impact: zero today —
disk_align.txtis 2 rows × 7 zeros, i.e. identity. The first non-zero DB commit makes cluster cog live in the aligned frame while its consumers transform it with unaligned geometry, with no compile-time or runtime signal. Also unstated: sim/digitization must never see the aligned entity. - Suggested fix: a short paragraph in the PR body (or a comment block in
AlignedCalorimeterMaker.hh) naming the consumers that migrate next and the sim/digi exclusion. Cheap now, expensive to reconstruct later.
C. 🟡 [S2] CaloConditions/src/SConscript gained 'CLHEP' but CaloConditions/CMakeLists.txt did not gain CLHEP::CLHEP — the same scons↔CMake asymmetry class as the fixed finding 4.
- Evidence: the SConscript diff un-comments
'CLHEP'(it was#'CLHEP') becauseAlignedCalorimeterMaker.ccnow usesCLHEP::Hep3VectorandCLHEP::HepRotation(lines 28-32).CaloConditions/CMakeLists.txtLIBRARIES PUBLICat head lists no CLHEP. - Impact: it links today —
Offline::CalorimeterGeomcarriesCLHEP::CLHEPas aPUBLICdep and CaloConditions links CalorimeterGeom, so the symbols arrive transitively. So this is not a build break; it is a first-order-dependency-not-declared issue (Mu2e coding standard: "require explicit first-order library dependencies in build files"), and it silently breaks the day CalorimeterGeom's CLHEP link is narrowed. The author's own SConscript edit is the proof that CLHEP is considered first-order here. - Suggested fix: add
CLHEP::CLHEP(and, for the same reason,cetlib_except::cetlib_except, already used by everythrowin this library) toCaloConditions/CMakeLists.txtLIBRARIES PUBLIC.
D. ⚪ [S3] Residual items from prior finding 8, and small nits in the new readFile.
- 🟢 Fixed: duplicate
#include ".../EventTimingCache.hh"inProditionsService.ccremoved; the commented-outfilenameCrystalreads removed from bothfromFclandfromDb; PR title typo corrected. - 🟡 Partial:
CalAlignParams.hhnow documents units ("dx,dy,dz = displacement of the disk in mm", "rx,ry,rz are rotations along the x,y,z axes in radians"). Still undocumented: composition order and pivot. As written (AlignedCalorimeterMaker.cc:29-32),HepRotation::rotateX/Y/Zpre-multiply, so the applied rotation isRz·Ry·Rx, andDisk::moveDiskpivots about the disk origin. One more comment line closes it. - ⚪ Unaddressed:
ProditionsHandle<Calorimeter>is still constructed insideproduce()(CaloClusterMaker_module.cc:86); sibling modules (e.g.StrawHitReco) hold it as a member. Per-event cost is negligible, so this is purely idiom-matching. (Note this is not the "don't cache handles across events" rule —ProditionsHandleis explicitly designed to be held as a member and queried withget(event.id()).) - ⚪ New:
AlignedCalorimeterMaker.ccusesstd::istringstream(line 63) andstd::abson floats (lines 30-32) but includes only<iostream>and<fstream>. It compiles —<sstream>arrives viaDbTables/inc/CalAlignElement.hh:10(and viacetlib_except/exception.h),<cmath>viaCLHEP/Vector/Rotation.h— but both are speculative-transitivity. Add<sstream>and<cmath>directly. The<cmath>one has teeth: if only the<cstdlib>std::abs(int)overload were visible, the guards would truncate every rotation below 1 rad to zero. - ⚪ New:
readFilenow throws on any line that does not parse, including a blank trailing line or a#comment.disk_align.txtat head is exactly two clean lines so nothing breaks, but a hand-edited alignment file that picks up a trailing newline-only line becomes a hard job abort. Consider skipping empty/#lines before the extraction. Also, the "nothing left on the line" guard isfloat extra; if (iss >> extra)— it catches a stray number but silently ignores trailing non-numeric junk;iss >> std::ws; if (!iss.eof()) throw;covers both. Finally, both format throws use categoryALIGNEDCAL_RANGE, which is the range-error category —ALIGNEDCAL_FORMATwould read better in a log. - ⚪ New (simplify lens): the
if (std::abs(disk_align.r?()) > 1e-6)guards buy nothing —rotateX(0.0)is already cheap and exact — while introducing a silent 1e-6 rad dead band. Dropping the three guards is simpler and strictly more faithful. - ⚪ New (simplify lens):
CaloClusterMaker_module.ccincludes noCaloConditionsorDbTablesheader — its only new first-order dep isProditionsService. SoOffline::CaloConditions(CMake),mu2e_CaloConditions(scons mainlib and plugins) andmu2e_DbTables(scons plugins) are over-declarations; the scons mainlib sources (ClusterAssociator.cc,ClusterUtils.cc,ClusterFinder.cc) touch none of them either. Harmless, and my previous review invited the CaloConditions entry conditionally — noting it only so the dependency list stays honest. Does not gate. - ⚪ New: with the crystal loop deleted,
Disk::moveCrystal(CalorimeterGeom/inc/Disk.hh,src/Disk.cc) has zero callers repo-wide. It pre-dates this PR, so removing it is out of scope, but a one-line comment saying it is retained for a possible future crystal-alignment pass would stop the next reader from treating it as live.
🟢 Verified correct — no action
- 🟢
const_castmutation cannot corrupt theGeomHandleoriginal (re-verified:alignCalorimeteris unchanged apart from the crystal-loop deletion).DiskCalorimeter's copy ctor copiesdisks_by value, plusG4Info_andtrackerCenter_, thenrebuildCrystalPtrs()repoints the crystal cache into the copy. The maker mutates only its private deep copy. - 🟢
moveDiskkeeps the object self-consistent — pose viasetPose, crystal global positions recomputed through the new pose, all frame transforms routed through the same pose. - 🟢 Crystal removal is complete and clean.
CalAlignCrystal, the_tacrys_phandle, itsmakeSet/makeIovcontributions, thefromDb/alignCalorimetercrystal parameters, and the zeroed placeholder vectors are all gone together. No half-removal, no unreachable branch, no dangling reference (verified by repo-wide search).CalAlignElement.hhstill includesCaloConst.hh, correctly —CalAlignDiskusesCaloConst::_nDisk. - 🟢
CalAlignElementDB contract.orderBy()is declaredconst std::string orderBy() constand the base isvirtual const std::string orderBy() const— signatures match, so it genuinely overrides (identical to theTrkAlignElementsibling, which likewise omitsoverride).addRowenforces strict sequential indices;rowToCsvmatches theTrkAlignElementconvention including the trailing comma. - 🟢 Proditions wiring. Cache registered under
Calorimeter::cxname, matchingProditionsHandle<Calorimeter>;makeSet/makeIov/makeEntityfollow the standard pattern with correctuseDbguards after the crystal removal;_caches[acc->name()] = acc;added alongside the other calo caches inProditionsService.cc. - 🟢 fcl contract.
CaloConditions/fcl/prolog.fclalignedCalorimeter { filenameDisk, verbose, useDb }exactly matches the three atoms ofAlignedCalConfigat head (filenameCrystalcorrectly absent); registered inProditionsService/fcl/prolog.fclunderProditionsand asfhicl::Table<AlignedCalConfig>inProditionsService.hh.disk_align.txtis 2 rows × 7 zeros = identity for both disks, and 2 ==CaloConst::_nDiskso the count check passes. - 🟢 Cross-repo config compatibility of the new non-defaulted fhicl table. Adding a
fhicl::Tablewith no defaults to the serviceConfigwould break any job that buildsservices.ProditionsServicefrom scratch. Checked every consumer:Offline/fcl/standardServices.fclandmu2e-trig-config/core/trigServices.fclboth useProditionsService: @local::Proditions; all 8Productionhits (JobConfig/reco|mixing|digitize/epilog.fcl,Processing/*.fcl,Validation/database-v1_1-compat.fcl) and the Offline test/EventDisplay fcls are dotted overrides on top of the prolog. No cross-repo change required. - 🟢 No linkage loop introduced.
Offline::ProditionsServiceis anINTERFACElibrary; the service itself is a separateart::serviceplugin that linksOffline::CaloConditions.CaloConditions → Offline::ProditionsService (INTERFACE)therefore does not close a cycle. - 🟢 Merge state.
mergeable: true;mainmovedf4084747 → 1ce31dbcbut the delta isMu2eG4proton-absorber geometry/materials only — no overlap with this PR.
Validation check
- Build/tests run: 🟡
mu2e/buildtestis PENDING atb7f10044— the previous green run was at2b0f3edaand FNALbuild has already marked it out of date (main moved). The2b0f3eda..b7f10044delta is not cosmetic: it deletes a DB table class, changes two function signatures, and rewrites the file parser. Nothing here should fail, but nothing here has been compiled by CI either. Re-run and confirm green before merge. - CMake build: 🟡 still not exercised by CI. Findings 1 and 4 were CMake-only and are now fixed; finding C is CMake-only and non-breaking. A local
cmakeconfigure would close this out in a minute. - Config contract: 🟢 pass (fcl ↔
Configkeys, prolog registration, data-file row count). - Cross-repo consistency: 🟢 pass — no
Production/mu2e-trig-configchange required. Still outstanding on the DB side:cal.aligndiskneeds DBAval.tablesregistration beforeuseDb: truecan ever be exercised (unchanged from the prior review; worth stating in the PR body so @rlcee can schedule it).
Residual risk
- Low. With alignment identity everywhere and
useDb: falseby default, the runtime behavior change is a per-IOV deep copy ofDiskCalorimeterplus one extra proditions cache in every job that runsCaloClusterMaker. - The real risk is deferred, not removed: finding B. The first non-zero
cal.aligndiskcommit produces cluster positions in a frame that no downstream consumer knows about. - No test exercises a non-zero alignment. A one-off job with a shifted disk showing the cog moves by the expected amount would pin the sign and frame conventions end-to-end — worth doing before the first real DB payload, not necessarily before this merge.
Author follow-ups
- Pass
disk_align_params(notcad_p->rows()) infromDb— finding A, one line. - Add
CLHEP::CLHEP(andcetlib_except::cetlib_except) toCaloConditions/CMakeLists.txtto mirror the SConscript — finding C. - Add a paragraph on which consumers migrate to the aligned entity next, and that sim/digi must not — finding B.
- Optional S3 polish: direct
<sstream>/<cmath>includes; tolerate blank/comment lines inreadFile; drop the1e-6rotation guards; document theRz·Ry·Rxorder and disk-origin pivot; note thatDisk::moveCrystalis retained for a possible future pass. - Re-run the build test at
b7f10044and confirm green. - Confirm with @rlcee that
cal.aligndiskwill be registered inval.tablesbefore anyone setsuseDb: true.
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.
Deployment on consumer modules will be done in a later stage by the corresponding experts