Repository navigation
Conversation
|
☀️ The build tests passed at ce894a5.
N.B. These results were obtained from a build of this Pull Request at ce894a5 after being merged into the base branch at 24a1aa5. For more information, please check the job page here. |
oksuzian
left a comment
There was a problem hiding this comment.
PR Review Summary — #2034
Reviewed at head ce894a55e6af689753d690efe48fcbc7496ec0aa. first pass
Decision
- 🟢 approve. The change is small and contained, it is off for every fit except CentralHelix, and the LoopHelix identity check plus the paired CentralHelix comparison are the right evidence. Two notes below, neither blocking. FNALbuild passes at this head.
Scope understood
- A new
KKFitConfigatom,StrawHitHintPieceCA. When it is true,KKStrawHit::updateReferencecomputes the closest approach onptraj.nearestTraj(hint.particleToca_), the piece holding the previous TPOCA, instead of lettingPiecewiseClosestApproachfollow the new TPOCA across pieces. The flag is passed at all threeKKSTRAWHITconstructions inKKFit.hh(seedmakeStrawHits, KalSeedregrowComponents,addStrawHits) and is copied by the clone constructor. - It is set
trueinCHSeedFitandCHDriftFitonly. - Who gets it.
KKCHmu(CHDriftFit) runs in ProductionReconstruction.CentralHelixReco, which is inOffSpillTrk. So the OffSpill reco job (recoMC/OffSpill.fcl), the nightlyValidation/offSpill.fcland themdc2025/reco.jsonOffSpill cosmic dataset get the new behaviour.KKCHSeedFitmuis defined but no Production path uses it. TheCHTruthSeedtable does not set the key and makes no straw hits. mu2e-trig-config (mainccc5ff5) has no CentralHelix fit, so the trigger is unaffected. OnSpill reco is unchanged.
Findings
-
🟡 [S2]
StrawHitHintPieceCAcarries a C++ default- Evidence:
KKFitSettings.hh:70declaresfhicl::Atom<bool> hintPieceCA{..., false}, and theKKStrawHitconstructor also defaultshintpiececa=false(KKStrawHit.hh:54). Every other algorithm key inKKFitConfigaround it (TPOCAPrecision,MaterialCorrection,AddHits, ...) is mandatory. This is the same point I raised on #2033 today. - Impact: small here, since
falsereproduces current behaviour for the LoopHelix and KinematicLine tables that do not mention it. But a choice that changes which fits converge is then invisible in those tables, and a newKKStrawHitconstruction site would silently getfalse. - Suggested fix: make the atom mandatory and add
StrawHitHintPieceCA : falsetoMu2eKinKal.KKFIT(the base table every fit includes), so the two CentralHelix tables override it. Dropping the constructor default would make any future construction site state its choice.
- Evidence:
-
🟡 [S2] This works around the KinKal#226 piece/weight mismatch for CentralHelix only
- Evidence: the mechanism is the one recorded on KinKal#226 (my row 61 there, agreed by @brownd1978 and yourself). After #226,
PiecewiseClosestApproachiteratesnearestIndex(phint.particleToca_)(KinKalfe16d12), soupdateReferencecan repoint the hit to a different piece while itsweight_is still the old piece's, andHit::unbiasedParameters()(Detector/Hit.hh:61-70at v3.8.0) subtracts the old weight from the new reference. #226 merged at71cc7d9without changing that. This PR keeps CentralHelix straw hits on the old piece, which removes the mismatch for them. You also measured a loss of about 7% on LoopHelix on #226 (data_like_no_primary_1BB), and LoopHelix is left on the piecewise closest approach here; the bit-identical LoopHelix check shows nothing changed, not that LoopHelix is unaffected by #226. - Suggested fix: none needed in this PR. Worth stating in the description that LoopHelix still runs the v3.8.0 behaviour, and whether the longer-term fix (computing the unbiased parameters before the closest-approach update, as @brownd1978 proposed on #226) belongs in KinKal.
- Evidence: the mechanism is the one recorded on KinKal#226 (my row 61 there, agreed by @brownd1978 and yourself). After #226,
🟢 Verified at this head:
- The piece lookup is safe.
ptraj.nearestTraj(t)ispieces_[nearestIndex(t)](PiecewiseTrajectory.hh:53). At KinKal v3.8.0,nearestIndexclamps a time before the trajectory to piece 0 and after it to the last piece, and scans otherwise (:197-213), so a previous TPOCA outside the new range or on a boundary still gets a valid piece. - Same hint and precision. The
CA(ktrajptr, straj, hint, precision)constructor runs onefindTCA(hint)(ClosestApproach.hh:95-98), the same first step the piecewise constructor takes, so only the follow-across-pieces iteration is skipped. The fallback branch (zTimehint when the first solution is off by more than 100 mm in z) uses the samefindca, so it also stays on the hinted piece when the flag is on. - Flag plumbing. It is set at all three
KKSTRAWHITconstruction sites inKKFit.hh(:250,:302,:431), and the clone constructor copies it (KKStrawHit.hh:68). No other code constructs aKKStrawHit.KKTruthSeeduses an empty straw-hit collection. - Off means off. With the flag
false,findcareturnsstatic_cast<CA>(PCA(...)), exactly the old expression, andca_ = pcaassigns the sameCAvalue as the oldca_ = static_cast<CA>(pca). That agrees with your bit-identical LoopHelix ntuple. - Compiles with KinKal v3.8.0.
g++ -std=c++20 -fsyntax-only -Wall -WextraofCentralHelixFit_module.cc,LoopHelixFit_module.cc,KinematicLineFit_module.cc,RegrowLoopHelix_module.ccandKKTruthSeed_module.ccat this head, with the p108 envset (KinKal v3.8.0), all pass. The only warnings in the touched headers predate this PR (KKStrawHit.hh:71unusedcontext). Script:claude-scratch/pr2034_review/syntax.sh.
Validation check
- Build/tests run: FNALbuild at this head passed, including
ceSimReco.triggerfails with return code 2, as on every Offline PR build since #2026, so that is not from this PR. Locally, the syntax check above. I did not rerun your LoopHelix identity or CentralHelix comparison. - Config contract check: pass, apart from finding 1.
- Cross-repo consistency: pass. No Production or trigger change needed.
Residual risk
- The CentralHelix gain (883 → 960 matched events of 10k) is on one cosmic MC sample. Real OffSpill data, and the KPP extracted runs, could still be checked once a CentralHelix data path exists.
Author follow-ups
- Optional: make
StrawHitHintPieceCAmandatory withfalseinKKFIT(finding 1). - Optional: one sentence in the description on LoopHelix and the KinKal-side fix (finding 2).
brownd1978
left a comment
There was a problem hiding this comment.
This PR's functional change is to KKStrawHit::updateReference, which gets called at the very end of the main iteration loop, just before replacing the track's fittraj_. That function sets the reference parameters and DOCA used in the next fit iteration. It also affects the computation of the unbiased DOCA stored in KalSeed after the last iteration.
This PR optionally changes how the reference trajectory piece is found, from using spatial information (closest approach in space using the full piecewise trajectory) to temporal information (closest piece in time with the hit). This isn't really finding the 'same piece' as stated in the comment; the original piece against which the previous hint was defined no longer even exists once fittraj_ is swapped out. The change is made only for KKStrawHit, and (currently) only for CH fits.
I don't fully understand why using temporal vs spatial minimization when computing CA and caching reference parameters improves the fit stability. I suspect it's because the previous hint implicitly references the previous fit's t0 value, which can change dramatically between iterations in the early stages. It could also be related to the fact that the PCA calculation explicitly requires the TPOCA to be in-range on the piece it finds, while the new calculation doesn't; ie the new CA can reference a time that isn't in-range of the piece used to define the reference parameters.
Making this change active only for CH fits and KKStrawHits is conservative, but if this change really improves iteration stability, I don't see why it shouldn't be used for all trajectory types, for all hits, and for material effects as well (which also use POCA to find reference parameters). It's also not clear if this additional stability is only needed in early iterations, or if there's any advantage to using POCA to define the reference in other fits, or later fit iterations (after t0 has converged).
I therefore suggest generalizing this concept into a new meta-iteration global parameter (say referencePieceStrategy), which could have value 'TOCA' or 'DOCA'. Passing miconfig to every effect's updateReference call along with the new ptraj makes the behavior configurable by trajectory type, by effect, and by meta-iteration. It requires a core KinKal change, but it's fairly minor, and I think it will make the intent and impact easier to understand.
|
Thanks Dave! I agree with your proposal and will open a PR to KinKal. I'm marking this PR as a draft since it will be blocked until the KinKal PR goes in and the KinKal version is bumped. |
|
📝 The HEAD of |
ce894a5 to
9121d10
Compare
Depends on KFTrack/KinKal#228 (per-meta-iteration
RefPieceStrategy): this PR builds only against a KinKal that includesit, so it stays a draft until #228 is merged and the KinKal version is bumped.
Summary
Replaces the
StrawHitHintPieceCAflag of the first version of this PR with KinKal's per-meta-iteration reference piecestrategy, as requested in review. Two optional
KinKalConfigsequences,HitReferencePieceandXingReferencePiece,give one strategy per
MetaIterationSettingsentry (DOCAorTOCA); absent means allDOCA. The Mu2eKinKal prolog setsHitReferencePiecetoTOCAfor every meta-iteration of the CentralHelix fits (CHSeedFit,CHDriftFit, seed fit andextension); crossings stay
DOCA. LoopHelix and KinematicLine fits useDOCAand their output is unchanged.Changes
Mu2eKinKal/inc/KKFitSettings.hh,src/KKFitSettings.cc: the twoOptionalSequence<std::string>keys;makeConfigsets each meta-iteration's strategy and throws on a length different from
MetaIterationSettingsor an unknown value.KKStrawHit,KKStrawXing: pass the strategy to theirPiecewiseClosestApproach(both calls inKKStrawHit, includingthe
zTimefallback).KKCaloHit(hint = measurement time),KKShellXing(piece from its intersection time) andKKStrawHitCluster(no reference of its own) take the new signature and ignore the strategy.Mu2eKinKal/fcl/prolog.fcl:HitReferencePiece : TOCAinCHSeedFitandCHDriftFit. They are set in the moduletables, not in
CHSEEDFIT/CHSEEDEXT/CHDRIFTEXT, becauseKLSeedFit/KLDriftFitreuse those.The keys are optional rather than mandatory: mu2e-trig-config copies
LHSEEDFIT/LHSEEDEXTwith 2-entryMetaIterationSettings, so explicit lists in the base tables would make the trigger configuration throw. A job thatoverrides
KKCHmu'sMetaIterationSettingswith a different length must also overrideHitReferencePiece.Why (see also the KinKal PR)
KinKal #226 made
PiecewiseClosestApproachfollow each hit's new TPOCA to another piece. In the CentralHelix seed fit oflow-momentum cosmics, which starts from a straight-line seed, t0 moves by ~1 ns per iteration while pieces are ~0.15 ns
long and disagree by up to 10-16 mm; following the TPOCA there moves hits off the piece consistent with their previous
linearization, and fits fail.
TOCAkeeps the hit on the piece holding its hint time and differs fromDOCAonly whenthe solution falls outside that piece.
Validation (10,000 events
CosmicSignalOffSpill.MDC2025an, 24,950CeMLeadingLogMix1BB.MDC2025au, 20,000CosmicCRYExtracted.MDC2025au; standard reco)Bitwise checks (EventNtuple, every numeric leaf, rtol 0):
DOCAStrawHitHintPieceCA: true)CentralHelix (
KKCHmu), events with a truth-matched cosmic muon track, paired over identical events:DOCADOCA(= Offline main + KinKal v3.8.0)TOCAfor straw hits, every meta-iteration (this PR)On the 820 events with one matched track in both configurations, fit quality is unchanged or slightly better (median
fitcon 4.9e-4 / 4.6e-4, unbiased distance pull core 1.50 / 1.47), and the non-positive unbiased DOCA variances (58 of
15,623 active hits) disappear. The 136 tracks only
TOCAfinds are better fits than the 59 onlyDOCAfinds (momentumcore vs truth 50 vs 90 MeV/c).
Alternatives tested (same events):
KKCHmuconfigurationTOCAin the seed fit onlyTOCAwhile the annealing temperature >= 1TOCAexcept the last meta-iterationTOCAfor hits and straw crossingsThe partial schedules also bring back 175-233 non-positive unbiased variances and widen the unbiased pulls by 0.04-0.06;
crossings on
TOCAare neutral. WithBFieldCorrection: true(not the production setting),TOCAgives 838 against 770for
DOCA(+4.1 sigma).LoopHelix with
TOCAfor hits (24,950 CE events, not enabled): events with a matched electron 23,135 -> 23,143 (+0.9sigma), median fitcon of the KKDe fit 0.264 -> 0.232, low-purity tracks 42 -> 62. No efficiency gain, so it is not
turned on; the
DOCAresult is bit-identical to main. KinematicLine withTOCA: 13,150 -> 13,155, quality unchanged.