Skip to content

Keep CentralHelix straw hits on their weight's trajectory piece when re-referencing - #2034

Draft
RobMina wants to merge 1 commit into
Mu2e:mainfrom
RobMina:strawhit-hintpiece-ca
Draft

RobMina wants to merge 1 commit into
Mu2e:mainfrom
RobMina:strawhit-hintpiece-ca

Conversation

@RobMina

@RobMina RobMina commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Depends on KFTrack/KinKal#228 (per-meta-iteration RefPieceStrategy): this PR builds only against a KinKal that includes
it, so it stays a draft until #228 is merged and the KinKal version is bumped.

Summary

Replaces the StrawHitHintPieceCA flag of the first version of this PR with KinKal's per-meta-iteration reference piece
strategy, as requested in review. Two optional KinKalConfig sequences, HitReferencePiece and XingReferencePiece,
give one strategy per MetaIterationSettings entry (DOCA or TOCA); absent means all DOCA. The Mu2eKinKal prolog sets
HitReferencePiece to TOCA for every meta-iteration of the CentralHelix fits (CHSeedFit, CHDriftFit, seed fit and
extension); crossings stay DOCA. LoopHelix and KinematicLine fits use DOCA and their output is unchanged.

Changes

  • Mu2eKinKal/inc/KKFitSettings.hh, src/KKFitSettings.cc: the two OptionalSequence<std::string> keys; makeConfig
    sets each meta-iteration's strategy and throws on a length different from MetaIterationSettings or an unknown value.
  • KKStrawHit, KKStrawXing: pass the strategy to their PiecewiseClosestApproach (both calls in KKStrawHit, including
    the zTime fallback). KKCaloHit (hint = measurement time), KKShellXing (piece from its intersection time) and
    KKStrawHitCluster (no reference of its own) take the new signature and ignore the strategy.
  • Mu2eKinKal/fcl/prolog.fcl: HitReferencePiece : TOCA in CHSeedFit and CHDriftFit. They are set in the module
    tables, not in CHSEEDFIT / CHSEEDEXT / CHDRIFTEXT, because KLSeedFit / KLDriftFit reuse those.

The keys are optional rather than mandatory: mu2e-trig-config copies LHSEEDFIT / LHSEEDEXT with 2-entry
MetaIterationSettings, so explicit lists in the base tables would make the trigger configuration throw. A job that
overrides KKCHmu's MetaIterationSettings with a different length must also override HitReferencePiece.

Why (see also the KinKal PR)

KinKal #226 made PiecewiseClosestApproach follow each hit's new TPOCA to another piece. In the CentralHelix seed fit of
low-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. TOCA keeps the hit on the piece holding its hint time and differs from DOCA only when
the solution falls outside that piece.

Validation (10,000 events CosmicSignalOffSpill.MDC2025an, 24,950 CeMLeadingLogMix1BB.MDC2025au, 20,000 CosmicCRYExtracted.MDC2025au; standard reco)

Bitwise checks (EventNtuple, every numeric leaf, rtol 0):

Configuration Reference Result
CentralHelix, all DOCA Offline main + KinKal v3.8.0 identical (530 leaves)
LoopHelix (CE MC) same identical (479 leaves)
KinematicLine same identical (481 leaves)
CentralHelix, this prolog the first version of this PR (StrawHitHintPieceCA: true) identical (530 leaves)

CentralHelix (KKCHmu), events with a truth-matched cosmic muon track, paired over identical events:

Configuration Events vs DOCA
DOCA (= Offline main + KinKal v3.8.0) 883
TOCA for straw hits, every meta-iteration (this PR) 960 +77 (136 gained, 59 lost), +5.5 sigma

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 TOCA finds are better fits than the 59 only DOCA finds (momentum
core vs truth 50 vs 90 MeV/c).

Alternatives tested (same events):

KKCHmu configuration Events vs this PR
TOCA in the seed fit only 949 -11, -2.2 sigma
TOCA while the annealing temperature >= 1 951 -9, -2.2 sigma
TOCA except the last meta-iteration 957 -3, -1.3 sigma
TOCA for hits and straw crossings 953 -7, -1.3 sigma

The partial schedules also bring back 175-233 non-positive unbiased variances and widen the unbiased pulls by 0.04-0.06;
crossings on TOCA are neutral. With BFieldCorrection: true (not the production setting), TOCA gives 838 against 770
for DOCA (+4.1 sigma).

LoopHelix with TOCA for hits (24,950 CE events, not enabled): events with a matched electron 23,135 -> 23,143 (+0.9
sigma), 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 DOCA result is bit-identical to main. KinematicLine with TOCA: 13,150 -> 13,155, quality unchanged.

@FNALbuild

Copy link
Copy Markdown
Collaborator

Hi @RobMina,
You have proposed changes to files in these packages:

  • Mu2eKinKal

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

⌛ The following tests have been triggered for ce894a5: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild

Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at ce894a5.

Test Result Details
test with ✅ Command did not list any other PRs to include
merge ✅ Merged ce894a5 at 24a1aa5
build (prof) ✅ Log file. Build time: 04 min 28 sec
ceSimReco ✅ Log file.
g4test_03MT ✅ Log file.
transportOnly ✅ Log file.
POT ✅ Log file.
g4study ✅ Log file.
cosmicSimReco ✅ Log file.
cosmicOffSpill ✅ Log file.
ceSteps ✅ Log file.
ceDigi ✅ Log file.
muDauSteps ✅ Log file.
ceMix ✅ Log file.
rootOverlaps ✅ Log file.
g4surfaceCheck ✅ Log file.
trigger ❌ Log file. Return Code 2.
check_cmake ✅ Log file.
FIXME, TODO ➡️ TODO (15) FIXME (9) in 3 files
clang-tidy ✅ 0 errors 0 warnings
whitespace check ✅ no whitespace errors found

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.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

@oksuzian oksuzian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 KKFitConfig atom, StrawHitHintPieceCA. When it is true, KKStrawHit::updateReference computes the closest approach on ptraj.nearestTraj(hint.particleToca_), the piece holding the previous TPOCA, instead of letting PiecewiseClosestApproach follow the new TPOCA across pieces. The flag is passed at all three KKSTRAWHIT constructions in KKFit.hh (seed makeStrawHits, KalSeed regrowComponents, addStrawHits) and is copied by the clone constructor.
  • It is set true in CHSeedFit and CHDriftFit only.
  • Who gets it. KKCHmu (CHDriftFit) runs in Production Reconstruction.CentralHelixReco, which is in OffSpillTrk. So the OffSpill reco job (recoMC/OffSpill.fcl), the nightly Validation/offSpill.fcl and the mdc2025/reco.json OffSpill cosmic dataset get the new behaviour. KKCHSeedFitmu is defined but no Production path uses it. The CHTruthSeed table does not set the key and makes no straw hits. mu2e-trig-config (main ccc5ff5) has no CentralHelix fit, so the trigger is unaffected. OnSpill reco is unchanged.

Findings

  1. 🟡 [S2] StrawHitHintPieceCA carries a C++ default

    • Evidence: KKFitSettings.hh:70 declares fhicl::Atom<bool> hintPieceCA{..., false}, and the KKStrawHit constructor also defaults hintpiececa=false (KKStrawHit.hh:54). Every other algorithm key in KKFitConfig around it (TPOCAPrecision, MaterialCorrection, AddHits, ...) is mandatory. This is the same point I raised on #2033 today.
    • Impact: small here, since false reproduces 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 new KKStrawHit construction site would silently get false.
    • Suggested fix: make the atom mandatory and add StrawHitHintPieceCA : false to Mu2eKinKal.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.
  2. 🟡 [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, PiecewiseClosestApproach iterates nearestIndex(phint.particleToca_) (KinKal fe16d12), so updateReference can repoint the hit to a different piece while its weight_ is still the old piece's, and Hit::unbiasedParameters() (Detector/Hit.hh:61-70 at v3.8.0) subtracts the old weight from the new reference. #226 merged at 71cc7d9 without 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.

🟢 Verified at this head:

  • The piece lookup is safe. ptraj.nearestTraj(t) is pieces_[nearestIndex(t)] (PiecewiseTrajectory.hh:53). At KinKal v3.8.0, nearestIndex clamps 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 one findTCA(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 (zTime hint when the first solution is off by more than 100 mm in z) uses the same findca, so it also stays on the hinted piece when the flag is on.
  • Flag plumbing. It is set at all three KKSTRAWHIT construction sites in KKFit.hh (:250, :302, :431), and the clone constructor copies it (KKStrawHit.hh:68). No other code constructs a KKStrawHit. KKTruthSeed uses an empty straw-hit collection.
  • Off means off. With the flag false, findca returns static_cast<CA>(PCA(...)), exactly the old expression, and ca_ = pca assigns the same CA value as the old ca_ = 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 -Wextra of CentralHelixFit_module.cc, LoopHelixFit_module.cc, KinematicLineFit_module.cc, RegrowLoopHelix_module.cc and KKTruthSeed_module.cc at this head, with the p108 envset (KinKal v3.8.0), all pass. The only warnings in the touched headers predate this PR (KKStrawHit.hh:71 unused context). Script: claude-scratch/pr2034_review/syntax.sh.

Validation check

  • Build/tests run: FNALbuild at this head passed, including ceSimReco. trigger fails 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

  1. Optional: make StrawHitHintPieceCA mandatory with false in KKFIT (finding 1).
  2. Optional: one sentence in the description on LoopHelix and the KinKal-side fix (finding 2).

@brownd1978 brownd1978 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RobMina

RobMina commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

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.

@RobMina
RobMina marked this pull request as draft October 16, 2026 14:20
@FNALbuild

Copy link
Copy Markdown
Collaborator

📝 The HEAD of main has changed to 8fdf0ea. Tests are now out of date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants