Skip to content

Calorimeter alignment condition support - #1916

Open
bechenard wants to merge 2 commits into
Mu2e:mainfrom
bechenard:alignDB
Open

Calorimeter alignment condition support#1916
bechenard wants to merge 2 commits into
Mu2e:mainfrom
bechenard:alignDB

Conversation

@bechenard

@bechenard bechenard commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

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

@FNALbuild

Copy link
Copy Markdown
Collaborator

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

  • DbTables
  • CaloConfig
  • ProditionsService
  • CaloCluster
  • CaloConditions

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)

About FNALbuild. Code review on Mu2e/Offline.

@bechenard bechenard changed the title Calorimete ralignment support Calorimeter alignment contition support Aug 4, 2026
@FNALbuild

Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at 2b0f3ed.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 2b0f3ed at f408474
build (prof) Log file. Build time: 04 min 24 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 10 files
clang-tidy ➡️ 6 errors 33 warnings
whitespace check no whitespace errors found

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.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

@oksuzian
oksuzian requested a review from rlcee August 4, 2026 02:37
@bechenard bechenard changed the title Calorimeter alignment contition support Calorimeter alignment condition support Aug 4, 2026

@oksuzian oksuzian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 entity Calorimeter ("alignedCalorimeter" in config).
  • Crystal-level alignment plumbed but deliberately zeroed ("won't budge without explosives"); only CaloClusterMaker consumes 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.txt adds configure_file(...data/crystal_align.dat ...) and configure_file(...data/disk_nominal.dat → data/disk_align.dat). CaloConditions/data/ at this head contains Sim_crystal.txt, caloDMAP_nominal.dat, disk_align.txt, nominal.txt — neither crystal_align.dat nor disk_nominal.dat exists. configure_file with a missing input is a hard configure-time error. The Jenkins buildtest is scons-only and green.
  • Impact: identical failure mode to the crystalPos.txt incident (#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; the configure_file(${CURRENT_BINARY_DIR}) staging idiom is exactly what #1914 removed from CalorimeterGeom — don't extend it. (The mismatched output name disk_nominal.dat → disk_align.dat in the second line is a symptom of the same copy-paste.) The runtime path is safe without them: the fcl name Offline/CaloConditions/data/disk_align.txt resolves via ConfigFileLookupPolicy from the source tree or the installed share/.

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::fromDb builds disk_align_params via readDb(...) (range+count validation) and a zeroed crystal_align_params, then calls alignCalorimeter(ptr, cad_p->rows(), cac_p->rows()) — both locals discarded.
  • Impact: for disks it's dead work (table addRow already enforces sequential indices, so contents are identical). For crystals it's a behavior contradiction: fromFcl forces zeros per the stated "crystals won't budge" scope, but fromDb applies whatever cal.aligncrystal contains. 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.cc adds only CalAlignDisk; the unknown-name fallthrough is throw cet::exception("DBFILE_BAD_TABLE_NAME"). AlignedCalorimeterCache::initialize() creates DbHandle<CalAlignCrystal> whenever useDb is 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 CalAlignCrystal case. Also worth stating in the PR body that cal.aligndisk/cal.aligncrystal need the DBA-side val.tables registration before useDb can 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/SConscript gains mu2e_CaloConditions (mainlib+plugins) and mu2e_DbTables (plugins), but CaloCluster/CMakeLists.txt is untouched: the CaloClusterMaker plugin lists neither Offline::ProditionsService nor Offline::CaloConditions. The sibling precedent is explicit: StrawHitReco (the canonical ProditionsHandle consumer) links Offline::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 (and Offline::CaloConditions if the entity headers are reached) to the CaloClusterMaker plugin 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 in CaloConfig/.
  • 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 sets failbit, never eofbit; 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 CaloClusterMaker migrates to the aligned geometry. Everything downstream that interprets cluster positions — LineFinder's calo matching (diskToMu2e/mu2eToTracker), CalPatRec, TrackCaloMatching, the truth-match cog checks — still uses GeomHandle (unaligned).
  • Impact: zero today (disk_align.txt is 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.cc now includes DAQConditions/inc/EventTimingCache.hh twice.
  • Commented-out filenameCrystal reads in both fromFcl/fromDb plus a config that has no filenameCrystal atom — either wire it or drop the comments (code-history comments).
  • ProditionsHandle<Calorimeter> constructed per produce() call; sibling modules hold it as a member.
  • Rotation convention undocumented: units of rx,ry,rz (radians?), composition order rotateX·rotateY·rotateZ, pivot = disk origin via setPose(origin+shift, rotation*rot). One comment in CalAlignParams.hh fixes it.
  • Title typo: "contition" → "condition".

🟢 Verified correct — no action

  • 🟢 The const_cast mutation cannot corrupt the GeomHandle original: DiskCalorimeter's copy ctor copies disks_ by value (std::vector<Disk>), G4Info_, and trackerCenter_ (the #1908 omission — fixed at main), then rebuildCrystalPtrs() 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/moveCrystal keep the object self-consistent: pose updated via setPose, 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"), matching ProditionsHandle<Calorimeter>; DiskCalorimeter → Calorimeter entity resolution is fine; makeSet/makeIov/makeEntity follow the standard cache pattern with correct useDb guards.
  • 🟢 fcl contract: prolog alignedCalorimeter table ↔ AlignedCalConfig keys match; registered in both the Proditions prolog and the service config; disk_align.txt = 2 rows × 7 zeros = identity alignment for both disks.
  • 🟢 rowToCsv matches the TrkAlignElement sibling convention exactly (including the trailing comma — checked, it's the established pattern, not a defect).
  • 🟢 addRow enforces strict sequential indices — good defensive parsing.
  • 🟢 Link transitivity for the maker library: GeomHandle use in CaloConditions resolves via Offline::ProditionsService's PUBLIC link of Offline::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 local cmake configure (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.tables registration needed before useDb (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

  1. Delete the two configure_file lines (finding 1).
  2. Pass the validated locals in fromDb (finding 2).
  3. Register CalAlignCrystal in the factory; note the val.tables prerequisite (finding 3).
  4. Mirror the SConscript deps into CaloCluster/CMakeLists.txt (finding 4).
  5. Fix the three include guards (finding 5).
  6. Harden readFile (finding 6) and document the rotation convention (finding 8).
  7. State the consumer-migration plan for the aligned entity (finding 7).

@rlcee

rlcee commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

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?

@FNALbuild

Copy link
Copy Markdown
Collaborator

📝 The HEAD of main has changed to 1ce31db. Tests are now out of date.

@bechenard

Copy link
Copy Markdown
Contributor Author

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 oksuzian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 2b0f3eda review (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 deleting CalAlignCrystal outright. What remains is two 🟡 S2s and a handful of ⚪ S3s, none of which gate. Flip to 🟢 approve once mu2e/buildtest is green at b7f10044 (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 the GeomHandle DiskCalorimeter, served as proditions entity Calorimeter (config key alignedCalorimeter).
  • Crystal-level alignment removed entirely in this revision (CalAlignCrystal, the crystal loop in alignCalorimeter, the crystal DbHandle, the commented-out filenameCrystal reads). Repo-wide code search for CalAlignCrystal at this head returns 0 hits — no dangling references.
  • Only CaloClusterMaker consumes the aligned geometry so far; everything downstream still uses GeomHandle.

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.txt at head has no configure_file lines 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.txt at this same head has install(DIRECTORY data ...) and zero configure_file. (Mechanically confirmed why the original lines were even parseable: CURRENT_BINARY_DIR is undefined anywhere in the tree — only CMAKE_CURRENT_BINARY_DIR exists — 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.cc at head registers CalAlignDisk only (line 133), and CalAlignCrystal no longer exists (DbTables/inc/CalAlignElement.hh now defines CalAlignElement + CalAlignDisk). Nothing references the removed class. This also answers @rlcee's question directly.
  • 4 (S1)CaloCluster/CMakeLists.txt CaloClusterMaker plugin now lists Offline::CaloConditions and Offline::ProditionsService. Matches the StrawHitReco precedent (TrkHitReco/CMakeLists.txt: Offline::ProditionsService + Offline::TrackerConditions in the plugin LIBRARIES 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 in Offline).
  • 6 (S2)readFile is now while (std::getline(...)) + per-line std::istringstream, throwing cet::exception on 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-152
    auto disk_align_params = readDb(cad_p, CaloConst::_nDisk);
    alignCalorimeter(ptr, cad_p->rows());
    disk_align_params is never read. The crystal half of the original finding is resolved by deletion, so this is no longer a behavior contradiction: readDb is still called, so its range and count checks still fire, and given CalAlignElement::addRow's strict index == _rows.size() invariant the vector it builds is element-wise identical to cad_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 makes fromDb structurally identical to fromFcl (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-87 takes ProditionsHandle<Calorimeter>; every other calorimeter consumer (LineFinder calo matching via diskToMu2e/mu2eToTracker, CalPatRec, TrackCaloMatching, the truth-match cog checks) still uses GeomHandle<DiskCalorimeter>.
  • Impact: zero today — disk_align.txt is 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') because AlignedCalorimeterMaker.cc now uses CLHEP::Hep3Vector and CLHEP::HepRotation (lines 28-32). CaloConditions/CMakeLists.txt LIBRARIES PUBLIC at head lists no CLHEP.
  • Impact: it links todayOffline::CalorimeterGeom carries CLHEP::CLHEP as a PUBLIC dep 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 every throw in this library) to CaloConditions/CMakeLists.txt LIBRARIES PUBLIC.

D. ⚪ [S3] Residual items from prior finding 8, and small nits in the new readFile.

  • 🟢 Fixed: duplicate #include ".../EventTimingCache.hh" in ProditionsService.cc removed; the commented-out filenameCrystal reads removed from both fromFcl and fromDb; PR title typo corrected.
  • 🟡 Partial: CalAlignParams.hh now 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/Z pre-multiply, so the applied rotation is Rz·Ry·Rx, and Disk::moveDisk pivots about the disk origin. One more comment line closes it.
  • Unaddressed: ProditionsHandle<Calorimeter> is still constructed inside produce() (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 — ProditionsHandle is explicitly designed to be held as a member and queried with get(event.id()).)
  • New: AlignedCalorimeterMaker.cc uses std::istringstream (line 63) and std::abs on floats (lines 30-32) but includes only <iostream> and <fstream>. It compiles — <sstream> arrives via DbTables/inc/CalAlignElement.hh:10 (and via cetlib_except/exception.h), <cmath> via CLHEP/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: readFile now throws on any line that does not parse, including a blank trailing line or a # comment. disk_align.txt at 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 is float 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 category ALIGNEDCAL_RANGE, which is the range-error category — ALIGNEDCAL_FORMAT would 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.cc includes no CaloConditions or DbTables header — its only new first-order dep is ProditionsService. So Offline::CaloConditions (CMake), mu2e_CaloConditions (scons mainlib and plugins) and mu2e_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_cast mutation cannot corrupt the GeomHandle original (re-verified: alignCalorimeter is unchanged apart from the crystal-loop deletion). DiskCalorimeter's copy ctor copies disks_ by value, plus G4Info_ and trackerCenter_, then rebuildCrystalPtrs() repoints the crystal cache into the copy. The maker mutates only its private deep copy.
  • 🟢 moveDisk keeps the object self-consistent — pose via setPose, 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_p handle, its makeSet/makeIov contributions, the fromDb/alignCalorimeter crystal 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.hh still includes CaloConst.hh, correctly — CalAlignDisk uses CaloConst::_nDisk.
  • 🟢 CalAlignElement DB contract. orderBy() is declared const std::string orderBy() const and the base is virtual const std::string orderBy() const — signatures match, so it genuinely overrides (identical to the TrkAlignElement sibling, which likewise omits override). addRow enforces strict sequential indices; rowToCsv matches the TrkAlignElement convention including the trailing comma.
  • 🟢 Proditions wiring. Cache registered under Calorimeter::cxname, matching ProditionsHandle<Calorimeter>; makeSet/makeIov/makeEntity follow the standard pattern with correct useDb guards after the crystal removal; _caches[acc->name()] = acc; added alongside the other calo caches in ProditionsService.cc.
  • 🟢 fcl contract. CaloConditions/fcl/prolog.fcl alignedCalorimeter { filenameDisk, verbose, useDb } exactly matches the three atoms of AlignedCalConfig at head (filenameCrystal correctly absent); registered in ProditionsService/fcl/prolog.fcl under Proditions and as fhicl::Table<AlignedCalConfig> in ProditionsService.hh. disk_align.txt is 2 rows × 7 zeros = identity for both disks, and 2 == CaloConst::_nDisk so the count check passes.
  • 🟢 Cross-repo config compatibility of the new non-defaulted fhicl table. Adding a fhicl::Table with no defaults to the service Config would break any job that builds services.ProditionsService from scratch. Checked every consumer: Offline/fcl/standardServices.fcl and mu2e-trig-config/core/trigServices.fcl both use ProditionsService: @local::Proditions; all 8 Production hits (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::ProditionsService is an INTERFACE library; the service itself is a separate art::service plugin that links Offline::CaloConditions. CaloConditions → Offline::ProditionsService (INTERFACE) therefore does not close a cycle.
  • 🟢 Merge state. mergeable: true; main moved f4084747 → 1ce31dbc but the delta is Mu2eG4 proton-absorber geometry/materials only — no overlap with this PR.

Validation check

  • Build/tests run: 🟡 mu2e/buildtest is PENDING at b7f10044 — the previous green run was at 2b0f3eda and FNALbuild has already marked it out of date (main moved). The 2b0f3eda..b7f10044 delta 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 cmake configure would close this out in a minute.
  • Config contract: 🟢 pass (fcl ↔ Config keys, prolog registration, data-file row count).
  • Cross-repo consistency: 🟢 pass — no Production / mu2e-trig-config change required. Still outstanding on the DB side: cal.aligndisk needs DBA val.tables registration before useDb: true can 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: false by default, the runtime behavior change is a per-IOV deep copy of DiskCalorimeter plus one extra proditions cache in every job that runs CaloClusterMaker.
  • The real risk is deferred, not removed: finding B. The first non-zero cal.aligndisk commit 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

  1. Pass disk_align_params (not cad_p->rows()) in fromDb — finding A, one line.
  2. Add CLHEP::CLHEP (and cetlib_except::cetlib_except) to CaloConditions/CMakeLists.txt to mirror the SConscript — finding C.
  3. Add a paragraph on which consumers migrate to the aligned entity next, and that sim/digi must not — finding B.
  4. Optional S3 polish: direct <sstream> / <cmath> includes; tolerate blank/comment lines in readFile; drop the 1e-6 rotation guards; document the Rz·Ry·Rx order and disk-origin pivot; note that Disk::moveCrystal is retained for a possible future pass.
  5. Re-run the build test at b7f10044 and confirm green.
  6. Confirm with @rlcee that cal.aligndisk will be registered in val.tables before anyone sets useDb: true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants