From dcb981519c41c611987e73270bcc27d25933f5cf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Apr 2026 16:59:12 +0000 Subject: [PATCH 1/4] Fix critical bugs found in KinKal code review - KKStrawMaterial: fix wallpath/wirepath typo in averagePathLengths - KKStrawHit: fix copy constructor to copy dVar_/dDdT_ from rhs - Chi2SHU: fix operator<< to use os parameter instead of std::cout - DriftANNSHU: fix copy-paste error printing wrong config index - KKFitUtilities/KKFit/KKStrawHitCluster: use double limits instead of float limits for min/max tracking - KKFit: initialize mutable cache variables to zero - KKShellXing: guard transitTime against division by zero - KKFit: guard dsig calculation against zero docaVar Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/c56c0d14-d58e-4d88-9e5a-7d64ba5efda6 Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com> --- Mu2eKinKal/inc/KKFit.hh | 20 ++++++++++---------- Mu2eKinKal/inc/KKShellXing.hh | 4 +++- Mu2eKinKal/inc/KKStrawHit.hh | 4 ++-- Mu2eKinKal/inc/KKStrawHitCluster.hh | 2 +- Mu2eKinKal/src/Chi2SHU.cc | 4 ++-- Mu2eKinKal/src/DriftANNSHU.cc | 2 +- Mu2eKinKal/src/KKFitUtilities.cc | 8 ++++---- Mu2eKinKal/src/KKStrawMaterial.cc | 2 +- 8 files changed, 24 insertions(+), 22 deletions(-) diff --git a/Mu2eKinKal/inc/KKFit.hh b/Mu2eKinKal/inc/KKFit.hh index 58255ce6d4..71a8790bd2 100644 --- a/Mu2eKinKal/inc/KKFit.hh +++ b/Mu2eKinKal/inc/KKFit.hh @@ -146,10 +146,10 @@ namespace mu2e { float maxStrawHitDoca_, maxStrawHitDt_, maxStrawDoca_, maxStrawDocaCon_, maxStrawUposBuff_; int maxDStraw_; // maximum distance from the track a strawhit can be to consider it for adding. // cached info computed from the tracker, used in hit adding; these must be lazy-evaluated as the tracker doesn't exist on construction - mutable double strawradius_; - mutable double ymin_, ymax_, umax_; // panel-level info - mutable double rmin_, rmax_; // plane-level info - mutable double spitch_; + mutable double strawradius_ = 0; + mutable double ymin_ = 0, ymax_ = 0, umax_ = 0; // panel-level info + mutable double rmin_ = 0, rmax_ = 0; // plane-level info + mutable double spitch_ = 0; mutable bool needstrackerinfo_ = true; // extrapolation and sampling options SurfaceMap::SurfacePairCollection sample_; // surfaces to sample the fit @@ -476,7 +476,7 @@ namespace mu2e { // require consistency with this track passing through this straw double du = fabs((pca.sensorPoca().Vect()-VEC3(straw.wirePosition(0.0))).Dot(VEC3(straw.wireDirection(0.0)))); double doca = fabs(pca.doca()); - double dsig = std::max(0.0,doca-strawradius_)/sqrt(pca.docaVar()); + double dsig = std::max(0.0,doca-strawradius_)/sqrt(std::max(pca.docaVar(),std::numeric_limits::min())); if(doca < maxStrawDoca_ && dsig < maxStrawDocaCon_ && du < straw.halfLength() + maxStrawUposBuff_){ addexings.push_back(std::make_shared(shptr,static_cast(pca),smat,straw)); oldstraws.insert(straw.id()); @@ -533,7 +533,7 @@ namespace mu2e { // require consistency with this track passing through this straw double du = fabs((pca.sensorPoca().Vect()-VEC3(straw.wirePosition(0.0))).Dot(VEC3(straw.wireDirection(0.0)))); double doca = fabs(pca.doca()); - double dsig = std::max(0.0,doca-strawradius_)/sqrt(pca.docaVar()); + double dsig = std::max(0.0,doca-strawradius_)/sqrt(std::max(pca.docaVar(),std::numeric_limits::min())); if(doca < maxStrawDoca_ && dsig < maxStrawDocaCon_ && du < straw.halfLength() + maxStrawUposBuff_){ addexings.push_back(std::make_shared(shptr,static_cast(pca),smat,straw)); oldstraws.insert(straw.id()); @@ -625,8 +625,8 @@ namespace mu2e { template TimeRange KKFit::range(KKSTRAWHITCOL const& strawhits, KKCALOHITCOL const& calohits, KKSTRAWXINGCOL const& strawxings) const{ - double tmin = std::numeric_limits::max(); - double tmax = -tmin; + double tmin = std::numeric_limits::max(); + double tmax = std::numeric_limits::lowest(); for( auto const& strawhit : strawhits) { tmin = std::min(tmin,strawhit->time()); tmax = std::max(tmax,strawhit->time()); @@ -763,8 +763,8 @@ namespace mu2e { } } else if (savetraj_ == detector ) { // only save segments inside the tracker volume. Find the limits for that. Start with the times of active hits - double tmin = std::numeric_limits::max(); - double tmax = -tmin; + double tmin = std::numeric_limits::max(); + double tmax = std::numeric_limits::lowest(); for(auto const& kkshp : kktrk.strawHits()){ if(kkshp->active()){ tmin = std::min(tmin,kkshp->time()); diff --git a/Mu2eKinKal/inc/KKShellXing.hh b/Mu2eKinKal/inc/KKShellXing.hh index 4da6162b6f..4e1f8a2772 100644 --- a/Mu2eKinKal/inc/KKShellXing.hh +++ b/Mu2eKinKal/inc/KKShellXing.hh @@ -101,7 +101,9 @@ namespace mu2e { } template double KKShellXing::transitTime() const { - double pathlen = thick_/(inter_.norm_.Dot(inter_.pdir_)); + double dotprod = inter_.norm_.Dot(inter_.pdir_); + if(std::abs(dotprod) < 1e-10) return 0.0; // particle parallel to surface + double pathlen = thick_/dotprod; return pathlen/reftrajptr_->speed(); } diff --git a/Mu2eKinKal/inc/KKStrawHit.hh b/Mu2eKinKal/inc/KKStrawHit.hh index 30d0160578..02810e55eb 100644 --- a/Mu2eKinKal/inc/KKStrawHit.hh +++ b/Mu2eKinKal/inc/KKStrawHit.hh @@ -54,8 +54,8 @@ namespace mu2e { KKStrawHit(KKStrawHit const& rhs): bfield_(rhs.bfield()), whstate_(rhs.hitState()), - dVar_(driftVariance()), - dDdT_(driftVelocity()), + dVar_(rhs.dVar_), + dDdT_(rhs.dDdT_), ca_(rhs.closestApproach()), resids_(rhs.refResiduals()), chit_(rhs.hit()), diff --git a/Mu2eKinKal/inc/KKStrawHitCluster.hh b/Mu2eKinKal/inc/KKStrawHitCluster.hh index 6ce3eafc66..342c0bbe4c 100644 --- a/Mu2eKinKal/inc/KKStrawHitCluster.hh +++ b/Mu2eKinKal/inc/KKStrawHitCluster.hh @@ -143,7 +143,7 @@ namespace mu2e { // return time just before the first hit's time. This insures hit clusters are updated before individual hits // This insures the weights subtracted correspond to the reference fit, and that any changes made to the // hits in the cluster get propagated to the residuals and weights before the next fit - double mintime(std::numeric_limits::max()); + double mintime(std::numeric_limits::max()); for(auto const& hit : hits_){ mintime = std::min(hit->time(),mintime); } diff --git a/Mu2eKinKal/src/Chi2SHU.cc b/Mu2eKinKal/src/Chi2SHU.cc index ceff9bc9bc..39ee35a93b 100644 --- a/Mu2eKinKal/src/Chi2SHU.cc +++ b/Mu2eKinKal/src/Chi2SHU.cc @@ -79,8 +79,8 @@ namespace mu2e { std::ostream& operator <<(std::ostream& os, ClusterState const& cstate ) { os << "ClusterState " << cstate.chi2_ << " states: "; - for(auto whstate : cstate.hitstates_) std::cout << " " << whstate.state_; - std::cout << std::endl; + for(auto whstate : cstate.hitstates_) os << " " << whstate.state_; + os << std::endl; return os; } diff --git a/Mu2eKinKal/src/DriftANNSHU.cc b/Mu2eKinKal/src/DriftANNSHU.cc index 704152b760..317c564afe 100644 --- a/Mu2eKinKal/src/DriftANNSHU.cc +++ b/Mu2eKinKal/src/DriftANNSHU.cc @@ -23,7 +23,7 @@ namespace mu2e { diag_ = std::get<7>(config); if(diag_ > 0) std::cout << "DriftANNSHU LR sign weights " << std::get<0>(config) << " cut " << signmvacut_ - << " cluster weights " << std::get<0>(config) << " cut " << clustermvacut_ << " dt cut " << dtmvacut_ + << " cluster weights " << std::get<2>(config) << " cut " << clustermvacut_ << " dt cut " << dtmvacut_ << " freezing " << freeze_ << " flags " << flag << " diag " << diag_ << std::endl; } diff --git a/Mu2eKinKal/src/KKFitUtilities.cc b/Mu2eKinKal/src/KKFitUtilities.cc index 14cf880c77..4bd7ed1dd3 100644 --- a/Mu2eKinKal/src/KKFitUtilities.cc +++ b/Mu2eKinKal/src/KKFitUtilities.cc @@ -42,8 +42,8 @@ namespace mu2e { KinKal::TimeRange timeBounds(ComboHitCollection const& chits) { if(chits.size() == 0) return KinKal::TimeRange(); - double tmin = std::numeric_limits::max(); - double tmax = std::numeric_limits::min(); + double tmin = std::numeric_limits::max(); + double tmax = std::numeric_limits::lowest(); for( auto const& hit : chits) { // filter out hits already flagged as bad TODO tmin = std::min(tmin,(double)hit.correctedTime()); @@ -54,8 +54,8 @@ namespace mu2e { double zMid(ComboHitCollection const& chits) { if(chits.size() == 0) return 0.0; - double zmin = std::numeric_limits::max(); - double zmax = std::numeric_limits::min(); + double zmin = std::numeric_limits::max(); + double zmax = std::numeric_limits::lowest(); for( auto const& hit : chits) { // filter out hits already flagged as bad TODO double zpos = hit.pos().z(); diff --git a/Mu2eKinKal/src/KKStrawMaterial.cc b/Mu2eKinKal/src/KKStrawMaterial.cc index a79a54cef3..4a7ee85d60 100644 --- a/Mu2eKinKal/src/KKStrawMaterial.cc +++ b/Mu2eKinKal/src/KKStrawMaterial.cc @@ -31,7 +31,7 @@ namespace mu2e { KKStrawMaterial::PathCalc KKStrawMaterial::averagePathLengths(double& wallpath, double& gaspath, double& wirepath) const { gaspath = avggaspath_; wallpath = avgwallpath_; - wallpath = 0.0; + wirepath = 0.0; return KKStrawMaterial::average; } From 08078aa2337c065235fcf37b15f5c401d2394090 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Apr 2026 17:13:25 +0000 Subject: [PATCH 2/4] Fix additional bugs from code review - LoopHelixFit: fix critical variable shadowing making physical consistency checks (helicity/charge) dead code - DriftANNSHU: initialize diag_ member to 0 - ExtrapolateIPA/ST/TCRV: use double limits instead of float - Chi2SHU_updateCluster: use double::min() for determinant check - KinematicLineFit: remove unnecessary variable shadowing of hptr Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/78f0b373-e173-4e76-96d0-cac405a5ea5b Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com> --- Mu2eKinKal/inc/Chi2SHU_updateCluster.hh | 2 +- Mu2eKinKal/inc/DriftANNSHU.hh | 2 +- Mu2eKinKal/inc/ExtrapolateIPA.hh | 4 ++-- Mu2eKinKal/inc/ExtrapolateST.hh | 8 ++++---- Mu2eKinKal/inc/ExtrapolateTCRV.hh | 4 ++-- Mu2eKinKal/src/KinematicLineFit_module.cc | 1 - Mu2eKinKal/src/LoopHelixFit_module.cc | 2 +- 7 files changed, 11 insertions(+), 12 deletions(-) diff --git a/Mu2eKinKal/inc/Chi2SHU_updateCluster.hh b/Mu2eKinKal/inc/Chi2SHU_updateCluster.hh index a05cd4b5fc..1ea417ee74 100644 --- a/Mu2eKinKal/inc/Chi2SHU_updateCluster.hh +++ b/Mu2eKinKal/inc/Chi2SHU_updateCluster.hh @@ -33,7 +33,7 @@ namespace mu2e { // in marginal fits removing even a few hits can leave the fit underconstrained, resulting in a zero determinant. For now, do nothing // with these clusters double determinant; - if(!uparams.covariance().Det(determinant) || determinant < std::numeric_limits::min()){ + if(!uparams.covariance().Det(determinant) || determinant < std::numeric_limits::min()){ if(diag_ > 2)std::cout << "Negative unbiased covar determinant = " << determinant << std::endl; return; } diff --git a/Mu2eKinKal/inc/DriftANNSHU.hh b/Mu2eKinKal/inc/DriftANNSHU.hh index 9c83c10a28..4bd1dfeaf6 100644 --- a/Mu2eKinKal/inc/DriftANNSHU.hh +++ b/Mu2eKinKal/inc/DriftANNSHU.hh @@ -34,7 +34,7 @@ namespace mu2e { double dtmvacut_ =0; // cut value for using dt constraint KKSHFlag flag_ = KKSHFlag(KKSHFlag::tot); // constrain time with TOT by default WHSMask freeze_; // states to freeze - int diag_; // diag print level + int diag_ = 0; // diag print level }; } #endif diff --git a/Mu2eKinKal/inc/ExtrapolateIPA.hh b/Mu2eKinKal/inc/ExtrapolateIPA.hh index b1462569b5..6c529241b9 100644 --- a/Mu2eKinKal/inc/ExtrapolateIPA.hh +++ b/Mu2eKinKal/inc/ExtrapolateIPA.hh @@ -17,8 +17,8 @@ namespace mu2e { public: using CylPtr = std::shared_ptr; ExtrapolateIPA() : maxDt_(-1.0), dptol_(1e10), intertol_(1e10), - zmin_(std::numeric_limits::max()), - zmax_(-std::numeric_limits::max()),debug_(0) {} + zmin_(std::numeric_limits::max()), + zmax_(std::numeric_limits::lowest()),debug_(0) {} ExtrapolateIPA(double maxdt, double dptol,double intertol, CylPtr const& ipa, int debug=0) : maxDt_(maxdt), dptol_(dptol), intertol_(intertol), ipa_(ipa), diff --git a/Mu2eKinKal/inc/ExtrapolateST.hh b/Mu2eKinKal/inc/ExtrapolateST.hh index 1c5628de0a..1dfac1c076 100644 --- a/Mu2eKinKal/inc/ExtrapolateST.hh +++ b/Mu2eKinKal/inc/ExtrapolateST.hh @@ -24,10 +24,10 @@ namespace mu2e { class ExtrapolateST { public: ExtrapolateST() : maxDt_(-1.0), dptol_(1e10), intertol_(1e10), - zmin_(std::numeric_limits::max()), - zmax_(-std::numeric_limits::max()), - rmin_(std::numeric_limits::max()), - rmax_(-std::numeric_limits::max()), + zmin_(std::numeric_limits::max()), + zmax_(std::numeric_limits::lowest()), + rmin_(std::numeric_limits::max()), + rmax_(std::numeric_limits::lowest()), debug_(0){} ExtrapolateST(double maxdt, double dptol, double intertol, StoppingTarget const& stoptarg, int debug=0) : diff --git a/Mu2eKinKal/inc/ExtrapolateTCRV.hh b/Mu2eKinKal/inc/ExtrapolateTCRV.hh index b820228626..7cc2d3942d 100644 --- a/Mu2eKinKal/inc/ExtrapolateTCRV.hh +++ b/Mu2eKinKal/inc/ExtrapolateTCRV.hh @@ -22,8 +22,8 @@ namespace mu2e { public: ExtrapolateTCRV() : maxDt_(-1.0), dptol_(1e10), intertol_(1e10), minv_(1e-5), step_(0), - ymin_(std::numeric_limits::max()), - ymax_(-std::numeric_limits::max()), + ymin_(std::numeric_limits::max()), + ymax_(std::numeric_limits::lowest()), debug_(0){} ExtrapolateTCRV(double maxdt, double dptol, double intertol, double minv, TestCRV const& tcrv, int debug=0) : diff --git a/Mu2eKinKal/src/KinematicLineFit_module.cc b/Mu2eKinKal/src/KinematicLineFit_module.cc index 3664cd7d64..94244fba4f 100644 --- a/Mu2eKinKal/src/KinematicLineFit_module.cc +++ b/Mu2eKinKal/src/KinematicLineFit_module.cc @@ -337,7 +337,6 @@ namespace mu2e { kkseedcol->push_back(kkseed); kkseedcol->back()._status.merge(TrkFitFlag::KKLine); // fill assns with the cosmic seed - auto hptr = art::Ptr(hseedcol_h,iseed); auto kseedptr = art::Ptr(KalSeedCollectionPID,kkseedcol->size()-1,KalSeedCollectionGetter); kkseedassns->addSingle(kseedptr,hptr); // save (unpersistable) KKTrk in the event diff --git a/Mu2eKinKal/src/LoopHelixFit_module.cc b/Mu2eKinKal/src/LoopHelixFit_module.cc index e9aec763d1..37745e5d9b 100644 --- a/Mu2eKinKal/src/LoopHelixFit_module.cc +++ b/Mu2eKinKal/src/LoopHelixFit_module.cc @@ -478,7 +478,7 @@ namespace mu2e { // require physical consistency: fit can succeed but the result can have changed charge or helicity. Test at the t0 segment auto t0 = Mu2eKinKal::zTime(ktrk.fitTraj(),0.0,ktrk.fitTraj().range().mid()); auto const& t0seg = ktrk.fitTraj().nearestPiece(t0); - bool retval = ktrk.fitStatus().usable() && t0seg.parameterSign()*seed.parameterSign() > 0 && t0seg.helicity()*seed.helicity() > 0; + retval = t0seg.parameterSign()*seed.parameterSign() > 0 && t0seg.helicity()*seed.helicity() > 0; // also check that the fit is inside the physical detector volume. Test where the StrawHits are if(retval){ for(auto const& shptr : ktrk.strawHits()) { From 1b7b2f834c8eb0dc6af80d5ae8efbf3a0e2012e3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Apr 2026 17:43:38 +0000 Subject: [PATCH 3/4] Fix KKShellXing::transitTime() per Dave Brown's review: clamp dotprod to min value instead of returning 0 Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/7d94d9d6-a4b8-48dc-8682-567dc69e2938 Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com> --- Mu2eKinKal/inc/KKShellXing.hh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Mu2eKinKal/inc/KKShellXing.hh b/Mu2eKinKal/inc/KKShellXing.hh index 4e1f8a2772..daa0b98506 100644 --- a/Mu2eKinKal/inc/KKShellXing.hh +++ b/Mu2eKinKal/inc/KKShellXing.hh @@ -101,8 +101,7 @@ namespace mu2e { } template double KKShellXing::transitTime() const { - double dotprod = inter_.norm_.Dot(inter_.pdir_); - if(std::abs(dotprod) < 1e-10) return 0.0; // particle parallel to surface + double dotprod = std::max(1e-6,inter_.norm_.Dot(inter_.pdir_)); double pathlen = thick_/dotprod; return pathlen/reftrajptr_->speed(); } From f33f89e9874e009a5282775ecb3ae196a9e3fe14 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Apr 2026 22:28:48 +0000 Subject: [PATCH 4/4] Add missing fabs() around dot product in KKShellXing::transitTime() per brownd1978 review Agent-Logs-Url: https://github.com/Mu2e/Offline/sessions/cb5b66ea-2ece-4f44-afab-f6cb67ed0bb1 Co-authored-by: oksuzian <48927306+oksuzian@users.noreply.github.com> --- Mu2eKinKal/inc/KKShellXing.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Mu2eKinKal/inc/KKShellXing.hh b/Mu2eKinKal/inc/KKShellXing.hh index daa0b98506..c53f3c2294 100644 --- a/Mu2eKinKal/inc/KKShellXing.hh +++ b/Mu2eKinKal/inc/KKShellXing.hh @@ -101,7 +101,7 @@ namespace mu2e { } template double KKShellXing::transitTime() const { - double dotprod = std::max(1e-6,inter_.norm_.Dot(inter_.pdir_)); + double dotprod = std::max(1e-6,fabs(inter_.norm_.Dot(inter_.pdir_))); double pathlen = thick_/dotprod; return pathlen/reftrajptr_->speed(); }