Clean up CPR a bit

[michaelmackenzie]

This includes various code cleaning changes, storing the helix fit results in fields instead of the fitter object, and other changes. I also deleted commented out code in many places as the algorithm has been fairly stable for a while now.

[FNALbuild]

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

• CalPatRec

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 649d2e6.

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

N.B. These results were obtained from a build of this Pull Request at 649d2e6 after being merged into the base branch at 95f0a83.

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]

Code Review — Mu2e/Offline PR #1829: "Clean up CPR a bit"

This PR is mostly a cleanup of CalPatRec: deleting commented-out code, refactoring the diagnostic-fill block of CalHelixFinder::produce into fillDiagnosticInfo, replacing the O(N×M) helix-overlap loop in pickBestHelix with an unordered_set, guarding _helTraj/shLabel removal, gating histogram booking on _diagLevel, and adding new _circle_chisq_dof / _dfdz_chisq_dof cached fit-quality fields on CalHelixFinderData.

Focused review on correctness, framework contracts, and Proditions/services (default focus, since no specific area was requested).

---

🔴 Critical Issues

• CalPatRec/src/CalHelixFinderAlg.cc:1066 — wrong source for _dfdz_chisq_dof (copy/paste bug).
  After a successful doLinearFitPhiZ, the new code assigns the φ-z (dφ/dz line) fit quality from the circle fitter:

  else if (success) {                               // update helix results
    Helix._fz0  = Helix._szphi.phi0();
    Helix._dfdz = Helix._szphi.dfdz();
    Helix._dfdz_chisq_dof = Helix._sxy.chi2DofCircle();   // <-- BUG: this is the XY-circle chi2/dof
  }

  _dfdz_chisq_dof is documented in the header as "dphi / dz fit quality" (CalHelixFinderData.hh L153) and is then compared against _chi2zphiMax in fitHelix (line 346):

  else if ((Helix._nXYSh < _minNHits) || (Helix._circle_chisq_dof > _chi2xyMax)) {
    Helix._fit = TrkErrCode(TrkErrCode::fail,3); // xy reconstruction failure
  }
  else if ((Helix._nZPhiSh < _minNHits) || (Helix._dfdz_chisq_dof > _chi2zphiMax) ||
           (fabs(Helix._dfdz) < _minDfDz) || (fabs(Helix._dfdz) > _maxDfDz)) {
    Helix._fit = TrkErrCode(TrkErrCode::fail,4); // phi-z reconstruction failure
  }

  With the current PR, a helix whose circle fit happens to have chi2/dof > _chi2zphiMax will be rejected with fail,4 even though its actual φ-z line fit is fine; conversely, a track with a poor φ-z line fit but a tight circle fit will sneak past the cut. This will change physics output (_fit success flag, downstream HelixSeed acceptance), not just diagnostics.

  Fix:

  Helix._dfdz_chisq_dof = Helix._szphi.chi2DofLine();

  Rationale: invariant — _dfdz_chisq_dof must always be the φ-z line χ²/dof, since that is the value the success criterion in fitHelix is based on.

---

🟡 Suggestions

• _dfdz_chisq_dof semantics: 0.f means "no chi2" but the cut treats it as "very good".
  In several paths the value is intentionally set to 0.f after findDfDz (which is a histogram-based estimate, not an LSQ line fit):

  • CalHelixFinderAlg.cc L1787 (after findDfDz in doPatternRecognition)
  • CalHelixFinderAlg.cc L2960 and L2987 in findTrack
  • CalHelixFinderAlg.cc L3091 at end of findTrack

  Once the bug above is fixed, 0.f will silently bypass the _chi2zphiMax cut at L346. If that is intentional ("we don't have a line χ² yet, accept on hit count alone"), please add a comment; otherwise consider a sentinel (-1.f) and adjust the cut to (_dfdz_chisq_dof >= 0.f && _dfdz_chisq_dof > _chi2zphiMax). Either way the choice should be explicit, not implicit.

• CalHelixFinder_module.cc — histogram booking now gated, but the corresponding _hmanager may not be safe to call in produce if it was never booked.

  void CalHelixFinder::beginJob(){
    if(_diagLevel != 0) { // only book histograms if running diagnostics
      art::ServiceHandle<art::TFileService> tfs;
      _hmanager->bookHistograms(tfs);
    }
  }

  The fill site is guarded the same way (if (_diagLevel > 0) _hmanager->fillHistograms(&_data);), so this is fine as long as every code path that calls _hmanager->fillHistograms is gated on _diagLevel > 0. The booking gate uses != 0 while the fill gate uses > 0; if _diagLevel can be negative, booking happens but fill doesn't — harmless, but inconsistent. Use the same predicate in both places.

• pickBestHelix early return leaves Index_best uninitialized for the caller.

  if (HelVec.size() < 2) { return; }

  This is preexisting, but since the function is now being touched, please make it explicit — e.g. Index_best = (HelVec.empty() ? -1 : 0); return;. Otherwise the caller's index_best may be whatever default the caller wrote (0), which is correct only by accident.

• fillDiagnosticInfo takes int index_best by value, then mutates it.

  if (index_best == 2){ index_best = 0; }

  Functionally fine (it's a copy), but consider naming the parameter differently or doing the remap in the caller — index_best == 2 means "both helices kept", and silently aliasing it to helix 0 only in diagnostics hides a real semantic question: which seed's diagnostics should be reported when both are kept? Worth a one-line comment.

• Helix overlap comparison uses float threshold against int count.

  constexpr float overlap_threshold(0.5f);
  ...
  if ((natc > nh1*overlap_threshold) || (natc > nh2*overlap_threshold)) {

  Matches the previous natc > nh1/2. semantics, but int * 0.5f for large nh1 is fine and equivalent to nh1/2 only because 0.5 is representable. For clarity and to keep behavior identical across compilers, prefer integer arithmetic: natc * 2 > nh1 (i.e., strictly more than half).

• Header field grouping in CalHelixFinderData.hh — _circle_chisq_dof is placed next to _radius, and _dfdz_chisq_dof next to _fz0. Good. Consider initializing both to a sentinel (e.g., -1.f) in clearTempVariables() / clearResults() (not visible in the diff but worth checking) so a never-fit helix doesn't quietly read 0 and pass the cut.

Summary

One real correctness bug (_dfdz_chisq_dof = Helix._sxy.chi2DofCircle() at L1066 must be _szphi.chi2DofLine()) plus a related semantics question about the 0.f sentinel — both worth fixing before merge because they directly affect the TrkErrCode::fail,4 decision and hence the produced HelixSeedCollection. Everything else is cleanup that improves the code.

[michaelmackenzie]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at eb9ae01.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged eb9ae01 at 23fc715
build (prof)	✅	Log file. Build time: 04 min 07 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 (0) FIXME (29) in 6 files
clang-tidy	➡️	4 errors 776 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at eb9ae01 after being merged into the base branch at 23fc715.

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.