Updates for ensembling - #567
Conversation
|
Hi @sophiemiddleton,
which require these tests: build. @Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main. ⌛ The following tests have been triggered for 274b68d: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 274b68d.
N.B. These results were obtained from a build of this Pull Request at 274b68d after being merged into the base branch at 5aa34ef. For more information, please check the job page here. |
michaelmackenzie
left a comment
There was a problem hiding this comment.
Overall I think this looks great, thank you for making these updates! My only request/comment is to ensure the RMC normalization is valid for arbitrary values of emin (which may already be true).
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #567
Reviewed at head 274b68d8c205c90201c45c9381bab6b67eb0226b. First pass; @michaelmackenzie's CHANGES_REQUESTED at this same head is carried forward below.
Decision
- 🟡 comment only
Both S1s are in the normalization arithmetic rather than the plumbing, and
neither fires at the committed defaults (Stage1 sets both thresholds to 80 MeV) —
hence comment rather than block. @michaelmackenzie's request is still open, and
his instinct was right; the numbers are below.
Scope understood
- Adds RMC 0-nucleon and 1-nucleon knockout normalizations (
rmc_0n_normalization,
rmc_1n_normalization) plus Plestid spectrum helpers, and wires them through
calculateEvents.py,make_template_fcl.pyand Stages 1/2/3. - The ensemble's plain
RMCInternal/RMCExternalentries are replaced by the
0N/1N pair, not supplemented —BR_0N_FRAC + BR_1N_FRAC = 0.099 + 0.901 = 1.0,
so this is the same RMC rate decomposed by knockout mode, and no background is
dropped. Worth one line in the description, since "adds support" reads as
additive. - Also carries an unrelated
DBVERSIONparsing fix inStage5_signal.shand a
test-livetime change.
Carried forward from @michaelmackenzie's review (state: CHANGES_REQUESTED, no commits since)
- "ensure the RMC normalization is valid for arbitrary values of emin" and the
two inline comments onnormalizations.py:605/:665— UNADDRESSED, and
finding 1 below gives the exact failure. - "Why both RMCN0External and RMCPhaseSpace0NExternal?" — UNADDRESSED in
code; the author has agreed to unify. See finding 5. - "This assumes the internal and external photon energies have the same cuts" —
CLEARED by the author's answer that it is intentional.
Findings
-
🟠 [S1] For
e_min ≤ 75the spectrum factor is an absolute fraction where a
relative one is required, under-predicting by 4.4× (0N) and 16× (1N)-
Evidence:
normalizations.py:601-606. The prefactor
RMC_BR_MUON_CAPTURE * RMC_RATE_GT_57 * RMC_BR_0N_FRAC_GT_57is already
BR(0N, E > 57)per stopped muon, soR_spectrumhas to be
R(>e_min)/R(>57). Thee_min > 75branch computes exactly that —
FRAC_0N_80 / FRAC_0N_57 = 0.03319/0.22887 = 0.14501. The default branch
instead usesFRAC_0N_57 = 0.22887, whose denominator is the whole
spectrum, applying a spectrum cut that the prefactor already applied. At
e_min = 57the correct factor is1.0, so the result is low by
1/0.22887 = 4.37×; for 1N, by1/0.061620 = 16.2×. -
Impact: any
--rmcn0emin/--rmcn1eminat or below 75 MeV silently yields a
too-small RMC background. Stage1 hardcodes 80, so today's default path takes
the correct branch — this is a latent wrong-physics path, not a live one.
The 75.0 cut is itself discontinuous: 74 MeV gets the E>57 normalization and
76 MeV gets the E>80 one, with nothing in between. -
Suggested fix: the machinery to do this exactly is already in the PR, unused.
plestid_integralis the correctly normalized CDF (I verified its
antiderivative integrates theplestid_spectrumshape to 1 over[0, kmax]),
so the whole branch collapses toR_spectrum = (plestid_integral(e_min, kmax, kmax, knockout) / plestid_integral(57.0, kmax, kmax, knockout))
and it then holds for arbitrary
e_min, which is precisely what
@michaelmackenzie asked for. The twokmaxvalues are recoverable from your
own constants — see finding 3.
-
-
🟠 [S1] Three constants are re-declared locally, and one disagrees with the
cited shared value- Evidence:
normalizations.py:62-70againstconstants.py, which this same
module already imports at line 14:RMC_RATE_GT_57 = 1.41e-5vsRMC_GT_57_PER_CAPTURE = 1.43e-5
— different values for the same quantity, and the shared one carries a
source (Phys. Rev. C 59, 2853 (1999)) while the new one carries none.RMC_BR_MUON_CAPTURE = 0.609duplicatesCAPTURES_PER_STOPPED_MUON = 0.609.RMC_INTERNAL_EXTERNAL_RATIO = 0.0069duplicatesINTERNAL_PER_RMC = 0.00690.
- Impact:
rmc_normalization(line 490-495) uses the imported constants while
rmc_0n_normalization/rmc_1n_normalizationuse the local copies, so the
old and new RMC paths now disagree by 1.4% on the same rate. Whichever value
is right, two homes for it means a future correction reaches only one. - Suggested fix: delete all three locals and use the imported names. If
1.41e-5is a deliberate update, change it inconstants.pywith its source
and letrmc_normalizationmove with it.
- Evidence:
-
🟡 [S2]
plestid_integralandplestid_spectrumare added but never called- Evidence:
normalizations.py:517-575, 59 lines; the only other occurrences
of "Plestid" in the file are docstring prose in the two normalization
functions, which use the hardcoded constants instead. - Worth keeping rather than deleting, because the four hardcoded fractions
are this function: solvingplestid_integral(e, kmax, kmax, knockout)for
thekmaxthat reproduces each one gives 101.866 MeV fromFRAC_0N_57
and 101.865 MeV fromFRAC_0N_80, and 95.449 MeV from both 1N
values — self-consistent to 0.001 MeV within each mode, and physically
sensible (1N is lower by roughly the neutron separation energy). Those two
numbers are the missing input; name them, and finding 1's fix is a two-line
change. Note neither is theRMC_kmax=90.1that Stage1 still writes and
Stage2 still passes as--rmckmax.
- Evidence:
-
🟡 [S2] The four new tests assert only
> 0, and CI does not run them- Evidence:
test_normalizations.py:204-275— each new test ends in
assertGreater(..., 0). A 4× or 16× normalization error passes. FNALbuild's
table for build 658 lists 16 jobs (build, ceSimReco, ceMix, rootOverlaps,
g4surfaceCheck, …) and no python test target, so nothing runs this file
automatically either. - Suggested fix: pin one expected yield per mode to a few significant figures.
Given finding 1, a second case ate_min = 57would have caught it.
- Evidence:
-
🟡 [S2]
on_spill_timeis scaled by an uncited88/496, identically for 1BB and 2BB- Evidence:
test_normalizations.py:28-29—4.4e6 * (88/496)= 0.1774, applied
to both modes.constants.pygivesONEBB_DF = 0.323andTWOBB_DF = 0.246;
0.1774 is neither, and the two modes have different duty factors, so one
shared literal cannot be right for both. - Suggested fix: use the named duty-factor constants, or comment where 88/496
comes from.
- Evidence:
-
🟡 [S2]
Stage2_build_sampler.sh: one dataset is validated on disk and then
listed from tape, and the new file-list check only warns- Evidence: line 214 validates every dataset with
mu2eDatasetFileList "$dataset_name" --disk, but the RPCInternal list at
line 287 is built with--tape— alone among the six list-building calls,
the other five pass no flag. Separately,check_file_lists(line 265, used
at 301 and 363) prints
⚠️ WARNING: Some file lists may be empty or incompleteand execution
continues; an empty list surfaces much later as anIndexErrorat
make_template_fcl.py:201or aZeroDivisionErrorat line 164. - Suggested fix: drop the stray
--tape, and make the file-list checkexit 1.
A check that warns and proceeds reads like a gate but is not one.
- Evidence: line 214 validates every dataset with
-
⚪ [S3] Three small dead/contradictory items
e_threshold = 57.0is assigned and never read, in both new functions
(normalizations.py:600,:660).- The alias assignments
norms["RMCN0External"] = norms["RMCPhaseSpace0NExternal"] = ...
(make_template_fcl.py:63-73) only ever populate keys nothing reads — the
consumer loop isfor signal in args.prc(line 85) and Stage2 passes only the
RMCPhaseSpace*names. This is the code side of @michaelmackenzie's
"why both names?"; dropping the short aliases resolves both. Stage1_initiate_ensemble.sh:24-25—INCLUDE_RMCN0=1 # ... (default: no).
The comment contradicts the value; both modes are on by default, so
--rmcn0/--rmcn1are opt-out, not opt-in.
Verified, no action needed
- 🟢 The
Stage5_signal.shDBVERSIONchange is a real fix, not a rewrite. The
old$(IFS='_'; echo "${PARTS[@]:2}")does not join onIFS— only"${arr[*]}"
does,"${arr[@]}"expands to separate words thatechojoins with spaces. Run
onMDC2025_best_v1_3it returnsv1 3; the new explicit loop returnsv1_3. - 🟢
make_template_fcl.py:52,55switchingargs.tmin→tminis safe and is
itself a fix:tminis defined at line 26 asfloat(args.tmin), so
rpc_normalizationnow receives a float rather than the raw string. - 🟢 CI is green at this head — FNALbuild build 658, all 16 rows ✅, including
rootOverlaps,g4surfaceCheck, clang-tidy (0/0) and FIXME/TODO (0/0).
Validation check
- Build/tests run: CI green at
274b68d8(build 658). The new python tests are
not part of that suite; I did not execute them. My own checks were arithmetic
on the constants and a bash reproduction of theDBVERSIONparse. - Config contract check: partial. The Stage1 → Stage2 variable handoff
(RMC_N0_emin/RMC_N1_emingating dataset lists,--prc, and sampling
options) is internally consistent; I did not run the stages end to end. - Cross-repo consistency: n/a — no Offline or trig-config interface is touched.
Residual risk
- I did not verify that the datasets the new Stage2 branches expect
(dts.mu2e.RMCPhaseSpace{0,1}N{Internal,External}.MDC2025{au,at}.art) exist with
enough files, nor thatRMCVERSIONINT=au/RMCVERSIONEXT=atare the intended
versions — that split is asymmetric and unexplained in the diff. - The physics inputs themselves (0.099/0.901 knockout split, and whether the
Plestid phase-space shape is the right model here) are outside what I can check;
I verified only internal consistency and the arithmetic.
Author follow-ups
- Fix the
e_min ≤ 75branch (finding 1) — usingplestid_integralanswers
@michaelmackenzie's arbitrary-e_minrequest in the same change. The twokmax
values you need are 101.87 MeV (0N) and 95.45 MeV (1N), recovered from your own
constants. - Reconcile
1.41e-5against the citedRMC_GT_57_PER_CAPTURE = 1.43e-5and drop
the three duplicate constants (finding 2). - Pin at least one expected yield in the new tests, and add an
e_min = 57case
(finding 4). - Drop the stray
--tapeand make the file-list check exit non-zero (finding 6).
|
I have now pushed updates and fixes to your responses |
| # Compute ratios: integral above threshold / integral from 0 to K_max | ||
| RMC_SPECTRUM_FRAC_0N_57 = frac_57_0n / frac_0_0n if frac_0_0n != 0 else 0.0 | ||
| RMC_SPECTRUM_FRAC_1N_57 = frac_57_1n / frac_0_1n if frac_0_1n != 0 else 0.0 | ||
| RMC_SPECTRUM_FRAC_0N_80 = frac_80_0n / frac_0_0n if frac_0_0n != 0 else 0.0 |
There was a problem hiding this comment.
I don't think the 80 MeV values are needed here, if emin is an input then in the RMC normalization it should evaluate the fraction above the input emin using the integral function directly
|
I think this is now ready for a merge. |
michaelmackenzie
left a comment
There was a problem hiding this comment.
This looks good to me. I just had a couple of comments about no longer used code, but these are just comments and not required
| # Note: These are the RAW spectrum fractions (not pre-multiplied by RMC_BR_MUON_CAPTURE) | ||
| RMC_SPECTRUM_FRAC_0N_57 = 0.22887 # Fraction of 0-knockout spectrum above 57 MeV | ||
| RMC_SPECTRUM_FRAC_1N_57 = 0.061620 # Fraction of 1-knockout spectrum above 57 MeV | ||
| RMC_SPECTRUM_FRAC_0N_80 = 0.03319 # Fraction of 0-knockout spectrum above 80 MeV |
There was a problem hiding this comment.
I don't think these 80 MeV versions are used anymore
|
|
||
| These fractions represent: R(* knockout | E > threshold) / R(* knockout) | ||
| """ | ||
| global RMC_SPECTRUM_FRAC_0N_57, RMC_SPECTRUM_FRAC_1N_57 |
There was a problem hiding this comment.
Are these values used anywhere? Is this function still needed?
There was a problem hiding this comment.
thanks, I thnk I was using these for validation
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #567
Reviewed at head 3c13c2828abb36da5f001b38079dc3568b55b305. Re-review of 274b68d8.
Decision
- 🔴 request changes
The RMC work is now right, and I verified it numerically rather than taking the
commit message for it — the arbitrary-e_min fix @michaelmackenzie asked for is
in, and it reproduces the production default to better than 0.02%. What blocks is
something else the delta carries: one line was deleted from rpc_normalization,
which multiplies the RPC background by 6.2 and is mentioned nowhere in the PR
description, the commit messages, or any review thread.
I note @michaelmackenzie approved at this head on 2026-08-20 with comments he
explicitly called non-blocking. His review was of the RMC normalization, and I
agree with it; finding 1 below is in a different function that the RMC discussion
never touched.
Scope of this pass
274b68d8..3c13c282, four commits ("fixed norms" ×2, "made emin an input arf"
×2), acrossnormalizations.py(+140/−89),calculateEvents.py(−16) and a new
test_rmc_fractions.py(+76).
Findings
-
🔴 [S0]
filter_efficiencywas deleted from the RPC normalization, raising the RPC yield by a factor of 6.2, in a PR about RMC.- Evidence:
normalizations.py:535still computes
filter_efficiency = float(num_pion_filters) / float(num_pion_stops), but the
product at:538-544no longer contains it. Onmainthe same product does
—normalizations.py:401there is thefilter_efficiency *line this delta
removes. The variable is now assigned and never read. - Impact: with the values the file's own comments record for the table it reads
(num_pion_stops = 41324703at:131,num_pion_filters = 6634478at
:133),filter_efficiency = 0.1605, so every RPC yield this module produces
— internal and external, both BB modes — comes out 6.23× larger than
before.rpc_normalizationis live:make_template_fcl.pycalls it for the
ensemble. Nothing in the PR description, the four commit messages, or any
review thread mentions RPC. - Suggested fix: if this is deliberate — say, because the filter efficiency is
already folded intotarget_stopped_pions_per_potand was double-counted —
then state it in the description, delete the now-deadfilter_efficiency
line, and split it into its own PR so the RPC change is reviewed as an RPC
change. If it is not deliberate, restore the line. The dead variable left
behind is what makes me think it is the second.
- Evidence:
-
🟠 [S1] Carried over: three constants are still re-declared locally, and one still disagrees with the cited shared value.
normalizations.py:62-63still hasRMC_BR_MUON_CAPTURE = 0.609and
RMC_RATE_GT_57 = 1.41e-5, againstconstants.py:9
CAPTURES_PER_STOPPED_MUON = 0.609andconstants.py:11
RMC_GT_57_PER_CAPTURE = 1.43e-5 # Source: Phys. Rev. C 59, 2853 (1999);
RMC_INTERNAL_EXTERNAL_RATIO = 0.0069still duplicates
constants.py:22 INTERNAL_PER_RMC = 0.00690. The oldrmc_normalizationuses
the imported names and the new 0N/1N functions use the locals, so the two RMC
paths still disagree by 1.4% on the same rate, and only one of the two values
carries a source. Delete the locals, or move1.41e-5intoconstants.pywith
its citation. -
🟡 [S2] The plain-RMC branch in
calculateEvents.pynow computes a yield and prints nothing, and its external twin was deleted.
calculateEvents.py:41-42isif(args.prc == "RMC" and int(args.internal) == 1): Yield = rmc_normalization(...)with noprint, where every other branch in the
function prints a<Name>_yield=line for the shell to parse; the
int(args.internal) == 0branch is gone entirely. The only remaining caller is
scripts/old/Stage1_makeinputs.sh:110,112, which would now append nothing to
${TAG}.txtinstead of failing. Either finish the removal — drop both branches,
the way theRMCN0*/RMCN1*aliases were dropped — or restore the print. -
🟡 [S2]
test_rmc_fractions.pyis not a test and duplicates the function it checks.
The new 76-line file re-declaresplestid_integralverbatim at:11-33rather
than importing it fromnormalizations.py, so it validates its own copy and
will silently stop tracking the real one. It contains no assertion — it computes
four numbers, prints a comparison table and exits 0 regardless — while the
test_prefix means unittest and pytest discovery will collect it and record a
pass. Importplestid_integraland assert the four fractions against the
literature values, or move it out oftest_*and call it a script. -
🟡 [S2] The four
RMC_SPECTRUM_FRAC_*constants and the function that computes them are now entirely dead — this is @michaelmackenzie's:79and:222comments, which he marked non-blocking; I am recording the fuller extent rather than raising the severity.
The literals at:77-80are overwritten at import by
compute_rmc_spectrum_fractions()(called at:260), and nothing then reads
any of the four:rmc_0n_normalizationandrmc_1n_normalizationgo through
rmc_spectrum_fraction()instead, andgit grep RMC_SPECTRUM_FRACacross
JobConfig/ensemblefinds no other consumer thantest_rmc_fractions.py, which
defines its own. So it is not only the 80 MeV pair — all four, plus the
compute_rmc_spectrum_fractionsfunction and itsglobalstatements, can go.
Prior findings
-
- 🟠 [was S1] The
e_min ≤ 75branch — FIXED ind61e3be, verified. Both
functions now callrmc_spectrum_fraction(e_min, RMC_KMAX_*, knockout=*), which
isplestid_integral(e_min)/plestid_integral(57)— the relative form the
prefactor requires. I checked the arithmetic rather than the diff: it returns
exactly1.000000ate_min = 57for both modes,0.0abovek_max, and there
is no discontinuity left at 75 MeV. See the verified section for the agreement at
the production default.
- 🟠 [was S1] The
-
- 🟠 [was S1] Duplicate constants — UNADDRESSED, carried as finding 2.
-
- 🟡 [was S2]
plestid_integraladded but never called — FIXED. It is now
the sole source of the spectrum fractions, and the twok_maxvalues in
:68-69(101.8667 and 95.4489 MeV) are the ones I recovered from the hardcoded
constants last pass, to within the precision I quoted.
- 🟡 [was S2]
-
- 🟡 [was S2] The four new tests in
test_normalizations.pyassert only> 0
— UNADDRESSED; that file is untouched by the delta. Finding 4 above is a
second instance of the same pattern.
- 🟡 [was S2] The four new tests in
-
- 🟡 [was S2]
on_spill_timescaled by an uncited88/496— UNADDRESSED.
- 🟡 [was S2]
-
- 🟡 [was S2]
Stage2_build_sampler.sh: the stray--tapeon the RPCInternal
list, andcheck_file_listswarning instead of exiting — UNADDRESSED;
Stage2_build_sampler.shis untouched by the delta.
- 🟡 [was S2]
-
- ⚪ [was S3] Of the three small items: the dead
e_threshold = 57.0in both
new functions is FIXED (removed with the branch). The
make_template_fcl.py:63-73aliases and the contradictory
Stage1_initiate_ensemble.sh:24-25comment are UNADDRESSED, both files being
untouched — though thecalculateEvents.pyside of the alias question is now
resolved, which was @michaelmackenzie's point.
- ⚪ [was S3] Of the three small items: the dead
Verified, no action needed
- 🟢 The new spectrum fraction reproduces the old correct branch at the production
threshold, so this fix does not move any yield anyone has already computed. At
e_min = 80,plestid_integral(80)/plestid_integral(57)gives0.145039for
0N against the old0.03319/0.22887 = 0.145017— a relative difference of
1.5e-4— and0.021381for 1N against0.0013175/0.061620 = 0.021381, to
1.7e-5. - 🟢 The two
k_maxvalues are consistent with all four literature fractions, not
just the two the fix uses: computingplestid_integral(E, k_max, k_max, n) / plestid_integral(0, k_max, k_max, n)gives 0.228873 vs 0.22887, 0.061619 vs
0.061620, 0.033195 vs 0.03319 and 0.0013175 vs 0.0013175 — the largest
disagreement being 0.016%. - 🟢
rmc_spectrum_fractionis well behaved outside the range that mattered
before:e_minabovek_maxreturns0.0rather than a negative or a divide
error, ande_min = 0returns4.369 = 1/0.22887, which is the right answer for
a quantity defined relative to E > 57.
Validation check
- Build/tests run: none complete at this head.
mu2e/buildtesthas been
pendingat3c13c282since 2026-08-20T15:52:51Z and FNALbuild has posted no
table for it; the green table on this PR is from build 658 at274b68d8. I did
not trigger another, since one is already queued — and it would not settle
anything here, because the Production job list contains no python test target, so
none of these files run in CI either way. My own checks were the arithmetic
above andgit grepfor the dead constants. - Config contract check: partial, unchanged from the last pass.
- Cross-repo consistency: n/a — no Offline or trig-config interface is touched.
Residual risk
- Whether removing
filter_efficiencyis physically correct is not something I can
judge; I can only say it is a 6.2× change that is not described anywhere. - The physics inputs themselves — the 0.099/0.901 knockout split, and the two
k_maxendpoints as values for Al-27 — remain outside what I can check. I
verified internal consistency, not the literature.
Author follow-ups
- Restore
filter_efficiencyinrpc_normalization, or state the reason for
removing it and split it out (finding 1). This is the one item I would hold the
merge for. - Reconcile
1.41e-5againstRMC_GT_57_PER_CAPTURE = 1.43e-5and drop the three
duplicate constants (finding 2, carried). - Finish or revert the plain-RMC branch removal in
calculateEvents.py
(finding 3). - Make
test_rmc_fractions.pyimportplestid_integraland assert, or rename it
(finding 4). - Delete the four dead
RMC_SPECTRUM_FRAC_*constants and
compute_rmc_spectrum_fractions(finding 5).
|
Responses: Author follow-ups 1 Restore filter_efficiency in rpc_normalization, or state the reason for |
This pull request adds support for Radiative Muon Capture (RMC) 0-nucleon (0N) and 1-nucleon (1N) knockout processes to the normalization and event calculation framework. It introduces new normalization functions, updates the event calculation and template generation scripts to handle these processes, and adds corresponding tests. There are also minor updates to configuration and test files to support these changes.
Normalization and Physics Calculation Enhancements:
rmc_0n_normalization,rmc_1n_normalization, and supporting Plestid spectrum utilities) innormalizations.py[1] [2].calculateEvents.pyto support new RMC 0N and 1N processes, including both internal and external conversion options [1] [2] [3].Template Generation and Configuration:
make_template_fcl.pyto allow selective calculation of normalization for new RMC 0N/1N processes, with argument parsing and logic for these processes [1] [2].mu2e::SpectrumConfigproduct, keeping output files clean.Testing Improvements:
test_normalizations.py[1] [2] [3] [4] [5].These changes collectively enable precise modeling and normalization of RMC 0N and 1N backgrounds, improving the physics reach and flexibility of the event calculation framework.