Fix IPA EndRing parameters and add debugging for wire support - #1915
Conversation
…EndRings half length and radial length as shown in protonAbsorber_cylindrical_v05.txt, changed the stopping material for the IPA EndRings and corrected the density to 0.426g/cm^3. Added debugging print to further investigate wire support placement in the End Rings
|
Hi @Etho-b02,
which require these tests: build. @Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main. 📝 The author of this pull request is not a member of the Mu2e github organisation. |
AndrewEdmonds11
left a comment
AndrewEdmonds11
left a comment
There was a problem hiding this comment.
This looks pretty good. Thanks, Bryan. There are a few comments in-ine below
| // | ||
|
|
||
| //Increment version number | ||
| int protonabsorber.version = 4; |
There was a problem hiding this comment.
I don't think we want to increment this since we did not change anything in the code itself
| @@ -278,7 +278,7 @@ namespace mu2e { | |||
|
|
|||
| mat = uniqueMaterialOrThrow( "IPAPolyethylene"); | |||
| { | |||
| G4Material* IPAPolyethylene = new G4Material( mat.name, 0.954*CLHEP::g/CLHEP::cm3, 2); | |||
| G4Material* IPAPolyethylene = new G4Material( mat.name, 1.05*CLHEP::g/CLHEP::cm3, 2); | |||
There was a problem hiding this comment.
Can you remind me why we changed this density?
| @@ -1334,6 +1334,13 @@ namespace mu2e { | |||
| CorrugatedPolypropylene->AddElement( getElementOrThrow("H"), 6); | |||
| } | |||
|
|
|||
| mat = uniqueMaterialOrThrow("IPAPolystyrene"); | |||
| { | |||
| G4double IPAEffectiveDensity = 0.426*CLHEP::g/CLHEP::cm3; //explicit density | |||
There was a problem hiding this comment.
Is this the corrected density or the proposed density? I would add a comment referring to our DocDB here
| @@ -1614,6 +1614,7 @@ namespace mu2e { | |||
| ostringstream wirename ; wirename << "IPAsupport_set" << iS << "_wire" << ++iW ; | |||
|
|
|||
| const double rStartOfWire = pabs1rOut0+(supportWire.originInMu2e().z()-zstartOfIPA)/pabs1len*(pabs1rOut1-pabs1rOut0); | |||
| std::cout << "Set " << iSet << ", Wire " << iWire << ": rStartOfWire = " << rStartOfWire << "\n"; // will be used for debugging | |||
There was a problem hiding this comment.
Either remove this line or hide it behind a verbosity / debug flag
oksuzian
left a comment
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — "Fix IPA EndRing parameters and add debugging for wire support" (#1915)
Reviewed at head 4d270bb1 (2026-08-03). 5 files, +143/−4: new protonAbsorber_cylindrical_v05.txt (a modified copy of v04), two run1 geometry files switched to it, one material redefined + one added in ConstructMaterials.cc, one debug print in constructProtonAbsorber.cc. The EndRing dimension corrections themselves check out against their own documentation (verified below). The blockers are in the materials file: an in-place density change to a material shared by 13 existing geometry configurations, and an undeclared partial scope for the v05 switch.
Decision
- 🔴 Request changes. Findings 1–2 change physics silently in geometries this PR does not claim to touch. Everything else is minor.
Scope understood
- Correct the IPA EndRing dimensions (to Doc-57487-v3) and material in a new v05 geometry file; point
geom_run1.txtandgeom_run1_a_stickman.txtat it. - Redefine
IPAPolyethylenedensity 0.954 → 1.05 g/cm³; addIPAPolystyrene(0.426 g/cm³) for the EndRings. - Add a wire-support placement print for ongoing debugging.
Findings
1. 🟠 [S1] IPAPolyethylene is redefined in place — a silent +10% density change to every geometry that names it, most of which this PR does not touch.
- Evidence:
ConstructMaterials.cc:281changes the density of the existing material. The material is named byprotonAbsorber_cylindrical_v04.txt(included by 8 geometries at main:geom_run1,geom_run1_a,geom_run1_a_stickman,geom_run2,geom_reduced_DSTS_shielding,geom_2021_PhaseI{,_v02,_v03}) and by the baseprotonAbsorber_cylindrical.txt(5 more, incl. the cd3 and DOE-review geometries). All 13 get the new density with zero geometry-file change — includinggeom_run1_a, which pins current MDC2025 production primaries. - Impact: re-running any pinned older geometry with a new Offline release silently produces a different IPA (energy loss, scattering). It also contradicts the comment retained on the next line — "density measured by S. Krave 6/22/2021" documented the 0.954 measurement. The PR body does not mention this change at all, and 1.05 g/cm³ is worth double-checking on physics grounds: polyethylene is 0.92–0.96 (1.05 is textbook polystyrene), so if this is a new measurement/estimate for the carbon-doped DW 402B material, cite it.
- Suggested fix: follow the repo's own convention for density variants (
Polyethylene092/094/096): keepIPAPolyethyleneat 0.954 and add a new name (e.g.IPAPolyethylene105orIPAPolyethylene_v2) at 1.05 with a DocDB reference, used only by v05. Old geometries then reproduce; v05 carries the new value explicitly. (This absorbs Andy's "why did we change this density?" — the review needs the answer written down either way.)
2. 🟠 [S1] The v05 switch covers only 2 of the 8 v04 geometries, with no statement of intent.
- Evidence:
geom_run1.txtandgeom_run1_a_stickman.txtmove to v05;geom_run1_a.txt,geom_run2.txt,geom_reduced_DSTS_shielding.txt, and the threegeom_2021_PhaseI*stay on v04. - Impact: the corrections are described as as-built fixes (Doc-57487-v3). If that's right,
geom_run1_aandgeom_run2describe the same hardware and are now inconsistent withgeom_run1— the run1 family diverges on IPA EndRings. If leaving them is deliberate (e.g. not perturbing a pinned production geometry mid-campaign), the PR body should say so explicitly so it reads as a decision rather than an oversight — and note that finding 1, as written, leaks the density change into them anyway. - Suggested fix: either switch the remaining current-era geometries (
geom_run1_a,geom_run2at minimum) or add one sentence to the PR body declaring the scope. Historical 2021/cd3 geometries should stay untouched — which is exactly why finding 1's material versioning matters.
3. 🟡 [S2] Unconditional std::cout in geometry construction (carries Andy's inline request)
- Evidence:
constructProtonAbsorber.cc:1617— barestd::coutin the wire-support loop (6 lines per job with v05 values), plus a "will be used for debugging" self-note comment. - Impact: unguarded prints in every job log, against the coding standard (verbosity flag / message facility).
- Suggested fix: the file already reads the knob at line 67 (
verbosityLevel = _config.getInt("protonabsorber.verbosityLevel", 0)); wrap the print inif (verbosityLevel > 0)and drop the comment — or remove the line and keep it in your local branch until the investigation is done.
4. 🟡 [S2] Make v05 #include v04 and override only the three EndRing keys (endorses Andy's suggestion, with evidence it works)
- Evidence: the real v04→v05 delta is 3 keys + comments; the other ~125 copied lines include v04's pre-existing duplicate definitions (
verbosityLevelat lines 39 and 46,isShorterConeat 51 and 74), now duplicated into a second file. - Impact: a 6-line v05 makes the version delta self-documenting, kills the copied duplicates, and auto-inherits future v04 fixes — concretely: PR #1913 (approved, unmerged) adds the
zStartInMu2edocumentation to v04; a copy-style v05 silently loses it for the new default geometry, an include-style v05 gets it for free. - Suggested fix:
#include "Offline/Mu2eG4/geom/protonAbsorber_cylindrical_v04.txt"+ the three overridden keys + a header comment stating the v05 delta and Doc-57487-v3.
5. ⚪ [S3] v05's header comment is v04's history. Lines 7–13 describe "This version introduces an OPA version number and implements the OPA as 24 slats…" — that was v04's changelog. Describe v05's actual delta (moot if finding 4 is adopted).
6. ⚪ [S3] Accidental whitespace change in ConstructMaterials.cc. The G4cout line near 1920 is un-indented by one space in an otherwise untouched block — revert.
Carry-forward: Andy's review (COMMENTED, 5 inline items)
- "#include v04 and add corrected dimensions" — endorsed as finding 4, with the #1913 synergy as an extra reason.
- "I don't think we want to increment this [version]" — dissolved with evidence: no increment occurred. v04 at main already contains
int protonabsorber.version = 4;under the same "//Increment version number" comment; the new-file diff renders every copied line as added, which reads like a change. v05 is byte-identical to v04 there. (The value 4 is also behaviorally safe: the construction code branches at>=2/>2only.) - "Why did we change this density?" — absorbed into finding 1; the answer belongs in the PR body and a code comment with a DocDB reference.
- "Corrected or proposed density? Add DocDB ref" (0.426 IPAPolystyrene) — endorsed; add the reference next to
IPAEffectiveDensity(the current "//explicit density" comment says nothing). - "Remove or hide the print behind a verbosity flag" — endorsed as finding 3, with the concrete knob already available at line 67.
🟢 Verified correct — no action
- 🟢 EndRing dimension corrections are self-consistent:
endRingHalfLength = 3.175now actually gives the ¼″ z-extent its comment always claimed (v04's 6.35 half-length was ½″ full), andendRingRadialLength = 19.05is the stated ¾″ (v04's 9.525 was ⅜″). - 🟢 All three EndRing keys are read by
MECOStyleProtonAbsorberMaker(lines 482–485);IPAPolystyreneis properly defined viauniqueMaterialOrThrow+AddElement(C8H8) sofindMaterialOrThrowat construction succeeds. - 🟢 No overlap regression: ring outer radius is pinned 0.01 mm inside the IPA inner surface and the radial growth to 19.05 mm extends inward into vacuum; the new half-length only shortens the pre-existing overhang past the IPA faces.
- 🟢 The debug print compiles as written (
iSet/iWireare the actual loop variables; the 1-basediS/iWare only used for volume names).
Validation check
- Build/tests: 🔴 not run — FNALbuild has only posted its greeting; no build triggered at
4d270bb1. Needs a@Mu2e/writemember to comment@FNALbuild run build test. - Config contract: 🟢 keys ↔ Maker reads all match.
- Cross-repo/coordination: 🟡 none required outside Offline, but #1913 (v04 doc comment, approved/unmerged) interacts with the copy-vs-include choice (finding 4); no git conflict either way.
Residual risk
- If merged as-is, all pre-v05 geometries change IPA density on the next release without any record in their geometry files (finding 1) — the kind of silent physics drift that surfaces months later as a validation mystery.
Author follow-ups
- Version the polyethylene material instead of mutating it; document both densities with DocDB refs (findings 1, Andy's two density questions).
- Declare or complete the v05 switch scope — what happens to
geom_run1_a/geom_run2? (finding 2) - Guard or drop the debug print (finding 3).
- Restructure v05 as include+override (finding 4; makes 5 moot).
- Explain "so that we get IPA EndRing output" in the PR body — EndRings were already built in v04 (
nEndRings = 2, construction not version-gated), so it's not obvious what output was missing; if something was in fact failing before, that belongs in the description. - Trigger the build test once updated.
made changes to IPApolyethylene - correct density should remain uncahed at 0.954, added guards to debug print in constructProtonAbsorber restructured v05 txt IPA EndRing output refers to the outputs in Produce, not in this directory
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for f857837: build (Build queue - API unavailable) |
|
☀️ The build tests passed at f857837.
N.B. These results were obtained from a build of this Pull Request at f857837 after being merged into the base branch at f408474. For more information, please check the job page here. |
oksuzian
left a comment
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — re-review of "Fix IPA EndRing parameters" (#1915)
Reviewed at head f857837e (2026-08-04). Delta since the previously reviewed 4d270bb1: one commit responding to both posted reviews. It resolves everything that gated: the shared-material density mutation is fully reverted, v05 is restructured as include+override, and the debug print is guarded. CI is green at the new head (vs main f4084747). Every prior finding is accounted for below; fixes verified in the diff, not the commit message.
Decision
- 🟢 Approve. All 🟠 findings from the request-changes review are resolved; what remains is one non-gating S2 (scope declaration in the PR body) and typo-level nits. This approval supersedes my earlier request-changes.
Carry-forward accounting
Mine (request-changes review of 4d270bb1):
1. 🟢 [was S1] IPAPolyethylene in-place density mutation — FIXED in f857837e, verified.
Resolved by full reversion: density back to 0.954 g/cm³, the material definition now untouched by this PR. Cleaner than the versioned-material prescription — the change is withdrawn, not relocated, so the 13-geometry blast radius is gone entirely. The S. Krave measurement comment is consistent again. (This also closes Andy's "why did we change this density?" — answer: it shouldn't have been; it's backed out.)
2. 🟡 [was S1, reduced] Partial v05 switch scope — carried over, reduced severity.
geom_run1 + geom_run1_a_stickman are on v05; geom_run1_a and geom_run2 remain on v04, and the PR body still doesn't say whether that's deliberate. With finding 1 fixed, the leak into untouched geometries is gone, so the remaining impact is confined to the intended mechanism: the run1 family now disagrees on as-built EndRing dimensions. One sentence in the PR body ("run1_a/run2 stay on v04 because …" — e.g. pinned MDC2025 reproduction, follow-up planned) settles it. Non-gating.
3. 🟢 [was S2] Unconditional debug print — FIXED in f857837e, verified.
Wrapped in if (verbosityLevel > 0) using the knob already read at line 67; the "will be used for debugging" self-note is gone. Compiles (CI green).
4. 🟢 [was S2] Copy-style v05 — FIXED in f857837e, verified.
v05 is now #include protonAbsorber_cylindrical_v04.txt + the three corrected EndRing keys (+4/−123). Valid by construction: SimpleConfig defaults allowReplacement = true, so the post-include definitions win. This also auto-inherits future v04 changes — including #1913's zStartInMu2e documentation when it merges — and it removed the copied duplicate keys.
5. 🟢 [was S3] Stale copied header — FIXED (new two-line header naming the actual delta and Doc-57487-v3). Typo: "Correced" → "Corrected".
6. 🟢 [was S3] Whitespace noise — FIXED and re-introduced. The G4cout indent is restored, but line 1 of ConstructMaterials.cc gained a stray leading space (// → //). Same class of accident, new location.
Andy's review (5 inline items):
- include v04 + override — adopted exactly (finding 4). ✔
- version increment — was dissolved with evidence (no increment ever existed); now doubly moot: v05 no longer contains the version line at all.
- why the density change — resolved by reversion (finding 1). ✔
- 0.426: corrected vs proposed + DocDB ref — addressed: the comment now states it reflects the corrected End Ring density and cites Doc-57487-v3. Typo "explicity"; the three stacked comment lines could be one.
- print behind verbosity flag — done (finding 3). ✔
Open from prior follow-ups: the PR body still doesn't explain "so that we get IPA EndRing output" (EndRings were already built in v04), and now also predates the restructure. A refreshed description would close both.
New findings (delta 4d270bb1..f857837e)
1. ⚪ [S3] Stray leading space on ConstructMaterials.cc line 1 — revert.
2. ⚪ [S3] Typos: "Correced" (v05 header), "explicity" (material comment); commit message is garbled ("Adressed PR changes: accidentally").
Validation check
- Build/tests: 🟢 FNALbuild green at
f857837e(merged against mainf4084747). - Config contract: 🟢 include+override verified valid (
allowReplacement = truedefault); the three overrides land after the include, so last-definition-wins yields the corrected values. - Cross-repo/coordination: 🟢 the include-style v05 now composes correctly with #1913's v04 edit.
Residual risk
- Only the declared-scope question (finding 2): run1 vs run1_a EndRing divergence until the author states or completes the plan.
Author follow-ups (non-blocking)
- Add one sentence to the PR body declaring the v04/v05 scope for
geom_run1_a/geom_run2, and refresh the description to match the restructured PR (including what "EndRing output" was about). - Typo sweep: "Correced", "explicity", line-1 whitespace.
4 participants
Made Changes to the IPA so that we get IPA EndRing output. Corrected EndRings half length and radial length as shown in protonAbsorber_cylindrical_v05.txt, changed the stopping material for the IPA EndRings and corrected the density to 0.426g/cm^3. Added debugging print to further investigate wire support placement in the End Rings.