Mu2eKinKal: fix defects found by a static sweep of the package - #1955
Mu2eKinKal: fix defects found by a static sweep of the package#1955oksuzian wants to merge 1 commit into
Conversation
RegrowKinematicLine indexed KalSeedMCAssns with a counter local to its own input KalSeedCollection. That product is a single flat Assns spanning every collection SelectRecoMC was configured with (KKDe, KKDmu, KKUe, KKUmu, KKLine, KKCHmu), so for KKLine the index is wrong whenever an earlier collection has seeds, and the ptr self-check throws. Replace the positional lookup with a KalSeed -> KalSeedMC map built once per event; this also removes the O(N*M) scan the sibling RegrowLoopHelix does. RegrowLoopHelix declared MustRegrow as a mandatory fhicl::Atom, but no job config supplies it: Production/JobConfig/reco/regrowLH.fcl builds the producer block literally instead of pulling @table::Mu2eKinKal.RegrowLH, which is the only place the key is set. Give the Atom the "None" default that table implies, so validation no longer fails before the module is constructed. BkgANNSHU passed sqrt(docaVar()) to the MVA unclamped. A non-positive docaVar gives NaN, and NaN < mvacut_ is false, so an unscoreable hit is left active or re-activated. DriftANNSHU, CADSHU and KKFit all clamp the same quantity; do the same here. KKFit::createSeed dereferenced rbegin() of the domain set when SaveDomains is set. That set is empty whenever the fit ran without BField correction, making this undefined behaviour. Throw a named exception instead; the CHTruthSeed prolog comment that documented the crash is updated to match. StrawHitUpdaters::name() indexed names_ with the enumerator value, but 'unknown' is -1, so printing that state at diag > 1 was undefined behaviour. Bounds-check and return "unknown". KLSeedFit took KKFitSettings straight from KKFIT, which deliberately does not set the mandatory SaveTrajectory key. Every sibling supplies its own; add T0 to match CHSeedFit, whose fit and extension settings KLSeedFit already shares. Found by a static review of the package at 0ef02a6. Not built locally -- relying on CI for the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
☀️ The build tests passed at 87d7c03.
N.B. These results were obtained from a build of this Pull Request at 87d7c03 after being merged into the base branch at 0ef02a6. For more information, please check the job page here. |
| // SaveDomains with a fit that ran without BField correction leaves no domains at all; | ||
| // rbegin() would then be rend() and dereferencing it is undefined behaviour | ||
| if(kktrk.domains().empty())throw cet::exception("RECO") | ||
| << "mu2e::KKFit: SaveDomains is set but the fit has no BField domains; set BFieldCorrection or SaveDomains:false" << std::endl; |
There was a problem hiding this comment.
I think this should be a warning and a clean return (with empty domains collection) rather than an exception. An uncaught exception would abort the entire art job.
|
I encountered an exception when running Production/JobConfig/reco/regrow_kl.fcl, but not the "wrong KalSeed ptr" exception addressed in this PR. Instead, it was "Invalid Time Range" in KinKal::TimeRange constructor: The Offline call is in toTrackerPerimeter here: Offline/Mu2eKinKal/inc/KKExtrap.hh Line 159 in 0ef02a6 The failure is unrelated to the suggested fix in this PR, but it hides any actual problem that this PR would fix. A cheap fix would be TimeRange(ftraj.range().begin(), tref, false). Maybe that should be added to this PR. |
3 participants
Six defects found by a static review of
Mu2eKinKalat0ef02a66d. All six are mechanical fixes with an existing in-package precedent; none changes fit acceptance or any tuning constant. I have not built this locally — relying onmu2e/buildtest.1.
RegrowKinematicLineindexes a flatKalSeedMCAssnswith a collection-local counterRegrowKinematicLine_module.cc:249did(*ksmca_H)[iseed], whereiseedcounts position within this module's own inputKalSeedCollection(wired toKKLine).KalSeedMCAssnsis one flat product:CommonMC/src/SelectRecoMC_module.cc:170does a singleproduces<KalSeedMCAssns>(), and:468-498runs one loop overgetMany<KalSeedCollection>callingaddSingleonce per seed per collection.Production/JobConfig/recoMC/prolog.fcl:32orders them["KKDe","KKDmu","KKUe","KKUmu","KKLine","KKCHmu"]— KKLine is 5th of 6. So on any event with a seed in an earlier collection,(*ksmca_H)[0]for the first KKLine seed returns a KKDe entry, the ptr self-check fails, and the art job dies withcet::exception("Reco").The sibling
RegrowLoopHelix(:305-317) does anart::Ptr-equality search, correct at any offset — which is also why the bug is invisible there:RegrowLHtargetsKKDe, index 0.Fixed with a
KalSeed -> KalSeedMCmap built once per event, which is correct at any offset and also drops the sibling's O(N·M) scan.2.
RegrowLoopHelix: mandatoryMustRegrowis absent from the live job configRegrowLoopHelix_module.cc:98declared a two-argumentfhicl::Atom<std::string>— mandatory, unlikedebugat:86in the same struct.No job config supplies it.
Production/JobConfig/reco/regrowLH.fcl:34-55buildsphysics.producers.RegrowLHas a literal block rather than pulling@table::Mu2eKinKal.RegrowLH, andProduction/JobConfig/reco/prolog.fcl:122-140omits it too. The onlyMustRegrow : "None"in the tree isMu2eKinKal/fcl/prolog.fcl:642, in the table neither Production block references —grep -rn MustRegrow Production/returns nothing. Validation fails before the module is constructed, so the constructor's own value check at:200-207is never reached.Fixed by giving the Atom the
"None"default that table already implies. (The alternative — adding the key toProduction— would need a companion PR there; this way the module is correct on its own.)3.
BkgANNSHU: unguardedsqrt(docaVar())feeds NaN to the MVABkgANNSHU.cc:30waspars[2] = sqrt(tpdata.docaVar());. A non-positivedocaVargives NaN;NaN < mvacut_is false under IEEE-754, so theelsebranch runs and the hit is left active — or, if it was inactive, re-activated at:45.Three sibling call sites clamp the identical quantity:
DriftANNSHU.cc:44,CADSHU.cc:31,KKFit.hh:510,567. Adopted the same clamp.4.
KKFit::createSeeddereferencesrbegin()of a possibly-empty domain setKKFit.hh:796did(*(kktrk.domains().rbegin()))->end()undersavedomains_.domains()is astd::set, empty whenever the fit ran without BField correction;rbegin()then equalsrend()and dereferencing it is UB.No committed config triggers it today — but only because
Mu2eKinKal/fcl/prolog.fcl:606-608carries a hand-written comment telling the next author not to setSaveDomains:truethere, citing this exact SIGSEGV. Replaced the load-bearing comment with a namedcet::exception, and updated the comment to say what now happens.5.
StrawHitUpdaters::name()indexes with a negative enumeratorunknown = -1(StrawHitUpdaters.hh:12), andname()didnames_[static_cast<size_t>(alg)]— UB whenever an unknown state is printed atdiag > 1. Bounds-checked; returns"unknown".6.
KLSeedFitomits the mandatorySaveTrajectorykeyKKFITdeliberately does not setSaveTrajectory, and every producer that builds from it supplies its own — verified atLHSeedFit:468,LHDriftFit:490,KLDriftFit:531,CHSeedFit:547,CHDriftFit:562,KLTruthSeed:587,CHTruthSeed:606,RegrowLH:636.KLSeedFitdid not, so its table cannot pass validation the moment it is added to a path. AddedSaveTrajectory : T0, matchingCHSeedFit, whoseFitSettingsandExtensionSettingsKLSeedFitalready shares.Validation
Productionandmu2e-trig-config, plus grep. Please letbuildtestbe the arbiter.mu2e -c Production/JobConfig/reco/regrowKL.fcland oneregrowLH.fclover a few events; if either already runs green today, the corresponding finding is wrong and I will withdraw it.fhicl-dump -aprovenance was produced. Add diagnostics #2 and Adjust parameters for new production #6 rest on reading the prologs and grepping@table::references; a resolution path I failed to model would change them.KinematicLineFit::goodFit()returning onlyfitStatus().usable()while both siblings also requireinDetector()/charge/helicity;KKBField::fieldGrad()returning the transpose of its own documenteddB_i/dx_jconvention; andExtrapolateCRVRegion's short-piece fallback being backward-only. Those need an author decision, so I left them out. Happy to open an issue with the full list.