Repository navigation
AgnosticHelixFinder: best-of-K candidates, 2π alias test, target-constrained refit - #2038
michaelmackenzie wants to merge 6 commits into
Conversation
The hit ratio (measured straw hits / expected plane-face crossings) counted every face the helix crosses inside the tracker active radius as an expected hit, so dead panels and straws (TrackerStatus noSignal/suppress) biased it low and made minHitRatio cuts depend on the tracker conditions. - HelixTool takes an optional StrawNoSignal function. With it, each face crossing is weighted by the live fraction of the nearest straw in each layer at the crossing point. A function is used instead of TrackerStatus to avoid a Mu2eUtilities -> TrackerConditions library dependency. - The hit ratio is evaluated lazily, on the first hitRatio() call; the filters apply it as the last cut, and skip it when minHitRatio <= 0. - HelixFilter and MultiHelixFilter: new useTrackerStatus parameter (default true) passes TrackerStatus noSignal || suppress; false keeps the previous behavior. ReadTriggerInfo uses the same definition. - Removed the unused HelixTool::nMinHitsLoop() (never set) and _trackerLength; nstrawhits() returns int. With the Sim_commissioning reduced-tracker conditions (31% of straws without signal) the median CE helix hit ratio goes from 0.56 to 1.0, so minHitRatio 0.4 keeps its meaning. With useTrackerStatus false the trigger results are identical to before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…strained refit Helix finding options for trackers with fewer hits per track (e.g. the commissioning tracker, where a CE crosses the active straws once per turn and its hits come in short groups ~2 pi lambda apart). All default to the previous behavior. - maxHelixCandidates (1), acceptHelixStrawHits (0): keep the best (most straw hits, then the lowest circle + line chi2/dof) of up to K viable candidates per search instead of the first; a candidate with >= acceptHelixStrawHits straw hits is saved at once. - minAliasGap (0 = off): refit the phi-z line with the hits beyond the largest z gap of the helix hits shifted by -+2pi; keep the lowest chi2/dof within the momentum range, then recover points again. - targetRefitSigma (0 = off), targetRefitPhiWindow (0.5 rad), targetRefitDphiDzBins (100): if the best of the K candidates has |d0| > targetRefitSigma, refit its circle through the stopping target (error targetRefitSigma) and find the phi-z line with a dphi/dz scan on that circle; the refit replaces the candidate if it has at least as many straw hits. It addresses the half-radius / half-lambda solutions that fit the hits of short arcs. With the Sim_commissioning tracker conditions (and tuned trigger cuts), K = 3, acceptHelixStrawHits 20, minAliasGap 100 and targetRefitSigma 100 raise the apr_TrkDe_75m70p_D0200 efficiency for CeM from 52.9% to 57.5% (K + alias) and 61.9% (with looser phi-z cuts and the target refit), CeP 28.5% -> 39.1%, with no increase of the NoPrimary D0200 rate; nominal tracker +1.5%. APR finder time 0.036 -> 0.067 ms/event. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ticle Per-event TTree comparing a helix seed collection (e.g. the trigger APR helices) with the MC truth of the main signal particle (the highest-momentum e+- or mu+- with >= minHits straw hits): its momentum, direction, circle and lambda at its first straw gas step, its combo-hit z span and gaps, and for each helix its parameters, straw hits and signal straw hits, circle chi2 with the reco and the true circle, plus the signal hits themselves. Optionally records the decisions of trigger paths of the current process (triggerPaths), so trigger efficiencies can be split by the signal truth (particle type, direction). Used to study the APR helix finding with the commissioning tracker conditions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Hi @michaelmackenzie,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. ⌛ The following tests have been triggered for 3aa9fd8: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 3aa9fd8.
N.B. These results were obtained from a build of this Pull Request at 3aa9fd8 after being merged into the base branch at fd9bfbd. For more information, please check the job page here. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #2038
Reviewed at head 3aa9fd8776f40e409366c1aa83c6c6e752844013. first pass
Decision
- 🟢 approve. Every new option is off by default. The trigger test confirms that the finder output is unchanged. The findings below are minor and do not block.
Scope understood
- Three opt-in
AgnosticHelixFinderoptions: best-of-K candidates, a 2π φ-z alias test across the largest z gap, and a refit through the stopping target. Plus a new truth analyzer,AprHelixTruthAna. - The branch also carries #2035 (
HelixToolhit ratio). I reviewed only this PR's increment82658397...3aa9fd87:AgnosticHelixFinder_module.cc,AprHelixTruthAna_module.ccandAnalyses/CMakeLists.txt. #2035 needs its own review and should merge first. - No fcl changes. Nothing in Offline, Production or mu2e-trig-config turns the new options on. The only
module_type : AgnosticHelixFindertable isCalPatRec/fcl/prolog.fcl:511, andTrigAprHelixFinder/TrigAprProtonHelixFinderin trig-config inherit it.
Findings
-
🟡 [S2]
resolveGapAliascompares line fits over different hit sets when the calo cluster is on the helix.- Evidence:
recoverPointscan recover the calo cluster. It skips only the stopping target (AgnosticHelixFinder_module.cc:1648) and puts the cluster into the line fit (:1702).resolveGapAliasthen takes the current_lineFitter.chi2Dof(), which includes the cluster, as the value to beat. But it builds the ±2π alternatives without the cluster (:1732). If the alternative wins and no further hit is recovered, the saved helix has a φ-z line without the cluster, while the cluster is stillusedand still in the circle fit.targetRefithas the same mismatch in the other direction. Its circle refit drops the cluster (:1003), andscanDphiDzadds it back (:1853). - Impact: the χ²/dof comparison favours an alternative with one point fewer. The cluster's φ error is large (
caloClusterSigma30 mm), so the bias should be small. This only matters withminAliasGap > 0. I read this from the code and did not run it. - Suggested fix: build the baseline and both alternatives over the same points, either all with the cluster or all without it. The simplest way is to refit the baseline from
idxexactly as the alternatives are built.
- Evidence:
-
🟡 [S2] The six new parameters carry C++ defaults (
:97-102).- Evidence:
maxHelixCandidates1,acceptHelixStrawHits0,minAliasGap0,targetRefitSigma0,targetRefitPhiWindow0.5,targetRefitDphiDzBins100. The PR's own results show the right values depend on the context: the commissioning tracker gets 3/20/100/100, and offline reco may want different ones. The house rule is that such parameters stay mandatory and get their values from a prolog. The same point was raised on #2033 and #2034. - Suggested fix: use the two-argument
fhicl::Atomform, and put the current values in theAgnosticHelixFindertable inCalPatRec/fcl/prolog.fcl. The trigger tables inherit that table through@table::, so nothing else needs to change.
- Evidence:
-
⚪ [S3] A timeout throws away candidates that were already found.
- Evidence: the timeout exit inside the triplet loop returns
false(:866-870) without callingsaveCandidates. With K = 1, a timeout only ever cut short a search that had found nothing. With K > 1, it can now discard a viable helix that was stored while the search went on. - Current impact: none. No Offline or trig-config fcl sets
timeoutMs. - Suggested fix: save the best stored candidate on that exit when
_candidatesis non-empty.
- Evidence: the timeout exit inside the triplet loop returns
-
⚪ [S3] The save logic is written out twice.
- Evidence:
saveAndContinue(:1061-1071) repeats the existing branch at:949-955line for line. - Suggested fix: have the K = 1 branch call
saveAndContinuetoo, so the "search for another helix" rule has one home.
- Evidence:
-
⚪ [S3] Nits in
AprHelixTruthAna.:236writes11,13,-211,-2212wherePDGCode::e_minusand its siblings should be used. Only e± and μ± can reach that line, so the-211/-2212terms are dead.- The comment on
_bzConvat:69says "mm per MeV/c". The value is in MeV/c per mm, as:149says. sdmcs.at(sh)(:255,:300) assumes the straw-hit index equals theStrawDigiMCindex. That holds whileFilterHitsis false, which it is in every Offline and trig-config table today. A one-line comment would save the next user from finding this out the hard way.
🟢 Verified at this head:
- The defaults reproduce the old finder.
- With
maxHelixCandidates1, the new branch is never taken (:927-928), and the old save path runs unchanged. WithminAliasGap0,resolveGapAliasis never called (:907). WithtargetRefitSigma0, there is no refit. - FNALbuild's
triggerjob agrees. At this head (build 3450), all 1013TrigReportlines are identical to #2035's own build (3449, same merge basefd9bfbdb). - The 8 "minor change" paths (
tpr_HlxDe_70m50p+28 and so on) therefore come from #2035'sHelixTool/HelixFilterchange, not from this increment. #2040 at the same base matches the reference counts exactly.
- With
scanDphiDzis self-consistent.- The slope range follows from p² = (R² + λ²)(cB)² with |dφ/dz| = 1/|λ|, clipped by
maxDphiDz. - The circular window count uses the full width
window. The adoption step keeps hits withinwindow/2of the window's midpoint, so the two agree. - The stopping-target point added in
targetRefitis at (0,0), which matches_stopTargPos.
- The slope range follows from p² = (R² + λ²)(cB)² with |dφ/dz| = 1/|λ|, clipped by
- The analyzer's truth geometry is right.
StrawGasSteppositions are in the tracker frame (StrawSD.ccsubtractsdetectorOrigin), sod0trueis measured from the tracker axis, as the helix d0 is. - The build is complete. The new plugin is in
Analyses/CMakeLists.txt, and the sconsSConscriptglobs it. It compiled in FNALbuild.
Validation check
- Build/tests run:
- FNALbuild passed at this head (build 3450, merged at
fd9bfbdb). Nothing in the files this PR touches has changed on main since then. triggerreturned code 1, "minor change". As explained above, that comes from #2035.- clang-tidy reported 6 errors. The two I could see are header-environment errors (
cetlib/LibraryManager.h,StopWatch.hh<format>), not this diff. - Locally I read the code and compared the trigger logs. I did not run anything with the new options on, so none of the opt-in paths ran in CI or here. The efficiency, background and timing tables in the PR description are the author's own numbers.
- FNALbuild passed at this head (build 3450, merged at
- Config contract check: pass. The new keys are optional and no fcl sets them.
- Cross-repo consistency: pass. Turning the options on in mu2e-trig-config is a separate change, as stated.
Residual risk
- The opt-in paths run only when trig-config turns them on. The CI reference log will need to be regenerated in that PR.
- Best-of-K picks by straw hits, then χ². It does not check
maxEDepAvgor the direction test insaveHelix, so the chosen candidate can be dropped there while a runner-up would have passed. That was also true before, but it is now more likely.
Author follow-ups
- Optional: compare like with like in
resolveGapAlias/targetRefit(finding 1). - Optional: move the six new values into the prolog and make the atoms mandatory (finding 2).
- Optional: findings 3-5.
- Merge #2035 first.
…rackerStatus - liveFraction: a face crossing with no straw of that face at the point (radially outside the panel's straws, or beyond the straw ends, e.g. between the panels of the face) keeps the old weight 1 instead of taking the end straws' status. - ReadTriggerInfo: add useTrackerStatus (default true, as in HelixFilter) so the hit-ratio histograms can match a filter run without the tracker status. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it options - resolveGapAlias: refit the current line over the same points as the +-2pi alternatives, with a used calo cluster in all of them (as in recoverPoints). - targetRefit/scanDphiDz: keep a used calo cluster in both the circle and the line fits, as in the other fits. - A timeout during the triplet search saves the best stored candidate instead of dropping it. - The K = 1 save path calls saveAndContinue, so the "search for another helix" rule has one home. - The six new parameters are mandatory; their values (all options off) are set in the AgnosticHelixFinder table of CalPatRec/fcl/prolog.fcl. - AprHelixTruthAna: PDGCode names, the _bzConv unit comment, and a note that the straw hit index is the StrawDigiMC index while FilterHits is false. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @oksuzian. Addressed in a336faf (with #2035's update merged in):
Local check: build OK. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for a336faf: build (Build queue - API unavailable) |
|
☀️ The build tests passed at a336faf.
N.B. These results were obtained from a build of this Pull Request at a336faf after being merged into the base branch at f122c5b. For more information, please check the job page here. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #2038
Reviewed at head a336faff6d6e1571cf1b1a1f14415ac1def0e11a. re-review of 3aa9fd87
Decision
- 🟢 approve. All five findings from my last review are fixed. With the defaults, the finder still does exactly what it did before, and FNALbuild passes at this head apart from the
trigger❓, which comes from #2035.
Scope understood
- Since
3aa9fd87: a merge of #2035's new commit8e8b3d1a(reviewed under #2035), and this PR's own commita336faff, which addresses my review. I graded onlya336faffhere:AgnosticHelixFinder_module.cc,CalPatRec/fcl/prolog.fclandAprHelixTruthAna_module.cc.
Findings
- None open.
🟢 Fixed in a336faff, verified at this head:
- [was S2] Like-with-like fits in the alias test.
resolveGapAliasnow refits the current line (shift 0) over the same points as the ±2π alternatives, and takes that χ²/dof as the value to beat (AgnosticHelixFinder_module.cc:1745-1763). A used calo cluster goes into all three fits with its own 2π correction, unshifted. It sits at higher z than any tracker hit, so it belongs on the unshifted high-z side, and it is left out of the gap search. This now matchesrecoverPoints, which keeps the cluster in both fits (:1686,:1698). - [was S2] Target refit.
targetRefit's circle refit now keeps a used cluster (:998).scanDphiDzstill leaves the cluster out of the scan, then adds it to both the circle and the line, with a 2π correction taken from the chosen line (:1855-1864). Both fits now see the same points as the other fits. - [was S2] Mandatory parameters. The six new atoms have no C++ default (
:97-102). Their values, with every option off, are in theAgnosticHelixFinderprolog table (CalPatRec/fcl/prolog.fcl:549-554). That table is the onlymodule_type : AgnosticHelixFinderin Offline. Both trig-config tables inherit it with@table::. Production uses it through theAgnosticHelixFinderlabel, and no fcl in Offline, mu2e-trig-config, Production, PassN or EventNtuple main sets any of the six keys.fhicl-dumpof the base table and ofTrigAprHelixFinderandTrigAprProtonHelixFinderfrom trig-config main, against this prolog, gives all six keys in all three tables, with values 1, 0, 0, 0, 0.5 and 100 (claude-scratch/pr2038_review/ahf_dump.sh). - [was S3] Timeout. The timeout exit inside the triplet loop now saves the best stored candidate when there is one (
:869-870). With K = 1 the store is always empty, so nothing changes there. - [was S3] One save path. The K = 1 branch now calls
saveAndContinue(:949-951). The function body is the old inline code line for line, including the remaining-hits test andresetFlags. - [was S3]
AprHelixTruthAna. It now usesPDGCode::e_minusandmu_minus, and the dead-211/-2212terms are gone. The_bzConvcomment reads "MeV/c per mm". There is a note that the straw-hit index is theStrawDigiMCindex whileFilterHitsis false.
🟢 Still holds:
- The defaults reproduce the old finder. With
maxHelixCandidates1 the best-of-K branch is never taken, withminAliasGap0resolveGapAliasis never called, and withtargetRefitSigma0 there is no refit. So none of the fit changes above runs by default. The K = 1 save path behaves as before. The only change with all options off is that the save logic now lives in one function.
Validation check
- Build/tests run: FNALbuild passed at this head (build 3463, merged at
f122c5b3). That includesceSimReco,cosmicSimRecoandcheck_cmake.triggeris ❓ (return code 1, "minor change"). Only the 7 helix paths moved against the reference, all by less than 0.5σ. They fall about halfway between the reference and #2035's earlier build, which fits #2035's new out-of-panel rule. The table is in my #2035 review. This PR's options are off by default, so I attribute the change to #2035, but no CI run separates the two.- clang-tidy reports 6 errors and 725 warnings; I did not read its log.
- Locally: I read the increment and the changed functions at this head, searched the five repositories for tables and keys, and ran the
fhicl-dumpabove. I did not run the finder with the options on. Your check withmaxHelixCandidates: 3, acceptHelixStrawHits: 20, minAliasGap: 100, targetRefitSigma: 100(APR helix paths 36 to 38 of 1000) is your own. - Config contract check: pass.
- Cross-repo consistency: pass. Turning the options on in mu2e-trig-config is a separate change.
- Merge order: #2035 first; this branch contains it.
Residual risk
- As in my first pass, best-of-K picks by straw hits, then χ², before
saveHelixappliesmaxEDepAvgand the direction test. A chosen candidate can still be dropped there while a runner-up would have passed. This only matters withmaxHelixCandidates > 1. - After a timeout,
saveCandidatesstill runs the target refit when it is on. That is one bounded scan, but it runs after the time limit.
Author follow-ups
- None.
|
📝 The HEAD of |
This PR adds three opt-in AgnosticHelixFinder options, aimed at trackers with fewer hits per track, and a truth analyzer for studying APR helix finding. All new options default to the previous behavior. With the defaults, the trigger decisions and helix parameters are bit-identical to before (checked on 3000 CeM and 3000 CeP events).
The PR sits on top of #2035 (HelixTool: don't count straws without signal in the helix hit ratio), so that commit appears here until it is merged.
Motivation
With the commissioning tracker conditions (
Sim_commissioning: 31% of straws dead, including all of station 7, lower gas gain, more threshold noise), the APR trigger loses most of its CE efficiency. A truth study (AprHelixTruthAna, below) shows the finder still finds a helix on the signal hits, but often with the wrong parameters. For CeM events with ≥ 15 signal straw hits:A CE crosses the active region once per turn, so its hits come in short groups about 2πλ apart. The half-radius / half-λ helix, or a 2π φ-z alias, fits the same hits.
Changes
CalPatRec/src/AgnosticHelixFinder_module.ccmaxHelixCandidatesacceptHelixStrawHitsminAliasGaptargetRefitSigmatargetRefitPhiWindow,targetRefitDphiDzBinsThe refit replaces the best candidate instead of adding a second helix. Adding one gave the same signal efficiency, but its duplicate helices fired the multi-helix and two-track paths and added about 20% background on the APR TrkDe paths.
Analyses/src/AprHelixTruthAna_module.cc(new)A per-event TTree that compares a helix seed collection with the MC truth of the signal particle (the highest-momentum e± or μ± with ≥
minHitsstraw hits):Optionally it records the trigger path decisions of the current process (
triggerPaths), so trigger efficiencies can be split by the signal truth (for example particle type and direction for cosmics).Results
Trigger re-run on MDC2025au CeM/CeP + 1BB pileup, re-digitized with
Sim_commissioning. The reduced-tracker baseline uses tuned trigger cuts (in mu2e-trig-config fcl, not part of this PR). Absolute efficiency is relative to the MC-triggerable selection of the dataset; 3000 events per sample.apr_TrkDe_75m70p_D0200efficiency (cumulative):maxHelixCandidates 3,acceptHelixStrawHits 20minAliasGap 100maxPhiZResidual 3,segMultiplier 2,maxSegmentChi2 4)targetRefitSigma 100apr_TrkDe_75m70p_D0200reaches 95.0% (CeM) and 90.4% (CeP). With the nominal tracker these are 97.5% and 94.1%.CosmicSignalMix1BB, p > 75 MeV/c):apr_TrkDe_75m70pgoes from 34.3% to 39.6% on the reduced tracker and from 48.1% to 51.8% on the nominal tracker.NoPrimaryMix1BB, 200k microbunches, paired, same file grouping):apr_TrkDe_75m70p_D0200passes 28 → 22 events. The non-D0200 APR TrkDe paths rise about 27%.TrigAprHelixFindergoes from 0.036 to 0.067 ms per event, and the APR KalSeed fits from 0.19 to 0.25 ms (more helix candidates). That is about +2–4% of the main-path trigger time.Example configuration:
Notes for reviewers
Found while reading
findSeedPhiLines, not changed here:lastAddedPositiveZandlastAddedNegativeZare never updated, somaxSeedLineGapacts as a maximum distance from the seed hit rather than a gap between hits.positiveDeltaZ.Fixing both made no measurable difference to the signal efficiency in these tests, so I left them for a separate change.
The trigger cut tuning for the commissioning tracker (mu2e-trig-config) is a separate change.
🤖 Generated with Claude Code