TrkPatRec: Add detailed code review document

[Copilot]

TrkPatRec: Code Review Fixes

Review Summary

Full review of all TrkPatRec source files, headers, FCL configs, and build system files. Issues found are categorized below. This PR addresses the safely fixable issues; remaining items are documented for future work.

---

Changes Made (fixes in this PR)

Bug Fix

• Duplicate histogram fill (RobustHelixFinderDiag_tool.cc:278-279): nseeds[k] was filled twice per helicity iteration — removed the duplicate line

Code Quality

• C-style cast → static_cast (RobustHelixFinderDiag_tool.cc:267): Changed (Data_t*) Data to static_cast<Data_t*>(Data) for type safety
• Duplicate includes removed: Both RobustMultiHelixFinderDiag_tool.cc and TimeAndPhiClusterFinderDiag_tool.cc had duplicate #include "fhiclcpp/ParameterSet.h"
• Unnecessary includes removed: Both diag tools included art/Framework/Core/EDAnalyzer.h which is not needed (these are art::tool, not EDAnalyzer)

Dead Code Removal

• KalSeedFit_types.hh removed: The entire file was wrapped in /* */ (all dead code) — deleted and removed from CMakeLists.txt

FCL Fix

• Duplicate key (prolog.fcl): CaloClusterWeight was set twice in TimeClusterFinder config (lines 32 and 35) — removed the duplicate

Typo Fixes

• "algorhithm" → "algorithm" in RobustMultiHelixFinder_module.cc fhicl comment
• "unrecognixed FitCirclestrategy" → "unrecognized FitCircleStrategy" in exception message
• "veccctor" → "vector" and "llok" → "look" in flagCompton comment
• "fot" → "for" in RobustMultiHelixFinder_types.hh and TimeAndPhiClusterFinder_types.hh

Removed from repo

• REVIEW.md: Review content belongs in this PR description, not committed to the repository

---

Issues Found But NOT Changed (for future work)

High Priority

#	File	Issue
1	RobustMultiHelixFinder_module.cc:346	chi2dXY repurposed as dz/dt slope — the chi2dXY field stores fita_zt_ (dz/dt slope) instead of actual chi2. This is a documented "dirty hack" that corrupts the data model semantics. Requires a data product change to fix properly.
2	RobustHelixFinder_module.cc:1212-1227	updateStereo is a no-op — the method always returns false and contains only a FIXME comment. Either implement or remove.

Medium Priority

#	File	Issue
3	RobustHelixFinder_module.cc:94-97	HelixHitMVA._hrho aliases _dtrans — both reference _pars[0], likely unintended after MVA variable reduction. The commented-out constructor had them at separate indices (7 and 8).
4	Multiple files	Hardcoded pi values with inconsistent precision — 6.2831, 6.28318, 6.283185, 6.2832 used across files. Should use M_PI or a named constexpr.
5	RobustMultiHelixFinder_types.hh	No bounds checking on fixed-size Data_t arrays (128 helices, 8192 hits). Could overflow in busy events.
6	Diag tools	dids[0] access without checking vector is non-empty in fillStrawDigiIndices usage
7	TimeClusterFinder_module.cc	_cccol dereferenced without verifying handle validity when _usecc is true

Low Priority (Code Quality)

#	File	Issue
8	RobustHelixFinder_module.cc, TimeClusterFinder_module.cc	using namespace std at file scope
9	RobustHelixFinder_module.cc	static local variables in member functions (thread-safety concern if ever modified)
10	RobustHelixFinder_module.cc	Unused _printfreq member
11	Multiple files	Commented-out code blocks should be cleaned up
12	Multiple files	Inconsistent naming conventions (leading _member vs trailing member_)
13	Multiple files	Raw pointer initialization with (0) instead of nullptr
14	RobustHelixFinder_module.cc:64	#include <float.h> should be <cfloat>
15	RobustMultiHelixFinder_module.cc:580	unsigned iWorst(-1) wraps to UINT_MAX as sentinel

Positive Observations

• Clean separation of concerns via ModuleHistToolBase diagnostic tools
• Modern fhicl validated configuration throughout
• RobustMultiHelixFinder and TimeAndPhiClusterFinder have clean, modern design
• Proper art framework usage (consumes/produces, ProductToken, art::Ptr)

[FNALbuild]

Hi @Copilot,
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 author of this pull request is not a member of the Mu2e github organisation.

About FNALbuild. Code review on Mu2e/Offline.

[oksuzian]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 0540140.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 0540140 at 5ec09f9
build (prof)	✅	Log file. Build time: 04 min 10 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 7 files
clang-tidy	➡️	8 errors 597 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 0540140 after being merged into the base branch at 5ec09f9.

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.

[brownd1978]

These are innocuous changes.