Fixes for issue 1974

[RobMina]

Issue #1974

Tested against main, no significant differences for production jobs.

For CentralHelix extrapolation with bfcorr=true, the needsExtrapolation fix is necessary to recover some CRV crossings on the forward leg. It is still not possible to run stably with bfcorr=true without upstream modifications to KinKal, which are in a separate PR. This limited fix can safely be merged now.

Edit: no change in track numbers and CRV crossings across KL/CH, no measurable change in reco time (<0.1% CPU time difference before/after).

There are a few additional TT_Outer crossings for the CH bfcorr=false run. These represent helices that loop several times around the tracker volume, an unphysical trajectory that can be resolved by setting bfcorr=true. But that fix requires significant upstream changes.

[FNALbuild]

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

• Mu2eKinKal

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at e525b16.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged e525b16 at 8a78d2f
build (prof)	✅	Log file. Build time: 04 min 22 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. Return Code 1.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (3) FIXME (2) in 3 files
clang-tidy	➡️	2 errors 15 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at e525b16 after being merged into the base branch at 8a78d2f.

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]

PR Review Summary — #1975

Reviewed at head e525b162829d67421d54b34bdea08153084459cb. First pass.

Disclosure: I opened issue #1974, so this review is of the answer to my own report. All three fixes are correct as written; the comments below are validation asks, not objections.

Decision

• 🟢 approve

Scope understood

• Answers all three findings of #1974: the goodFit omission in KinematicLineFit, the fieldGrad row/column convention in KKBField, and the backward-only short-piece fallback in ExtrapolateCRVRegion.
• Behaviour actually changes in two of the three: goodFit can now reject KL fits, and forward CRV extrapolation now continues past a short piece. fieldGrad is a no-op in practice.

Verified

• 🟢 KKBField::fieldGrad — the fix is correct and its blast radius is genuinely zero. Grad(i,j) = dB_i/dx_j per KinKal/General/BFieldMap.hh:19, and Place_in_col(v,row,col) writes v down column col starting at row, so Place_in_col(dBdxv,0,0) gives column 0 = (dBx/dx, dBy/dx, dBz/dx) — the convention. Searching both repos for consumers: in Offline only the two overrides and the two declarations; in KinKal only BFieldMap.hh (pure virtual, NullBFieldMap and GradientBFieldMap overrides) and CompositeBFieldMap::fieldGrad in BFieldMap.cc:13, which only sums the children. Nothing reads the matrix, so no fit result moves.
• 🟢 KinematicLineFit::goodFit — the new body matches CentralHelixFit_module.cc:397-405 line for line (same active() guard, same position3(shptr->time()), same early break), and Offline/Mu2eKinKal/inc/KKFitUtilities.hh was already included at line 53, so nothing was added speculatively. The stale charge/helicity comment is gone rather than left describing a test that is not run, which was the half of finding 1 that mattered most.
• 🟢 The ExtrapolateCRVRegion change is direction-symmetric, which was the question in finding 3. Both quantities in the surviving comparison are already direction-normalised: normvel carries timeDirSign(tdir) so time_to_sector is a positive time-to-surface in the extrapolation direction, and piece_span is fabs(stime-etime). Dropping the tdir gate therefore applies the same test forward that it applied backward, with no sign to get wrong. The identical edit in the strongback-plane loop matches.
• 🟢 CI is green at this head (build 3355). trigger returning 1 is not this PR: it is Job completed with warning over path-count deltas of 0.02-1.03 σ, and the same :question: appears on #1969, #1970, #1971 and #1972. The clang-tidy warnings in KinematicLineFit_module.cc and KKBField.cc are all on lines the diff does not touch (110, 137, 175, 202-213, 333-343, 383; and KKBField.cc:37).
• Not checked: I did not run a reco job, so the yield and timing numbers asked for below are yours, not mine.

Findings

1. 🟡 [S2] Two behaviour changes ship with a qualitative validation statement and no numbers.

   • Evidence: PR body says "Tested against main, no significant differences for production jobs." goodFit gates three things in KinematicLineFit_module.cc:308-316 — extension, extrapolation, and whether the seed is saved at all — and the new check can only ever move a track from saved to dropped. The CRV change can only ever add forward-leg crossings. The CI cosmic jobs (cosmicSimReco, cosmicOffSpill) grade on job success, not on track yield or intersection count, so nothing in the build result covers either.
   • Suggested fix: quote the two counts you already have — KL tracks saved before/after on a cosmic sample, and CRV KalIntersection count before/after on the CentralHelix sample. If the KL delta is exactly zero, that is worth saying explicitly, since it tells the next reader the check is a guard rather than a filter.

2. 🟡 [S2] Forward CRV extrapolation now runs to maxDt on tracks that will never reach a sector.

   • Evidence: ExtrapolateCRVRegion.hh:126-131. The sector planes are unbounded for the purposes of time_to_sector = signed_perp/normvel, so any track with normvel above the minvnorm_ floor gets a positive time-to-surface and now sets retval = true forward as well as backward. Termination falls back to maxDt_ and to the !added break in KKExtrap.hh, so there is no runaway — but the work is roughly doubled for tracks that previously stopped at the first short piece.
   • Impact: reco CPU, on every extrapolated track, not only the ones that gain a crossing.
   • Suggested fix: quote the per-event reco time from the same comparison run against main. If it is in the noise, say so in the PR body and this is settled.

3. ⚪ [S3] The new comment restates the code rather than the reason, and the three fixes share one commit.

   • // keep going, in BOTH time directions says what the line does; the useful sentence is the one in your PR body — that forward is needed to recover CRV crossings on the CentralHelix forward leg with bfcorr=true. Putting that in the comment is what finding 3 was asking for either way.
   • Separately, e525b162 squashes all three fixes together. The fieldGrad and goodFit changes are independent of the CRV one, and only the CRV one can plausibly regress, so splitting the commits would let it be reverted alone. Not worth re-pushing for on its own, but worth doing if you touch the branch again.

Validation check

• Build/tests run: yes — FNALbuild build 3355 at e525b162, all targets green including ceSimReco, cosmicSimReco, cosmicOffSpill, g4surfaceCheck and rootOverlaps. No independent build run by me.
• Config contract check: n/a — no FHiCL and no module parameters change.
• Cross-repo consistency: pass. fieldGrad has no consumer in Offline or KinKal, and the KL and CRV changes alter no data-product layout, module label or parameter name, so no Production or mu2e-trig-config companion is needed. The KinKal work you mention for stable bfcorr=true is upstream and separate, as you say.

Residual risk

• The inDetector envelope is the hardcoded Rho < 900 && |Z| < 1800 of KKFitUtilities.cc:20-22, carrying its own numbers should come from Tracker TODO. KL now depends on it, so if that envelope is ever tightened it silently becomes a cosmic-track filter. Pre-existing, and not something to fix here.
• The short-piece test is a straight-line estimate of the time to a curved trajectory's surface crossing. Forward pieces under bfcorr=true are the curved ones, so the estimate is loosest exactly where the new branch now fires. It errs toward extrapolating too far rather than stopping too early, which is the safe direction.

Author follow-ups

1. Add the before/after KL track count and CRV intersection count to the PR body.
2. Add the per-event reco timing delta, or state that it was below noise.
3. Replace the BOTH time directions comment with the bfcorr=true forward-leg reason from the PR body.

[brownd1978]

Thanks for the fixes Rob.