Skip to content

Add Run 2 CRV SurfaceIds and update CRVModule material definition#1858

Open
RobMina wants to merge 5 commits into
Mu2e:mainfrom
RobMina:crv-run2-surfaceids-src
Open

Add Run 2 CRV SurfaceIds and update CRVModule material definition#1858
RobMina wants to merge 5 commits into
Mu2e:mainfrom
RobMina:crv-run2-surfaceids-src

Conversation

@RobMina

@RobMina RobMina commented Jun 12, 2026

Copy link
Copy Markdown

Run 2 CRV sector IDs and material definition from Mu2eG4/geom/crv_counters_v10.txt

@FNALbuild

Copy link
Copy Markdown
Collaborator

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

  • TrackerConditions
  • DataProducts

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.

About FNALbuild. Code review on Mu2e/Offline.

@brownd1978 brownd1978 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.

Looks good, a few minor changes requested.

Comment thread DataProducts/inc/SurfaceId.hh Outdated
@RobMina RobMina requested a review from brownd1978 June 12, 2026 20:51
# CRV active module stack from crv_parameters.xlsx (DocDB 862 https://mu2e-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=862):
# 4 scintillator layers (4*19.80 mm) + 3 aluminum absorbers (3*9.525 mm)
# + aluminum strongback (12.7 mm) + thin aluminum cover (3.175 mm),
# represented as one effective material without the inter-layer air gaps.

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.

The air gaps don't matter in terms of material, but the density of the composite needs to be compatible with the thickness ascribed to a module, currently set here. Can you please check these are consistent?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I scaled the density up to match the half-width from the KinKalGeomMaker code. While testing I discovered that the half-width of the U/D sectors was negative:

CRV sector CRV_R2 midpoint (-2581.19,275.53,-2871.1) wdir (-1,0,0) udir (0,1,0) vdir (0,0,-1) uhw 2275 vhw 5433.42 whw 53.8875
CRV sector CRV_D1 midpoint (0,1761.5,8457.26) wdir (0,0,1) udir (1,0,0) vdir (0,1,0) uhw 2850 vhw 1237.92 whw -34.0875
CRV sector CRV_U midpoint (650,1599.75,-12526.8) wdir (0,0,-1) udir (1,0,0) vdir (0,-1,0) uhw 3450 vhw 1651.17 whw -34.0875

Added a std::abs around that so half-widths are consistently 53.8875 mm.

…lso fix sign error in half-width calculation for CRV U and D sectors.

@brownd1978 brownd1978 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.

Good catch on the width calculation!
The module width calculation under-estimates the true thickness by 15.8mm. I would have guessed the difference would be just the absorbers, but maybe the air gaps enter too? What matters is that a particle passing through the full module see the effect of the total mass.

I agree scaling is the right way to fix this; fixing the width calculation would be difficult and doesn't provide any benefit. Your comment provides clear provenance.

@brownd1978

Copy link
Copy Markdown
Collaborator

@FNALbuild run build test

@FNALbuild

Copy link
Copy Markdown
Collaborator

⌛ The following tests have been triggered for 5b7e1a9: build (Build queue - API unavailable)

@FNALbuild

Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at 5b7e1a9.

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

N.B. These results were obtained from a build of this Pull Request at 5b7e1a9 after being merged into the base branch at ba2d4e5.

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

Copy link
Copy Markdown
Collaborator

1. Summary

Author RobMina (first-time contributor, not yet a Mu2e org member)
Intent Register the Run 2 CRV sectors from crv_counters_v10.txt as SurfaceIds, fix the CRV module material density, and fix a width-calculation sign bug
Scope 4 files, +40 / −5, 3 commits
Reviews brownd1978: CHANGES_REQUESTED → APPROVED (×2)
CI ✅ Build tests passed at 5b7e1a9 (build, ceSimReco, cosmic*, rootOverlaps, etc.). clang-tidy: 2 errors / 41 warnings (pre-existing baseline)
Mergeable clean / rebaseable
Risk Low–Medium — small, well-contained, but a few correctness details below are worth confirming before merge

The PR does three logically distinct things:

  1. DataProducts/SurfaceId.{hh,cc} — adds 23 new CRV sector enum IDs + name mappings.
  2. GeometryService/KinKalGeomMaker.cc — wraps the half-width calc in std::abs().
  3. TrackerConditions/data/MaterialsList.data — replaces the placeholder pure-aluminum CRVModule with a scaled Polystyrene+Aluminum composite.

All reviewer comments so far have been addressed. Below are the issues I found that are not yet raised in the PR thread.


2. Issues found

🔴 Issue 1 — Enum list and name-map are out of order / risk silent omission (maintainability + correctness)

The enum_type in SurfaceId.hh declares sectors grouped by region, but the name map in SurfaceId.cc lists them in a different order (R, L, then T3/T4/T5, then E/U/D/C). Worse, the two lists must stay perfectly in sync by hand, and there's no compile-time check that every enumerator has a corresponding std::make_pair.

