From e4be51fd1baa14ba9958bb7fa00f705cac057171 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 01:19:37 +0000 Subject: [PATCH 1/2] Add detailed code review of TrkPatRec module Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/1cd4187d-9c04-4a32-bdcd-243be1983ccc Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com> --- TrkPatRec/REVIEW.md | 326 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 326 insertions(+) create mode 100644 TrkPatRec/REVIEW.md diff --git a/TrkPatRec/REVIEW.md b/TrkPatRec/REVIEW.md new file mode 100644 index 0000000000..2c717c6da9 --- /dev/null +++ b/TrkPatRec/REVIEW.md @@ -0,0 +1,326 @@ +# TrkPatRec Module — Detailed Code Review + +**Date**: April 2026 +**Scope**: Full review of `TrkPatRec/` — all source files, headers, fcl configuration, and build system. + +--- + +## 1. Module Overview + +`TrkPatRec` implements the tracker pattern recognition chain for the Mu2e experiment. It consists of: + +| Component | Type | Purpose | +|---|---|---| +| `TimeClusterFinder` | art::EDProducer | Finds time clusters in ComboHit collections using histogram peak-finding, MVA refinement, and optional calo cluster seeding | +| `TimeAndPhiClusterFinder` | art::EDProducer | An alternative/newer time+phi clustering algorithm using time histogram scanning and phi-based cluster splitting | +| `RobustHelixFinder` | art::EDProducer | Finds helical tracks by fitting circles and z-phi lines to time-clustered hits (TPR path) | +| `RobustMultiHelixFinder` | art::EDProducer | Alternative helix finder using triplet-based circle search and iterative track extraction (MPR path) | +| `RobustHelixFinderDiag` | art::tool | Diagnostic histograms for `RobustHelixFinder` | +| `RobustMultiHelixFinderDiag` | art::tool | Diagnostic TTree for `RobustMultiHelixFinder` | +| `TimeAndPhiClusterFinderDiag` | art::tool | Diagnostic TTree for `TimeAndPhiClusterFinder` | + +--- + +## 2. Build System + +### CMakeLists.txt +- Clean and well-structured. Uses `cet_make_library(INTERFACE)` for headers and `cet_build_plugin` for each module/tool. +- Dependencies are appropriately declared per-plugin. +- `TimeAndPhiClusterFinder` has a notably minimal dependency set (only `Mu2eUtilities` and `RecoDataProducts`), reflecting its simpler design. + +### SConscript (Legacy) +- Still present but likely only used for legacy Scons builds. Contains commented-out references (`mu2e_CalPatRec`). Should be kept in sync or removed if Scons is no longer supported. + +### Observation +- The `KalSeedFit_types.hh` header is entirely commented out (wrapped in `/* */`) but is still listed (also commented out) in `CMakeLists.txt`. This is dead code that should be cleaned up. + +--- + +## 3. Detailed File-Level Review + +### 3.1 `RobustHelixFinder_module.cc` (~1339 lines) + +This is the largest and most complex file in the module. + +#### Architecture +The module ingests `ComboHitCollection` and `TimeClusterCollection`, then for each time cluster: +1. Fills face-ordered hits (`fillFaceOrderedHits`) +2. Optionally prefilters hits by azimuth (`prefilterHits`) +3. Fits a circle (`_hfit.fitCircle`) +4. Loops over helicities, fitting full helixes (`fitHelix`) including iterative circle and z-phi fitting with outlier removal +5. Picks the best helix via `pickBestHelix` +6. Optionally applies MVA-based hit filtering + +#### Issues Found + +**Bug: Duplicate fill in diagnostic code (line 278–279)** +```cpp +_hist.nseeds[k]->Fill(_data->nseeds[k]); +_hist.nseeds[k]->Fill(_data->nseeds[k]); // duplicate fill +``` +In `RobustHelixFinderDiag::fillHistograms`, `nseeds` is filled twice per iteration. This appears to be a copy-paste bug. + +**Thread-safety concern: `static` local variables (lines 574, 648, 964, 1213)** +```cpp +static XYZVectorF zaxis(0.0,0.0,1.0); +static StrawHitFlag stereo(StrawHitFlag::stereo); +``` +Multiple methods use function-scope `static` variables. While these are const-like in practice (constructed once, never modified), the pattern is fragile. If a `static XYZVectorF` were ever modified, there would be data races in multi-threaded art scheduling. Consider making these `const` members or `constexpr` where possible, or at a minimum `const static`. + +**`using namespace std` at file scope (line 68)** +This pollutes the global namespace across the entire translation unit and can cause subtle name collisions. It is used in both `RobustHelixFinder_module.cc` and `TimeClusterFinder_module.cc`. + +**HelixHitMVA aliasing issue (lines 94–97)** +```cpp +HelixHitMVA() : _pars(7,0.0),_pars2(2,0.0), + _dtrans(_pars[0]),_dwire(_pars[1]),_chisq(_pars[2]),_dt(_pars[3]), + _drho(_pars[4]),_dphi(_pars[5]),_rwdot(_pars[6]), + _hrho(_pars[0]),_hhrho(_pars2[1]) {} +``` +`_hrho` aliases `_pars[0]` (same as `_dtrans`). This means writing to `_hrho` overwrites `_dtrans`. The commented-out constructor shows these were originally separate indices (7 and 8). The current aliasing means these two semantically different quantities share storage. This should be documented if intentional, or fixed if not. + +**Unused member `_printfreq` (line 163/242)** +The `_printfreq` member is initialized from config but never actually used in `produce()` or elsewhere. In contrast, `TimeClusterFinder` does use its `_printfreq` for debug output. + +**`updateStereo` is a no-op (lines 1212–1227)** +The method `updateStereo` always returns `false` and never actually updates anything. The body contains a commented-out implementation with a `// needs re-implementing with ComboHits FIXME!` note. This function should either be implemented or the call site should be removed/disabled. + +**`pickBestHelix` unreachable code (line 522)** +The condition `if (nh1 == nh2)` on line 522 is always true at that point because the `nh2 > nh1` and `nh1 > nh2` cases have already returned. The `if` guard is redundant. + +**Division by `chCounter` initialized to `1e-10` (lines 656, 971)** +`chCounter` is initialized to `1e-10` rather than zero to avoid division by zero. This means that if no hits pass filters, the chi2 values will be extremely large (divided by `1e-10`) rather than yielding a divide-by-zero. While functional, this is an unusual pattern that should be documented, or better, checked explicitly. + +**`float.h` include (line 64)** +Should be `` for C++ code. + +**Raw pointer patterns** +Multiple raw pointers initialized to `nullptr` via `(0)`: +```cpp +ComboHit* hit(0); +ComboHit* worsthit(0); +``` +Prefer `nullptr` for clarity. + +--- + +### 3.2 `RobustMultiHelixFinder_module.cc` (~1053 lines) + +#### Architecture +A more modern helix finder that: +1. Iterates over time clusters +2. For each, performs triplet-based circle finding (`findCircleCandidate`) +3. Fits z-phi and z-t linear relations (`init_dzdp`, `fit_dzdp`, `fit_dzdt`) +4. Iteratively refines circles and removes bad hits +5. Filters duplicate helices + +#### Issues Found + +**Dirty hack documented but present (line 346–347)** +```cpp +//Dirty hack to save particle propagation direction, will be gone when we have updated the data products +chi2dXY = bestHelix.fita_zt_; +``` +The `chi2dXY` field of `RobustHelix` is being repurposed to store `dz/dt` slope (`fita_zt_`). This means the `chi2dXY` value is **not** actually a chi2 for helices produced by MPR. This is a significant semantic issue that affects any downstream code inspecting `chi2dXY`. The diagnostic code on line 1022 and 1041 stores this overloaded value via `heldzdt_` — confirming the intent but not the cleanliness. + +**Hardcoded pi approximations** +Many instances of `6.283185` (≈ 2π), `6.2831`, `6.28318`, and `6.2832` are used inconsistently: +- `flagCompton`: uses `6.28318` and `6.2831` +- `init_dzdp`, `fit_dzdp`: uses `6.283185` +- The differences in precision (`6.2831` vs `6.283185` vs `6.28318`) could cause subtle numerical inconsistencies. + +**Recommendation**: Use `M_PI` from `` or `CLHEP::twopi` consistently. A `constexpr float twopi = 2.0f * M_PI;` at namespace scope would be clearer. + +**O(n³) triplet search in `findCircleCandidate` (lines 408–499)** +The circle candidate finder uses a triple-nested loop over all hits. For large hit collections this scales cubically. While the hit count within a time cluster is typically bounded, this could become a bottleneck. There's no early-exit optimization beyond the `minDXY2Circle_` distance cut. + +**Potential out-of-bounds in `Data_t` arrays** +`Data_t` has fixed-size arrays: `helhel_[128]`, `helrad_[128]`, etc. The `Nhel_` counter is incremented without bounds checking. If more than 128 helices are found (unlikely but possible in busy events), this would cause a buffer overflow. Similarly, `kMaxHits = 8192` in various `Data_t` structs — if events exceed this, there's no protection. + +**`unsigned iWorst(-1)` (line 580)** +Initializing an `unsigned` to `-1` wraps to `UINT_MAX`, which works as a sentinel but is confusing. Consider using `std::optional` or a signed type. + +**Missing `beginRun` for geometry setup** +The calorimeter geometry is obtained in `findAllHelices` via `GeomHandle()` on every event. `RobustHelixFinder` does this in `beginRun`. Moving it to `beginRun` in `RobustMultiHelixFinder` would be more efficient. + +**Typo in comments** +- Line 101: `"Fit Circle algorhithm"` → `"algorithm"` +- Line 213: `"FitCirclestrategy"` → `"FitCircleStrategy"` +- Line 532: `"veccctor"` and `"llok"` (in `flagCompton`) +- Line 16 in `RobustMultiHelixFinder_types.hh`: `"Needed fot backward compatibility"` → `"for"` + +--- + +### 3.3 `TimeAndPhiClusterFinder_module.cc` (~623 lines) + +#### Architecture +A cleaner, more modern design than `TimeClusterFinder`. Uses a local max scanning algorithm on a time histogram, then optionally splits clusters in phi, and optionally applies MVA filtering. + +#### Issues Found + +**Magic numbers in phi wrapping** +Multiple occurrences of `6.2832` and `3.14159` (lines 403, 418, 464, 465). Same recommendation as above — use named constants. + +**`hit = chcol.size()+1` used as sentinel (line 474)** +```cpp +if (outMVA < minCutMVA_) {cand.nsh_ -= ch.nStrawHits(); hit = chcol.size()+1;} +``` +Filtering is done by setting the hit index to an invalid value (`chcol.size()+1`), then erasing. This is fragile — if `StrawHitIndex` changes type or if the collection size approaches `UINT_MAX`, this breaks. A boolean flag vector would be safer. + +**Comment typo (line 220)** +```cpp +//----------------------------------------------------------ch---------------------------------------------------- +``` +Appears to be an accidental insertion of "ch" in the separator comment. Same on line 547. + +--- + +### 3.4 `TimeClusterFinder_module.cc` (~588 lines) + +#### Architecture +The original time cluster finder. Uses a TH1F time spectrum, peak-finding, MVA-based refinement, prefiltering by phi, and optional hit recovery. + +#### Issues Found + +**`_timespec` as a member TH1F** +The module holds a `TH1F _timespec` as a direct member variable (not pointer), which is reset and reused every event. This is unusual in art modules and could cause issues with ROOT's global directory management. It works but is not conventional. + +**Calo cluster energy compared after `isNonnull` check but without energy check in `findCaloSeeds` (line 288)** +```cpp +if (calo.energyDep() > _ccmine) +``` +This is correct, but `_cccol` is dereferenced without checking that `ccH.isValid()`. If `_usecc` is true but the collection handle is invalid, this would crash. The `ccH = event.getHandle(...)` call doesn't guarantee the handle is valid — it returns an invalid handle if the product is missing. + +**`using namespace std` at file scope (line 42)** +Same concern as in `RobustHelixFinder`. + +--- + +### 3.5 Header Files + +#### `RobustHelixFinder_types.hh` +- Large fixed-size arrays: `kMaxSeeds = 100`, `kMaxNHits = 500`. The 3D array `hitDr[2][100][500]` and `hitRWDot[2][100][500]` consume `2×100×500×8 = 800 KB` of `double` data each (1.6 MB total for these two alone), plus all the other arrays. The total `Data_t` struct is approximately **3+ MB**. This is allocated on the stack as a module member, which is fine, but the size is notable. +- `maxSeeds()` is not `const` (line 104). + +#### `RobustMultiHelixFinder_types.hh` +- Reasonable. Fixed arrays (`kMaxHits = 8192`) with `Int_t`/`Float_t` ROOT types for TTree compatibility. +- `reset()` method correctly clears vectors and counters. + +#### `TimeAndPhiClusterFinder_types.hh` +- Same pattern as above. Typo: `"fot"` → `"for"` (line 16). + +#### `KalSeedFit_types.hh` +- **Entirely dead code** — the entire file is wrapped in `/* */`. Should be removed. + +#### `TrkHitFilter.hh` +- Simple 18-line struct. Uses `CLHEP::Hep3Vector` and ROOT `Float_t`. Not referenced by any module in TrkPatRec. May be used externally. + +--- + +### 3.6 Diagnostic Tools + +#### `RobustHelixFinderDiag_tool.cc` +- Very large histogram booking function (~130 histograms). Well-structured but the sheer volume makes maintenance challenging. +- **Bug**: Double fill of `nseeds[k]` (lines 278-279) as noted above. +- Uses `void*` cast for data: `_data = (Data_t*) Data;` — should use `static_cast`. + +#### `RobustMultiHelixFinderDiag_tool.cc` and `TimeAndPhiClusterFinderDiag_tool.cc` +- Use `static_cast` correctly for the data pointer (good). +- Both have `#ifndef` include guards but use `DEFINE_ART_CLASS_TOOL` at the end, which is unusual for a file that has include guards — these are `.cc` files that should not be included. The include guards are unnecessary (but harmless). +- Both have duplicate `#include "fhiclcpp/ParameterSet.h"` (lines 7 and 18 in both files). +- Both include `EDAnalyzer.h` which is not needed for tools (these are `art::tool`, not `EDAnalyzer`). +- The MC truth filling in both diag tools has the comment `// taking 1st digi: is there a better idea??` — this should be resolved or documented as a known limitation. + +--- + +### 3.7 FCL Configuration + +#### `prolog.fcl` +- Well-organized with particle-specific configurations (De, Ue, Dmu, Umu, Dpi, Upi). +- Duplicate key in `TimeClusterFinder`: `CaloClusterWeight` is set twice (lines 33 and 36). FCL takes the last value, but this is confusing. +- Comment on line 179: `"NOt needed anymore"` — typo, and if truly not needed, the referenced code should be cleaned up. + +#### `AmbigResolver.fcl`, `PanelAmbigResolver.fcl`, `DoubletAmbigResolver.fcl` +- These appear to be for downstream fitting, not directly used by the TrkPatRec modules themselves. They're referenced via includes from other packages. + +--- + +## 4. Cross-Cutting Concerns + +### 4.1 Code Style Inconsistencies +- **Naming**: Mixed conventions — `_memberVar` (leading underscore) in some classes, `memberVar_` (trailing underscore) in others (`RobustMultiHelixFinder` uses trailing, `RobustHelixFinder` uses leading). +- **Indentation**: Generally consistent within files but varies between files. +- **Braces**: Some files use Allman style, others K&R. + +### 4.2 Memory Safety +- `Data_t` structs use fixed-size C-style arrays with no bounds checking. The diagnostic fill code in `fillPluginDiag` (RobustHelixFinder) does check `loc < _data.maxSeeds()`, but the diagnostic tools for Multi and TimeAndPhi don't check array bounds before filling TTree arrays. +- The `fillStrawDigiIndices` call in diag tools accesses `dids[0]` without checking that the vector is non-empty. If a ComboHit has no underlying straw digi indices, this would crash. + +### 4.3 Numerical Robustness +- Hardcoded pi values with varying precision (see Section 3.2). +- Division-by-zero guards via `1e-10` initialization (see Section 3.1). +- `abs()` vs `fabs()` — some places use `abs()` on floats, which in C++ calls `std::abs` for `float`/`double` (correct in C++11+), but mixing with `fabs()` in the same file is inconsistent. + +### 4.4 Commented-Out Code +- `KalSeedFit_types.hh` — entire file is commented out. +- Various commented-out code blocks in `RobustHelixFinder_module.cc` (e.g., lines 94–95, 400, 744, 856–857, 905, 1001, 1025). +- Commented-out `#include "RecoDataProducts/inc/KalSeed.hh"` in `RobustHelixFinder_types.hh` (line 20). + +--- + +## 5. Summary of Findings by Severity + +### High Priority +| # | File | Issue | +|---|---|---| +| 1 | `RobustMultiHelixFinder_module.cc:346` | **`chi2dXY` repurposed as `dz/dt` slope** — semantic corruption of the data model | +| 2 | `RobustHelixFinder_module.cc:1212-1227` | **`updateStereo` is a no-op with FIXME** — dead code path that silently does nothing | +| 3 | `RobustHelixFinderDiag_tool.cc:278-279` | **Duplicate histogram fill** — `nseeds` filled twice per helicity | + +### Medium Priority +| # | File | Issue | +|---|---|---| +| 4 | `RobustHelixFinder_module.cc:94-97` | `HelixHitMVA._hrho` aliases `_dtrans` — likely unintended | +| 5 | Multiple files | Hardcoded pi values with inconsistent precision | +| 6 | `RobustMultiHelixFinder_module.cc` | No bounds checking on `Data_t` fixed-size arrays (128 helices, 8192 hits) | +| 7 | Diag tools | `dids[0]` access without checking vector is non-empty | +| 8 | `TimeClusterFinder_module.cc:288` | `_cccol` dereferenced without verifying handle validity | +| 9 | `prolog.fcl:33,36` | Duplicate `CaloClusterWeight` key | + +### Low Priority (Code Quality) +| # | File | Issue | +|---|---|---| +| 10 | Multiple files | `using namespace std` at file scope | +| 11 | Multiple files | `static` local variables in member functions (thread-safety concern) | +| 12 | `KalSeedFit_types.hh` | Entirely dead code (commented out) | +| 13 | Multiple files | Commented-out code blocks should be removed | +| 14 | Multiple files | Inconsistent naming conventions (leading vs trailing underscore) | +| 15 | Multiple files | Typos in comments and parameter descriptions | +| 16 | Multiple files | Raw pointer initialization with `(0)` instead of `nullptr` | +| 17 | Diag tools | Unnecessary include guards on `.cc` files; duplicate includes | +| 18 | `RobustHelixFinder_module.cc` | Unused `_printfreq` member | +| 19 | `RobustHelixFinder_module.cc:522` | Redundant `if (nh1 == nh2)` condition | +| 20 | `RobustMultiHelixFinder_module.cc:580` | `unsigned iWorst(-1)` wraps to `UINT_MAX` | + +--- + +## 6. Positive Observations + +- **Clean separation of concerns**: Diagnostic tools are properly factored into separate art::tools using the `ModuleHistToolBase` interface, keeping the main module code focused on physics. +- **Configurable via fhicl**: All modules use the modern fhicl validated configuration (`fhicl::Atom`, `fhicl::Table`) rather than raw `ParameterSet::get()`. +- **`RobustMultiHelixFinder` is well-designed**: The MPR path has cleaner code structure, proper use of `const`, sensible abstractions (`CandHelix`, `LSFitter`), and fewer legacy artifacts than TPR. +- **`TimeAndPhiClusterFinder` is clean and modern**: Minimal dependencies, clear algorithm flow, good use of lambdas and STL algorithms. +- **Comprehensive diagnostic capabilities**: All modules support optional diagnostic output controlled by `DiagLevel`, with no overhead when diagnostics are off. +- **Proper art framework usage**: Correct use of `consumes`/`produces`, `ProductToken`, `art::Ptr`, and `ValidHandle`. + +--- + +## 7. Recommendations + +1. **Address the `chi2dXY` semantic corruption in MPR** by adding a proper field to the `RobustHelix` data product for propagation direction, or use a separate output product. +2. **Either implement or remove `updateStereo`** in `RobustHelixFinder`. The FIXME has been pending for a long time. +3. **Fix the duplicate `nseeds` fill** in the diagnostic tool. +4. **Introduce a named constant for 2π** and use it consistently across all files. +5. **Add bounds checking** to diagnostic `Data_t` array fills, particularly in `RobustMultiHelixFinderDiag` and `TimeAndPhiClusterFinderDiag`. +6. **Remove dead code**: `KalSeedFit_types.hh` and various commented-out blocks. +7. **Eliminate `using namespace std`** at file scope in favor of qualified names or targeted `using` declarations. From 054014083f58081f68e8b5e5fc77028671705d9c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 01:30:28 +0000 Subject: [PATCH 2/2] TrkPatRec: fix review issues - remove REVIEW.md, fix duplicate histogram fill, fix typos, remove dead code, clean includes - Remove REVIEW.md (review content moved to PR description) - Fix duplicate nseeds histogram fill in RobustHelixFinderDiag_tool.cc - Replace C-style cast with static_cast in RobustHelixFinderDiag_tool.cc - Remove duplicate #include fhiclcpp/ParameterSet.h from diag tools - Remove unnecessary #include EDAnalyzer.h from diag tools - Fix typos: algorhithm, FitCirclestrategy, unrecognixed, veccctor, llok, fot - Remove duplicate CaloClusterWeight key in prolog.fcl - Remove dead code file KalSeedFit_types.hh and its CMakeLists.txt entry Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/c5bd8c31-be93-4ec7-b9db-c6d87aa92206 Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com> --- TrkPatRec/CMakeLists.txt | 1 - TrkPatRec/REVIEW.md | 326 ------------------ TrkPatRec/fcl/prolog.fcl | 1 - TrkPatRec/inc/KalSeedFit_types.hh | 39 --- TrkPatRec/inc/RobustMultiHelixFinder_types.hh | 2 +- .../inc/TimeAndPhiClusterFinder_types.hh | 2 +- TrkPatRec/src/RobustHelixFinderDiag_tool.cc | 3 +- .../src/RobustMultiHelixFinderDiag_tool.cc | 2 - .../src/RobustMultiHelixFinder_module.cc | 6 +- .../src/TimeAndPhiClusterFinderDiag_tool.cc | 2 - 10 files changed, 6 insertions(+), 378 deletions(-) delete mode 100644 TrkPatRec/REVIEW.md delete mode 100644 TrkPatRec/inc/KalSeedFit_types.hh diff --git a/TrkPatRec/CMakeLists.txt b/TrkPatRec/CMakeLists.txt index 8c233580f4..e864ebf70e 100644 --- a/TrkPatRec/CMakeLists.txt +++ b/TrkPatRec/CMakeLists.txt @@ -1,7 +1,6 @@ cet_make_library(INTERFACE INSTALLED_PATH_BASE Offline SOURCE #inc/KalFinalFit_types.hh - inc/KalSeedFit_types.hh inc/RobustHelixFinder_types.hh inc/RobustMultiHelixFinder_types.hh inc/TimeAndPhiClusterFinder_types.hh diff --git a/TrkPatRec/REVIEW.md b/TrkPatRec/REVIEW.md deleted file mode 100644 index 2c717c6da9..0000000000 --- a/TrkPatRec/REVIEW.md +++ /dev/null @@ -1,326 +0,0 @@ -# TrkPatRec Module — Detailed Code Review - -**Date**: April 2026 -**Scope**: Full review of `TrkPatRec/` — all source files, headers, fcl configuration, and build system. - ---- - -## 1. Module Overview - -`TrkPatRec` implements the tracker pattern recognition chain for the Mu2e experiment. It consists of: - -| Component | Type | Purpose | -|---|---|---| -| `TimeClusterFinder` | art::EDProducer | Finds time clusters in ComboHit collections using histogram peak-finding, MVA refinement, and optional calo cluster seeding | -| `TimeAndPhiClusterFinder` | art::EDProducer | An alternative/newer time+phi clustering algorithm using time histogram scanning and phi-based cluster splitting | -| `RobustHelixFinder` | art::EDProducer | Finds helical tracks by fitting circles and z-phi lines to time-clustered hits (TPR path) | -| `RobustMultiHelixFinder` | art::EDProducer | Alternative helix finder using triplet-based circle search and iterative track extraction (MPR path) | -| `RobustHelixFinderDiag` | art::tool | Diagnostic histograms for `RobustHelixFinder` | -| `RobustMultiHelixFinderDiag` | art::tool | Diagnostic TTree for `RobustMultiHelixFinder` | -| `TimeAndPhiClusterFinderDiag` | art::tool | Diagnostic TTree for `TimeAndPhiClusterFinder` | - ---- - -## 2. Build System - -### CMakeLists.txt -- Clean and well-structured. Uses `cet_make_library(INTERFACE)` for headers and `cet_build_plugin` for each module/tool. -- Dependencies are appropriately declared per-plugin. -- `TimeAndPhiClusterFinder` has a notably minimal dependency set (only `Mu2eUtilities` and `RecoDataProducts`), reflecting its simpler design. - -### SConscript (Legacy) -- Still present but likely only used for legacy Scons builds. Contains commented-out references (`mu2e_CalPatRec`). Should be kept in sync or removed if Scons is no longer supported. - -### Observation -- The `KalSeedFit_types.hh` header is entirely commented out (wrapped in `/* */`) but is still listed (also commented out) in `CMakeLists.txt`. This is dead code that should be cleaned up. - ---- - -## 3. Detailed File-Level Review - -### 3.1 `RobustHelixFinder_module.cc` (~1339 lines) - -This is the largest and most complex file in the module. - -#### Architecture -The module ingests `ComboHitCollection` and `TimeClusterCollection`, then for each time cluster: -1. Fills face-ordered hits (`fillFaceOrderedHits`) -2. Optionally prefilters hits by azimuth (`prefilterHits`) -3. Fits a circle (`_hfit.fitCircle`) -4. Loops over helicities, fitting full helixes (`fitHelix`) including iterative circle and z-phi fitting with outlier removal -5. Picks the best helix via `pickBestHelix` -6. Optionally applies MVA-based hit filtering - -#### Issues Found - -**Bug: Duplicate fill in diagnostic code (line 278–279)** -```cpp -_hist.nseeds[k]->Fill(_data->nseeds[k]); -_hist.nseeds[k]->Fill(_data->nseeds[k]); // duplicate fill -``` -In `RobustHelixFinderDiag::fillHistograms`, `nseeds` is filled twice per iteration. This appears to be a copy-paste bug. - -**Thread-safety concern: `static` local variables (lines 574, 648, 964, 1213)** -```cpp -static XYZVectorF zaxis(0.0,0.0,1.0); -static StrawHitFlag stereo(StrawHitFlag::stereo); -``` -Multiple methods use function-scope `static` variables. While these are const-like in practice (constructed once, never modified), the pattern is fragile. If a `static XYZVectorF` were ever modified, there would be data races in multi-threaded art scheduling. Consider making these `const` members or `constexpr` where possible, or at a minimum `const static`. - -**`using namespace std` at file scope (line 68)** -This pollutes the global namespace across the entire translation unit and can cause subtle name collisions. It is used in both `RobustHelixFinder_module.cc` and `TimeClusterFinder_module.cc`. - -**HelixHitMVA aliasing issue (lines 94–97)** -```cpp -HelixHitMVA() : _pars(7,0.0),_pars2(2,0.0), - _dtrans(_pars[0]),_dwire(_pars[1]),_chisq(_pars[2]),_dt(_pars[3]), - _drho(_pars[4]),_dphi(_pars[5]),_rwdot(_pars[6]), - _hrho(_pars[0]),_hhrho(_pars2[1]) {} -``` -`_hrho` aliases `_pars[0]` (same as `_dtrans`). This means writing to `_hrho` overwrites `_dtrans`. The commented-out constructor shows these were originally separate indices (7 and 8). The current aliasing means these two semantically different quantities share storage. This should be documented if intentional, or fixed if not. - -**Unused member `_printfreq` (line 163/242)** -The `_printfreq` member is initialized from config but never actually used in `produce()` or elsewhere. In contrast, `TimeClusterFinder` does use its `_printfreq` for debug output. - -**`updateStereo` is a no-op (lines 1212–1227)** -The method `updateStereo` always returns `false` and never actually updates anything. The body contains a commented-out implementation with a `// needs re-implementing with ComboHits FIXME!` note. This function should either be implemented or the call site should be removed/disabled. - -**`pickBestHelix` unreachable code (line 522)** -The condition `if (nh1 == nh2)` on line 522 is always true at that point because the `nh2 > nh1` and `nh1 > nh2` cases have already returned. The `if` guard is redundant. - -**Division by `chCounter` initialized to `1e-10` (lines 656, 971)** -`chCounter` is initialized to `1e-10` rather than zero to avoid division by zero. This means that if no hits pass filters, the chi2 values will be extremely large (divided by `1e-10`) rather than yielding a divide-by-zero. While functional, this is an unusual pattern that should be documented, or better, checked explicitly. - -**`float.h` include (line 64)** -Should be `` for C++ code. - -**Raw pointer patterns** -Multiple raw pointers initialized to `nullptr` via `(0)`: -```cpp -ComboHit* hit(0); -ComboHit* worsthit(0); -``` -Prefer `nullptr` for clarity. - ---- - -### 3.2 `RobustMultiHelixFinder_module.cc` (~1053 lines) - -#### Architecture -A more modern helix finder that: -1. Iterates over time clusters -2. For each, performs triplet-based circle finding (`findCircleCandidate`) -3. Fits z-phi and z-t linear relations (`init_dzdp`, `fit_dzdp`, `fit_dzdt`) -4. Iteratively refines circles and removes bad hits -5. Filters duplicate helices - -#### Issues Found - -**Dirty hack documented but present (line 346–347)** -```cpp -//Dirty hack to save particle propagation direction, will be gone when we have updated the data products -chi2dXY = bestHelix.fita_zt_; -``` -The `chi2dXY` field of `RobustHelix` is being repurposed to store `dz/dt` slope (`fita_zt_`). This means the `chi2dXY` value is **not** actually a chi2 for helices produced by MPR. This is a significant semantic issue that affects any downstream code inspecting `chi2dXY`. The diagnostic code on line 1022 and 1041 stores this overloaded value via `heldzdt_` — confirming the intent but not the cleanliness. - -**Hardcoded pi approximations** -Many instances of `6.283185` (≈ 2π), `6.2831`, `6.28318`, and `6.2832` are used inconsistently: -- `flagCompton`: uses `6.28318` and `6.2831` -- `init_dzdp`, `fit_dzdp`: uses `6.283185` -- The differences in precision (`6.2831` vs `6.283185` vs `6.28318`) could cause subtle numerical inconsistencies. - -**Recommendation**: Use `M_PI` from `` or `CLHEP::twopi` consistently. A `constexpr float twopi = 2.0f * M_PI;` at namespace scope would be clearer. - -**O(n³) triplet search in `findCircleCandidate` (lines 408–499)** -The circle candidate finder uses a triple-nested loop over all hits. For large hit collections this scales cubically. While the hit count within a time cluster is typically bounded, this could become a bottleneck. There's no early-exit optimization beyond the `minDXY2Circle_` distance cut. - -**Potential out-of-bounds in `Data_t` arrays** -`Data_t` has fixed-size arrays: `helhel_[128]`, `helrad_[128]`, etc. The `Nhel_` counter is incremented without bounds checking. If more than 128 helices are found (unlikely but possible in busy events), this would cause a buffer overflow. Similarly, `kMaxHits = 8192` in various `Data_t` structs — if events exceed this, there's no protection. - -**`unsigned iWorst(-1)` (line 580)** -Initializing an `unsigned` to `-1` wraps to `UINT_MAX`, which works as a sentinel but is confusing. Consider using `std::optional` or a signed type. - -**Missing `beginRun` for geometry setup** -The calorimeter geometry is obtained in `findAllHelices` via `GeomHandle()` on every event. `RobustHelixFinder` does this in `beginRun`. Moving it to `beginRun` in `RobustMultiHelixFinder` would be more efficient. - -**Typo in comments** -- Line 101: `"Fit Circle algorhithm"` → `"algorithm"` -- Line 213: `"FitCirclestrategy"` → `"FitCircleStrategy"` -- Line 532: `"veccctor"` and `"llok"` (in `flagCompton`) -- Line 16 in `RobustMultiHelixFinder_types.hh`: `"Needed fot backward compatibility"` → `"for"` - ---- - -### 3.3 `TimeAndPhiClusterFinder_module.cc` (~623 lines) - -#### Architecture -A cleaner, more modern design than `TimeClusterFinder`. Uses a local max scanning algorithm on a time histogram, then optionally splits clusters in phi, and optionally applies MVA filtering. - -#### Issues Found - -**Magic numbers in phi wrapping** -Multiple occurrences of `6.2832` and `3.14159` (lines 403, 418, 464, 465). Same recommendation as above — use named constants. - -**`hit = chcol.size()+1` used as sentinel (line 474)** -```cpp -if (outMVA < minCutMVA_) {cand.nsh_ -= ch.nStrawHits(); hit = chcol.size()+1;} -``` -Filtering is done by setting the hit index to an invalid value (`chcol.size()+1`), then erasing. This is fragile — if `StrawHitIndex` changes type or if the collection size approaches `UINT_MAX`, this breaks. A boolean flag vector would be safer. - -**Comment typo (line 220)** -```cpp -//----------------------------------------------------------ch---------------------------------------------------- -``` -Appears to be an accidental insertion of "ch" in the separator comment. Same on line 547. - ---- - -### 3.4 `TimeClusterFinder_module.cc` (~588 lines) - -#### Architecture -The original time cluster finder. Uses a TH1F time spectrum, peak-finding, MVA-based refinement, prefiltering by phi, and optional hit recovery. - -#### Issues Found - -**`_timespec` as a member TH1F** -The module holds a `TH1F _timespec` as a direct member variable (not pointer), which is reset and reused every event. This is unusual in art modules and could cause issues with ROOT's global directory management. It works but is not conventional. - -**Calo cluster energy compared after `isNonnull` check but without energy check in `findCaloSeeds` (line 288)** -```cpp -if (calo.energyDep() > _ccmine) -``` -This is correct, but `_cccol` is dereferenced without checking that `ccH.isValid()`. If `_usecc` is true but the collection handle is invalid, this would crash. The `ccH = event.getHandle(...)` call doesn't guarantee the handle is valid — it returns an invalid handle if the product is missing. - -**`using namespace std` at file scope (line 42)** -Same concern as in `RobustHelixFinder`. - ---- - -### 3.5 Header Files - -#### `RobustHelixFinder_types.hh` -- Large fixed-size arrays: `kMaxSeeds = 100`, `kMaxNHits = 500`. The 3D array `hitDr[2][100][500]` and `hitRWDot[2][100][500]` consume `2×100×500×8 = 800 KB` of `double` data each (1.6 MB total for these two alone), plus all the other arrays. The total `Data_t` struct is approximately **3+ MB**. This is allocated on the stack as a module member, which is fine, but the size is notable. -- `maxSeeds()` is not `const` (line 104). - -#### `RobustMultiHelixFinder_types.hh` -- Reasonable. Fixed arrays (`kMaxHits = 8192`) with `Int_t`/`Float_t` ROOT types for TTree compatibility. -- `reset()` method correctly clears vectors and counters. - -#### `TimeAndPhiClusterFinder_types.hh` -- Same pattern as above. Typo: `"fot"` → `"for"` (line 16). - -#### `KalSeedFit_types.hh` -- **Entirely dead code** — the entire file is wrapped in `/* */`. Should be removed. - -#### `TrkHitFilter.hh` -- Simple 18-line struct. Uses `CLHEP::Hep3Vector` and ROOT `Float_t`. Not referenced by any module in TrkPatRec. May be used externally. - ---- - -### 3.6 Diagnostic Tools - -#### `RobustHelixFinderDiag_tool.cc` -- Very large histogram booking function (~130 histograms). Well-structured but the sheer volume makes maintenance challenging. -- **Bug**: Double fill of `nseeds[k]` (lines 278-279) as noted above. -- Uses `void*` cast for data: `_data = (Data_t*) Data;` — should use `static_cast`. - -#### `RobustMultiHelixFinderDiag_tool.cc` and `TimeAndPhiClusterFinderDiag_tool.cc` -- Use `static_cast` correctly for the data pointer (good). -- Both have `#ifndef` include guards but use `DEFINE_ART_CLASS_TOOL` at the end, which is unusual for a file that has include guards — these are `.cc` files that should not be included. The include guards are unnecessary (but harmless). -- Both have duplicate `#include "fhiclcpp/ParameterSet.h"` (lines 7 and 18 in both files). -- Both include `EDAnalyzer.h` which is not needed for tools (these are `art::tool`, not `EDAnalyzer`). -- The MC truth filling in both diag tools has the comment `// taking 1st digi: is there a better idea??` — this should be resolved or documented as a known limitation. - ---- - -### 3.7 FCL Configuration - -#### `prolog.fcl` -- Well-organized with particle-specific configurations (De, Ue, Dmu, Umu, Dpi, Upi). -- Duplicate key in `TimeClusterFinder`: `CaloClusterWeight` is set twice (lines 33 and 36). FCL takes the last value, but this is confusing. -- Comment on line 179: `"NOt needed anymore"` — typo, and if truly not needed, the referenced code should be cleaned up. - -#### `AmbigResolver.fcl`, `PanelAmbigResolver.fcl`, `DoubletAmbigResolver.fcl` -- These appear to be for downstream fitting, not directly used by the TrkPatRec modules themselves. They're referenced via includes from other packages. - ---- - -## 4. Cross-Cutting Concerns - -### 4.1 Code Style Inconsistencies -- **Naming**: Mixed conventions — `_memberVar` (leading underscore) in some classes, `memberVar_` (trailing underscore) in others (`RobustMultiHelixFinder` uses trailing, `RobustHelixFinder` uses leading). -- **Indentation**: Generally consistent within files but varies between files. -- **Braces**: Some files use Allman style, others K&R. - -### 4.2 Memory Safety -- `Data_t` structs use fixed-size C-style arrays with no bounds checking. The diagnostic fill code in `fillPluginDiag` (RobustHelixFinder) does check `loc < _data.maxSeeds()`, but the diagnostic tools for Multi and TimeAndPhi don't check array bounds before filling TTree arrays. -- The `fillStrawDigiIndices` call in diag tools accesses `dids[0]` without checking that the vector is non-empty. If a ComboHit has no underlying straw digi indices, this would crash. - -### 4.3 Numerical Robustness -- Hardcoded pi values with varying precision (see Section 3.2). -- Division-by-zero guards via `1e-10` initialization (see Section 3.1). -- `abs()` vs `fabs()` — some places use `abs()` on floats, which in C++ calls `std::abs` for `float`/`double` (correct in C++11+), but mixing with `fabs()` in the same file is inconsistent. - -### 4.4 Commented-Out Code -- `KalSeedFit_types.hh` — entire file is commented out. -- Various commented-out code blocks in `RobustHelixFinder_module.cc` (e.g., lines 94–95, 400, 744, 856–857, 905, 1001, 1025). -- Commented-out `#include "RecoDataProducts/inc/KalSeed.hh"` in `RobustHelixFinder_types.hh` (line 20). - ---- - -## 5. Summary of Findings by Severity - -### High Priority -| # | File | Issue | -|---|---|---| -| 1 | `RobustMultiHelixFinder_module.cc:346` | **`chi2dXY` repurposed as `dz/dt` slope** — semantic corruption of the data model | -| 2 | `RobustHelixFinder_module.cc:1212-1227` | **`updateStereo` is a no-op with FIXME** — dead code path that silently does nothing | -| 3 | `RobustHelixFinderDiag_tool.cc:278-279` | **Duplicate histogram fill** — `nseeds` filled twice per helicity | - -### Medium Priority -| # | File | Issue | -|---|---|---| -| 4 | `RobustHelixFinder_module.cc:94-97` | `HelixHitMVA._hrho` aliases `_dtrans` — likely unintended | -| 5 | Multiple files | Hardcoded pi values with inconsistent precision | -| 6 | `RobustMultiHelixFinder_module.cc` | No bounds checking on `Data_t` fixed-size arrays (128 helices, 8192 hits) | -| 7 | Diag tools | `dids[0]` access without checking vector is non-empty | -| 8 | `TimeClusterFinder_module.cc:288` | `_cccol` dereferenced without verifying handle validity | -| 9 | `prolog.fcl:33,36` | Duplicate `CaloClusterWeight` key | - -### Low Priority (Code Quality) -| # | File | Issue | -|---|---|---| -| 10 | Multiple files | `using namespace std` at file scope | -| 11 | Multiple files | `static` local variables in member functions (thread-safety concern) | -| 12 | `KalSeedFit_types.hh` | Entirely dead code (commented out) | -| 13 | Multiple files | Commented-out code blocks should be removed | -| 14 | Multiple files | Inconsistent naming conventions (leading vs trailing underscore) | -| 15 | Multiple files | Typos in comments and parameter descriptions | -| 16 | Multiple files | Raw pointer initialization with `(0)` instead of `nullptr` | -| 17 | Diag tools | Unnecessary include guards on `.cc` files; duplicate includes | -| 18 | `RobustHelixFinder_module.cc` | Unused `_printfreq` member | -| 19 | `RobustHelixFinder_module.cc:522` | Redundant `if (nh1 == nh2)` condition | -| 20 | `RobustMultiHelixFinder_module.cc:580` | `unsigned iWorst(-1)` wraps to `UINT_MAX` | - ---- - -## 6. Positive Observations - -- **Clean separation of concerns**: Diagnostic tools are properly factored into separate art::tools using the `ModuleHistToolBase` interface, keeping the main module code focused on physics. -- **Configurable via fhicl**: All modules use the modern fhicl validated configuration (`fhicl::Atom`, `fhicl::Table`) rather than raw `ParameterSet::get()`. -- **`RobustMultiHelixFinder` is well-designed**: The MPR path has cleaner code structure, proper use of `const`, sensible abstractions (`CandHelix`, `LSFitter`), and fewer legacy artifacts than TPR. -- **`TimeAndPhiClusterFinder` is clean and modern**: Minimal dependencies, clear algorithm flow, good use of lambdas and STL algorithms. -- **Comprehensive diagnostic capabilities**: All modules support optional diagnostic output controlled by `DiagLevel`, with no overhead when diagnostics are off. -- **Proper art framework usage**: Correct use of `consumes`/`produces`, `ProductToken`, `art::Ptr`, and `ValidHandle`. - ---- - -## 7. Recommendations - -1. **Address the `chi2dXY` semantic corruption in MPR** by adding a proper field to the `RobustHelix` data product for propagation direction, or use a separate output product. -2. **Either implement or remove `updateStereo`** in `RobustHelixFinder`. The FIXME has been pending for a long time. -3. **Fix the duplicate `nseeds` fill** in the diagnostic tool. -4. **Introduce a named constant for 2π** and use it consistently across all files. -5. **Add bounds checking** to diagnostic `Data_t` array fills, particularly in `RobustMultiHelixFinderDiag` and `TimeAndPhiClusterFinderDiag`. -6. **Remove dead code**: `KalSeedFit_types.hh` and various commented-out blocks. -7. **Eliminate `using namespace std`** at file scope in favor of qualified names or targeted `using` declarations. diff --git a/TrkPatRec/fcl/prolog.fcl b/TrkPatRec/fcl/prolog.fcl index 36578d102d..eb8efcbde2 100644 --- a/TrkPatRec/fcl/prolog.fcl +++ b/TrkPatRec/fcl/prolog.fcl @@ -32,7 +32,6 @@ TrkPatRec : { @table::TrkPatRec CaloClusterWeight : 10.0 TestFlag : true CaloClusterMinE : 50.0 - CaloClusterWeight : 10.0 T0Calculator : {} DtMax : 25 MinNHits : 10 diff --git a/TrkPatRec/inc/KalSeedFit_types.hh b/TrkPatRec/inc/KalSeedFit_types.hh deleted file mode 100644 index 3074f783b7..0000000000 --- a/TrkPatRec/inc/KalSeedFit_types.hh +++ /dev/null @@ -1,39 +0,0 @@ -/* -#ifndef TrkPatRec_KalSeedFit_types_hh -#define TrkPatRec_KalSeedFit_types_hh - -#include "TObject.h" -#include - -class TH1F; -class TH2F; - -namespace art { - class Event; -} - -namespace fhicl { - class ParameterSet; -} - -#include "Offline/RecoDataProducts/inc/KalSeed.hh" - -namespace mu2e { - - class KalFitData; - - namespace KalSeedFitTypes { - - struct Data_t { - const art::Event* event; - KalFitData* result; - fhicl::ParameterSet* timeOffsets; - - KalSeedCollection* tracks; // these report their momentum very unwillingly - std::vector nrescued; // per track - std::vector mom; - }; - } -} -#endif -*/ diff --git a/TrkPatRec/inc/RobustMultiHelixFinder_types.hh b/TrkPatRec/inc/RobustMultiHelixFinder_types.hh index 5159de8b6a..ba2ad11259 100644 --- a/TrkPatRec/inc/RobustMultiHelixFinder_types.hh +++ b/TrkPatRec/inc/RobustMultiHelixFinder_types.hh @@ -13,7 +13,7 @@ namespace mu2e { namespace RobustMultiHelixFinderTypes { struct Config{ - fhicl::Atom tool_type{ fhicl::Name("tool_type"), fhicl::Comment("Needed fot backward compatibility"), "" }; + fhicl::Atom tool_type{ fhicl::Name("tool_type"), fhicl::Comment("Needed for backward compatibility"), "" }; fhicl::Atom mcDiag{ fhicl::Name("MCDiag"), fhicl::Comment("Switch to perform MC diag"), true }; fhicl::Atom strawDigiMCCollection{ fhicl::Name("StrawDigiMCCollection"), fhicl::Comment("StrawDigiMC collection name"), "makeSD" }; }; diff --git a/TrkPatRec/inc/TimeAndPhiClusterFinder_types.hh b/TrkPatRec/inc/TimeAndPhiClusterFinder_types.hh index 1e1cf9e79e..963a929b14 100644 --- a/TrkPatRec/inc/TimeAndPhiClusterFinder_types.hh +++ b/TrkPatRec/inc/TimeAndPhiClusterFinder_types.hh @@ -13,7 +13,7 @@ namespace mu2e { namespace TimeAndPhiClusterFinderTypes { struct Config{ - fhicl::Atom tool_type{ fhicl::Name("tool_type"), fhicl::Comment("Needed fot backward compatibility"), "" }; + fhicl::Atom tool_type{ fhicl::Name("tool_type"), fhicl::Comment("Needed for backward compatibility"), "" }; fhicl::Atom mcDiag{ fhicl::Name("MCDiag"), fhicl::Comment("Switch to perform MC diag"), true }; fhicl::Atom strawDigiMCCollection{ fhicl::Name("StrawDigiMCCollection"), fhicl::Comment("StrawDigiMC collection name"), "makeSD" }; }; diff --git a/TrkPatRec/src/RobustHelixFinderDiag_tool.cc b/TrkPatRec/src/RobustHelixFinderDiag_tool.cc index d44fbe1baf..4ba9ad3bc5 100644 --- a/TrkPatRec/src/RobustHelixFinderDiag_tool.cc +++ b/TrkPatRec/src/RobustHelixFinderDiag_tool.cc @@ -264,7 +264,7 @@ namespace mu2e { //----------------------------------------------------------------------------- int RobustHelixFinderDiag::fillHistograms(void* Data, int Mode) { - _data = (Data_t*) Data; + _data = static_cast(Data); //----------------------------------------------------------------------------- // fill helix-level histograms @@ -276,7 +276,6 @@ namespace mu2e { for (int k=0; kFill(_data->nseeds[k]); - _hist.nseeds[k]->Fill(_data->nseeds[k]); for (int i=0; i<_data->nseeds[k]; i++) { _hist.nChPerPanel->Fill(_data->nChPPanel[k][i] ); diff --git a/TrkPatRec/src/RobustMultiHelixFinderDiag_tool.cc b/TrkPatRec/src/RobustMultiHelixFinderDiag_tool.cc index 871b503328..ae36eb633c 100644 --- a/TrkPatRec/src/RobustMultiHelixFinderDiag_tool.cc +++ b/TrkPatRec/src/RobustMultiHelixFinderDiag_tool.cc @@ -15,10 +15,8 @@ #include "TTree.h" #include "art/Framework/Principal/Event.h" -#include "fhiclcpp/ParameterSet.h" #include "art/Framework/Principal/Handle.h" #include "Offline/GeometryService/inc/GeomHandle.hh" -#include "art/Framework/Core/EDAnalyzer.h" #include "Offline/GeometryService/inc/DetectorSystem.hh" diff --git a/TrkPatRec/src/RobustMultiHelixFinder_module.cc b/TrkPatRec/src/RobustMultiHelixFinder_module.cc index 2193cee52c..8e3ca8deb0 100644 --- a/TrkPatRec/src/RobustMultiHelixFinder_module.cc +++ b/TrkPatRec/src/RobustMultiHelixFinder_module.cc @@ -98,7 +98,7 @@ namespace mu2e { fhicl::Atom maxDTHelFit {Name("MaxDTHelFit"), Comment("Maximum time difference between hit time and z-time fit") }; fhicl::Atom maxChi2Hit {Name("MaxChi2Hit"), Comment("Maximum chi2 for a hit to be associated to the helix") }; fhicl::Atom minDZTrk {Name("MinDZTrk"), Comment("Minimum z span of a trk") }; - fhicl::Atom fitCircleStr {Name("FitCircleStrategy"), Comment("Fit Circle algorhithm HyperFit or ChiSquared") }; + fhicl::Atom fitCircleStr {Name("FitCircleStrategy"), Comment("Fit Circle algorithm HyperFit or ChiSquared") }; fhicl::Atom minStrawHits {Name("MinStrawHits"), Comment("Minimum number of Straw hits for a helix candidate") }; fhicl::Atom nMaxTrkIter {Name("NMaxTrkIter"), Comment("Number of track finding iterations ") }; fhicl::Atom diagLevel {Name("DiagLevel"), Comment("Diag level"), 0 }; @@ -210,7 +210,7 @@ namespace mu2e { if (config().fitCircleStr()=="HyperFit") fitCircleStrategy_ = circleFitter::HyperFit; else if (config().fitCircleStr()=="ChiSquared") fitCircleStrategy_ = circleFitter::ChiSquared; - else throw cet::exception("CATEGORY")<< "RobustMultiHelixFinder: unrecognixed FitCirclestrategy specified"; + else throw cet::exception("CATEGORY")<< "RobustMultiHelixFinder: unrecognized FitCircleStrategy specified"; } @@ -529,7 +529,7 @@ chi2dXY = bestHelix.fita_zt_; } //compute the rolling sum over three bins and find the maximum. - //use cyclic buffer trick to go past the end of the veccctor to correctly llok at boundaries + //use cyclic buffer trick to go past the end of the vector to correctly look at boundaries int sumMax(0),imax(0); for (int i=0;i