Further checks in MHF

[bechenard]

Additional (and maybe redundant) checks to fix issue 1608

[FNALbuild]

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

• TrkPatRec

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 2b9ae36.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 2b9ae36 at aa12407
build (prof)	✅	Log file. Build time: 04 min 05 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 (0) in 1 files
clang-tidy	➡️	2 errors 159 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 2b9ae36 after being merged into the base branch at aa12407.

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]

I'll review this pull request by first loading the relevant ability, then examining the PR details and code changes.Now let me look at the full file for broader context around these changes:Now I have the full picture. Here's my review of PR #1749 — "Further checks in MHF":

PR Summary

This PR adds 4 lines (0 deletions) to TrkPatRec/src/RobustMultiHelixFinder_module.cc to fix issue #1608. The changes add additional validation checks on fit parameters in two locations.

---

Changes Reviewed

Change 1: In findHelixCandidate() — lines 592–593

    //final dz/dphi and dz/dt refit
    fit_dzdp(helix,chcol,helicity);
    fit_dzdt(helix,chcol);
    if (abs(helix.fita_zp_)<1e-3 || abs(helix.fita_zt_)<1e-3) {helix.hitIndices_.clear(); helix.nStrawHits_=0; return;}
    if (!std::isnormal(helix.fita_zp_)  || !std::isnormal(helix.fita_zt_))  {helix.hitIndices_.clear(); helix.nStrawHits_=0; return;}

Change 2: In fit_dzdp() — line 697

    float fa = zphiFitter.fa();
    float fb = zphiFitter.fb();

    if (fabs(fa) < 1e-3) return;
    if ((helicity==Helicity::poshel && fa<1e-3) || (helicity==Helicity::neghel  && fa>-1e-3)) return;

---

Issues Found

1. 🔴 Inconsistent use of abs() vs fabs() vs std::abs()

• Line 592: Uses abs() (unqualified) for float values
• Line 697: Uses fabs() for float values
• Line 560 (pre-existing): Uses abs() for float values

The unqualified abs() on a float argument can resolve to the C integer abs(int) depending on which headers are included and compiler settings, silently truncating the float to an integer. This would make the check meaningless (e.g., abs(0.5f) → 0). Use std::abs() or std::fabs() consistently for float types. This is a pre-existing issue throughout the file (lines 450, 544, 560, 641, 649, 679, 734, 735, 753, 815), but the new code at line 592 propagates it.

2. 🟡 Redundancy between the new fabs(fa) < 1e-3 check and the existing helicity check in fit_dzdp()

At line 697, the new guard if (fabs(fa) < 1e-3) return; is added before the existing check:

if ((helicity==Helicity::poshel && fa<1e-3) || (helicity==Helicity::neghel && fa>-1e-3)) return;

The existing check already catches fa values near zero for both helicities:

• poshel: returns if fa < 1e-3 (covers fabs(fa) < 1e-3 when fa ≥ 0)
• neghel: returns if fa > -1e-3 (covers fabs(fa) < 1e-3 when fa ≤ 0)

The new check is truly redundant except for the edge case where no data was added to the fitter and fa == 0.0 exactly (since LSFitter::fa() returns 0.0 as a degenerate case). In that scenario, both checks already catch it. The PR description itself acknowledges this may be redundant. This is fine as defensive programming but worth noting.

3. 🟡 std::isnormal() rejects subnormals but also rejects 0.0

At line 593:

if (!std::isnormal(helix.fita_zp_) || !std::isnormal(helix.fita_zt_)) { ... }

std::isnormal() returns false for 0.0, subnormals, infinity, and NaN. This is probably the intended behavior here (zero slopes are already rejected by the preceding line). However, if fit_dzdp returned early without updating the slopes (leaving them at their pre-existing values from init_dzdp), the slopes would retain their previous values — those could potentially be subnormal. This guard is good defensive coding for catching NaN/Inf propagation from degenerate fits.

4. 🟡 Missing <cmath> include for std::isnormal

The file includes <numeric> and <stdint.h>, but std::isnormal is defined in <cmath>. It likely compiles because <cmath> is transitively included by one of the other headers, but for correctness and portability, it should be explicitly included.

5. 🟠 Pre-existing: Typo in filterZPhi — 6.293185 instead of 6.283185 (2π)

This is not introduced by this PR, but is worth flagging. In filterZPhi() at lines 734–735:

int   n      = round((phi-(chcol[ich].pos().z()-circle.fitb_zp_)/circle.fita_zp_)/6.293185);
float delta  = abs((chcol[ich].pos().z()-circle.fitb_zp_)/circle.fita_zp_-phi+n*6.293185);

The constant 6.293185 should be 6.283185 (2π). This appears to be a pre-existing typo that introduces a ~0.16% systematic error in the phi wrapping calculation.

6. 🟠 Pre-existing: Division by zero not guarded in findCircleCandidate

At line 437, denominator = 2*(x2*y3 - x3*y2) can be zero when the three points are collinear. The code divides by denominator on lines 441–442 without checking. Similarly, nsh at line 480 can be zero leading to division by zero in sumChisq /= nsh. These are pre-existing issues.

---

Verdict

The changes are small, focused, and defensive — they guard against degenerate/pathological fit results (near-zero slopes, NaN, Inf) that could cause downstream issues (likely the crash in issue #1608). The logic is correct and appropriate.

Suggested improvements before merging:

1. Replace abs() with std::abs() on line 592 (and ideally throughout the file) to avoid potential integer abs() resolution
2. Consider adding #include <cmath> explicitly for std::isnormal

[FNALbuild]

📝 The HEAD of main has changed to 9ffc000. Tests are now out of date.

[bechenard]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 41bd766.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 41bd766 at 9ffc000
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 (0) in 1 files
clang-tidy	➡️	2 errors 159 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 41bd766 after being merged into the base branch at 9ffc000.

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.