I verified the counts: the header adds exactly 23 enumerators (CRV_T3..T5, R1..R6, L1..L3, E1..E2, U, D1..D4, C1..C2) and the .cc adds 23 pairs — so they currently match. But note how name() behaves if one is ever missed:

    std::string const& name() const {
      // We checked for the validity of _id upon construction.  No need to check now.
      typename map_type::const_iterator value = Detail::names().find(_id);
      return value->second;
    }

If an enumerator is missing from the map, find() returns end() and this dereferences the end iterator → undefined behavior (not a clean throw). Since these IDs are populated by name (SurfaceId(sector.sname_) in KinKalGeomMaker::makeCRV), a missing name-map entry would also make findByName throw out_of_range at geometry-build time. Recommend either:

  • Add a brief comment in both files: "keep enum and name map in sync — every CRV_ must appear in both"*, or
  • (Better, follow-up) consider generating the map or adding a static assert on names().size().

Not a blocker since they currently match, but it's the most fragile part of this change.


🟡 Issue 2 — CRV_T1/CRV_T2 are now ambiguous: "deprecated test planes" and live Run 2 sectors

Look carefully at the header comment vs. the geometry file:

        TCRV=200, CRV_EX, CRV_T1, CRV_T2, // CRV test planes (deprecated); EX/T1/T2 reused for extracted geometry
        // Run 2 CRV sectors (from crv_counters_v10.txt, prefixed CRV_ by CosmicRayShieldMaker)
        CRV_T3=204, CRV_T4, CRV_T5,                           // CRV-Top (T1/T2 above)
        CRV_R1=210, CRV_R2, CRV_R3, CRV_R4, CRV_R5, CRV_R6,   // CRV-Right
        ...

In crv_counters_v10.txt, T1 and T2 are the CRV-TS modules (sectors 9 & 10), while T3/T4/T5 are CRV-Top. The comment says "CRV-Top (T1/T2 above)" — but T1/T2 are TS, not Top. So:

  • CRV_T1/CRV_T2 simultaneously mean "deprecated test plane" and "Run 2 CRV-TS sector," and the inline comment mislabels them as CRV-Top.
  • Since makeCRV() looks sectors up by name ("T1", "T2"), the Run 2 T1/T2 shields will map onto the enum values originally reserved for the deprecated test planes (201, 202). That's probably the intent (reuse), but the comment is misleading and should be corrected to say T1/T2 = CRV-TS.

Please confirm the reuse of 201/202 for live TS sectors is intentional and fix the "CRV-Top (T1/T2 above)" comment, which is inaccurate.


🟡 Issue 3 — Material density scaling: re-derive the constant and confirm thickness consistency

This was raised by brownd1978 and "addressed," but the numbers deserve a second look since they directly affect tracking material budget.

# CRV active module stack from crv_parameters.xlsx (DocDB 862 ...):
# 4 scintillator layers (4*19.80 mm) + 3 aluminum absorbers (3*9.525 mm)
# + aluminum strongback (12.7 mm) + thin aluminum cover (3.175 mm),
# represented as one effective material over the KinKal CRV thickness.
# Multiply density by (full module width)/(KinKal CRV thickness) = 123.65 / 107.775
Polystyrene 1.060000 0. 0. -2 8 Carbon 0 8 Hydrogen 0 -10 -20 -30 20.0 1.0 solid
CRVModule 1.892526 0. 0. +2 41.2e-2 Polystyrene 1 58.8e-2 Aluminum 0 -10 -20 -30 20.0 1.0 solid

A few things to verify:

  • Full module width = 123.65 mm? From the comment: 4×19.80 + 3×9.525 + 12.7 + 3.175 = 79.20 + 28.575 + 12.7 + 3.175 = 123.65 mm. ✔️ self-consistent.
  • KinKal thickness = 107.775 mm? The half-width the author reports from the debug output is whw 53.8875 → full thickness 2 × 53.8875 = 107.775 mm. ✔️ consistent with the std::abs fix.
  • Density: whw in KinKalGeomMaker is computed as 0.5*|(llaypos-flaypos)·wdir| + firstbar.getHalfThickness(). That is 0.5 × (3 gaps of 9.525) + 0.5 × 19.80… let me check: the distance between layer-0 and layer-3 centers is 3 × layer-pitch. With gapBetweenLayers = 9.525 and bar thickness 19.80, layer pitch ≈ 19.80 + 9.525 = 29.325, so 3 × 29.325 = 87.975, half = 43.9875, plus half-thickness 9.90 = 53.8875 mm → full 107.775 mm. ✔️ This matches and confirms the scaling denominator is the geometric KinKal thickness, which is exactly what the density must be normalized to. Good.
  • Composite mass fractions (41.2% polystyrene / 58.8% Al): these should reflect the mass ratio of (4×19.80 polystyrene) vs (3×9.525 + 12.7 + 3.175 aluminum). Quick check: scint volume-thickness 79.20 mm × ρ 1.06 = 83.95; Al thickness 44.45 mm × ρ 2.70 = 120.0; total 203.96 → polystyrene fraction = 83.95/203.96 = 41.2%, Al = 58.8%. ✔️ Excellent, the mass fractions are correct.

