Skip to content

Per-meta-iteration reference piece strategy (DOCA / TOCA) for effect re-referencing - #228

Open
RobMina wants to merge 1 commit into
KFTrack:mainfrom
RobMina:refpiece-strategy
Open

RobMina wants to merge 1 commit into
KFTrack:mainfrom
RobMina:refpiece-strategy

Conversation

@RobMina

@RobMina RobMina commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

Requested by @brownd1978 in the review of Mu2e/Offline#2034.

Summary

Adds RefPieceStrategy (DOCA, TOCA) and makes it a meta-iteration setting, separately for hits and for material
crossings. When an effect is re-referenced to a new piecewise trajectory, the strategy chooses which piece its closest
approach is computed on:

  • DOCA (default): follow the closest approach in space across pieces until the solution lies on the piece used to
    compute it. This is the behaviour of v3.8.0, and the code path is unchanged.
  • TOCA: compute the closest approach once, on the piece holding the hint time, and stay there. The solution's time may
    lie outside that piece's range.

With every setting at its default the output is bit-identical to v3.8.0 (validation below).

Changes

  • General/RefPieceStrategy.hh (new): the enum, name conversion and operator<<.
  • Fit/MetaIterConfig: hitRefPiece() / xingRefPiece() with setters, default DOCA; printed with the meta-iteration.
  • Trajectory/PiecewiseClosestApproach.hh: optional last constructor argument, default DOCA. TOCA runs one
    findTCA(hint) on nearestIndex(hint); DOCA runs the existing loop.
  • Effect::updateReference(ptraj, miconfig). Measurement passes miconfig.hitRefPiece() and Material passes
    miconfig.xingRefPiece() to Hit::updateReference(ptraj, strategy) / ElementXing::updateReference(ptraj, strategy).
    DomainWall and ParameterHit ignore the strategy (their piece is set by a fixed time). The Measurement and
    Material constructors take an optional miconfig (default-constructed, so DOCA).
  • Track: iterate() passes the current meta-iteration; createEffects and replaceDomains, which run outside the fit
    loop, use the first meta-iteration of the current configuration (firstMetaIter()). Effects added during
    extrapolation use DOCA.
  • Examples (ScintHit, SimpleWireHit, StrawXing, ShellXing) and tests (HitTest, ToyMC, FitTest) follow the
    new signatures; FitTest gains --HitRefPiece / --XingRefPiece.

