restore STM poly shielding for run 2 - #1934
Conversation
|
☀️ The build tests passed at 0443c00.
N.B. These results were obtained from a build of this Pull Request at 0443c00 after being merged into the base branch at da434b8. For more information, please check the job page here. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #1934
Reviewed at head 0443c0097b023e128febba4e1be391ad85734b2b. First pass.
Decision
- 🟢 approve
This is the correction @YongyiBWu asked for on #1932, and it is right for the
reason he gave. Both findings below are about neighbouring files, not this diff.
Scope understood
- One line:
geom_run2.txtgoes back fromSTM_v10(poly shielding removed) to
STM_v09(poly kept). - #1932 bumped all five geometry configs to
STM_v10; @YongyiBWu commented
afterwards that "the change does not apply togeom_run2.txt. All the rest
changes are good." This PR implements exactly that.
Findings
-
🟡 [S2]
geom_run1.txtis internally inconsistent by the same criterion —
full downstream shielding with the poly removed-
Evidence: @YongyiBWu's criterion on #1932 is that the poly may only be
dropped where the blocks it is embedded in are gone — "the external
shielding downstream blocks are removed in run1. The poly is embedded in
the blocks so it cannot float in the air." Applying that to every config
that references an STM version onmain:config STM downstream shielding geom_run1.txtv10 ExtShieldDownstream_v06(full)geom_run1_a.txtv10 reduced_ExtShieldDownstream_v06geom_run1_a_stickman.txtv10 reduced_ExtShieldDownstream_v06geom_reduced_DSTS_shielding.txtv10 reduced_ExtShieldDownstream_v06geom_run2.txt(this PR)v09 ExtShieldDownstream_v06(full)geom_2021_PhaseI_v03.txtv09 ExtShieldDownstream_v06(full)Every row lines up except
geom_run1.txt, which pairs the full downstream
shielding at line 52 withSTM_v10at line 63 — the same pairing this PR is
fixing ingeom_run2.txt. -
Impact: with the blocks present and the poly not built, that config has an
unfilled hole in the downstream shielding. It is a live file, not dead —
EventDisplay/fcl/EventDisplayRun1.fclandCRVReco/test/printChannelMap.fcl
include it — though neither is production simulation, which is why this is
S2 and not higher. It predates this PR and is not a regression from it. -
Suggested fix: decide which of the two lines is the stale one. If the file is
meant to describe the same shielding layout asgeom_run2.txt, line 63 goes
back toSTM_v09as here; if it is meant to be a run-1 layout, line 52 should
be thereduced_variant. Note @YongyiBWu did clear this file on #1932
("all the rest changes are good"), so there may be a reason I cannot see —
worth a one-line confirmation either way.
-
-
⚪ [S3] The reason for the split lives only in a PR comment
STM_v10.txtis#include STM_v09plus one bool, with no comment saying why
or when it applies, and the PR body says only "Fix #1932" — which is a merged
PR, not an issue, so it does not close anything and the reader has to find
@YongyiBWu's inline comment on it. A year from now the version numbers alone
imply v10 supersedes v09, when in fact they are two live variants selected by
the downstream shielding layout. One comment line inSTM_v10.txtnaming that
condition would fix it, and it is the single place both files' readers land.
Verified, no action needed
- 🟢 The result is a clean revert:
geom_run2.txtat this head is byte-identical
(md5bd1b6486a35b796254d38df7c607f273) to its state ata83d63f2, the last
commit before #1932'sd53f259ftouched it. No other line moved. - 🟢 The physics premise holds where I can check it.
geom_run1_a.txt:52includes
reduced_ExtShieldDownstream_v06.txtwhilegeom_run2.txt:52includes the full
ExtShieldDownstream_v06.txt, so run 2 does still build the blocks the poly sits
in — which is the condition @YongyiBWu named for keeping it. - 🟢 CI is green at this head: FNALbuild build 3295, all 20 rows ✅ including
g4surfaceCheck,rootOverlaps,check_cmake, clang-tidy (0/0) and the
whitespace check.
Validation check
- Build/tests run: CI green at
0443c009. Note the two overlap jobs do not
exercise the changed file:surfaceCheck.fcl:38points atgeom_SurfaceCheck.txt,
which includesgeom_common_current.txt→geom_common.txt→
geom_run1_a_stickman.txt.geom_run2.txtis in no CI chain, before or after
this change, so the green tells you the build is fine and nothing about run 2's
geometry. The revert-to-a-prior-state argument above is what covers that gap. - Config contract check: pass — see the table in finding 1.
- Cross-repo consistency: n/a, single geometry text file.
Residual risk
- I took the embedding relationship — that the poly fills a hole in the downstream
shielding blocks — from @YongyiBWu's description rather than reading
ExtShieldDownstream_v06.txtagainstSTM_v09.txtmyself. Finding 1 rests on it.
Author follow-ups
- Confirm or fix the
geom_run1.txtline 52 / line 63 pairing (finding 1). - Optional: one comment line in
STM_v10.txtrecording that it applies only to
configs usingreduced_ExtShieldDownstream, so the v09/v10 split survives
without the PR thread.
|
I have deleted by mistake the branch before the merging. I have recreated it. In case of need please delete this PR and I will open a new one. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 1cc8775: build (Build queue - API unavailable) |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #1934
Reviewed at head 1cc8775d74a9c394b8f7c0c370f7de00a644835f. Re-review of 0443c009.
Decision
- 🔴 request changes
To be clear about what changed and why the decision moved: I approved this PR
at 0443c009, when it was a one-line STM revert. The new commit adds an
unrelated OPA support material change — the same change that was split out of
#1918 at review request on 2026-08-07. My earlier approval is still showing on
this PR and no longer describes what is in it, so the first thing this review
has to do is withdraw it.
The STM half is unchanged and still correct.
Scope understood (delta only)
0443c009..1cc8775dis one commit, "restore STM poly shielding for run2 and
change OPA support material", touchingprotonAbsorber_cylindrical_v04.txt:
all tenStainlessSteel316occurrences in the OPA support block become
Al6061.geom_run2.txtis untouched by the delta and is byte-identical to what I
approved (md5bd1b6486a35b796254d38df7c607f273).- The PR title and body still describe only the STM change.
Findings
-
🔴 [S0] The OPA support material change is unrelated to this PR, and is a
re-submission of a change already withdrawn from #1918 at review request- Evidence: on #1918 I raised this as finding 1 (🟠 S1, review 4879365261).
You removed it in5038da5bon 2026-08-07, whose commit message is
"remove OPA support changes from this PR", and my second pass recorded
"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." It is now back, in a PR whose stated
purpose is a one-line STM include, and with a different alloy
(Al7075on #1918,Al6061here). - Impact: a material-budget change to every production geometry is merged
under a title that says "keep the STM poly shielding for run2", which is
what makes it hard to find later when someone bisects a background
change. It also puts a stale APPROVED review on a PR that no longer
matches it. Separately, the Mu2e convention is that a PR does one thing. - Suggested fix: drop
protonAbsorber_cylindrical_v04.txtfrom this branch
so the STM revert can merge on its existing green, and open the OPA
material change as its own PR carrying the justification below. If you
would rather keep them together, retitle the PR and rewrite the body to
describe both, and say so — but the separate PR is what I asked for last
time and it is still the cleaner path.
- Evidence: on #1918 I raised this as finding 1 (🟠 S1, review 4879365261).
-
🟠 [S1] The alloy is unsourced, changed between attempts, and the physics
change is unvalidated- Evidence:
protonAbsorber_cylindrical_v04.txtlines 67, 83-88, 94, 125.
StainlessSteel316is 8.00 g/cm³ (ConstructMaterials.cc:403);
Al6061is 2.70 g/cm³ (ConstructMaterials.cc:623-630). The six support
rings alone are 49.2 L of material by their own radii and half-lengths,
so this takes them from about 393 kg to about 133 kg — roughly 260 kg
removed from inside the DS, at a mean Z dropping from Fe/Cr/Ni to Al.
Nothing in the PR says where the alloy comes from, and #1918 used
Al7075(2.81 g/cm³) for the same parts with no stated reason for the
switch. By contrastprotonAbsorber_cylindrical_v05.txtcites
Doc-57487-v3 in its header for its material change. - Impact: a mass and composition change of that size around the stopping
target moves neutron and photon production and the material seen by DIO
and cosmic tracks. It reachesgeom_run1_a.txt,geom_run1.txt,
geom_run2.txt,geom_2021_PhaseI{,_v02,_v03}.txt,
geom_reduced_DSTS_shielding.txt, and through
protonAbsorber_cylindrical_v05.txtalsogeom_run1_a_stickman.txt—
which is whatgeom_common.txt:6includes, so it is every production and
CI geometry. Editingv04in place also means MDC and Run-1 samples
already produced against that file name are no longer reproducible from
it. - Suggested fix: cite the drawing or doc-db item that gives the alloy, say
why it isAl6061and not theAl7075of #1918, and state whether the
in-place edit is deliberate rather than a newprotonAbsorber_cylindrical_v06.txt.
If simulation and production conveners are content with in-place, one
line in the PR body recording that decision is enough.
- Evidence:
-
⚪ [S3] The PR body and title do not mention the material change.
Body reads "Fix #1932: keep the STM poly shielding for run2"; only the
commit message mentions OPA. Whichever way finding 1 resolves, the body
should describe what merges.
Carried forward from my review at 0443c009 (both non-blocking then,
both still open — the delta touches neither file)
- 🟡 [S2]
geom_run1.txtis still internally inconsistent by @YongyiBWu's own
criterion — line 52 includes the fullExtShieldDownstream_v06.txtwhile
line 63 includesSTM_v10.txt(poly removed). That is the same pairing this
PR is fixing ingeom_run2.txt. Verified again at this head. - ⚪ [S3]
STM_v10.txtstill carries no comment saying when it applies, so
the v09/v10 split lives only in the #1932 thread.
Verified, no action needed
- 🟢 The STM half is exactly what I approved:
geom_run2.txtat1cc8775dis
byte-identical to the file at0443c009, line 63 readsSTM_v09.txt, and
nothing else in that file moved. - 🟢 The material substitution is complete — all ten
StainlessSteel316
occurrences in the OPA support block are converted (oPASupportMaterialName,
sixoPASupportMaterials, twooPASupportSlatMaterials,
crossSupportMaterial), with no stragglers left in the file. The unrelated
oPASupportSlatFillMaterials(BronzeC642,G4_Al) are correctly untouched. - 🟢
Al6061is a real, defined material (ConstructMaterials.cc:623, also used
bytracker_v7.txt), so nothing will throw at construction. - 🟢 The change agrees with the file's own documentation: the comment at line
65-66, "oPA Supports are Aluminum rings that support and 'hold together' the
OPA", has been there since 2017 while the values said stainless steel. That
is a real argument that aluminium is right, and it is worth putting in the
PR body — it does not by itself settle which alloy.
Validation check
- Build/tests run: there is no CI result at
1cc8775d— the statuses list is
empty and pending. FNALbuild's green table is at0443c009and predates the
material change. I triggered a run
(@FNALbuild run build test, comment 5318060958). Note the bare@FNALbuild
comment at 15:18 is not a command and did not trigger anything. CI will
exercise the material change:geom_common.txt:6→
geom_run1_a_stickman.txt:66→protonAbsorber_cylindrical_v05.txt→
v04.txt. - Config contract check: pass — the parameter names and vector lengths are
unchanged, andMECOStyleProtonAbsorberMaker.cc:344-347reads both the
vector and the scalar fallback, both of which were updated consistently. - Cross-repo consistency:
Mu2e/ProductionJobConfig/cosmic/geom_cosmic_run1_a.txt
is a one-line include ofgeom_run1_a.txt, so the cosmic campaigns inherit
the material change. No Production edit is required, but the campaigns are
affected.
Residual risk
- I have not estimated what the 260 kg changes in any physics observable. That
is the validation finding 2 asks for, and it belongs to whoever owns the
material budget.
Author follow-ups
- Split the OPA material change out of this PR, as on #1918, so the STM revert
can merge on its own (finding 1). - In the OPA PR, cite the source for the alloy and explain
Al6061versus the
Al7075of #1918, and say whether the in-placev04edit is deliberate
(finding 2). - Whichever way this goes, update the PR body to describe what actually merges.
- Still open from the previous pass: confirm or fix the
geom_run1.txtline 52
/ line 63 pairing.
|
☀️ The build tests passed at 1cc8775.
N.B. These results were obtained from a build of this Pull Request at 1cc8775 after being merged into the base branch at da434b8. For more information, please check the job page here. |
|
AI comments referring to the obsolete geom_run1.txt can be ignored. |
Fix #1932: keep the STM poly shielding for run2