Fix critical bugs in KinKal integration code#1786
Conversation
- 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>
|
Hi @Copilot,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. 📝 The author of this pull request is not a member of the Mu2e github organisation. |
- 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>
brownd1978
left a comment
brownd1978
left a comment
There was a problem hiding this comment.
Mostly correct or innocuous. There's 1 incorrect fix. The only fix that might affect results is the goodtrack test.
| template <class KTRAJ,class SURF> double KKShellXing<KTRAJ,SURF>::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 |
There was a problem hiding this comment.
This fix is incorrect; it returns 0 when the result would (formally) be infinite. The correct fix is to require a minimum value for dotprod.
double dotprod = std::max(1e-6,inter_.norm_.Dot(inter_.pdir_);
… 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>
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 1b7b2f8: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 1b7b2f8.
N.B. These results were obtained from a build of this Pull Request at 1b7b2f8 after being merged into the base branch at e229837. For more information, please check the job page here. |
brownd1978
left a comment
brownd1978
left a comment
There was a problem hiding this comment.
Another bug, not caught by the AI
|
|
||
| template <class KTRAJ,class SURF> double KKShellXing<KTRAJ,SURF>::transitTime() const { | ||
| double pathlen = thick_/(inter_.norm_.Dot(inter_.pdir_)); | ||
| double dotprod = std::max(1e-6,inter_.norm_.Dot(inter_.pdir_)); |
There was a problem hiding this comment.
I think there's another bug here, missing fabs(inter_....)
There was a problem hiding this comment.
Added fabs() around the dot product in commit f33f89e:
double dotprod = std::max(1e-6,fabs(inter_.norm_.Dot(inter_.pdir_)));…er 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>
Summary
This PR fixes 17 bugs discovered during a code review of the
Mu2eKinKalpackage. The bugs range from critical logic errors (dead code due to variable shadowing) to numeric precision issues (floatlimits used fordoublevariables) and uninitialized members. No functional behavior is intentionally changed beyond correcting these defects.Critical Bugs
Fix 1:
KKStrawMaterial.cc— Typo assignswallpathtwice instead ofwirepathFile:
Mu2eKinKal/src/KKStrawMaterial.ccline 34Bug: In
averagePathLengths(), the output parameterwirepathis never set — insteadwallpathis assigned0.0a second time (overwriting the correct value just assigned on the previous line). The caller receives an uninitializedwirepathand a zeroedwallpath.Fix: Change
wallpath = 0.0→wirepath = 0.0.Fix 2:
KKStrawHit.hh— Copy constructor reads from uninitializedthisinstead ofrhsFile:
Mu2eKinKal/inc/KKStrawHit.hhlines 57–58Bug: The copy constructor initializes
dVar_anddDdT_by callingdriftVariance()anddriftVelocity()on the partially-constructed object (this). These methods depend on members that haven't been initialized yet (e.g.whstate_,chit_), producing undefined behavior.Fix: Copy from
rhs.dVar_andrhs.dDdT_directly.Fix 11:
LoopHelixFit_module.cc— Variable shadowing makes helicity/charge checks dead codeFile:
Mu2eKinKal/src/LoopHelixFit_module.ccline 481Bug: Inside
goodFit(), a newbool retvalis declared in the inner scope (line 481), shadowing the outerretval(line 476). The inner variable captures the helicity sign check, charge sign check, and detector-volume check — but it goes out of scope at the closing brace. The outerretval(which only checksfitStatus().usable()) is what gets returned. This means tracks with wrong helicity or charge sign are never rejected, and the in-detector check is never applied.Fix: Remove the
boolkeyword so line 481 assigns to the existing outerretval.Stream/Diagnostic Bugs
Fix 3:
Chi2SHU.cc—operator<<writes tostd::coutinstead ofosFile:
Mu2eKinKal/src/Chi2SHU.cclines 82–83Bug: The
operator<<(std::ostream& os, ClusterState const&)overload writes the hit states tostd::coutinstead of the passedosstream. This breaks logging to files or string streams.Fix: Replace
std::coutwithos.Fix 4:
DriftANNSHU.cc— Copy-paste prints sign weights path for cluster weightsFile:
Mu2eKinKal/src/DriftANNSHU.ccline 26Bug: The diagnostic message says "cluster weights" but prints
std::get<0>(config)(the sign weights file) instead ofstd::get<2>(config)(the cluster weights file).Fix: Change to
std::get<2>(config).Numeric Precision:
floatLimits Used fordoubleVariables (10 locations)All of these share the same root cause:
std::numeric_limits<float>::max()(≈3.4e38) is used to initializedoublevariables that should hold extreme sentinel values. For "minimum so far" patterns this works by accident (3.4e38 is large enough), but for "maximum so far" patterns,-std::numeric_limits<float>::max()is used instead ofstd::numeric_limits<double>::lowest(). More critically,std::numeric_limits<float>::min()(≈1.2e-38, the smallest positive normalized float) is used where a negative lower bound is needed, which silently prevents finding any maximum value less than 1.2e-38.Fix 5:
KKFitUtilities.cc—timeBounds()andzMid()File:
Mu2eKinKal/src/KKFitUtilities.cclines 45–46, 57–58Bug:
tmaxinitialized withfloat::min()(positive ~1e-38) instead ofdouble::lowest(). Any negative time would never be selected as max.Fix: Use
std::numeric_limits<double>::max()for min-trackers andstd::numeric_limits<double>::lowest()for max-trackers.Fix 6:
KKFit.hh—range()andcreateSeed()detector trajectory savingFile:
Mu2eKinKal/inc/KKFit.hhlines 628–629, 766–767Fix: Same pattern —
float→double, using::lowest()fortmax.Fix 10:
KKStrawHitCluster.hh—time()File:
Mu2eKinKal/inc/KKStrawHitCluster.hhline 146Fix:
float::max()→double::max()for the min-time tracker.Fix 13:
ExtrapolateIPA.hh— default constructor boundsFile:
Mu2eKinKal/inc/ExtrapolateIPA.hhlines 20–21Fix:
float::max()/-float::max()→double::max()/double::lowest()forzmin_,zmax_.Fix 14:
ExtrapolateST.hh— default constructor boundsFile:
Mu2eKinKal/inc/ExtrapolateST.hhlines 27–30Fix: Same for
zmin_,zmax_,rmin_,rmax_.Fix 15:
ExtrapolateTCRV.hh— default constructor boundsFile:
Mu2eKinKal/inc/ExtrapolateTCRV.hhlines 25–26Fix: Same for
ymin_,ymax_.Fix 16:
Chi2SHU_updateCluster.hh— determinant toleranceFile:
Mu2eKinKal/inc/Chi2SHU_updateCluster.hhline 36Bug:
determinantis adouble, but compared againststd::numeric_limits<float>::min()(~1.2e-38). For double-precision covariance matrices, valid small determinants betweenfloat::minanddouble::minwould be rejected.Fix: Use
std::numeric_limits<double>::min().Uninitialized / Unsafe Variables
Fix 7:
KKFit.hh— Mutable cache variables uninitializedFile:
Mu2eKinKal/inc/KKFit.hhlines 149–152Bug:
strawradius_,ymin_,ymax_,umax_,rmin_,rmax_,spitch_are mutable and lazy-initialized, but have no default value. If any code path reads them before the lazy init fires, the values are garbage.Fix: Initialize all to
0.Fix 8:
KKShellXing.hh— Division by zero and missingfabsintransitTime()(updated per @brownd1978 review)File:
Mu2eKinKal/inc/KKShellXing.hhlines 103–105Bug:
thick_ / (norm · pdir)divides by the dot product of the surface normal and particle direction. Two issues: (1) when the particle travels nearly parallel to the surface the dot product is near zero, producing an extremely large or infinite result, and (2) the dot product can be negative when the particle direction is opposite to the surface normal, which would produce a negative (unphysical) path length.Fix: Take the absolute value and clamp to a minimum:
std::max(1e-6, fabs(inter_.norm_.Dot(inter_.pdir_))). This ensures the path length is always positive and capped at a physically reasonable maximum.Fix 9:
KKFit.hh— Division by zero insqrt(docaVar())File:
Mu2eKinKal/inc/KKFit.hhlines 479, 536Bug:
dsig = max(0, doca - strawradius_) / sqrt(pca.docaVar()). If the closest approach variance is zero (e.g. degenerate fit), this divides by zero.Fix: Clamp the denominator:
sqrt(max(docaVar(), double::min())).Fix 12:
DriftANNSHU.hh— Uninitializeddiag_memberFile:
Mu2eKinKal/inc/DriftANNSHU.hhline 37Bug:
int diag_has no default initializer. All other updater classes (Chi2SHU,CADSHU,BkgANNSHU) initialize theirdiag_to0. If a code path readsdiag_before the constructor body sets it, the value is indeterminate.Fix:
int diag_ = 0.Minor
Fix 17:
KinematicLineFit_module.cc— Unnecessary variable shadowingFile:
Mu2eKinKal/src/KinematicLineFit_module.ccline 340Bug:
auto hptr = art::Ptr<CosmicTrackSeed>(hseedcol_h,iseed)re-declareshptr, shadowing the identical variable from the outer scope (line 288). The shadowed version is used on the next line. While the result is the same, it's confusing and masks the intent.Fix: Remove the redundant declaration; the outer
hptris already correct.Files Changed (15 files)
Mu2eKinKal/src/KKStrawMaterial.ccMu2eKinKal/inc/KKStrawHit.hhMu2eKinKal/src/Chi2SHU.ccMu2eKinKal/src/DriftANNSHU.ccMu2eKinKal/src/KKFitUtilities.ccMu2eKinKal/inc/KKFit.hhMu2eKinKal/inc/KKShellXing.hhMu2eKinKal/inc/KKStrawHitCluster.hhMu2eKinKal/src/LoopHelixFit_module.ccMu2eKinKal/inc/DriftANNSHU.hhMu2eKinKal/inc/ExtrapolateIPA.hhMu2eKinKal/inc/ExtrapolateST.hhMu2eKinKal/inc/ExtrapolateTCRV.hhMu2eKinKal/inc/Chi2SHU_updateCluster.hhMu2eKinKal/src/KinematicLineFit_module.cc