So the physics checks out. The one thing I'd still flag: the comment's density value is a derived constant with no rounding note. Effective density = (203.96 mg/cm² over 123.65 mm) → 203.96/12.365 = 16.495 → then ×(123.65/107.775)… The final 1.892526 is precise to 7 figures; consider noting it's computed, not measured, so future editors don't try to "round" it. Minor.

Action for author/reviewer: the derivation is sound; just confirm crv_parameters.xlsx widths still match crv_counters_v10.txt (gapBetweenLayers = {9.525,9.525,9.525}, scintillatorBarThickness = 19.80, strongBackThickness = 12.7, aluminumSheetThickness = 3.175) — they do in the file I read, so this is consistent. ✔️


🟡 Issue 4 — The std::abs fix is correct, but the sign problem is a symptom worth a comment

      double wpos = 0.5*(flaypos+llaypos).Dot(wdir);
      double whw = 0.5*std::abs((llaypos-flaypos).Dot(wdir)) + firstbar.getHalfThickness();

The author found that U/D sectors produced negative whw (−34.0875) because (llaypos − flaypos)·wdir is negative when wdir points opposite to the layer-stacking order. std::abs is the right minimal fix and CI confirms it. Two observations:

  • wpos on the line above is computed from (flaypos+llaypos) and is unaffected — good, no sign issue there.
  • Consider a one-line comment explaining why the abs is needed (wdir orientation vs. layer ordering is sector-dependent), so it isn't "optimized away" by a future reader. The reviewer already praised this catch; a comment would lock in the rationale.

🟢 Issue 5 — Minor: stray inconsistency between PR description and code

The PR body says "Run 2 CRV sector IDs … from Mu2eG4/geom/crv_counters_v10.txt", and the header comment credits CosmicRayShieldMaker for the CRV_ prefix. That's accurate — sector names in the geom file are "R1", "T1", etc. (no prefix), and the surfaces are inserted via SurfaceId(sector.sname_).

⚠️ One thing to double-check: makeCRV() builds SurfaceId from shield.getName() (e.g. "R1"), not "CRV_R1". But the enum/name-map entries are "CRV_R1". If shield.getName() returns the bare "R1", then SurfaceId("R1") will fail findByName and throw, because the only registered names are "CRV_R1"-style. The header comment claims the names are "prefixed CRV_ by CosmicRayShieldMaker" — so this hinges entirely on whether CosmicRayShieldMaker actually stores names as "CRV_R1" or "R1". The geom file lists them unprefixed ("R1"). This is the single most important thing to verify before merge — if the shield names are bare (R1), the map insertion in makeCRV will throw at geometry construction for every Run 2 sector. CI's ceSimReco/cosmicSimReco passing is reassuring, but those may not exercise the KinKal CRV map build path.


3. Merge readiness

Status: Close, but confirm Issue 5 and Issue 2 first.

Check State
CI build + sim tests ✅ pass
Reviewer approval ✅ brownd1978 approved
Material physics ✅ verified self-consistent
std::abs fix ✅ correct, CI-confirmed
Sector-name prefix (R1 vs CRV_R1) ⚠️ must confirm — potential throw at geometry build
T1/T2 comment accuracy ⚠️ misleading ("CRV-Top" should be "CRV-TS")
enum/name-map sync safety 🟡 fragile, no compile-time guard

Critical before merge: verify that CosmicRayShieldMaker registers shield names as CRV_R1 (prefixed) and not the bare R1 from the geom file. If bare, the new IDs won't match the names used in makeCRV() and geometry construction will throw.


4. Possible improvements (not raised in the PR thread)

  1. Add a static assertion or comment coupling the enum count with names().size() to prevent silent enum/map drift (Issue 1).
  2. Fix the // CRV-Top (T1/T2 above) comment — T1/T2 are CRV-TS per the geom file (Issue 2).
  3. Add a one-line rationale comment on the std::abs (Issue 4).
  4. Note in MaterialsList.data that 1.892526 is a computed value so future editors don't round it.

…y CRV sector IDs, need for std::abs, and computed density value.
@RobMina

RobMina commented Jun 14, 2026

Copy link
Copy Markdown
Author

Issues raised above addressed:

  • static assertion added to check enum/map consistency
  • corrected CRV sector ID comment
  • comment about computed density value
  • comment about necessity of std::abs call
  • verified that CosmicRayShieldMaker prepends "CRV_":
    makeSingleSector(counterHalfLengths, isector, "CRV_"+_crvSectorNames[isector],

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