Interface change: Hit::updateReference, ElementXing::updateReference and Effect::updateReference gain an
argument, so experiment code that implements them must be updated (Mu2e: Mu2e/Offline#2034). Hits and crossings receive
only the strategy, not the whole MetaIterConfig; they already get the configuration in updateState.

Why TOCA helps, and when (measured in Mu2e, Offline#2034)

  • TOCA and DOCA give the same answer whenever the TOCA solution lies inside the hint piece: the DOCA loop starts on
    the hint piece and stops there. A variant that falls back to DOCA when the TOCA solution is out of range was
    bit-identical to DOCA over 10,000 Mu2e CentralHelix events. So TOCA differs only by evaluating the hint piece
    beyond its range.
  • That matters while t0 is still moving. Per-hit logs of both answers over 2,000 events show that on tracks both choices
    fit, two thirds of hit updates move one or more pieces at the first meta-iteration, but the two answers agree to
    < 0.03 mm. On the tracks only TOCA fits, t0 moves ~1 ns per iteration at the start (0.26 ns on good tracks), the
    pieces disagree by 10-16 mm, and the DOCA fits keep wandering until they fail; under TOCA the same tracks settle by
    the third meta-iteration.
  • Capping the DOCA search at 1 or 2 pieces recovers only 17 or 10 of the 77 events gained by TOCA: what matters is
    staying on the hint piece itself.
  • TOCA removes every non-positive unbiased DOCA variance (Mu2e CentralHelix: 58 of 15,623 active hits under DOCA,
    0 under TOCA; LoopHelix KKDe: 3,579 of 778k, 0), i.e. the case where Hit::unbiasedParameters() subtracts a hit's
    weight from a piece other than the one it was applied to.

Which fits benefit is experiment-specific; in Mu2e only the CentralHelix fit gains (Offline#2034).

Validation

  • Unit tests: 26/26 ctest pass.
  • Default path (all DOCA), Mu2e production reco, EventNtuple compared leaf by leaf at rtol 0 against v3.8.0 on the same
    Offline: LoopHelix (CE MC, 24,950 events, 479 leaves), CentralHelix (cosmics, 10,000 events, 530 leaves), KinematicLine
    (extracted cosmics, 20,000 events, 481 leaves): identical.
  • TOCA for CentralHelix straw hits reproduces Keep CentralHelix straw hits on their weight's trajectory piece when re-referencing Mu2e/Offline#2034 bit-for-bit (530 leaves).
  • FitTest toys (1000 events, same seed): LoopHelix 970 / 971 converged (DOCA / TOCA hits), CentralHelix 846 / 851.

…, depending on fit type, temp, metaiteration number.

@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 provides a generalized implementation of Offline PR 2034, thanks for doing that.

The only thing that gives me pause is how the PCA 'hint' now gets used as a hard constraint in the TOCA case, not just a starting value. This is far from the original intent of providing a rough guess to disambiguate local minima due to branches in loop helices. OTOH overloading PCA is a simple and elegant way to implement this change.

My suggestion is to add a clear comment that in the TOCA case the hint is used to define the reference piece, not just a starting value.

@brownd1978

Copy link
Copy Markdown
Collaborator

Another possible solution: pass maxiter to PCA constructor instead of the strategy, and set it to 0 in the TOCA case. This makes the behavior clear, and addresses an existing TODO by passsing kkconfig::maxniter_ in the DOCA case. To get this to work would require re-arranging the do{}while loop in PCA so that it doesn't advance to the next piece in the iter=0 case.

@oksuzian oksuzian left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR Review Summary — #228

Reviewed at head bc037cf2474edab279acacdfe3ef638308feb498. first pass

Decision

  • 🟢 approve. The default path is unchanged by construction, the strategy reaches every effect type, and GitHub Actions pass on all four configurations. The interface change needs Offline#2034 to land with the KinKal version bump. That is a merge-order note, not a blocker.

Scope understood

  • Adds a per-meta-iteration choice of how a hit or material crossing picks its reference piece: DOCA (follow the closest approach across pieces, as in v3.8.0) or TOCA (stay on the piece holding the hint time).
  • Hit, ElementXing and Effect::updateReference gain an argument, so every experiment-side override must change. Mu2e's side is Offline#2034, a draft that depends on this PR.

Findings

  1. ⚪ [S3] No ctest runs the TOCA branch. FitTest gains --HitRefPiece / --XingRefPiece, but the *Fit_unit.cc drivers pass only --Schedule (and --Bz / --constrainpar for KinematicLine). The new early-return in PiecewiseClosestApproach.hh:23-28 is therefore not compiled into any CI test path that executes it. The toy numbers in the description (970/971 LoopHelix, 846/851 CentralHelix) were run by hand.
    • Suggested fix: one extra test target, e.g. CentralHelixFit with --HitRefPiece TOCA, so a later change to the closest-approach loop cannot silently break it.
  2. ⚪ [S3] The empty-schedule branch in firstMetaIter() cannot be reached. Track.hh:162-164 falls back to a function-local static default when config().schedule() is empty. Both ways a configuration enters a Track already throw on an empty schedule: the constructor at Track.hh:185 and extend at Track.hh:282. config() is config_.back(), which extend has already pushed, so the fallback is never taken.
    • Suggested fix: return config().schedule().front() directly, or keep the guard with a comment saying it is defensive.

🟢 Verified at this head:

  • The DOCA path is the v3.8.0 code. In PiecewiseClosestApproach the only change is an early if(strategy == TOCA){ findTCA(hint); return; } before the existing loop. The loop, including the #226 phint fix (#225 row 8) and the unconverged override, is byte-identical below it. MetaIterConfig initialises both strategies to DOCA at the declaration (MetaIterConfig.hh), and the Measurement / Material constructors default to a default-constructed config. With nothing set, every call reaches the old loop. I did not re-run the author's leaf-by-leaf ntuple comparison.
  • The strategy reaches every effect.
    • In the fit loop, Track::iterate passes the current meta-iteration (Track.hh:627).
    • createEffects and replaceDomains pass firstMetaIter() (:501-507, :397). During extend, that is the first meta-iteration of the extension configuration, because the configuration is pushed before either runs (:284).
    • Effects added by extrapolate use the default, so DOCA (:1098, :1104), as the description says.
    • Measurement routes hitRefPiece() and Material routes xingRefPiece().
    • DomainWall and ParameterHit ignore the strategy, which is right: their piece is fixed by a time.
    • All four Examples classes and the three test harnesses (HitTest, ToyMC, FitTest) are updated.
    • A git grep at the head finds no other updateReference override or Measurement / Material construction.
  • The header installs. General/RefPieceStrategy.hh is header-only and is picked up by the top-level install(DIRECTORY ... PATTERN "*.hh") (CMakeLists.txt:209-211). It is not in the ROOT dictionary list, and nothing persists it, so that is fine.
  • refPieceStrategy(name) throws on an unknown name rather than defaulting, and the two new FitTest option letters (H, G) do not collide with existing ones.
  • #225 overlap. No #225 row is reopened. Row 8's fix stays on the DOCA path. Under TOCA that loop is skipped by design. This addresses the row 61 mechanism (a weight subtracted from a piece other than the one it was applied to). That fits the author's count of zero non-positive unbiased variances under TOCA, which I did not reproduce.

On @brownd1978's comment. I agree the doc comment should say that under TOCA the hint chooses the piece and is not just a starting value. One detail is worth adding to it. For KKStrawHit (Offline Mu2eKinKal/inc/KKStrawHit.hh:164) and for the Examples hits, the hint is ca_.hint(), which is the previous iteration's solution time (ClosestApproach.hh:69). Under TOCA that solution may lie outside its piece. The next iteration then starts on the piece holding that time. So TOCA does not pin a hit to one piece for the whole fit: the piece can move by at most one hint per iteration, one iteration late. That reading is from the code; I have not measured how often it happens.

Validation check

  • Build/tests run: GitHub Actions Tests pass at this head on all four configurations (ubuntu and macOS, Debug and Release). Locally, nothing was built or run: the session could not write the build script, so KinKal ctest and the default-path toy comparison are not run. All statements above come from reading the diff and the files at bc037cf2 in a clone, with greps at the head and at Offline main f122c5b3.
  • Config contract check: n/a in KinKal. Mu2e exposes the setting through the Offline#2034 fhicl keys, which are reviewed there.
  • Cross-repo consistency: needs follow-up, by design. Offline main overrides the old updateReference(PTRAJ const&) in KKCaloHit, KKShellXing, KKStrawHit, KKStrawHitCluster and KKStrawXing, and will not compile against this PR. Offline#2034 (draft, head 9121d101) carries the matching changes. The order is: merge this, tag a KinKal release, bump the spack pin, then take Offline#2034 out of draft.

Residual risk

  • TOCA has no automated test, so its correctness rests on the author's hand-run toys and Mu2e samples.

Author follow-ups

  1. Optional: a ctest target that runs a fit with --HitRefPiece TOCA (finding 1).
  2. Optional: simplify firstMetaIter() (finding 2).
  3. When you add the doc comment @brownd1978 asked for, consider noting that the hint is the previous solution time, so the piece can still move between iterations.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants