Repository navigation
Conversation
…, depending on fit type, temp, metaiteration number.
brownd1978
left a comment
There was a problem hiding this comment.
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.
|
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
left a comment
There was a problem hiding this comment.
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) orTOCA(stay on the piece holding the hint time). Hit,ElementXingandEffect::updateReferencegain an argument, so every experiment-side override must change. Mu2e's side is Offline#2034, a draft that depends on this PR.
Findings
- ⚪ [S3] No ctest runs the
TOCAbranch.FitTestgains--HitRefPiece/--XingRefPiece, but the*Fit_unit.ccdrivers pass only--Schedule(and--Bz/--constrainparfor KinematicLine). The new early-return inPiecewiseClosestApproach.hh:23-28is 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.
CentralHelixFitwith--HitRefPiece TOCA, so a later change to the closest-approach loop cannot silently break it.
- Suggested fix: one extra test target, e.g.
- ⚪ [S3] The empty-schedule branch in
firstMetaIter()cannot be reached.Track.hh:162-164falls back to a function-local static default whenconfig().schedule()is empty. Both ways a configuration enters aTrackalready throw on an empty schedule: the constructor atTrack.hh:185andextendatTrack.hh:282.config()isconfig_.back(), whichextendhas 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.
- Suggested fix: return
🟢 Verified at this head:
- The
DOCApath is the v3.8.0 code. InPiecewiseClosestApproachthe only change is an earlyif(strategy == TOCA){ findTCA(hint); return; }before the existing loop. The loop, including the #226phintfix (#225 row 8) and theunconvergedoverride, is byte-identical below it.MetaIterConfiginitialises both strategies toDOCAat the declaration (MetaIterConfig.hh), and theMeasurement/Materialconstructors 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::iteratepasses the current meta-iteration (Track.hh:627). createEffectsandreplaceDomainspassfirstMetaIter()(:501-507,:397). Duringextend, that is the first meta-iteration of the extension configuration, because the configuration is pushed before either runs (:284).- Effects added by
extrapolateuse the default, soDOCA(:1098,:1104), as the description says. MeasurementrouteshitRefPiece()andMaterialroutesxingRefPiece().DomainWallandParameterHitignore 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 grepat the head finds no otherupdateReferenceoverride orMeasurement/Materialconstruction.
- In the fit loop,
- The header installs.
General/RefPieceStrategy.hhis header-only and is picked up by the top-levelinstall(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 newFitTestoption letters (H,G) do not collide with existing ones.- #225 overlap. No #225 row is reopened. Row 8's fix stays on the
DOCApath. UnderTOCAthat 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 underTOCA, 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
Testspass 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 atbc037cf2in a clone, with greps at the head and at Offlinemainf122c5b3. - 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
mainoverrides the oldupdateReference(PTRAJ const&)inKKCaloHit,KKShellXing,KKStrawHit,KKStrawHitClusterandKKStrawXing, and will not compile against this PR. Offline#2034 (draft, head9121d101) 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
TOCAhas no automated test, so its correctness rests on the author's hand-run toys and Mu2e samples.
Author follow-ups
- Optional: a ctest target that runs a fit with
--HitRefPiece TOCA(finding 1). - Optional: simplify
firstMetaIter()(finding 2). - 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.
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 materialcrossings. 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 tocompute 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 maylie 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 andoperator<<.Fit/MetaIterConfig:hitRefPiece()/xingRefPiece()with setters, defaultDOCA; printed with the meta-iteration.Trajectory/PiecewiseClosestApproach.hh: optional last constructor argument, defaultDOCA.TOCAruns onefindTCA(hint)onnearestIndex(hint);DOCAruns the existing loop.Effect::updateReference(ptraj, miconfig).Measurementpassesmiconfig.hitRefPiece()andMaterialpassesmiconfig.xingRefPiece()toHit::updateReference(ptraj, strategy)/ElementXing::updateReference(ptraj, strategy).DomainWallandParameterHitignore the strategy (their piece is set by a fixed time). TheMeasurementandMaterialconstructors take an optionalmiconfig(default-constructed, soDOCA).Track:iterate()passes the current meta-iteration;createEffectsandreplaceDomains, which run outside the fitloop, use the first meta-iteration of the current configuration (
firstMetaIter()). Effects added duringextrapolation use
DOCA.ScintHit,SimpleWireHit,StrawXing,ShellXing) and tests (HitTest,ToyMC,FitTest) follow thenew signatures;
FitTestgains--HitRefPiece/--XingRefPiece.Interface change:
Hit::updateReference,ElementXing::updateReferenceandEffect::updateReferencegain anargument, 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 inupdateState.Why TOCA helps, and when (measured in Mu2e, Offline#2034)
TOCAandDOCAgive the same answer whenever the TOCA solution lies inside the hint piece: the DOCA loop starts onthe hint piece and stops there. A variant that falls back to
DOCAwhen the TOCA solution is out of range wasbit-identical to
DOCAover 10,000 Mu2e CentralHelix events. SoTOCAdiffers only by evaluating the hint piecebeyond its range.
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
TOCAfits, t0 moves ~1 ns per iteration at the start (0.26 ns on good tracks), thepieces disagree by 10-16 mm, and the
DOCAfits keep wandering until they fail; underTOCAthe same tracks settle bythe third meta-iteration.
DOCAsearch at 1 or 2 pieces recovers only 17 or 10 of the 77 events gained byTOCA: what matters isstaying on the hint piece itself.
TOCAremoves every non-positive unbiased DOCA variance (Mu2e CentralHelix: 58 of 15,623 active hits underDOCA,0 under
TOCA; LoopHelix KKDe: 3,579 of 778k, 0), i.e. the case whereHit::unbiasedParameters()subtracts a hit'sweight 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
ctestpass.DOCA), Mu2e production reco, EventNtuple compared leaf by leaf at rtol 0 against v3.8.0 on the sameOffline: LoopHelix (CE MC, 24,950 events, 479 leaves), CentralHelix (cosmics, 10,000 events, 530 leaves), KinematicLine
(extracted cosmics, 20,000 events, 481 leaves): identical.
TOCAfor 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).FitTesttoys (1000 events, same seed): LoopHelix 970 / 971 converged (DOCA / TOCA hits), CentralHelix 846 / 851.