Repository navigation
High-KE capture proton trigger bands (apr_proton_p335/p250/p150) - #191
Conversation
… menus - apr_proton -> apr_proton_p335 (bit 270) so all three proton bands carry their momentum cut in the name; modules renamed to AprProtonP335PS/HSFilter/TriggerInfoMerger - add apr_proton_p335/p250/p150 (bits 270-272) to valMenu and mdsMenu with nested OnSpill prescales 25/400/20000 (~24 Hz total), disabled OffSpill - set useTrackerStatus in AprProtonP250HSFilter and AprProtonP150HSFilter (now a required HelixFilter parameter) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- drop maxMass 1200 from AprProtonHelixSelection: the bands now accept all charged capture particles (p, d, t, He3, alpha), separated offline. The cut removed only 1-3% of KE>60 protons but depends on the hit-time calibration. - valMenu/mdsMenu OnSpill prescales 25/400/20000 -> 100/800/40000 (~23 Hz) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #191
Reviewed at head 25c38f11c812c77fc272ed4de607216dae2b9859. First pass.
Decision
- 🟡 comment, with one S1 first. The PR changes the trigger paths that this repository's CI reference log records, but it does not update
ci/reference_log_file.log. The description plans that update for after the merge. Until it lands, thetriggerrow of Offline's FNALbuild report fails on every Offline PR (finding 1). That row is allowed to fail, so it blocks no merge and no job, which is why this is a comment and not a request for changes. Committing the new log here closes the gap, and I would approve with it. Everything else I checked matches the description: the names and bits, the cuts, the prescales, the nesting, and the generated menus. The new paths run in the phys, val and mds menus, and no other path changes its counts.
Scope understood
- Renames
apr_proton(bit 270) toapr_proton_p335, and addsapr_proton_p250(271) andapr_proton_p150(272). They are helix-momentum bands above 335, 250 and 150 MeV/c, for the spectra of charged particles from muon capture. They share one proton helix finder and run no track fit. - There is no particle-ID cut. The last commit,
25c38f1, removedmaxMass: 1200, so deuterons, tritons and heavier particles are kept and separated offline. - The paths are disabled off spill. On spill the prescales are 1/40/2000 in the phys menu and 100/800/40000 in the val and mds menus.
Findings
-
🟠 [S1]
ci/reference_log_file.logis not updated, so FNALbuild'striggerrow will fail on every Offline PR after the merge- Evidence: the reference log is unchanged at this head. It still has
apr_protonand its modules (ci/reference_log_file.log:443, 680, 1010-1025). The checker counts a path that is in only one of the two logs as a major change (ci/check_trigger_results.py:164-170), and then exits 2. - I ran
test/triggerTest.fclon the 20,000 events ofci/data_files.txt, thenci/check_trigger_results.pyagainst the reference log. Both workdirs use the Offline CI buildmain/7fb240fc_ec057b9e_7b20730e.- With this repository's
main, the checker exits 0 with "No problems detected". 5,642 events pass, as in the reference. - At this head it exits 2: "New trigger path apr_proton_p335 in local found, not in the reference!" (the same for
p250andp150), and "Trigger path apr_proton in reference not found in local!". - At this head, the other 51 paths match
maincount for count. Timing is within the limits (|time_new / time_ref − 1| = 0.089).
- With this repository's
- Impact: FNALbuild's
triggerjob runs this check on every Offline PR, using this repository'smain(codetoolsbin/github/jenkins_tests/mu2e-offline-build-test/build.sh:236-256).- Exit 2 marks the row ❌ "Return Code 2".
triggeris inFAIL_OK(job.sh:24), so the failure does not failmu2e/buildtestor block merges.- But the row stays red on every Offline PR until the log is updated, and a real trigger regression in that time would look the same. The last such gap, after Offline#2129, lasted 18 hours: from 02:37 to 20:38 UTC on 2026-11-10, when #190 updated the log.
- Suggested fix: produce the log at this head the way
ci/update_reference_log.py --localdoes, and commit it in this PR. That script opens its own PR frommain, so here commit the log it produces rather than letting it open the PR. The diff of the log would also show reviewers exactly which path counts this PR changes.
- Evidence: the reference log is unchanged at this head. It still has
-
🟡 [S2] The phys menu turns
apr_proton_p335on with no prescale, but the body does not give the phys-menu rate- On
main,apr_protonis disabled on spill (prescale -1 atdata/physMenu.json:262). At this head,apr_proton_p335has prescale 1 (data/physMenu.json:262). - The body gives the val/mds total, about 23 Hz. By the body's own table the phys total is about 1.5 kHz: 1.3 kHz from
p335alone, plus 7.3 kHz / 40 and 89 kHz / 2000. - On the CI sample, the three paths add 155 events to the phys menu's passed total: 5,642 on
mainand 5,797 here. The sample is a skim of pileup events, so this is not a rate. - Please state the phys-menu total in the body, and say whether the trigger group has agreed to it.
- On
Checked, no action needed
-
🟢 Names and bits. Bits 270, 271 and 272 are the same three paths in
physMenu.json,valMenu.jsonandmdsMenu.json. No menu has a duplicate bit or path name; I checked all five with a JSON parser.- At this head,
apr_protonand the old module names appear only inci/reference_log_file.log. - Offline
main(7fb240fc) and Productionmain(ec057b9e) contain neitherapr_protonnorAprProton(git grep). GitHub code search over the Mu2e organization findsapr_protononly in this repository.
- At this head,
-
🟢 Generated menus.
muse buildexits 0 atmainand at this head.fhicl-dumpexits 0 in both workdirs for all five test configs. It also exits 0 for the phys, val and mds menus in their on-spill, off-spill and combined forms (review-only copies oftriggerTest.fclpointed at each generated menu). -
🟢 Offline parameters, from Offline
main7fb240fc:useTrackerStatusis a requiredfhicl::Atom<bool>with no default (TrkFilters/src/HelixFilter_module.cc:243). All 17 helix filters incore/filters/trigAprFilters.fclset it.maxMassis an optional cut (HelixFilter_module.cc:68). NomaxMassorminMasssetting remains in the PR's files. The word appears only in a comment (core/filters/trigAprFilters.fcl:784), andfhicl-getfinds neither key in any of the six selections.- The helix finder's defaults are 400 and 140 MeV/c (
CalPatRec/fcl/prolog.fcl:556,558). The new limits, 800 and 300 MeV/c (core/producers/trigAprProducers.fcl:36-37), apply only toTrigAprProtonHelixFinder. The electron finderTrigAprHelixFinderkeeps 400 and 140.
-
🟢 Final configuration, read with
fhicl-getfrom the generated configs:- Both helicity selections of all three filters have |λ| from 40 to 2000 mm and d0 from −600 to 600 mm.
- The momentum thresholds are 335, 250 and 150 MeV/c.
- The minimum straw-hit counts are 20, 20 and 15.
- The cuts inherited from
AprHelixSelections_posare the same as onmain: χ²(XY) and χ²(φZ) at most 8, hit ratio at least 0.4, at most 30 loops, and momentum at most 2000 MeV/c. - Off spill, the prescale is -1 for all three paths in all three menus.
doHelicityCheckchanges from true to false, which the body does not mention. It has no effect:HelixFilter::checkHelixFromHelicityalready sends each helix to the selection for its own helicity (HelixFilter_module.cc:318-327), so the check at:131cannot fail.
-
🟢 Nesting.
PrescaleEventkeeps an event whene.event() % prescale == 0(Trigger/src/PrescaleEvent_module.cc:106). A prescale of -1 becomes 0, which rejects every event (:50,:106).- In the val and mds menus, 800 = 8 × 100 and 40000 = 50 × 800. In the phys menu, 2000 = 50 × 40, and
p335has prescale 1. - So an event that passes a lower band's prescale also passes the higher bands' prescales, and their helix filters run on it.
- In the val and mds menus, 800 = 8 × 100 and 40000 = 50 × 800. In the phys menu, 2000 = 50 × 40, and
-
🟢 Jobs on the 20,000 CI events (
skim_NoPrimary1BB_1a/1b.art, pileup only):path filters pass, prescale set to 1 phys menu: prescale pass / path pass val menu: prescale pass / path pass apr_proton_p335249 20,000 / 249 200 / 1 apr_proton_p2501,422 500 / 33 25 / 3 apr_proton_p15012,770 10 / 8 0 / 0 - The time-cluster filter passes 17,748 of the 20,000 events when every prescale is 1.
- In the val run, all 43 other paths have the same counts as
main. The phys comparison is under finding 1. - The proton helix finder costs 0.20 ms per event. The full-event average is 27.0 ms at this head and 27.1 ms on
main, with both runs at the same time on the same machine.
Not a finding: test/triggerMDSTest.fcl already fails on main
- Confirmed. At
mainand at this head it exits 9 at configuration: "Unknown path calo_MVANNCE has been specified in 'trigger_paths'".data/mdsMenu.json:274onmainnamescalo_MVANNCE, while the path iscalo_NNCE(core/trigSequences.fcl:257). - To check that this PR adds no new failure behind that error, I made a review-only copy of the test that defines
calo_MVANNCEascalo_NNCE. With it, both versions run 2,000 events with exit 0, and 4 events pass. The other 51 paths match, and the three new paths run. - Open #188 by the same author deletes the
calo_MVANNCEentry, which fixes this.
Merge order (not a finding)
git merge-treemerges this head cleanly with open #186, #188 and #189. With #188, the merged mds menu has 53 paths and no duplicate bits.
Validation check
- Build/tests run: yes.
- Two muse workdirs backed by the Offline CI build
main/7fb240fc_ec057b9e_7b20730e(Offlinemain7fb240fc, Productionmainec057b9e, this repository'smain7b20730e). One has this repository atmain, the other at this head. Both build with exit 0. - Jobs:
triggerTest.fclon all 20,000 CI events in both workdirs, followed byci/check_trigger_results.py; the val menu on all 20,000 events in both; the three proton paths with prescale 1 on all 20,000 events; andtriggerMDSTest.fcland the patched copy in both. ci/check_trigger_results.shitself was not run. I ran its two steps,mu2eand the Python checker, separately.
- Two muse workdirs backed by the Offline CI build
- CI at this head: the five GitHub checks pass. They are whitespace, format, two single-package builds and a build against develop, completed 21:36-21:43 UTC. There are no commit statuses. None of these runs a
mu2ejob, and this repository defines no tests formake test. The trigger comparison runs only in Offline's FNALbuild, against this repository'smain, so it cannot see this PR before the merge. - Config contract check: pass.
- Cross-repo consistency: pass. Nothing in Offline or Production uses the old or new names.
Residual risk
- I did not reproduce the efficiencies or rates in the body, and I did not run a signal sample.
- The CI sample is a skim, so its pass fractions are not rates. As a loose cross-check, its ratio of
p250top335before the prescale is 5.7 (1,422 / 249), against 5.6 (7.3 / 1.3) in the body. Forp150top335it is 51, against 68 in the body. - I did not run a raw-data job.
Author follow-ups
- Commit the updated
ci/reference_log_file.log, produced at this head (finding 1). - State the phys-menu total rate of the three bands in the body, and whether the trigger group has agreed to it (finding 2).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Responding to the AI comments:
|
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #191
Reviewed at head 60c82a990ac472bb2efda740dffe18c5edfefce7. Re-review of 25c38f1.
Decision
- 🟢 approve. Thanks for the answers; both findings are settled. You keep the reference log out of this PR on purpose, and the phys menu now follows the convention that physics paths run unprescaled in simulation. I checked that this does not grow Production's on-spill digitization output.
What changed since 25c38f1
- One commit,
60c82a9"physMenu: run the proton bands unprescaled" (data/physMenu.json, +2 −2). It sets the on-spill prescale ofapr_proton_p250from 40 to 1 and ofapr_proton_p150from 2000 to 1.apr_proton_p335was already 1. Not a rebase.
Prior findings (from my review at 25c38f11)
- 🟢 [was 🟠 S1]
ci/reference_log_file.logis not updated. Answered, not changed, and I accept the reason: you regenerate the log after merges, because doing it per PR costs a full run and conflicts with every PR merged before it.- After the merge, Offline FNALbuild's
triggerrow will show "Return Code 2" until your update lands. It is inFAIL_OK, so it blocks nothing, and I will read it as expected in that window.
- After the merge, Offline FNALbuild's
- 🟢 [was 🟡 S2] The phys-menu rate was not stated. Addressed differently: the three bands are now unprescaled in the phys menu, as you explain.
- This matches the rest of the menu. At this head, 48 of the 54 phys paths have on-spill prescale 1. Only
minBias_SDCount,minBias_CDCountandcalo_N0Sourceare prescaled (10000), and 3 are off. - It does not grow Production's on-spill digitization output.
JobConfig/digitize/OnSpill.fcl:11writes only events fromDigitize.SignalTriggers(cpr_Trk*_80*,apr_Trk*_80*,prolog.fcl:235) and the triggerable paths, and noapr_proton_*name matches either. - The shared proton helix finder already ran on every event at
25c38f11, becauseapr_proton_p335had prescale 1 there. So the commit adds only the twoHelixFilterdecisions per event.
- This matches the rest of the menu. At this head, 48 of the 54 phys paths have on-spill prescale 1. Only
Validation check
- Build/tests run: a muse workdir at this head on the Offline CI build
main/7fb240fc_ec057b9e_7b20730e.muse buildexits 0.fhicl-dumpexits 0 for all 16 configs from the first pass: the five test configs, the nine menu wrappers, and the two job wrappers.- Against the same dumps at
25c38f11, only two lines differ:AprProtonP250PSandAprProtonP150PSprescalebecome 1. They show up inmenu_physMenu_OnSpillandtriggerTest, which uses the phys menu. - The val and mds menus and the phys off-spill menu are identical.
- Against the same dumps at
- No jobs were rerun. The commit changes only two prescale values, and my first-pass run with the three bands at prescale 1 already covers this setting (249, 1,422 and 12,770 of the 20,000 CI events).
- The five GitHub checks pass at this head (checked 01:20 UTC).
- Config contract check: pass.
- Cross-repo consistency: pass. Nothing in Offline or Production names the old or new paths.
Residual risk
- The efficiencies and rates in the description were not reproduced.
test/triggerMDSTest.fclstill fails onmainand here (calo_MVANNCE); #188 fixes that.
Prescaled momentum bands for measuring the energy spectra of charged particles emitted after muon capture (protons, deuterons, tritons, He-3, alpha). The three paths share one helix finder for these particles and run no track fit. There is no particle-ID cut in the trigger: species are separated offline.
Paths
apr_proton_p335(wasapr_proton)apr_proton_p250apr_proton_p150AprProtonHelixSelection): nsh ≥ 20 (15 for p150), 40 < |λ| < 2000 mm, |d0| < 600 mm.maxAbsLambda: 1000removed most of the signal, because these protons are very forward (|λ| ≈ 1000 mm).HelixFilter maxMass) was tried. It removes only 1–3% of KE > 60 MeV protons, but it depends on the hit-time calibration, and it removes the deuterons and tritons, whose spectra are also of interest.TrigAprProtonHelixFinderraisesmaxHelixMomentum/maxHelixPerpMomentumto 800 / 300 MeV/c, so deuterons and tritons are not cut by the finder.PrescaleEventkeeps events withevent % PS == 0, so every event a band keeps also carries the higher bands' filter decisions, and their turn-ons can be measured from data. The total is about 23 Hz with the simulation below.Expected performance
These numbers are from simulation: MDC2025 capture-particle samples without pileup mixing, and NoPrimaryMix1BB MDC2025au for the rates.
Tests
fhicl-dumpoftest/triggerTest.fclandtest/triggerMDSTest.fclwith the regenerated menus.mu2eon 100 signal events with the phys and val menus: all three paths run and are prescaled as configured.triggerMDSTest.fclfails at configuration on main as well:mdsMenu.jsonlistscalo_MVANNCE, but that path is now calledcalo_NNCE. This PR doesn't touch it.apr_proton*paths, and will need updating after merge.🤖 Generated with Claude Code