Fixes from AI static scans

[brownd1978]

This PR addresses issues raised by AI static scans (issue #225 )

[brownd1978]

Response to issue #225:

1. fixed as suggested. However, the sagitta function isn't used in intersect(); instead that uses the 1st order approximation to sagitta to directly solve for the maximum time step in tolerance. That solution avoids the problem the AI mentions in followup comparing 3D vs 2D chords and arcs. I also added a test that the linear approximation stays within tolerance due to surface curvature.
2. fixed as suggested
3. fixed as suggested
4. fixed as suggested
5. fixed, but not as suggested; the suggested fix would result in a logically unexpected result A.overlaps(A) returning false.
6. fixed as suggested
7. fixed (covariance rotation fully disabled)
8. fixed by making completing copy constructors. The suggested fix of making that constructor private doesn't work as it then can't be reached in 'clone' , which is the only use case. Also fixed Domain as suggested.
   Followup findings:

Intersect.hh:300 fixed as suggested
DetMaterial::scatterAngleVar fixed as suggested
MatDBInfo::findDetMaterial replaced ErrMsg calls with throws
Detector/ResidualHit.hh comment is correct, pvar now set to 0 in this case
copy constructors: fixed where applicable. Note Hit explicitly disalllows a copy constructor, it is a pure virtual interface
LoopHelix::constructible: added

[edcallaghan]

I only really inspected at the copy-constructors, which look fine to me. I can put effort into the rest if that would be helpful.

[edcallaghan]

I'm missing the context of why dpdpdb_ is consistently commented out, so I'm pointing it out in case it was meant to be temporary.

[edcallaghan]

== 0.0 can be a bit dodgy. I don't know the source of momdir, maybe there are semantics that will "guarantee" a true-zero, but if it is always a computed quantity then may want to do a tolerance instead.

[RobMina]

In testing with the production job fcls, this PR shows a reduction in the number of good tracks, especially for CentralHelix. See the comment above on the specific line.

Edit: I think the root of the problem is Detector/Hit.hh line 66:

KinKal/Detector/Hit.hh

Line 66 in dbc9aef

wt -= weight();

After the change in PieceWiseClosestApproach.hh, the trajectory piece where a hit's weight was applied at construction might differ from the piece its closest approach points to, so that the subtraction in Detector/Hit.hh can result in a negative projected distance variance. When that happens, the hit gets turned off. Sometimes (about 6% of the time), this results in a degraded fit that causes more hits to fail distance and variance cuts, and the track is ultimately dropped with LowNDOF.

[RobMina]

I think this change is correct in itself, but in testing it reveals another bug downstream, see below.

In testing with production LoopHelix and CentralHelix job fcls, this small change results in a ~6% drop in the number of tracks. Some of the lost tracks are actually good fits, especially for CentralHelix. I verified that this particular line is the suspect by reverting just this change and re-running. I am still trying to pin down the exact reason. It seems to be related to a negative unbiased variance caused when Hit::unbiasedParameters() in Detector/Hit.hh subtracts out the weight for a hit from a piece that is different from the one where its weight was originally applied.

[brownd1978]

I'm also seeing low reco efficiency.

It seems to be related to a negative unbiased variance caused when

Hit::unbiasedParameters() That's a plausible explanation: the hit cache is WRT the previous residual/trajectory; after POCA is updated, if the piece has changed it won't reference the same parameters or derivatives, so the 1st order variance subtraction can fail. I think this says we need to compute uparams before updating POCA. Since UPOCA is only used by the updaters it won't directly affect the fit. There could be more knock-on effects though...

…

On Thu, Oct 1, 2026 at 12:10 PM Rob Mina ***@***.***> wrote: ***@***.**** commented on this pull request. In testing with the production job fcls, this PR shows a reduction in the number of good tracks, especially for CentralHelix. See the comment above on the specific line. I am still working on diagnosing the problem. ------------------------------ In Trajectory/PiecewiseClosestApproach.hh <#226 (comment)>: > @@ -30,7 +30,7 @@ namespace KinKal { this->findTCA(phint); phint = this->hint(); oldindex = pindex; - pindex = ptraj.nearestIndex(hint.particleToca_); + pindex = ptraj.nearestIndex(phint.particleToca_); I think this change is correct in itself, but in testing it reveals another bug downstream, see below. In testing with production LoopHelix and CentralHelix job fcls, this small change results in a ~6% drop in the number of tracks. Some of the lost tracks are actually good fits, especially for CentralHelix. I verified that this particular line is the suspect by reverting just this change and re-running. I am still trying to pin down the exact reason. It seems to be related to a negative unbiased variance caused when Hit::unbiasedParameters() in Detector/Hit.hh subtracts out the weight for a hit from a piece that is different from the one where its weight was originally applied. — Reply to this email directly, view it on GitHub <#226?email_source=notifications&email_token=ABAH57YPA4Z7MHDR5KQ2CI35R2TZHA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMZYGQZDEMBXGQ3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-5384220746>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABAH57YF5P3B6RH7H6WDSQT5R2TZHAVCNFSNUABFKJSXA33TNF2G64TZHMZDIMJZHE2TENJYHNEXG43VMU5TKNRUHE2DGOJZGA22C5QC> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- David Brown ***@***.*** Office Phone (510) 486-7261 Lawrence Berkeley National Lab M/S 50R5008 (50-6026C) Berkeley, CA 94720

[oksuzian]

PR Review Summary — #226

Reviewed at head a3406dd56dd438b75c5cbc19cc0929f2a5530f80. first pass

Decision

• 🟡 comment only. The fixes for the eight original #225 items are in, and most of them are right. Hold the merge until the unbiasedParameters ordering issue Rob found is resolved. There are also two incomplete fixes and one behaviour change worth a look.

Scope understood

• 21 files, 251 lines, addressing issue #225: the original eight items plus several follow-ups (pdir_, scatterAngleVar statics, findDetMaterial, ResidualHit pvar, LoopHelix::constructible, transverseSpeed, KinematicLine mass/charge, PiecewiseTrajectory::clone).

Findings

1. 🟠 [S1] The track loss Rob reported. I agree with the diagnosis, and here is one data point.

   • The PiecewiseClosestApproach fix (item 2) is correct. Before it, the closest approach never left the hinted piece. Now it can, which exposes the mismatch in Hit::unbiasedParameters(): the reference parameters now come from the new piece, while weight_ is still from the old one.
   • In Offline, the order is KKStrawHit::updateReference (moves ca_, and with it refTrajPtr()) → updateState(first) → updateWHS → unbiasedClosestApproach() → unbiasedParameters(). updateWeight() runs only after that. KKCaloHit::unbiasedClosestApproach follows the same order.
   • My measurement: Offline#2021 head, 2000 CeEndpoint OnSpill events, KinKal main dbc9aef vs this PR. LoopHelix KalmanConverged is unchanged to slightly up: KKDe 1889→1910, KKDmu 1895→1905, KKUe 1848→1847, KKUmu 1869→1891.
   • So the loss doesn't show up in LoopHelix fits of conversion electrons, which fits Rob seeing it worst in CentralHelix. I couldn't run CentralHelix: I have no field-on cosmic digi sample.
   • I also tried the obvious Offline-side fix: caching unbiasedParameters() in updateReference before ca_ moves. It made things worse, with upstream fits down 13–23%, so it's not the right fix. Dave's suggestion needs more than reordering that one call.

2. 🟡 [S2] TimeRange::overlaps now counts touching ranges as overlapping.

   • Executed at this head and at main:
     • [0,10) vs [10,20): overlaps is true here, false on main.
     • The null range [10,10) against [0,10): now correctly false.
   • The new inRange(other.end()) term is true at the shared endpoint.
   • It feeds PiecewiseTrajectory::setRange and pieceRange and the domain trim at Track.hh:226, so a neighbouring piece or domain that only touches the boundary is now kept.
   • That may be harmless, but it's a behaviour change beyond the null-range fix, and it's worth checking against the CentralHelix efficiency.

3. 🟡 [S2] Two fixes are incomplete.

   • MatElementList: the failed open still spins. With a nonexistent path and -DNDEBUG, assert(elements.good()) is compiled out and the failed open sets failbit, not eofbit. So the new !elements.eof() guard never fires. I ran it: still looping after 5 s. Test good() or fail() instead.
     • The same tag-search loop is untouched in MatMaterialList.cc:74 and MatIsotopeList.cc:72.
   • FitData::invert still never checks vec_. isfinite on mat_(0,0) is better than isnan, but the overflow I showed in #225 is in vec_ = mat_*vec_, a finite-but-huge matrix times a normal vector, and vec_ is still not checked.

4. ⚪ [S3] Small items.

   • Domain.hh:43: the new operator<< is a non-inline definition in a header. Two translation units that include Domain.hh give a multiple-definition link error; I reproduced it. The copy constructor and clone already have the same problem on main. inline on all three fixes it.
   • Intersect.hh:300: pdir_ = kkline.direction(retval.time_) reads time_ one line before it is assigned. That's harmless for a line, whose direction doesn't depend on time, but swapping the two lines matches stepIntersect.
   • PiecewiseTrajectory::clone: rv-> is fixed, but push_back(*piece) still pushes a KTRAJ into a deque of shared_ptr, and no trajectory class defines clone. It still won't compile if anything instantiates it.
   • The commented-out dpdpdb_ lines (Ed's comment): if the covariance rotation is being dropped, delete them rather than comment them out.

Not in this PR (about 35 items from #225 are untouched, almost all latent or unreachable from Offline):

• Track.hh Status(0) ×3;
• prepend(…,true);
• setRange with an empty container;
• the PointPiecewiseClosestApproach compile errors;
• the SensorLine unit error;
• ConstantDistanceToTime abs();
• the CentralHelix::dPardPardB identity stub;
• all of the General/ BField-map items;
• most of the MatEnv and Geometry items.

Fine to defer, but it would help to say whether #225 stays open for them.

Validation check

• Build/tests run:
  • KinKal built at this head and at main (CMake, Release).
  • Offline#2021 built against each.
  • 2000-event LoopHelix reco A/B (numbers in finding 1).
  • Standalone programs for TimeRange overlaps, the MatElementList failed open, and the Domain.hh link.
• Not run: KinKal ctest at this head (GitHub Actions is green on 4 configs), and any CentralHelix sample.
• Cross-repo: Offline#2021 depends on #224, and this PR changes fit behaviour under it.

Residual risk

• CentralHelix efficiency, until the unbiasedParameters ordering is fixed and re-measured.

Author follow-ups

1. The unbiasedParameters fix, then Rob's efficiency re-run (finding 1).
2. Confirm the touching-range change in overlaps is intended (finding 2).
3. MatElementList failed-open, vec_ in invert (finding 3); inline in Domain.hh (finding 4).

[oksuzian]

PR Review Summary — #226

Reviewed at head 3f849fe15d436927635eb16488c4f4be0b298e68. Re-review of a3406dd56dd438b75c5cbc19cc0929f2a5530f80.

Decision

• 🟡 comment only. Nothing in this increment changes my first-pass review.

Scope understood

• One commit, "disambiguate messages": Fit/DomainWall.hh:124, in DomainWall::extrapolate, changes the exception text from "Can't append piece" to "Can't extrapolate piece".
• 🟢 This is correct: the throw is inside extrapolate, and append has its own message. It's text only, with no behaviour change.

Carried forward from a3406dd5, all still open at this head

1. 🟠 [S1] The unbiasedParameters piece mismatch behind Rob's track loss. The ordering fix Dave proposed isn't in yet.
   • Since my last review, I tried a field-on OffSpill cosmic sample (CosmicSignalOffSpill.MDC2025an, 2000 events). It gave only 15 CentralHelix tracks, which is too few to measure anything.
   • On the same sample, the LoopHelix path kept 1194 events with this PR against 1214 with main (−1.6%), and converged LoopHelix fits fell from 40 to 27. Also small numbers, but they point the same way as Rob's result.
   • Rob's 112-file sample is still the right test.
2. 🟡 [S2] TimeRange::overlaps now counts touching ranges as overlapping.
3. 🟡 [S2] MatElementList: a failed open still spins. FitData::invert still doesn't check vec_.
4. ⚪ [S3] The non-inline operator<< in Domain.hh, Intersect.hh:300 reading time_ before it is set, PiecewiseTrajectory::clone, and the commented-out dpdpdb_ lines.

Validation check

• Build/tests run: none for this increment, a one-string change I read in full. GitHub Actions results at this head: not checked.

[oksuzian]

PR Review Summary — #226

Reviewed at head 79fefa9134f51ce89d47525b2f3a59b28e277aa8. Re-review of 3f849fe15d436927635eb16488c4f4be0b298e68.

Decision

• 🟡 comment only. "Small fixes" clears most of my S2/S3 items, thank you. The unbiasedParameters ordering (S1) isn't in this commit, so the track loss is still open.

Scope understood

• One commit across six files. It answers my earlier findings 2, 3 and 4, and adds a forceRange change.

Verified in this increment, by reading the diff

• 🟢 TimeRange::overlaps goes back to inRange(other.begin()) || other.inRange(begin()). Touching ranges [0,10)/[10,20) no longer overlap, and a null range at the boundary doesn't either. This resolves my finding 2.
• 🟢 MatElementList: a stream that fails to open now throws (!good() || eof()) instead of hitting a release-mode no-op assert. This resolves the failed-open part of finding 3.
• 🟢 Domain.hh operator<< is now inline.
• 🟢 Intersect.hh: pdir_ is now set after time_.
• 🟢 PiecewiseTrajectory::clone: it now pushes make_shared<KTRAJ>(*piece), which matches the deque's shared_ptr element type. Not compiled, because nothing in the repo instantiates it.
• 🟢 The commented-out dpdpdb_ lines are deleted.

Findings

1. 🟠 [S1] Still open: the Hit::unbiasedParameters() piece mismatch behind the CentralHelix loss. Dave agreed on 10-01 that uparams should be computed before the POCA update, but that change isn't in this commit. My cosmic numbers from the last review stand (LoopHelix converged fits 40 → 27 on 2000 OffSpill events), and Rob's 112-file sample is still the right test.
2. 🟡 [S2] The new forceRange upper clamp is a no-op. end() - std::numeric_limits<double>::min() subtracts the smallest normalised double, about 2.2e-308. For any end() above roughly 1e-292, that rounds back to end() exactly. For example, one ULP at 1695 ns is about 2.3e-13. So a time clamped to the end still fails inRange, which uses t < end().
   • Suggested fix: std::nextafter(end(), begin()). Or, if one ULP is too fine for the callers, an explicit small epsilon in ns.
   • Not run. This is double-precision arithmetic, and I haven't checked which callers depend on the clamp.
3. 🟡 [S2] Still open: FitData::invert doesn't check vec_, the overflow from #225. This commit doesn't touch FitData.hh.
4. ⚪ [S3] The other two non-inline definitions in Domain.hh remain. At this head, Domain::Domain(Domain const&) (line 33) and Domain::clone (line 38) are still non-inline out-of-class definitions in the header. operator<< was the one I reproduced. These two give the same multiple-definition link error once two translation units include the header. Adding inline to both finishes the job.

Validation check

• Build/tests run: none locally for this increment. I read the compare diff 3f849fe1...79fefa91 in full.
• GitHub Actions: Tests pass on all 4 configurations (Debug/Release × ubuntu/macos) at this head.
• Cross-repo: Offline#2021 still needs a KinKal tag with #224 and #226, then an envset and a .muse bump.

Author follow-ups

1. The unbiasedParameters ordering, then a re-measurement of CentralHelix efficiency (finding 1).
2. nextafter in forceRange (finding 2).
3. A vec_ finiteness check in invert (finding 3).

[RobMina]

I tested again at current head (79fefa9)

I still see the reco efficiency loss of about 7% on the LoopHelix sample and 6% of the CentralHelix sample. The LoopHelix tracks dropped are mostly marginal quality, but the CentralHelix loses some good tracks.

The LoopHelix base config is Production/JobConfig/reco/OnSpill.fcl and the input file was dig.mu2e.data_like_no_primary_1BB.MDC2025af_best_v1_1.001430_00000000.art

The CentralHelix base config is Production/JobConfig/recoMC/OffSpill.fcl and the input file was dig.mu2e.CosmicSignalOffSpill.MDC2025an_best_v1_1.001430_00000000.art

I have a proposed fix that I am testing now.

[oksuzian]

PR Review Summary — #226

Reviewed at head cbab3c3f450d009c0f5f14e111a8afcd98d4c788. Re-review of 79fefa9134f51ce89d47525b2f3a59b28e277aa8.

Decision

• 🟡 comment only. "Address last issues" closes three of my four remaining items. The new invert check only looks at one component, though, and the track loss is still open. Rob is testing a fix for it, per his comment.

Scope understood

• One commit:
  • Domain copy constructor and clone move out of the header into a new Fit/Domain.cc, added to the Fit library;
  • forceRange now uses std::nextafter;
  • FitData::invert adds a finiteness check on vec_.

Verified in this increment, by reading the diff

• 🟢 Domain.hh no longer holds any non-inline out-of-class definition, so the multiple-definition link error is gone. The two definitions are now in Domain.cc, which is in Fit/CMakeLists.txt. Not compiled locally.
• 🟢 forceRange: std::nextafter(end(), begin()) is one ULP below end(), so a clamped time now passes inRange. For a null range it gives begin(), which is harmless. This resolves my previous finding 2.

Findings

1. 🟠 [S1] Still open: the Hit::unbiasedParameters() piece mismatch. Rob measured it again at 79fefa91 and still sees the loss: about 7% on LoopHelix and 6% on CentralHelix, including good CentralHelix tracks. This commit doesn't touch it, and he says a fix is being tested.
2. 🟡 [S2] The new vec_ check tests only vec_[0]. static double ones[DVEC::kSize] = {1.0}; sets the first element to 1 and zero-initialises the rest; that's aggregate initialisation. So uvec is (1,0,0,0,0,0), and Dot(vec_,uvec) is just vec_[0]. An overflow in any of the other five components still gets through. That includes the case I showed in #225, a finite-but-huge mat_ times a normal vector.
   • Suggested fix: a plain loop with no statics, for(size_t i=0;i<DVEC::kSize;++i) if(!std::isfinite(vec_[i])) throw std::runtime_error("Inversion failure");. Alternatively, fill ones explicitly. A NaN or ±inf in any element makes the all-ones dot product non-finite, so that version would also work.
   • Minor point: function-local statics in a header member function are thread-safe to initialise, but the loop avoids them altogether.

Validation check

• Build/tests run: none locally. I read the compare diff 79fefa91...cbab3c3f in full. Finding 2 is a language rule, not a measurement.
• GitHub Actions at this head: all 4 configurations were pending when I checked.
• Cross-repo: Offline#2021 still needs a KinKal tag with #224 and #226, then an envset and a .muse bump.

Author follow-ups

1. The unbiasedParameters fix and a re-measurement on Rob's two samples (finding 1).
2. Check all six vec_ components in invert (finding 2).

[oksuzian]

PR Review Summary — #226

Reviewed at head c3ead80e1e97c595281dc5053267b49fce4f7099. Re-review of cbab3c3f450d009c0f5f14e111a8afcd98d4c788.

Decision

• 🟡 comment only. The vec_ check is fixed, thank you. The only thing still open is the track-loss item.

Scope understood

• One commit, "Test all elements". In FitData::invert, ones is now std::vector<double>(DVEC::kSize, 1.0), so uvec is all ones.

Verified in this increment, by reading the diff

• 🟢 Dot(vec_, uvec) is now the sum of all six components. A NaN or ±inf in any of them makes the sum non-finite, and so does an opposing pair +inf/−inf, which gives NaN. The overflow case from #225 is now caught. This resolves my previous finding 2.
• The only edge case: finite components whose sum overflows, which needs values around 1e308. That would throw on a result that was technically finite, which is the safe direction.

Findings

1. 🟠 [S1] Still open: the Hit::unbiasedParameters() piece mismatch behind the reco efficiency loss Rob measured at 79fefa91 (about 7% LoopHelix, about 6% CentralHelix). This commit doesn't touch it, and Rob's proposed fix hasn't been pushed yet.

Validation check

• Build/tests run: none locally. I read the compare diff cbab3c3f...c3ead80e in full.
• GitHub Actions at this head: Tests pass on all 4 configurations.
• Cross-repo: Offline#2021 still needs a KinKal tag with #224 and #226, then an envset and a .muse bump.

Author follow-ups

1. The unbiasedParameters fix, then a re-measurement on Rob's two samples.

[RobMina]

I'll keep digging into the CH efficiency drop. In the meantime, this PR can be merged.

[brownd1978]

Adding documentation of performance form this PR: top stats are release 3.7.0, bottom are this PR.
CH (offSpill):

KL (extracted):

LH (Ce)