From dcb0886f71f7ea28c20c8c7d868a50c45a0ab8d3 Mon Sep 17 00:00:00 2001 From: Yuri Oksuzian Date: Sat, 29 Aug 2026 14:30:40 -0500 Subject: [PATCH 1/2] TrkHitReco: fix defects found by a static sweep of the package CombineStrawHits set the output ComboHitCollection's parent to the original input handle, but in the Unsorted branch it permuted the hits into a local chcolsort and combined from that, so every index stored by ComboHit::init and ComboHit::addIndex was a position in the permutation while resolving against the unsorted original. Per-hit provenance was silently wrong, and wrong precisely when the permutation is non-trivial, which is the only reason that branch exists. It is live on VST data processing via Production/Processing/vstdata_epilog.fcl, which sets makePH.Unsorted:true; the prolog default is false, so MC reco is unaffected. combine() now takes the index mapping and stores original-collection indices in both branches. The four FilterHits cuts in the same loop used `if(_filter)break;`, which abandons every later hit in the event rather than dropping the hit that failed the cut. Dormant, since no committed fcl sets makePH.FilterHits:true. StrawHitReco called flagCrossTalk inside the per-digi loop, where it rescans the whole collection, so it ran once per digi instead of once per event. It also dereferences shCol, which is only allocated when WriteStrawHitCollection is true, so FlagCrossTalk:true with WriteStrawHitCollection:false null-dereferences a unique_ptr. Hoist the call out of the loop and reject that configuration in the constructor. Both are dormant: every committed config sets FlagCrossTalk:false. StereoLineTest declared its random angle as `uniform_int_distribution phirange(-M_PI,M_PI)`, which truncates to [-3,3], so the stereo stress test only ever sampled 7 discrete orientations. Deliberately not included, and offered as follow-ups: the ComboHit quality chi-square at CombineStrawHits_module.cc:296 looks dimensionally inconsistent but needs a physics sign-off on the intended formula; and six defects in FlagBkgHits, TNTClusterer, Chi2Clusterer and DBSClusterer, which no committed configuration reaches -- both Production and the trigger bind flagPH to CalPatRec DeltaFinder. Found by a static review of the package at 0ef02a66d. Not built locally -- relying on CI for the build. Co-Authored-By: Claude Opus 5 --- TrkHitReco/src/CombineStrawHits_module.cc | 34 ++++++++++++++++------- TrkHitReco/src/StereoLineTest_main.cc | 2 +- TrkHitReco/src/StrawHitReco_module.cc | 13 ++++++--- 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/TrkHitReco/src/CombineStrawHits_module.cc b/TrkHitReco/src/CombineStrawHits_module.cc index 22814b28da..35a5f0c05b 100644 --- a/TrkHitReco/src/CombineStrawHits_module.cc +++ b/TrkHitReco/src/CombineStrawHits_module.cc @@ -21,6 +21,8 @@ #include "TMath.h" #include +#include +#include namespace mu2e { @@ -61,7 +63,10 @@ namespace mu2e { void produce( art::Event& e); private: - void combine(EventWindowMarker const& ewm, ComboHitCollection const& chcOrig, ComboHitCollection& chcol); + // origIndex maps a position in chcOrig to the corresponding index in chcol's parent + // collection; it is the identity unless the input was re-sorted first + void combine(EventWindowMarker const& ewm, ComboHitCollection const& chcOrig, ComboHitCollection& chcol, + std::vector const& origIndex); void combineHits(const ComboHitCollection& chcOrig, ComboHit& combohit); int _debug; @@ -143,20 +148,29 @@ namespace mu2e { // select hits based on flag panels[ch.strawId().uniquePanel()].push_back(ish); } + // chcolNew's parent is chcH (the unsorted input), so record where each sorted hit came + // from: without this the ComboHits would store positions in chcolsort while resolving + // them against chcOrig, silently corrupting per-hit provenance + std::vector origIndex; + origIndex.reserve(nsh); for (uint16_t ipanel=0;ipanel origIndex(chcOrig.size()); + std::iota(origIndex.begin(), origIndex.end(), 0); + combine(ewm, chcOrig, *chcolNew, origIndex); } event.put(std::move(chcolNew)); } - void CombineStrawHits::combine(EventWindowMarker const& ewm, ComboHitCollection const& chcOrig, ComboHitCollection& chcol) + void CombineStrawHits::combine(EventWindowMarker const& ewm, ComboHitCollection const& chcOrig, ComboHitCollection& chcol, + std::vector const& origIndex) { float minT = _minT; @@ -175,7 +189,7 @@ namespace mu2e { if ( _testflag && hit1.flag().hasAnyProperty(StrawHitFlag::dead)) continue; if ( _testflag && (!hit1.flag().hasAllProperties(_shsel) || hit1.flag().hasAnyProperty(_shmask))) continue; ComboHit combohit; - combohit.init(hit1,ich); + combohit.init(hit1,origIndex[ich]); int panel1 = hit1.strawId().uniquePanel(); for (size_t jch=ich+1;jch _maxwdchi) continue; - bool ok = combohit.addIndex(jch); + bool ok = combohit.addIndex(origIndex[jch]); if (!ok){ std::cout << "CombineStrawHits past limit" << std::endl; } else { @@ -206,7 +220,7 @@ namespace mu2e { combohit._flag = initialFlag; int nch = combohit.nCombo(); if(nch < _minN || nch > _maxN){ - if(_filter)break; + if(_filter)continue; } else combohit._flag.merge(StrawHitFlag::nhitsel); // actually combine the hits if necessar, and make the cuts @@ -214,19 +228,19 @@ namespace mu2e { auto time = _useTOT ? combohit.correctedTime() : combohit.time(); if (time < minT || time > maxT ){ - if(_filter)break; + if(_filter)continue; } else combohit._flag.merge(StrawHitFlag::timesel); auto energy = combohit.energyDep(); if( energy > _maxE || energy < _minE ) { - if(_filter)break; + if(_filter)continue; } else combohit._flag.merge(StrawHitFlag::energysel); auto r2 = combohit.pos().Perp2(); if( r2 < _minR2 || r2 > _maxR2 ) { - if(_filter)break; + if(_filter)continue; } else combohit._flag.merge(StrawHitFlag::radsel); combohit._mask = _mask; diff --git a/TrkHitReco/src/StereoLineTest_main.cc b/TrkHitReco/src/StereoLineTest_main.cc index 11624381eb..3f08d04687 100644 --- a/TrkHitReco/src/StereoLineTest_main.cc +++ b/TrkHitReco/src/StereoLineTest_main.cc @@ -77,7 +77,7 @@ int main(int argc, char** argv) { double dz = deltaz/(npts-1); std::random_device r; std::default_random_engine eng(r()); - std::uniform_int_distribution phirange(-M_PI,M_PI); + std::uniform_real_distribution phirange(-M_PI,M_PI); std::normal_distribution urand{0.0, ures}; std::normal_distribution vrand{0.0, vres}; diff --git a/TrkHitReco/src/StrawHitReco_module.cc b/TrkHitReco/src/StrawHitReco_module.cc index 1071e86aa4..21220f4d15 100644 --- a/TrkHitReco/src/StrawHitReco_module.cc +++ b/TrkHitReco/src/StrawHitReco_module.cc @@ -5,6 +5,7 @@ // Merged with flag and position creation B. Echenard, CalTech // // framework +#include "cetlib_except/exception.h" #include "art/Framework/Principal/Event.h" #include "art/Framework/Principal/Handle.h" #include "Offline/GeometryService/inc/GeomHandle.hh" @@ -147,6 +148,9 @@ namespace mu2e { produces(); produces(); if (_writesh) produces(); + // flagCrossTalk reads the StrawHitCollection, which is only created when it is written out + if (_flagXT && !_writesh) + throw cet::exception("RECO")<<"mu2e::StrawHitReco: FlagCrossTalk requires WriteStrawHitCollection"< 0) std::cout << "In StrawHitReco constructor " << std::endl; } @@ -213,10 +217,11 @@ namespace mu2e { _shrUtils.createComboHit(ewm, isd, chCol, shCol, caloClusters, pbtOffset, digi.strawId(), digi.TDC(), digi.TOT(), pmp, trackerStatus, srep, tt); - //flag straw and electronic cross-talk - if(_flagXT){ - _shrUtils.flagCrossTalk(shCol, chCol); - } + } + //flag straw and electronic cross-talk: this scans the whole collection, so it must run once + //per event, not once per digi + if(_flagXT){ + _shrUtils.flagCrossTalk(shCol, chCol); } if(_writesh)event.put(std::move(shCol)); intInfo->setNTrackerHits(chCol->size()); From 43e40f5d70082899fda67fb5f39fffc3449995cd Mon Sep 17 00:00:00 2001 From: Iuri Oksuzian Date: Mon, 31 Aug 2026 11:35:44 -0500 Subject: [PATCH 2/2] fix: propagate TimeDivision instead of asserting it on every ComboHit CombineStrawHits seeded every output ComboHit's flag with a hard-coded StrawHitFlag("TimeDivision") and never re-derived it: combineHits() does not touch the flag word, and the nCombo()==1 path never calls it at all. TimeDivision is a quality tag, not a formality. StrawHitRecoUtils.cc:201 sets it only when StrawResponse::wireDistance() returned true; that function returns false exactly for hits whose longitudinal position exceeded the straw length, was clamped to the straw end and had its error inflated ("these come from missing a cluster on one end"). Consumers select on it: TrkPatRec/fcl/prolog.fcl:167 and mu2e-trig-config trigTprProducers.fcl:114,300 both set HitSelectionBits: ["TimeDivision"], tested at RobustHelixFinder_module.cc:841. Because makePH asserted the bit unconditionally, that selection admitted 100% of hits and the clamped-position hits entered the helix fit indistinguishable from hits with a real longitudinal measurement. Seed the flag empty and propagate tdiv from the constituents: kept for a single-straw ComboHit, and for a multi-straw one only when every hit that was actually combined carries it. Also fix an off-by-one in the index guard in combineHits(): index == chcOrig.size() passed `index > chcOrig.size()` and then read one past the end. Unreachable today, since the indices come from addIndex() on the same collection, but the check exists to catch exactly that case. This changes which hits pass HitSelectionBits in reco and the trigger. Not built locally - relying on CI for the build. Co-Authored-By: Claude Opus 5 --- TrkHitReco/src/CombineStrawHits_module.cc | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/TrkHitReco/src/CombineStrawHits_module.cc b/TrkHitReco/src/CombineStrawHits_module.cc index 35a5f0c05b..88d456510f 100644 --- a/TrkHitReco/src/CombineStrawHits_module.cc +++ b/TrkHitReco/src/CombineStrawHits_module.cc @@ -190,6 +190,10 @@ namespace mu2e { if ( _testflag && (!hit1.flag().hasAllProperties(_shsel) || hit1.flag().hasAnyProperty(_shmask))) continue; ComboHit combohit; combohit.init(hit1,origIndex[ich]); + // TimeDivision is a quality tag: StrawHitRecoUtils sets it only when the longitudinal + // position was actually measured, and leaves it off when StrawResponse had to clamp the + // position to the straw end. It must therefore be propagated from the constituents. + bool tdiv = hit1.flag().hasAllProperties(StrawHitFlag::tdiv); int panel1 = hit1.strawId().uniquePanel(); for (size_t jch=ich+1;jch _maxN){ if(_filter)continue; @@ -263,7 +268,7 @@ namespace mu2e { { size_t index = combohit.index(ich); if (_debug > 3)std::cout << index << ", "; - if (index > chcOrig.size()) + if (index >= chcOrig.size()) throw cet::exception("RECO")<<"mu2e::CombineStrawHits: inconsistent index "<