CalPatRec: static-review audit — correctness, robustness, convention, dead-code findings

[oksuzian]

Preamble

The items below come from a static review of CalPatRec/ on main — no build, no runtime test, no physics validation was performed. Each finding is either a direct read of the source or (where noted) a cross-reference against sibling modules. I've verified the top-tier bugs by re-fetching the exact lines against main; the rest is best-effort.

Intent is to give maintainers a single triaged punch list. Items marked Bugs / Blockers are high-confidence; the rest decrease in certainty as the tier name suggests.

---

Bugs / Blockers

1. src/DeltaFinderAlg.cc:596 — float y1 = dc1->Xc(); should be dc1->Yc();. The subsequent d2 = (x1-x2)² + (y1-y2)² compares the wrong coordinate, so two-delta merging uses (Xc, Xc) vs (Xc, Yc).
2. src/DeltaFinderAlg.cc:792 — in pruneSeeds: float chi2_ds2 = ds1->Chi2TotN() + ds1->Chi2Time(); uses ds1 twice; should be ds2. ds2 is currently only rejected on ds1's recomputed value.
3. src/PhiClusterFinder_module.cc:416–420 — right-walk loop has no nsteps/nbx guard that the left-walk has. If every bin ≥ _threshold, infinite loop.
4. src/PhiClusterFinder_module.cc:404 — if (bincheck > _threshold) bincheck--; compares a bin index against a content threshold (category error). Should be bincheck > 1.
5. src/AgnosticHelixFinder_module.cc:831, 843 — for (size_t i = 0; i < _tcHits.size() - 2; i++) unsigned-underflows when _tcHits.size() < 2. Add if (_tcHits.size() < 3) return false; before the triplet loops.
6. src/CalHelixFinderAlg.cc:1916 — Helix._sxy.addPoint(fCaloX, fCaloY, 1./100.); is called unconditionally. When setCaloCluster took the "no cluster" path, fCaloX/Y = -9999 still enters the weighted circle fit with weight 1/100, biasing the fit. Line 834 (addCaloClusterToFitPhiZ) has the same pattern with fCaloZ. The correct guard exists elsewhere (line 1175, fCaloTime > 0).
7. src/CalHelixFinderAlg.cc:1559 — float lambda = (Helix._dfdz != 0.) ? 1./Helix._dfdz : 0.; then phi_pred = fz0 + z/lambda;. When _dfdz == 0, the guard produces a division by zero on the very next use. Should early-return / skip the hit loop instead of setting lambda = 0.
8. src/CalHelixFinder_module.cc:241 — goto END; on the error-return path bypasses event.put() of the unique_ptr'd output collections. Downstream art gets a "missing product" error with a confusing trace. Either put empty collections before the goto or throw cet::exception (as DeltaFinder_module.cc:249 does).
9. fcl/prolog.fcl — MergeHelixFinder block and six derived tables (MHFinderTprDe, …CprDe, …De, plus the three Dmu variants) reference a module that has no backing source file. Any job instantiating them fails at art load time. Likely wants either deletion or replacement with TrkReco/src/MergeHelices_module.cc.

Risks / Robustness

10. Stale handles between events. CalHelixFinder_module.cc:176–207 and CalTimePeakFinder_module.cc:101–122 keep art::Handles as class members; on findData partial-failure, previous-event pointers remain live. Clear at the top of produce() and avoid storing Handles as members.
11. Throw-prone getValidHandle with unreachable "error" paths. AgnosticHelixFinder_module.cc:421–425 and PrefetchData_module.cc:167–205 call getValidHandle unchecked — it throws on absence, so the return false / null-handle branches are dead. Either switch to getByLabel + explicit check, or remove the dead error path.
12. Inconsistent failure handling across modules. DeltaFinder_module.cc:249 and ComboHitFilter_module.cc:147 throw cet::exception("RECO"); CalHelixFinder_module.cc:181, CalTimePeakFinder_module.cc:109, TZClusterFinder_module.cc:258, TZClusterFilter_module.cc:226 printf and continue. Pick one convention module-wide.
13. Silent fall-through in src/TZClusterFinder_module.cc:261–268. If _useCaloClusters==1 but the label is wrong, _data._ccColl = NULL silently; only downstream caloIdx != -1 guards catch this. Throw, or log at least once.
14. Unguarded divisions / sqrts in hot paths.
    • DeltaFinderAlg.cc:321, 326: q12 = 1 - w1w2² divisor, no nearly-parallel-wires guard.
    • DeltaFinderAlg.cc:159, 479: d = snx2*sny2 - snxy² divisor, no positivity guard.
    • DeltaFinderAlg.cc:98–99: rho = sqrt(xseed² + yseed²), seed_nx = xseed/rho, no guard at origin.
    • CalHelixFinderAlg.cc:1837, 1870: costh2 = dxn²/(dx²+dy²) — zero denominator if hit sits at helix centre.
    • CalHelixFinderAlg.cc:3209: DfDz32 = dphi32/dz32, no guard; coplanar points → NaN propagates through refinement.
    • AgnosticHelixFinder_module.cc:650, 1641: lambda = 1/dfdz, no check.
    • AgnosticHelixFinder_module.cc:1464: nHitsRatio = seedCircleHits / nLineHits, no nLineHits > 0 guard.
