Add protection in phi peak finding

[michaelmackenzie]

Address the bug reported in issue #1804

[FNALbuild]

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

• CalPatRec

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at fe502d9.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged fe502d9 at 95f0a83
build (prof)	✅	Log file. Build time: 08 min 25 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.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 1 files
clang-tidy	➡️	2 errors 37 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at fe502d9 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]

Review: PR #1832 — Add protection in phi peak finding

This is a small, well-scoped bug fix in PhiClusterFinder::clusterminmax(). Touches one file (CalPatRec/src/PhiClusterFinder_module.cc, +10/-5). No framework / Proditions / Geant4 / fhicl surface changes.

🔴 Critical Issues

None. The original code was buggy in two ways, both now fixed:

• Left-loop wrap condition fixed (PhiClusterFinder_module.cc:403): the previous if (bincheck > _threshold) bincheck--; compared a bin index against a hit-count threshold — clearly wrong (mixed semantics, and would cause incorrect wrap when _threshold > 1). New if (bincheck > 1) bincheck--; correctly wraps at the first ROOT bin (ROOT histogram bins are 1..nbx, with 0 = underflow). ✅
• Right-loop infinite-loop protection added (PhiClusterFinder_module.cc:414-425): the right-side scan previously had no nsteps guard, so if every bin in _hist1 was ≥ _threshold (a hot/uniform event) the while would never terminate. The new nsteps reset + early return mirrors the left side. ✅

🟡 Suggestions

• Symmetry / DRY (PhiClusterFinder_module.cc:402-426): the two while-loops are mirror images. Consider factoring into a small lambda or helper int scanDirection(int start, int step, int nbx) to avoid drift between the two halves in future fixes. Optional, not blocking.
• Termination invariant comment: a one-line comment near line 402 explaining that bins wrap (1 → nbx, nbx → 1) and that the histogram has at least one bin at < _threshold would help future readers understand why the loop terminates in the common case.
• max_bin underflow/overflow guard (line 399): TH1::GetMaximumBin() can in principle return the under/overflow bin if those were filled; here Fill(phi, …) with phi ∈ [0, 2π] should keep it in-range given _phimin/_phimax, but if _phimin/_phimax ever drift this would silently misbehave. Consider assert(max_bin >= 1 && max_bin <= nbx) in debug builds. Minor.

Framework / contracts checklist

• produces<TimeClusterCollection>() ↔ event.put(std::move(tccol1)) — matches.
• consumes<…>() on both input tags — matches getValidHandle usage.
• No new conditions/geometry access; no Geant4 surface; no new ROOT file / TFileService misuse; no fhicl changes.

Verdict: LGTM with the optional refactor suggestion. The fix correctly addresses the infinite-loop / wrong-comparison bug from #1804.

[FNALbuild]

📝 The HEAD of main has changed to 23fc715. Tests are now out of date.