Aug 26 geometry updates - #1918
Conversation
|
☀️ The build tests passed at e77630c.
N.B. These results were obtained from a build of this Pull Request at e77630c after being merged into the base branch at 1ce31db. For more information, please check the job page here. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary
Mu2e/Offline PR #1918 — "Aug 26 geometry updates" (sdifalco, branch AugGeomFixes)
Reviewed at head e77630c6
Base 1ce31dbc · 27 files · +458 / −103 · no prior reviews or review comments on the PR (only the FNALbuild CI posts), no locally staged draft — this is a first review, nothing to carry forward.
Decision
- 🟡 comment only — the geometry work checks out against every structural and overlap test I could run, and the stated intent is measurably achieved. Two items should be answered before merge: the OPA support material change (#1) and the 0.1 mm slit through the DS-area first floor (#2).
Scope understood
- 24 building/dirt extruded-solid definitions under
Mu2eG4/geom/bldg/are edited or added (8 new files), plus the include/prefix wiring inMu2eG4/geom/mu2eHall_v03.txt. Net effect: close air gaps around the TS-area W1 notch, the central stairwell and the DS-area elevator shield; splitdsAreaFirstFloorSinto a thinner concrete slab (S2) with dirt underneath; fix two wrong materials. - One unrelated file:
Mu2eG4/geom/protonAbsorber_cylindrical_v04.txtchanges the OPA support structure material fromStainlessSteel316toAl7075. - Everything reaches production:
mu2eHall_v03.txt←mu2eHall_v04.txt←geom_run1_a.txt/geom_run1.txt/geom_2021_PhaseI*/geom_common.txt, andprotonAbsorber_cylindrical_v04.txtis included by the samegeom_run1*.txt/geom_2021_PhaseI*set.Mu2e/ProductionJobConfig/cosmic/geom_cosmic_run1_a.txtis a one-line#include "Offline/Mu2eG4/geom/geom_run1_a.txt", so the cosmic campaigns inherit all of it. - No C++, no FHiCL, no build files. No data product, module, or interface change.
Findings
-
🟠 [S1] OPA support material change is a physics change to a versioned file used by all production geometry, made in place, with no stated justification or validation
- Evidence:
Mu2eG4/geom/protonAbsorber_cylindrical_v04.txt—protonabsorber.oPASupportMaterialName, the six-entryoPASupportMaterials,oPASupportSlatMaterials, andcrossSupportMaterialall goStainlessSteel316→Al7075. FromMu2eG4/src/ConstructMaterials.ccat head:StainlessSteel316is 8.00 g/cm³ (line 403),Al7075is 2.81 g/cm³ (line 635) — a ~65 % mass reduction on six support rings, three slats and three cross supports sitting inside the DS.Al7075is a real, defined material (also used byMu2eG4/geom/tracker_v7.txt), so nothing will throw. - Consumers verified at head:
geom_run1.txt:66,geom_run1_a.txt:66,geom_2021_PhaseI{,_v02,_v03}.txt,geom_reduced_DSTS_shielding.txt— i.e. every production and CI geometry.geom_common.txt→geom_2021_PhaseI_v03.txt, so the green CI ran against the new material. - Impact: (a) this silently re-defines what
protonAbsorber_cylindrical_v04means, so MDC/Run-1 samples produced before this merge are no longer reproducible from the same file name; (b) it is a material-budget change in the DS with no accompanying background/acceptance check; (c) it is a different topic from the hall/building work that makes up the other 26 files. - Suggested fix: state in the PR body which item of doc-db 57505 this implements (drawing/serial number for the OPA support alloy), and say explicitly whether an in-place edit is intended rather than a
protonAbsorber_cylindrical_v05.txt. If the simulation/production conveners are content with in-place, a one-line note in the PR body recording that decision is enough. Splitting it into its own PR would make it far easier to bisect later.
- Evidence:
-
🟡 [S2] The
dsAreaFirstFloorS/dsAreaFirstFloorS2split leaves a 0.1 mm air slit running the full length of the first floor- Evidence:
dsAreaFirstFloorS.txtnow ends atyPositions = -12014.2(indices 22, 23), whiledsAreaFirstFloorS2.txtstarts at-12014.3(indices 0, 1);dirtDsAreaFirstFloorS2.txtalso uses-12014.3. Both slabs share the vertical band 7543.8–7772.4 mm above the floor surface. Sampling the head include chain at (x = 20000, y = −12014.25) returns no solid at all between 7366 and 7800, where base returnsbuilding.dsArea.firstFloor.Sover 7366–7772.4. At y = −12013 and y = −12016 the head stack is fully covered. The slit is 0.1 mm wide, ~406 mm tall, and runs from x = −2921 to x = 38163.5 (~41 m). - Impact: physically negligible for shielding, but it is exactly the sliver class this PR is meant to remove, and coincident-ish thin gaps are the usual source of G4 navigation noise.
g4surfaceCheckis green, so nothing is failing today. - Suggested fix: one character — make
dsAreaFirstFloorS2.yPositions[0,1]anddirtDsAreaFirstFloorS2.yPositions[0,1]-12014.2, matchingdsAreaFirstFloorSanddsAreaUpper(which this PR moves to-12014.2).
- Evidence:
-
🟡 [S2] Sub-millimetre standoffs and non-shared coordinates appear throughout the new volumes — confirm they are deliberate
- Evidence, all at head:
dsAreaElevatorShield.txt:offsetFromFloorSurface.y = 1143.10withyHalfThickness = 1143.00→ bottom at +0.10 mm above the floor surface. (This is the gap fix: base left 152.4 mm of air under the block; probing (31000, −9000) gives base gaps(0, 152.4)and head(0, 0.1).)dirtCentralStairwellLower3.txt:1260.5 ± 1260.4→ bottom at +0.10 mm.dirtCentralStairwellMiddle2top = 2825.65 vsMiddle3bottom = 2825.75 → 0.10 mm apart (Middle3/Middle4abut exactly at 5120.64, so the convention is not applied uniformly).dirtCentralStairwellMiddle5.txtyPositionsuse-23376.25, whileMiddle2/3/4andcentralStairwellUpperWall{E,W}.txtuse-23376.1— 0.15 mm mismatch on what should be one shared plane.centralStairwellLowerWallN.txtandtsAreaStairwell.txtmove11366.5→11617.2, butcentralStairwellUpperWallW2.txtstill carries11617.325— 0.125 mm off the value this PR is standardising on.
- Impact: none measurable; it is a maintainability/consistency issue that makes future "does this abut?" reviews harder, and it defeats the intent of the gap-closing exercise at the 0.1 mm level.
- Suggested fix: if the 0.1 mm standoff is a deliberate anti-coincident-surface convention, add a one-line comment saying so in one of the new files and apply it uniformly; otherwise round all of the above onto the shared coordinate.
- Evidence, all at head:
-
🟡 [S2]
psAreaUpperN_v02moves off a coordinate shared with five other files- Evidence:
psAreaUpperN_v02.txtyPositions[2,3]go6502.40→6857.90.6502.4is still used bypsAreaUpper2N.txt,dirtPsAreaUpper2N.txt,psAreaHatchLower.txt,dirtBeamlineBerm_Layer748a.txtanddirtBeamlineBerm_Layer750a.txt;6857.9now appears in no other file inMu2eG4/geom/bldg/. - Impact: no overlap is produced (checked, see Validation), but if
6502.40was a shared building line, the neighbours are now out of step with it. - Suggested fix: confirm the neighbours are meant to stay at
6502.40; if not, move them in the same PR.
- Evidence:
-
🟡 [S2] Central-stairwell backfill:
dirtCentralStairwellMiddle2..5place MBOverburden inside the stairwell footprint — worth one sentence of justification- Evidence: the four new volumes occupy x ∈ [12014.3, 13233.3], y ∈ [−14892.65, −25908], i.e. 0.1 mm inside the walls
centralStairwellUpperWallN(x from 12014.2) andcentralStairwellUpperWallE2(x from 13233.4), which this PR simultaneously extends south to −23376.1 and up to 7543.8. Probing (12600, −20000) shows the head stack gainingmiddle2(2520.95–2825.65),middle3(2825.75–5120.64) andmiddle4(5120.64–5297.16), where base had onlydirt.central.stairwell.lowerup to 2520.95. - Impact: adds several tens of m³ of MBOverburden inside what reads, from the volume names, like a stairwell shaft. If correct it is a shielding improvement; if the shaft is meant to stay open below the first floor it over-shields that column. I could not settle this from the geometry alone.
- Suggested fix: one line in the PR body (or a comment in
mu2eHall_v03.txt) saying which part of the stairwell is backfilled and which stays open, referencing the doc-db 57505 figure.
- Evidence: the four new volumes occupy x ∈ [12014.3, 13233.3], y ∈ [−14892.65, −25908], i.e. 0.1 mm inside the walls
-
⚪ [S3]
dirtExtMonStairsGappolygon is asymmetric — confirm150.1is not a typo- Evidence:
dirtExtMonStairsGap.txtxPositions = {75.9, −75.9, −75.9, 75.9}butyPositions = {306.05, 150.1, −306.05, −306.05}, giving a wedge rather than the 151.8 × 612.1 mm rectangle the other three vertices imply. - Impact: cosmetic if intended (the sibling
extMonStairsWallN_v02.txtis a genuine staircase profile, so an irregular quad is plausible here). - Suggested fix: confirm; if the intent was a rectangle,
150.1should be306.05.
- Evidence:
-
⚪ [S3] Vertex-label comments degraded in three of the touched files
dirtCentralStairwellMiddle5.txt:xPositionslabelledg1, g2, g4, g4(nog3,g4twice) whileyPositionsare labelledh, h0, h1, h2— inconsistent with each other and withMiddle2/3/4.dsAreaUpper.txt: indices 31/32 relabelledi8/i9→h1/h2, which now collide with theh1/h2already used at indices 15/16.backfillTSarea-W1UpperNotchUpper3.txt: the auto-generated provenance header is rewritten fromgeom/geom_FillW-TSUpper1notch4.ccltogeom/geom_FillW-TSUpper2notch2.ccl, which is the same source the newUpper2.txtclaims. If the.cclfiles are the upstream source of truth, one of the two headers is now wrong.- Suggested fix: relabel consistently; point
Upper3at whichever.cclactually produced it.
-
⚪ [S3] Pre-existing, out of scope, noted only because you are in these files:
dirt.extMon.upper(dirtExtMonUpper_v02.txt) is fully defined but appears in no prefix list, so it is never built; andbuilding.N.retaining.Wall.W.extension3,building.foundation.ExtMon.PSarea.R,dirt.foundation.ExtMon.PSarea.Rhave self-intersecting polygons. None are touched by this PR — no action required here.
Verified correct — no action needed
- 🟢 The headline material fix is real and large.
dirtNRetainingWallExtension_v02.txtwas declaring a 13.8 m³ dirt volume in the berm asStainlessSteel(8.02 g/cm³) — ≈111 t of steel where there should be ≈30 t ofMBOverburden(2.15 g/cm³). Straightforward bug, correctly fixed. - 🟢 The stated intent is measurably achieved. Sampling vertical material coverage over the touched regions, base vs head: TS-area W1 notch column mean uncovered thickness 514 mm → 32 mm; central-stairwell column 3304 mm → 2060 mm; elevator shield floor gap 152.4 mm → 0.1 mm.
- 🟢 Include / prefix wiring is complete. Parsing the whole
mu2eHall_v04 → v03chain at head (383 files) and resolving all four prefix lists: 255 bldg + 107 dirt + 14 rotated + 3 dirt-trap prefixes, and every one resolves to a complete block (.name,.material,.xPositions,.yPositions, all three offsets,.yHalfThickness);len(x) == len(y)everywhere; no listed-but-undefined and no newly-defined-but-unlisted volume. All eight new files are both#included and listed. - 🟢 The new rotated solid is wired correctly.
dirt.extMon.stairs.gapis added torotated.prefix.listonly, which matches howMu2eHallMaker::makeRotatedconsumes that list into the separaterotatedSolids_map, and matches the existing rotated entries (none of which appear inbldg.prefix.list). It supplies the required.anglesvector. - 🟢 No macroscopic solid–solid overlap introduced: all 24 changed/new volumes tested pairwise against all 362 listed hall solids for simultaneous vertical-interval and footprint intersection — zero hits. Consistent with green
rootOverlapsandg4surfaceCheck. - 🟢 No new self-intersecting or zero-area polygon among the changed volumes.
- 🟢 The DS-area first-floor split preserves coverage at the stairwell walls: raising
centralStairwellUpperWall{E2,W2,N}from 6578.6 ± 787.4 to 6667.5 ± 876.3 keeps the bottom at 5791.2 and lifts the top to exactly 7543.8, which is exactly where the thinneddsAreaFirstFloorS2concrete now begins. Probes at (11800, −17000) and (13400, −17000) show zero gap in head. - 🟢 The TS-area notch stack is contiguous:
Upper3[5181.6, 6096] →Upper2[6096, 6553.2] →Upper1[6553.2, 7366], with no overlap.Upper3was previously present in the tree but never#included or listed, so re-purposing its values regresses nothing. Deleting the commented-out//#include ".../backfillTSarea-W1UpperNotch2.txt"is pure cleanup — that file does not exist at head. - 🟢 Both build systems are satisfied with no build-file change. The eight new files land in the existing
Mu2eG4/geom/bldg/, which CMake installs wholesale viainstall(DIRECTORY geom DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/Offline/Mu2eG4)(Mu2eG4/CMakeLists.txt:307at head), and which scons/muse resolves offMU2E_SEARCH_PATHfrom the source tree (Mu2eG4/src/SConscriptpresent at head, data files not enumerated there). No plugin, no source file, noconfigure_fileinput added or removed, so noLIBRARIESmirroring is at stake. CIcheck_cmakeis green. - 🟢 No cross-repo change required.
Mu2e/ProductionandMu2e/mu2e-trig-configcontain zero references toprotonAbsorber_cylindrical*ormu2eHall_v0*; Production reaches this geometry only throughJobConfig/cosmic/geom_cosmic_run1_a.txt→Offline/Mu2eG4/geom/geom_run1_a.txt, which needs no edit. - 🟢 Units and conventions are consistent throughout: all lengths mm, angles in radians in
.angles(-1.5708), matching the sibling rotated files.
Validation check
- Build/tests run: yes, by CI at this exact head. FNALbuild reports all green at
e77630c6merged into1ce31dbc(job 3267): build (prof) 4 min 24 s,ceSimReco,g4test_03MT,transportOnly,POT,g4study,cosmicSimReco,cosmicOffSpill,ceSteps,ceDigi,muDauSteps,ceMix,rootOverlaps,g4surfaceCheck,trigger,check_cmake, clang-tidy 0/0, whitespace clean. For a geometry PR the two overlap/surface tests are the load-bearing ones, and they exercise the changed files —surfaceCheck.fcl→geom_SurfaceCheck.txt→geom_common_current.txt→geom_2021_PhaseI_v03.txt→ bothmu2eHall_v04.txtandprotonAbsorber_cylindrical_v04.txt. - Independent checks I ran (read-only, against the PR head and base tarballs):
gh api repos/Mu2e/Offline/tarball/e77630c6and.../tarball/1ce31dbc, extractingMu2eG4/geom/- a SimpleConfig include-chain parser over
mu2eHall_v04.txt(383 files) → prefix-list completeness, x/y length parity,.anglespresence for rotated solids, self-intersection and zero-area scan - sampled footprint-intersection × vertical-interval-overlap test, 24 changed volumes × 362 listed solids
- vertical material-coverage profiles, base vs head, over the DS-area slab band, the central-stairwell column, and the TS-area W1 notch column
- Limitation to state plainly: the overlap and coverage tests are grid-sampled (90×90 per pair; 300–400 random points per region), so sub-millimetre slivers below the sampling pitch would not be caught by them. Finding #2 was found by targeted probing at the seam coordinate, not by the scan. The green
g4surfaceCheckis the better evidence at that scale.
- Config contract check: pass — no FHiCL touched; all SimpleConfig keys referenced by
Mu2eHallMaker::loadSolids/loadRotSolidsare present for every listed prefix. - Cross-repo consistency: pass — nothing required in
Productionormu2e-trig-config. - PR hygiene: the description cites doc-db 57505 with three category bullets, which is a reasonable anchor, but there is no per-file mapping and no validation section, and the proton-absorber material change is a separate topic from the 26 hall/building files. Best-practice reminder, not a gate.
Residual risk
- All of this lands on the production geometry chain used by the Run-1/MDC campaigns and by Production's cosmic configs (
geom_run1_a.txt). The berm steel→dirt fix alone removes ~81 t of high-Z material from the overburden, which will move cosmic-induced rates; campaign owners should expect a step change and re-baseline rather than treat it as a regression. - The OPA support stainless→aluminium change alters the material budget inside the DS; if any tuned background estimate depends on
protonAbsorber_cylindrical_v04, it silently changes meaning at merge. - Concrete→dirt substitution in the DS-area first floor: over the
S2footprint, 177.8 mm ofCONCRETE_MARS(2.35 g/cm³) becomesMBOverburden(2.15 g/cm³). Small (~8 % density over 178 mm), but it is a real shielding change, not just a gap fix. - I could not independently verify the numbers against doc-db 57505; consistency with the drawings is taken on the author's word.
Author follow-ups
- Confirm the OPA support material change (
StainlessSteel316→Al7075) is intended as an in-place edit ofprotonAbsorber_cylindrical_v04.txtrather than a new_v05, and note in the PR body which doc-db 57505 item backs it. Consider moving it to its own PR. - Align the
dsAreaFirstFloorS/dsAreaFirstFloorS2/dirtDsAreaFirstFloorS2seam onto a single value (-12014.2) to close the 0.1 mm slit. - Say whether the ~0.1 mm standoffs (elevator shield,
dirtCentralStairwellLower3,Middle2↔Middle3) are a deliberate anti-coincident-surface convention; if so apply it uniformly and comment it, otherwise roundMiddle5's-23376.25→-23376.1andcentralStairwellUpperWallW2's11617.325→11617.2. - Confirm
psAreaUpper2N/dirtPsAreaUpper2N/psAreaHatchLower/dirtBeamlineBerm_Layer748a/_750aare meant to stay at6502.40now thatpsAreaUpperN_v02moved to6857.90. - Add one sentence on which part of the central stairwell is backfilled with
MBOverburden(dirtCentralStairwellMiddle2..5) and which stays open. - Confirm
dirtExtMonStairsGap.yPositions[1] = 150.1is intended and not a typo for306.05. - Tidy the vertex-label comments in
dirtCentralStairwellMiddle5.txtanddsAreaUpper.txt, and fix the.cclprovenance header onbackfillTSarea-W1UpperNotchUpper3.txt. - Best-practice reminder: please add a short validation section to the PR body (what you checked beyond CI — e.g. a GDML/event-display sweep of the touched regions) and, where practical, keep a PR to a single topic.
|
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — second pass on "Aug 26 geometry updates" (#1918)
Reviewed at head 84674ab2 (2026-08-07). Prior review: 🟡 at e77630c6 (review 4879365261, 8 findings). The delta is four commits, e77630c6..84674ab2, touching 15 files: the OPA change reverted, three files deleted, and coordinate fixes. Every prior finding is accounted for below and verified in the geometry at this head rather than taken from the commit messages or the reply.
Decision
- 🟡 Comment. Every substantive item is resolved — the OPA material change is fully backed out, the 0.1 mm floor slit is closed exactly, and the two standoffs I flagged now sit at zero. Your answers on the deliberate items are good enough for me; I am not carrying them further. Two things keep this from being an approve, and neither is about the geometry: CI has not run at this head (
mu2e/buildtestis still pending androotOverlaps/g4surfaceCheckare the load-bearing tests for this PR), and the three file deletions left two#includelines pointing at files that no longer exist. Both are cheap. Ping me when CI is green and I will approve.
Scope understood (delta only)
5038da5brevertsprotonAbsorber_cylindrical_v04.txt;cfdd3fc2/f4a756a8deletedirtExtMonUpper.txt,dirtExtMonUpper_v02.txt,psAreaUpper2N.txt;84674ab2applies the coordinate fixes across 12bldg/files.
Findings
-
🟡 [S2] Three files were deleted but two of them are still
#included.- Evidence, at this head:
Mu2eG4/geom/mu2eHall.txt:83—#include ".../bldg/psAreaUpper2N.txt", file deleted. The prefix is also still listed twice, at:195("building.psArea.upper2.N") and:346("dirt.psArea.upper2.N").Mu2eG4/geom/mu2eHall_v02.txt:473—#include ".../bldg/dirtExtMonUpper_v02.txt", file deleted.dirtExtMonUpper.txthad no references at all — that deletion is clean, and it is the dead volume I flagged as out-of-scope last time, so thank you for taking it.
- Impact: none today, and I want to be precise about why rather than overstate it. I parsed the include graph at base and at head:
mu2eHall.txtandmu2eHall_v02.txtwere already unloadable before this PR — between them they#include17 files that do not exist at base (calorimeter_CsI.txt,protonBeamDump_v02.txt,extMonBitN.txt,dirtSRetainingWallFoot.txt, thedirtTempDirtBackfillPsArea*_v02set, and others). So the eight legacy configs that reach them (geom_common_cd3_s3p2,geom_common_cd3_s4p2,geom_common_hayman_v2,geom_MARS_2019,geom_common_MARSrunMay17,geom_common_DOE_review_2017,geom_common_haymanLowerDensity,geom_common_TrackerShldStdyJun17) were already broken and this PR does not change that. Nothing in the live chain, in CI, or inProductionis affected. - Suggested fix: delete the two
#includelines and the twopsArea.upper2.Nprefix entries inmu2eHall.txt. It is four lines, it keeps the deletion self-consistent, and it means the next person auditing those legacy files has two fewer false leads.
- Evidence, at this head:
-
⚪ [S3] Vertex-label comments — carried over from finding 7, unaddressed (the files were not touched in this delta).
dirtCentralStairwellMiddle5.txtstill labelsxPositionsasg1, g2, g4, g4(nog3,g4twice) againstyPositionslabelledh, h0, h1, h2.dsAreaUpper.txtstill labels indices 31/32 ash1/h2, colliding with theh1/h2at indices 15/16.- Your answer that the
.cclfiles are no longer used settles the third part of that finding — the stale provenance header onbackfillTSarea-W1UpperNotchUpper3.txtis then documentation of a retired workflow, not a wrong pointer. Worth deleting those headers wholesale someday, but not here.
Carry-forward accounting (vs review 4879365261 at e77630c6)
- 🟢 [was S1] OPA support material — WITHDRAWN FROM THE PR, verified.
protonAbsorber_cylindrical_v04.txtat this head is byte-identical to base1ce31dbc(diff -qclean):oPASupportMaterialName, all sixoPASupportMaterials,oPASupportSlatMaterialsandcrossSupportMaterialare back toStainlessSteel316. This was the right call — it takes a material-budget change to every production geometry out of a hall-geometry PR, and it can now get the validation it deserves on its own. - 🟢 [was S2] The 0.1 mm slit through the DS-area first floor — FIXED, verified.
dsAreaFirstFloorS.yPositions[22,23]is-12014.2, and bothdsAreaFirstFloorS2anddirtDsAreaFirstFloorS2now start at-12014.2(was-12014.3). The seam is exact; the 41 m long, 0.1 mm wide air sliver is gone. - 🟡 [was S2] Sub-millimetre standoffs — MOSTLY FIXED, and the remainder is your call, accepted.
dsAreaElevatorShield:offsetFromFloorSurface.y = 1143.10,yHalfThickness = 1143.10→ bottom at exactly 0.00. Fixed.dirtCentralStairwellLower3:1260.45 ± 1260.45→ bottom at exactly 0.00. Fixed.centralStairwellUpperWallW2:11617.325→11617.2, now matchingcentralStairwellLowerWallNandtsAreaStairwell. Fixed.Middle2top (2673.3 + 152.35 = 2825.65) vsMiddle3bottom (3973.17 − 1147.465 = 2825.705): the gap is now 0.055 mm, down from 0.10 mm. That matches what you described — reduced below 0.1 mm where possible, kept non-zero to avoid coincident surfaces, and checked withsurfaceCheck. That is a legitimate G4 practice and I am not going to argue it. It would still be worth one comment line in one of these files recording the convention, so the next reviewer does not re-derive this exchange.Middle5at-23376.25— I withdraw this one. New evidence:-23376.25is not an outlier, it is one of two consistently-used planes.centralStairwellUpperLanding.txt,centralStairwellUpperStairs2.txtanddirtCentralStairwellMiddle.txtall use-23376.25, whilecentralStairwellUpperWall{E,W}andMiddle2/3/4use-23376.1.Middle5joining the landing/stairs family reads as deliberate, and none of those three files is touched by this PR. My original framing — "what should be one shared plane" — was wrong.
- 🟢 [was S2]
psAreaUpperN_v02moving off 6502.40 — ANSWERED, accepted. You describe it as filling a gap without creating a new volume, andpsAreaUpper2N.txt— the file I cited as still sitting at 6502.40 — is deleted in this PR. Confirmed it was reachable only frommu2eHall.txt, never from the livemu2eHall_v04 → v03chain, so deleting it changes no production geometry (the leftover#includeis finding 1). - 🟢 [was S2] Central-stairwell backfill — ANSWERED. Drawing SC-19; the volume under the steps was originally filled with concrete rather than MBOverburden to keep the volume count down, leaving air gaps, and
dirtCentralStairwellMiddle2..5now fill those. That is exactly the sentence I was asking for — please put it in the PR body, where it survives the comment thread. - 🟢 [was S3]
dirtExtMonStairsGapasymmetry — ANSWERED. Deliberate, to fit the Extinction Monitor stairs basement. Not a typo. - ⚪ [was S3] Vertex labels — PARTIAL, now finding 2; the
.cclsub-item is answered. - 🟢 [was S3] Pre-existing dead
dirt.extMon.upper— ACTED ON. BothdirtExtMonUpper.txtanddirtExtMonUpper_v02.txtare deleted, which is more than I asked for on an out-of-scope note. The leftover#includeinmu2eHall_v02.txtis the only loose end (finding 1). The three self-intersecting polygons I mentioned in the same finding are untouched, as expected.
Verified 🟢 — no action needed
- 🟢 The live production chain is complete and self-consistent at this head. Re-parsed
mu2eHall_v04 → v03from the head tarball: 382 files, zero missing includes; the four prefix lists resolve to 255bldg+ 107dirt+ 14rotated+ 3dirt.trap, and every prefix has a complete block (.name,.material,.xPositions,.yPositions, all three offsets,.yHalfThickness, plus.anglesfor rotated); no x/y length mismatch; no listed-but-undefined and no newly-defined-but-unlisted volume. The three deletions do not touch this chain. - 🟢 I chased three apparent overlaps to ground and they are test artifacts, not findings. My grid overlap scan flagged
dirt.central.stairwell.lower3×building.tsArea.stairwellanddirt.central.stairwell.middle3×building.central.stairwell.upper.wall.{e,w}. Running the identical test ate77630c6reproduces all three with the same point counts — androotOverlapsandg4surfaceCheckwere green at that head. The delta moves those boundaries by ≤0.055 mm. So the flags come from my projected point-in-polygon test, not from a real solid intersection. Recording it so you know the check was run and resolved rather than skipped. - 🟢 No cross-repo impact, re-confirmed. With the proton-absorber file reverted, this PR touches only
Mu2eG4/geom/bldg/andmu2eHall_v03.txt.Productionreaches this geometry solely viaJobConfig/cosmic/geom_cosmic_run1_a.txt→Offline/Mu2eG4/geom/geom_run1_a.txt→mu2eHall_v04.txt, which is intact;mu2e-trig-confighas no geometry reference. - 🟢 Both build systems still need no edit. No file is added in this delta and the three deletions are data files inside
Mu2eG4/geom/bldg/, which CMake installs wholesale viainstall(DIRECTORY geom ...)and scons resolves offMU2E_SEARCH_PATH. No source, plugin orconfigure_fileinput changes. - 🟢 Everything cleared in the first review that this delta does not touch still holds — the
dirtNRetainingWallExtension_v02steel→dirt fix (≈111 t → ≈30 t), the gap closures (TS-area W1 notch 514 → 32 mm, elevator shield 152.4 → now 0 mm), the rotated-solid wiring fordirt.extMon.stairs.gap, and the TS-area notch stack contiguity.
Validation check
- Build/tests run: not yet at this head.
mu2e/buildtestat84674ab2ispending("This test has not been triggered yet"); onlyjenkins/ghprbhas reported. The combined status ispending, which is why the PR readsmergeable_state: unstable. The previous heade77630c6was fully green includingrootOverlapsandg4surfaceCheck— for a geometry PR those two are the evidence that matters, and they have not seen this head. - Independent checks I ran (read-only, on the base/prev/head tarballs): full SimpleConfig include-graph parse of the whole
Mu2eG4/geomtree at base and head to separate new from pre-existing broken includes; prefix-list completeness and x/y parity over the livemu2eHall_v04chain; the seam and standoff arithmetic quoted above; grid footprint × vertical-interval overlap for the changed volumes against all 379 listed solids, cross-checked ate77630c6. - Limitation, same as last time and worth repeating: my overlap scan is grid-sampled, so sub-millimetre slivers below the sampling pitch will not show up in it.
g4surfaceCheckis the better instrument at that scale, and it has not run on this head. - Config contract check: pass — no FHiCL touched; all SimpleConfig keys required by
Mu2eHallMaker::loadSolids/loadRotSolidsare present for every listed prefix in the live chain. - Cross-repo consistency: pass.
Residual risk
- The berm
StainlessSteel→MBOverburdenfix still removes ~81 t of high-Z material from the overburden and the DS-area first-floor concrete → dirt substitution is still a real shielding change. Campaign owners should expect a step change in cosmic-induced rates and re-baseline rather than treat it as a regression. That was true last pass and reverting the OPA change does not affect it. - Two legacy hall configs gain dangling includes on top of the ones they already had (finding 1). The risk is not that something breaks now, it is that
mu2eHall.txt/mu2eHall_v02.txtdrift further from repairable. - Still taking consistency with doc-db 57505 on your word — I have no way to check the drawings.
Author follow-ups
- Remove the two dead
#includelines (mu2eHall.txt:83,mu2eHall_v02.txt:473) and the twopsArea.upper2.Nprefix entries atmu2eHall.txt:195,346(finding 1). - Let CI finish —
rootOverlapsandg4surfaceCheckare the ones I want to see green at this head before approving. - Move your answers on the stairwell backfill (SC-19) and the ExtMon stairs gap into the PR body; the comment thread is not where the next person will look.
- Optional: one comment line recording the deliberate sub-0.1 mm standoff convention, and the vertex labels in finding 2.
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 84674ab: build (Build queue - API unavailable) |
|
I have to update dimension and position of Remote Handling Room south door, I am checking the numbers provided by George with what's in reduced_ExtShielsUpstream_v07.txt. |
|
☀️ The build tests passed at 84674ab.
N.B. These results were obtained from a build of this Pull Request at 84674ab after being merged into the base branch at 1ce31db. For more information, please check the job page here. |
|
Remote handling room south door dimension and position fixed following George's findings (see updated doc-db 57505). I have also updated ExtShieldUpstream_v07.txt since this fix is common to the two geometries. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — third pass on "Aug 26 geometry updates" (#1918)
Reviewed at head faffc80f. Prior reviews: 🟡 at e77630c6 (review 4879365261, 8 findings) and 🟡 at 84674ab2 (review 4887335976, 2 open findings). The delta since the last pass is one commit, 84674ab2..faffc80f, touching two files and four lines: the Remote Handling Room south door. Every prior finding is re-checked at this head below, and the two conclusions I reached by withdrawal/attribution last time were re-derived from scratch rather than repeated.
Decision
- 🟡 Comment. Last pass I said I would approve once
rootOverlapsandg4surfaceCheckwent green. They did, at84674ab2(build 3273, both ✅) — that condition was met. But the head then moved, and the new commit is not a no-op: the resized door introduces one 🟠 [S1] that I do not think either of those tests can see, because both look for overlaps and what the new numbers create is a gap. One question to answer and, if the answer is "yes, re-seat it", two numbers to change. Happy to approve straight after.
Scope understood (delta only)
faffc80fchangesExtShieldUpstream.dimsType20from{609.6,4418,6426.2}to{609.6,3810,6400.8}andcenterType20Box1from{9381,-103,-12077.7}to{9392.9,-109.6,-12077.7}, identically inMu2eG4/geom/ExtShieldUpstream_v07.txtandMu2eG4/geom/reduced_ExtShieldUpstream_v07.txt.- Reach: this is live production geometry, and more of it than the earlier
bldg/commits.reduced_…_v07.txtis pulled in bygeom_run1_a.txt,geom_run1_a_stickman.txt, and therefore bygeom_common.txt(→geom_run1_a_stickman.txt),geom_run1_b_v01.txt,geom_SurfaceCheck_run1a.txt, andProduction/JobConfig/cosmic/geom_cosmic_run1_a.txt.ExtShieldUpstream_v07.txtis pulled in bygeom_run1.txtandgeom_run2.txt. Verified by resolving the include graphs at this head.
Findings
-
🟠 [S1] The door was resized about its old centre, so all three faces that used to be flush now stand off — the "closed" door has ~0.3 m of air under it, ~0.3 m over it, and 12.7 mm at the wall it closes against.
- Evidence.
ExtShieldUpstreamMaker::makehalves the config dimensions (tempDoubleVec[itmp] *= (CLHEP::mm/2.0), with the comment "Divide dimensions by 2.0 because G4 boxes are created in terms of half-lengths"), andcenterType20Box1is "the center of the box in Mu2e coords". So:- was: half-lengths
{304.8, 2209, 3213.1}about{9381, -103, -12077.7}→ x ∈ [9076.2, 9685.8], y ∈ [-2312.0, 2106.0], z ∈ [-15290.8, -8864.6] - now: half-lengths
{304.8, 1905, 3200.4}about{9392.9, -109.6, -12077.7}→ x ∈ [9088.1, 9697.7], y ∈ [-2014.6, 1795.4], z ∈ [-15278.1, -8877.3]
- was: half-lengths
- The room around it, resolved from the live chain (
geom_run1_a→mu2eHall_v04→v03):floorRemote(floorRemote_v02.txt,offsetFromFloorSurface.y = -76.2,yHalfThickness = 76.2) has its top at y = -2312.0, and its footprint covers 100% of the door footprint (3321/3321 sample points).remoteHandlingCeiling(remoteHandlingCeiling_v03.txt,4800.6 ± 381) has its bottom at y = +2107.6, footprint coverage again 100%.remoteHandling(remoteHandling_v02.txt,2133.6 ± 2286) spans y ∈ [-2464.4, 2107.6]; its polygon's face at z = -15290.8 (vertices 4→5, x from 7805.4 to 16949.4) is the NW end wall the door closes against —OpenDistanceopens the door in +z, soOpenDistance = 0.0is meant to be against that wall.
- So the room's clear height is 2107.6 − (−2312.0) = 4419.6 mm = 14′6″ exactly. The old door was 4418 mm tall and sat with its bottom at exactly -2312.0 (on the floor) and its top 1.6 mm under the ceiling — a full-height plug, flush against the NW wall. The new door is 3810 mm = 12′6″, and at
y = -109.6it leaves 297.4 mm of HallAir below it and 312.2 mm above it; the 25.4 mm length reduction was split evenly about the unchangedz, leaving 12.7 mm between the door and the wall face at z = -15290.8. Nothing fills any of the three: I grid-tested the new door volume against all 379 hall solids and the other 28ExtShieldUpstreamboxes and there is no volume in the gaps (and no overlap either — see below). - Note the new dimensions are 2′ × 12′6″ × 21′0″, all exact — they read as correct drawing numbers. It is the seating that looks unintended: -109.6 is 7.4 mm off the room's vertical mid-plane (-102.2), i.e. the box was shrunk about roughly where it already was rather than re-placed.
- Impact: a 2 ft thick
CONCRETE_MARSshielding door with a continuous 0.61 m × 6.4 m slot under it and another over it is not a closed door. The physics consequence is modest — this is an internal partition in a service room well north of the detector, air on both sides — but it is ingeom_common.txtand in the cosmic production geometry, so it is not a niche config, and it silently weakens a shielding boundary that used to seal.g4surfaceCheckandrootOverlapswill not catch this: both hunt for intersecting solids, and this is the opposite failure. - Suggested fix: if the door rests on the floor and closes against the NW wall,
centerType20Box1 = {9392.9, -407.0, -12090.4}puts the bottom at -2312.0 and the closing face at -15290.8 with the new drawing dimensions unchanged (−2312 + 1905 = −407; −15290.8 + 3200.4 = −12090.4). Then the 609.6 mm now spare above the door is a header that wants concrete, so either extend the ceiling/wall down or say why air is right there. If instead the door really does hang clear of the floor, please put that in the PR body with the elevation it comes from — it is the kind of thing the next reviewer will re-flag.
- Evidence.
-
🟡 [S2] Three deleted files, two of them still
#included — carried over from finding 1 of review 4887335976, unaddressed (the delta did not touch these files).- Verified again at
faffc80f:Mu2eG4/geom/mu2eHall.txt:83still#includesbldg/psAreaUpper2N.txtand still lists the prefix twice, at:195("building.psArea.upper2.N") and:346("dirt.psArea.upper2.N");Mu2eG4/geom/mu2eHall_v02.txt:473still#includesbldg/dirtExtMonUpper_v02.txt; all three deleted files are absent from the tree. - Impact unchanged and still bounded — see the re-derivation in the verified section: those two legacy hall files were already unloadable before this PR, and the count of dangling includes in each is unchanged by this PR. Nothing live is affected.
- Suggested fix: four lines — drop the two
#includes and the twopsArea.upper2.Nprefix entries.
- Verified again at
-
🟡 [S2] Pre-existing, not introduced here, and explicitly not gating — the door open/close mechanism moves the wrong box, and this PR is the natural place to notice it.
- Evidence:
GeometryService/src/ExtShieldUpstreamMaker.ccbuilds every box, then doesCLHEP::Hep3Vector WorkingPosition = sites.back(); … sites.pop_back(); sites.push_back(ShieldDoorCenter);whereShieldDoorCenter = {x+FrameGap, y, z+OpenDistance}. WithnumberOfBoxTypes = 21,nBoxType20 = 1andnBoxType21 = 3,sites.back()is Type21Box3 at{2531.4,-252.35,2810.65}— the North-East external TS shielding — not the Type20 door this PR is editing. - Inert today:
OpenDistanceandFrameGapare0.0in every file that defines them (ExtShieldUpstream_v0{6,7}.txt,reduced_…_v0{6,7}.txt) and nothing else in the repo overrides them, so the pop/push is a no-op. It is wrong the moment anyone opens the door for a study — they would displace TS shielding and leave the door shut. Present since the mechanism landed (74290484, 2023) and in v06 as well as v07, so it is not yours. - While you are in the file:
nBoxType21 = 3; // Remote handling room dooris a copy-paste comment — Type21 is the NE external TS shielding.
- Evidence:
-
⚪ [S3] Vertex-label comments — carried over from finding 2 of review 4887335976, unaddressed.
bldg/dirtCentralStairwellMiddle5.txtstill labelsxPositionsg1, g2, g4, g4(nog3,g4twice).bldg/dsAreaUpper.txtstill labels the last threeyPositionsh1, h2, j0where the matchingxPositionsarei8, i9, j0, so entries 32 and 33 of 34 disagree between the two lists (and re-useh1/h2, already spelledH1/H2at entries 16/17).
Carry-forward accounting
vs review 4887335976 (84674ab2):
- 🟡 [was S2] Dangling
#includes after the deletions — UNADDRESSED, now finding 2. - ⚪ [was S3] Vertex labels — UNADDRESSED, now finding 4.
- The approval condition I stated ("ping me when CI is green") — MET at
84674ab2(build 3273:rootOverlaps✅,g4surfaceCheck✅, all 21 checks ✅) and then invalidated byfaffc80f. See Validation.
vs review 4879365261 (e77630c6) — all eight were closed out in review 4887335976; re-verified at this head that the closures still hold:
- 🟢 [was S1] OPA support material — still withdrawn from the PR:
protonAbsorber_cylindrical_v04.txtdoes not appear in the PR's 34-file list at this head. - 🟢 [was S2] DS-area first-floor 0.1 mm slit — still closed:
dsAreaFirstFloorS,dsAreaFirstFloorS2anddirtDsAreaFirstFloorS2all sit on-12014.2, no12014.3anywhere in the three. - 🟢/🟡 [was S2] Sub-millimetre standoffs — still as accepted last pass:
dsAreaElevatorShield1143.10 ± 1143.10anddirtCentralStairwellLower31260.45 ± 1260.45both bottom out at exactly 0.00;centralStairwellUpperWallW2is on11617.2, matchingcentralStairwellLowerWallNandtsAreaStairwell. The 0.055 mmMiddle2/3gap stands as your deliberate convention. - 🟢 [was S2]
psAreaUpperN_v02— answered;psAreaUpper2N.txtdeleted. - 🟢 [was S2] Central-stairwell backfill (drawing SC-19) — answered.
- 🟢 [was S3]
dirtExtMonStairsGapasymmetry — answered. - ⚪ [was S3] Vertex labels — see finding 4.
- 🟢 [was S3] Pre-existing dead
dirt.extMon.upper— acted on.
Re-derived, not inherited
- 🟢 The
Middle5-23376.25withdrawal stands. Re-derived from the tree at this head, not from the earlier note:-23376.25appears incentralStairwellUpperLanding.txt,centralStairwellUpperStairs2.txt,dirtCentralStairwellMiddle.txt,dirtCentralStairwellMiddle5.txt;-23376.1incentralStairwellUpperWall{E,W}.txtanddirtCentralStairwellMiddle{2,3,4}.txt. All nine are in the livemu2eHall_v03chain and all nine carryoffsetFromMu2eOrigin.x = 947.4, so the raw numbers are directly comparable — two coexisting planes, andMiddle5joining the landing/stairs family is consistent, not an outlier. - 🟢 The dangling-include attribution stands, with my own numbers. I re-parsed both include graphs at the merge base (
1ce31dbc) and atfaffc80f:mu2eHall.txthas 4 missing includes at base and 4 at head;mu2eHall_v02.txthas 14 at base and 14 at head. This PR is net-neutral on both counts — it repairs pre-existing dangles (e.g. it addsbackfillTSarea-W1UpperNotchUpper2.txt, missing at base) while its deletions create the two in finding 2. Both files were already unloadable before this PR; the live chain is not.
Verified 🟢 — no action needed
- 🟢 The two
v07files were kept in lockstep, exactly as you said. The84674ab2..faffc80fpatch is character-for-character the same two lines inExtShieldUpstream_v07.txtandreduced_ExtShieldUpstream_v07.txt;diffagainstmainfor each file shows only those two lines. - 🟢 No new overlap is introduced by the door change. Point-in-polygon grid test (41 × 81 over the footprint) of both the old and the new door volume against all 379 live hall solids, plus 3-D interval overlap against the other 28
ExtShieldUpstreamboxes: zero hits in both cases. The new box does reach 11.9 mm further north (x max 9697.7 vs 9685.8) but stays clear of theremoteHandlingconcrete (0/4141 sample points inside its polygon). Consistent with your local surface check passing — and, again, that is exactly why the check cannot be the evidence for finding 1. - 🟢 The live production chain is complete and self-consistent at this head. Re-parsed
geom_run1_a→mu2eHall_v04→v03: zero missing includes; the four prefix lists resolve to 255 bldg + 107 dirt + 14 rotated + 3 dirt.trap = 379; every prefix has a complete key block (.name,.material,.xPositions,.yPositions, all three offsets,.yHalfThickness); noxPositions/yPositionslength mismatch; no duplicate volume name. Same forgeom_run1.txt,geom_run2.txt,geom_run1_a_stickman.txt. - 🟢 Both build systems still need no edit — checked, not assumed. The delta modifies two existing data files under
Mu2eG4/geom/; nothing is added or removed. CMake installs the directory wholesale (Mu2eG4/CMakeLists.txt:307,install(DIRECTORY geom DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/Offline/Mu2eG4)), there is noSConscriptunderMu2eG4/at all so scons resolves these offMU2E_SEARCH_PATH, and there is no source, pluginLIBRARIES, orconfigure_fileinput in the delta.check_cmakewas ✅ at84674ab2. - 🟢 Cross-repo consistency: pass.
Productionreaches the changed file throughJobConfig/cosmic/geom_cosmic_run1_a.txt→Offline/Mu2eG4/geom/geom_run1_a.txt→reduced_ExtShieldUpstream_v07.txt; the chain is intact and needs noProductionedit.mu2e-trig-confighas no geometry reference.
Validation check
- Build/tests run: not at this head. At
faffc80f,mu2e/buildtestispending— "This test has not been triggered yet"; onlyjenkins/ghprbhas reported, so the combined status ispending. For the record,84674ab2was fully green (build 3273, all 21 checks includingrootOverlapsandg4surfaceCheck), so the condition from my last review was genuinely satisfied — it just no longer applies to the code in the branch. Your@FNALbuildcomment at 08:08 had an empty body; the trigger phrase is@FNALbuild run build test. - Independent checks I ran (read-only, on the merge-base and
faffc80ftarballs): SimpleConfig include-graph resolution formu2eHall{,_v02,_v03,_v04}.txtand all fourgeom_run*configs at both commits; prefix-list completeness and x/y parity over the live chain; the box-half-length arithmetic above againstExtShieldUpstreamMaker.cc; footprint coverage offloorRemoteandremoteHandlingCeilingover the door; grid overlap of old and new door against 379 solids + 28 boxes;OpenDistance/FrameGapoverride search across all.txt/.fcl. - Limitation: my overlap test is grid-sampled, so slivers below the sampling pitch would not show. That is
g4surfaceCheck's job, and it has not seen this head. - Config contract check: pass — no FHiCL touched; all keys
ExtShieldUpstreamMakerreads for types 1–21 are present in both files. - Cross-repo consistency: pass.
Residual risk
- Finding 1 is the only one that changes physics at this head, and it is a quiet change: no test in the suite fails on a missing shield, so if the seating is wrong it merges silently and stays wrong.
- Unchanged from the previous passes, and worth repeating for campaign owners rather than treating as a regression: the berm
StainlessSteel→MBOverburdenfix removes ~81 t of high-Z material from the overburden, and the DS-area first-floor concrete → dirt substitution is a real shielding change. Expect a step change in cosmic-induced rates and re-baseline. - Two legacy hall configs gain dangling includes on top of the ones they already had (finding 2). Nothing breaks now; the cost is that
mu2eHall.txt/mu2eHall_v02.txtdrift further from repairable. - Consistency with doc-db 57505 and George's numbers is still taken on your word — I have no way to read the drawings, which is exactly why finding 1 is a question about seating and not about the dimensions.
Author follow-ups
- Confirm or correct the door seating (finding 1). If it rests on the floor and closes against the NW wall:
centerType20Box1 = {9392.9, -407.0, -12090.4}in bothv07files, and say what fills the 609.6 mm above it. If the standoffs are real, one line in the PR body with the source elevation. - Trigger CI at this head with
@FNALbuild run build test— and please note for the record thatrootOverlaps/g4surfaceCheckgreen is necessary but not sufficient for finding 1. - Remove the two dead
#includelines (mu2eHall.txt:83,mu2eHall_v02.txt:473) and the twopsArea.upper2.Nprefix entries (mu2eHall.txt:195,346) (finding 2). - Optional, and separable into its own PR:
sites.back()inExtShieldUpstreamMaker.ccand thenBoxType21comment (finding 3); the vertex labels (finding 4); and moving your SC-19 / ExtMon-stairs answers from the comment thread into the PR body.
|
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for faffc80: build (Build queue - API unavailable) |
|
☀️ The build tests passed at faffc80.
N.B. These results were obtained from a build of this Pull Request at faffc80 after being merged into the base branch at a126eb4. For more information, please check the job page here. |
dirtCentralStairwellLower3, added in Mu2e#1918, abuts CentralStairwellLowerStairs at Mu2e z = 6375.4. That neighbour is rotated by -1.5707963268, which is pi/2 truncated at the 10th decimal, so its face is tilted ~5e-12 rad off square and pokes into the new volume. overlapCheck.C uses CheckOverlaps(1e-12), so the artifact is reported on the default geometry: Overlap ov00000: HallAir/dirtCentralStairwellLower3 overlapping HallAir/CentralStairwellLowerStairs ovlp=3.11047e-10 Move the abutting face out by 1 um so the volumes no longer touch. This is the same remedy already applied to the neighbour itself, whose yHalfThickness carries "remove 1 um to prevent conflict as the rotation angle is not exactly pi/2". Verified by regenerating mu2e_common.gdml from the patched geometry and rerunning overlapCheck.C: 1 illegal overlap -> 0. The regenerated GDML differs from the baseline in exactly the two twoDimVertex entries for this face, with identical volume counts (9987 physvol), so no other volume is perturbed. Fixes Mu2e#1925
These are the geometry fixes described in doc-db 57505: