Mu2eKinKal: fix defects found by a static sweep of the package

[oksuzian]

Six defects found by a static review of Mu2eKinKal at 0ef02a66d. All six are mechanical fixes with an existing in-package precedent; none changes fit acceptance or any tuning constant. I have not built this locally — relying on mu2e/buildtest.

1. RegrowKinematicLine indexes a flat KalSeedMCAssns with a collection-local counter

RegrowKinematicLine_module.cc:249 did (*ksmca_H)[iseed], where iseed counts position within this module's own input KalSeedCollection (wired to KKLine).

KalSeedMCAssns is one flat product: CommonMC/src/SelectRecoMC_module.cc:170 does a single produces<KalSeedMCAssns>(), and :468-498 runs one loop over getMany<KalSeedCollection> calling addSingle once per seed per collection. Production/JobConfig/recoMC/prolog.fcl:32 orders them ["KKDe","KKDmu","KKUe","KKUmu","KKLine","KKCHmu"] — KKLine is 5th of 6. So on any event with a seed in an earlier collection, (*ksmca_H)[0] for the first KKLine seed returns a KKDe entry, the ptr self-check fails, and the art job dies with cet::exception("Reco").

The sibling RegrowLoopHelix (:305-317) does an art::Ptr-equality search, correct at any offset — which is also why the bug is invisible there: RegrowLH targets KKDe, index 0.

Fixed with a KalSeed -> KalSeedMC map built once per event, which is correct at any offset and also drops the sibling's O(N·M) scan.

2. RegrowLoopHelix: mandatory MustRegrow is absent from the live job config

RegrowLoopHelix_module.cc:98 declared a two-argument fhicl::Atom<std::string> — mandatory, unlike debug at :86 in the same struct.

No job config supplies it. Production/JobConfig/reco/regrowLH.fcl:34-55 builds physics.producers.RegrowLH as a literal block rather than pulling @table::Mu2eKinKal.RegrowLH, and Production/JobConfig/reco/prolog.fcl:122-140 omits it too. The only MustRegrow : "None" in the tree is Mu2eKinKal/fcl/prolog.fcl:642, in the table neither Production block references — grep -rn MustRegrow Production/ returns nothing. Validation fails before the module is constructed, so the constructor's own value check at :200-207 is never reached.

Fixed by giving the Atom the "None" default that table already implies. (The alternative — adding the key to Production — would need a companion PR there; this way the module is correct on its own.)

3. BkgANNSHU: unguarded sqrt(docaVar()) feeds NaN to the MVA

BkgANNSHU.cc:30 was pars[2] = sqrt(tpdata.docaVar());. A non-positive docaVar gives NaN; NaN < mvacut_ is false under IEEE-754, so the else branch runs and the hit is left active — or, if it was inactive, re-activated at :45.

Three sibling call sites clamp the identical quantity: DriftANNSHU.cc:44, CADSHU.cc:31, KKFit.hh:510,567. Adopted the same clamp.

4. KKFit::createSeed dereferences rbegin() of a possibly-empty domain set

KKFit.hh:796 did (*(kktrk.domains().rbegin()))->end() under savedomains_. domains() is a std::set, empty whenever the fit ran without BField correction; rbegin() then equals rend() and dereferencing it is UB.

No committed config triggers it today — but only because Mu2eKinKal/fcl/prolog.fcl:606-608 carries a hand-written comment telling the next author not to set SaveDomains:true there, citing this exact SIGSEGV. Replaced the load-bearing comment with a named cet::exception, and updated the comment to say what now happens.

5. StrawHitUpdaters::name() indexes with a negative enumerator

unknown = -1 (StrawHitUpdaters.hh:12), and name() did names_[static_cast<size_t>(alg)] — UB whenever an unknown state is printed at diag > 1. Bounds-checked; returns "unknown".

6. KLSeedFit omits the mandatory SaveTrajectory key

KKFIT deliberately does not set SaveTrajectory, and every producer that builds from it supplies its own — verified at LHSeedFit:468, LHDriftFit:490, KLDriftFit:531, CHSeedFit:547, CHDriftFit:562, KLTruthSeed:587, CHTruthSeed:606, RegrowLH:636. KLSeedFit did not, so its table cannot pass validation the moment it is added to a path. Added SaveTrajectory : T0, matching CHSeedFit, whose FitSettings and ExtensionSettings KLSeedFit already shares.

---

Validation

• Build: not run locally. Every claim above is static — read from the source, Production and mu2e-trig-config, plus grep. Please let buildtest be the arbiter.
• No art job was run, so Update README.md #1 and Add diagnostics #2 are predicted failures, not observed ones. The cheapest check is one mu2e -c Production/JobConfig/reco/regrowKL.fcl and one regrowLH.fcl over a few events; if either already runs green today, the corresponding finding is wrong and I will withdraw it.
• No fhicl-dump -a provenance was produced. Add diagnostics #2 and Adjust parameters for new production #6 rest on reading the prologs and grepping @table:: references; a resolution path I failed to model would change them.
• Scope held deliberately narrow. The sweep also raised items that change fit acceptance or physics and are not in this PR — KinematicLineFit::goodFit() returning only fitStatus().usable() while both siblings also require inDetector()/charge/helicity; KKBField::fieldGrad() returning the transpose of its own documented dB_i/dx_j convention; and ExtrapolateCRVRegion's short-piece fallback being backward-only. Those need an author decision, so I left them out. Happy to open an issue with the full list.

[FNALbuild]

Hi @oksuzian,
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 87d7c03: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 87d7c03.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 87d7c03 at 0ef02a6
build (prof)	✅	Log file. Build time: 04 min 31 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 (14) FIXME (4) in 5 files
clang-tidy	➡️	4 errors 48 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 87d7c03 after being merged into the base branch at 0ef02a6.

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.

[RobMina]

I think this should be a warning and a clean return (with empty domains collection) rather than an exception. An uncaught exception would abort the entire art job.

[brownd1978]

I agree with Rob, there is no need to throw here. A track with no domains will produce an empty domainsbounds container, regardless of how savedomains is set.

[RobMina]

I encountered an exception when running Production/JobConfig/reco/regrow_kl.fcl, but not the "wrong KalSeed ptr" exception addressed in this PR. Instead, it was "Invalid Time Range" in KinKal::TimeRange constructor:
#1 0x00007ffff214af99 in KinKal::TimeRange::TimeRange (this=this@entry=0x7fffffff4e60, begin=, end=end@entry=11456.136692011529, ordered=true) at /cvmfs/mu2e.opensciencegrid.org/spackages/241207/spack/var/spack/environments/muse-al9-prof-e29-p106/.spack-env/view/include/KinKal/General/TimeRange.hh:17
#2 0x00007ffff2197f41 in mu2e::KKExtrap::toTrackerPerimeterKinKal::KinematicLine(mu2e::KKTrackKinKal::KinematicLine&) const::{lambda(KinKal::TimeDir)#1}::operator()(KinKal::TimeDir) const (__closure=__closure@entry=0x7fffffff50b0, tdir=, tdir@entry=KinKal::TimeDir::backwards) at /cvmfs/mu2e.opensciencegrid.org/spackages/241207/spack/var/spack/environments/muse-al9-prof-e29-p106/.spack-env/view/include/KinKal/General/TimeRange.hh:23
#3 0x00007ffff219858f in mu2e::KKExtrap::toTrackerPerimeterKinKal::KinematicLine (this=this@entry=0xde9650, ktrk=...) at ./Offline/Mu2eKinKal/inc/KKExtrap.hh:174
#4 0x00007ffff21c465b in mu2e::KKExtrap::extrapolateKinKal::KinematicLine (this=0xde9650, ktrk=...) at ./Offline/Mu2eKinKal/inc/KKExtrap.hh:89
#5 0x00007ffff215b030 in mu2e::RegrowKinematicLine::produce (this=0xde4680, event=...) at Offline/Mu2eKinKal/src/RegrowKinematicLine_module.cc:239