15. Unconditional printf debug output. CalHelixFinder_module.cc:154–165 dumps geometry on every beginRun; AgnosticHelixFinder_module.cc:382, 384, 521, 547, 805, 910 print unconditionally. Gate on _diagLevel / _debug.
16. Magic-sentinel overload. -9999, -1000-idx, -1 are used as both "invalid" and encoded indices (CalHelixFinderAlg.cc:239–242; DeltaFinderAlg.cc:376, 630, 795, 824, 835, 838). Any future >= 0 check could misclassify.

Code quality & convention drift

17. fhicl key casing is inconsistent within CalPatRec itself. CalTimePeakFinder_module.cc binds PascalCase (DiagLevel, DebugLevel, StrawHitCollectionLabel); CalHelixFinder_module.cc, AgnosticHelixFinder_module.cc, DeltaFinder_module.cc, TZClusterFinder_module.cc bind lowerCamel (diagLevel, debugLevel, chCollLabel). User fcl overriding DiagLevel silently misses half the modules.
18. No ProditionsHandle use anywhere in CalPatRec (grep 0 hits). TrkPatRec uses it (RobustHelixFinder_module.cc). CalPatRec fetches geometry via raw GeomHandle<Tracker>, against the current Offline norm for per-run-varying conditions.
19. Per-module .hh headers expose class layout (inc/CalHelixFinder_module.hh, inc/CalTimePeakFinder_module.hh). TrkPatRec keeps module internals inside the .cc. Drift from project norm; accidentally shares ABI.
20. Dictionary registration is incomplete. src/classes_def.xml registers only mu2e::DeltaSeed; DeltaCandidate, ProtonCandidate, CalHelixFinderData, DeltaFinder_types are built into the library but not registered. Persisting any of these to art output throws at runtime. Also, CMake uses NO_CHECK_CLASS_VERSION with a comment "segfaults" — that workaround should be root-caused.
21. src/HlPrint_ComboHit.cc / inc/HlPrint.hh compiled but never used.
22. Stray SConscript in CalPatRec/src/ left over from pre-CMake days (same in TrkPatRec/src/).
23. Offline::BTrkLegacy / ROOT::Physics linked PUBLIC but usage not evident in headers (CalPatRec/CMakeLists.txt). Audit; if only .cc uses, move to PRIVATE or drop.
24. Module-naming drift vs TrkPatRec. TrkPatRec uses TimeClusterFinder / TimeAndPhiClusterFinder; CalPatRec has CalTimePeakFinder, CalLineTimePeakFinder, and PhiClusterFinder (missing Cal prefix though it's in CalPatRec). Grepping for "TimeCluster…" misses the CalPatRec equivalents.
25. using namespace hoisting at file scope. CalHelixFinder_module.cc hoists boost::accumulators plus CLHEP types; CalTimePeakFinder_module.cc has using namespace std; at file scope.
26. Dead __GCCXML__A guard in inc/CalTimePeakFinder_module.hh. GCCXML has been inactive for years; the A suffix suggests it was already neutered.
27. src/CalHelixFinderAlg.cc double-includes its own header.

Dead code & stale assets

28. data/v5_7_7/ (10 files, ~136 KB). Only referenced by configure_file copies in CMakeLists.txt. No source code loads MLP_weights_* or cpr_qual_logfcons*. Delete the directory and the corresponding CMake lines.
29. src/MergePatRecDiag_tool.cc + inc/MergePatRec_types.hh. Zero tool_type: "MergePatRecDiag" hits in any fcl across the repo.
30. inc/ObjectDumpUtils.hh. Zero #include hits anywhere in Offline.
31. test/ (46 files, ~233 KB). No Validation/, Trigger/, Production/, or CalPatRec CMakeLists.txt references any file in this directory except via doc/CalPatRec.org's mention of deltaFinder_debug.fcl. Either delete or move to legacy/ with a README noting none of it is wired into CI.
32. Commented-out blocks in fcl/prolog.fcl — CalTrkFit (~201–225), CalSeedFit (~382–398), MHSeedFit (~408–424), CalHelixFinderDeP/CalHelixFinderDmuP (~651–656). All reference removed KalSeedFit/KalFinalFit.
33. Commented-out algorithm loops in src/CalHelixFinderAlg.cc at lines 2549–2578 (30 lines) and 2993–3013 (21 lines). Replaced by countUsedHits(); the FIXME referenced is resolved.
34. doc/CalPatRec.org is the only file still referencing deltaFinder_debug.fcl. If test/ is deleted, update or remove.

Nits

• Per-file maintainer tags (P.Murat, G.Pezzullo, etc.) — Offline has been moving away from this convention.
• Include-guard style CalPatRec_CalHelixFinder_module (no _hh suffix) diverges from Offline_..._hh used more broadly.
• fcl/prolog.fcl whitespace inconsistency (module_type:X vs module_type : X).
• Aspirational comment // try to order routines alphabetically left in CalHelixFinder_module.cc.
• Commented #include "CalPatRec/inc/KalFitResult.hh" and // std::string _shpLabel; left in headers.