The Offline call is in toTrackerPerimeter here:

Offline/Mu2eKinKal/inc/KKExtrap.hh

Line 159 in 0ef02a6

TimeRange(tref,ftraj.range().end()) : TimeRange(ftraj.range().begin(),tref);

The failure is unrelated to the suggested fix in this PR, but it hides any actual problem that this PR would fix.

A cheap fix would be TimeRange(ftraj.range().begin(), tref, false). Maybe that should be added to this PR.

Edit: I confirmed that adding ordered=false (last argument of TimeRange) to both the forward and backward branches allows the regrow_kl.fcl job to complete without exceptions.

I also confirmed that this PR produces no changes in output across standard production KL, CH, and LH jobs.

[oksuzian]

@RobMina Thanks for running regrow_kl.fcl and for confirming this PR produces no output changes in the standard KL/CH/LH jobs — that was the check I couldn't run.

I looked into the TimeRange throw before adopting the ordered=false fix, and I think that variant would trade the exception for silently wrong faces on exactly the affected tracks. Evidence below; the code is yours, so the call is yours.

Root cause

tref falls back to ftraj.t0() when the track has no TT_Mid crossing (KKExtrap.hh:154). PiecewiseTrajectory::t0() (KinKal PiecewiseTrajectory.hh:243) converges to a piece's t0 — and KinematicLine::t0() is parameter 4: the time at pos0(), the line's point of closest approach to the detector z-axis. That is a geometric reference unrelated to the fitted hit span, so for a cosmic crossing the tracker off-axis it routinely lies outside ftraj.range(), and the backwards TimeRange(range().begin(), tref) throws. LoopHelix::t0() sits near the hits, which is why only the KL path trips this. The failing population is precisely the no-TT_Mid tracks: a good midinter is in range by construction.

Why ordered=false gives wrong faces there

TimeRange(a, b, false) swaps the endpoints (TimeRange.hh:19). With tref before the whole trajectory:

• forwards window becomes [tref, range().end()]. The KinematicLine intersect casts its ray from trange.begin() (Intersect.hh:289) — a position upstream of the entire track — so the first crossing found is the entrance face, recorded as the forward-crossed face.
• backwards window becomes [tref, range().begin()]. The real crossing sits near range().begin(), so dt = -(t - tref) < 0 and the dt >= 0 guard silently rejects it: no backwards face recorded.

Net effect on the no-TT_Mid subset: entrance labeled as exit, backwards face lost — the same wrong-face mode that e95d39d fixed for calo-cluster tracks. "Completes without exceptions" and "no change in standard jobs" are both expected, since standard jobs don't reach this fallback.

Suggested alternative

double const tref = midinter.good() ? midinter.time_ : ftraj.range().mid();

tref is computed before the perimeter extrapolations, so range().mid() is the interior of the fitted hit span — the "tracker interior" the anchor was meant to be. Both windows are then valid (begin <= mid <= end, so the throw is impossible), the entrance crossing falls in the backwards window, the exit in the forwards one, and dt keeps its meaning.

Caveat: I derived this from the KinKal headers (TimeRange.hh, Intersect.hh, PiecewiseTrajectory.hh, KinematicLine.hh) and have not run it — your regrow_kl.fcl reproduction is the test that would confirm the faces come out right, e.g. by checking that the recorded SurfaceIds for a few of the previously-throwing tracks are entrance-backwards / exit-forwards.

Happy to add either version as a separate commit here, or leave it for a follow-up PR since it is a different defect than the Assns lookup this PR fixes — whichever you prefer.

[brownd1978]

I have no objection adding prolog default. Hard coded defaults should not be introduced.
Adding a map for KKLine KalSeed/KalSeedMC mapping is fine: it future-proofs against potential changes in production, but is currently unnecessary: there is no known use cases reconstructing KKLine with any other kind of KKTrack.

SaveDomains configuration 'inconsistency' checking is unnecessary: if a KKTrack has domains they should be saved. The SaveDomains config should simply be retired, now that regrowing is a standard workflow, as there's no way to correctly regrow a KalSeed that didn't save its domains. Seeds without domains (from KKLine) aren't affected either way.

[brownd1978]

I agree with Rob, there is no need to throw here. A track with no domains will produce an empty domainsbounds container, regardless of how savedomains is set.

[brownd1978]

hardcoded defaults for parameters which have no default 'correct' value are against Mu2e policy. The MustRegrow setting is context dependent and mustbe set outside the code.

[oksuzian]

@brownd1978 @RobMina Thanks both. I agree the throw is wrong — aborting the art job over a configuration state the code can handle is not the right response. But I don't think deleting it gets us to the behaviour you both described, and I'd rather flag that now than have it come back as a segfault.

The empty-container claim doesn't hold for the current code

A track with no domains will produce an empty _domainbounds container, regardless of how savedomains_ is set.

That is what we want, but it isn't what happens today. KKFit.hh:790-797:

if(savedomains_){
  kseed._domainbounds.reserve(kktrk.domains().size()+1);
  for (auto const& domain : kktrk.domains()){
    kseed._domainbounds.push_back(domain->begin());
  }
  // save end of last domain
  kseed._domainbounds.push_back((*(kktrk.domains().rbegin()))->end());
}

The loop is correctly a no-op on an empty set, but the final push_back is unconditional. domains() is a std::set, so when it is empty rbegin() equals rend() and dereferencing it is undefined behaviour — we get a bad read, not an empty container. That is the failure mode the comment at Mu2eKinKal/fcl/prolog.fcl:606-608 was written to warn the next author about, and removing the throw without touching anything else puts us back exactly there.

Proposed replacement

Guard the block on the set instead of throwing:

if(savedomains_ && !kktrk.domains().empty()){

This gives the clean return with an empty _domainbounds that you both asked for, with no exception and no reachable UB.

It also lands where you want to end up, David:

if a KKTrack has domains they should be saved. The SaveDomains config should simply be retired

With the guard written this way, retiring the config later is a one-token deletion — the !kktrk.domains().empty() condition is precisely "save them if there are any", and nothing else in the block depends on savedomains_. Happy to do the retirement here instead if you'd prefer it in one go, though it reaches into the prologs and the Production reco configs, so my instinct is a separate PR.

I'll also drop the amended comment in prolog.fcl and restore something closer to the original wording, since with the guard in place SaveDomains : true on a bfcorr=false fit is simply a no-op rather than an error.

MustRegrow

Agreed, I'll revert RegrowLoopHelix_module.cc:98 to the mandatory two-argument form — the policy point is right and I shouldn't have introduced the default.

One note on where the fix then belongs. The prolog default you have no objection to already exists: Mu2eKinKal/fcl/prolog.fcl:642 sets MustRegrow : "None" in the RegrowLH table. The gap is that neither Production block referencing this module pulls that table — Production/JobConfig/reco/regrowLH.fcl:34 and Production/JobConfig/reco/prolog.fcl:122 both build RegrowLH as literal blocks, and grep -rn MustRegrow Production/ returns nothing. So with the default reverted, regrowLH.fcl should fail validation before the module is constructed.

I'll open a companion Production PR adding the key to both blocks. Worth saying that this one is still unverified by a run: Rob, your regrow_kl.fcl reproduction wouldn't have caught it, since RegrowKinematicLine has no MustRegrow parameter at all — it's RegrowLoopHelix only. A single mu2e -c Production/JobConfig/reco/regrowLH.fcl over a couple of events would confirm or kill it, and if it runs green today I'll withdraw the finding.

Extrapolation TimeRange

I'd like to take the KKExtrap::toTrackerPerimeter issue out of this PR and into its own, since it's an unrelated defect and it would otherwise hold up the four items nobody has objected to. Rob, my reading of ordered=false versus using ftraj.range().mid() as the fallback anchor is in the comment above — I have not run either, so your reproduction is the thing that would settle which one produces the right faces.

[RobMina]

I created the TimeRange issue here: #1958

Edit: and the PR is #1959