feat: CRTP track state creator for the CKF - #5849
Draft
andiwand wants to merge 8 commits into
Draft
Conversation
Extract the body of `MeasurementSelector::calculateChi2` verbatim into a free `Acts::calculatePredictedChi2` in the same translation unit, and make `MeasurementSelector::Cuts` and `getCuts` public. This is a pure refactor. It prepares a track state creator that computes the compatibility chi2 directly from a measurement container instead of from a temporary track state: such a creator needs both the resolved, eta-binned cuts and the exact same chi2 expression, and re-deriving either of them would silently change the selection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UckMSEcXcYZMgokrPeSQTW
Adds `Acts::TrackStateCreatorBase`, a CRTP base which computes the compatibility chi2 of a measurement directly from whatever the derived class calls a measurement container, and only materialises track states for the measurements it actually selects. `Acts::TrackStateCreator` instead creates a temporary track state in the trajectory for every measurement on a surface, calibrates it, and then throws almost all of them away. On a busy detector that pushes every measurement through the track EDM for nothing. The default customization points reproduce the selection of `Acts::MeasurementSelector` exactly, including the rules that an outlier surface yields exactly one track state and that an empty result means the caller creates a hole. Cuts are taken from `MeasurementSelector::getCuts` and the chi2 from `calculatePredictedChi2` rather than being re-derived, so the selection cannot drift. `CombinatorialKalmanFilterExtensions` gains `trackStateCreator`, which has the same contract as `createTrackStates` but without the scratch buffer for temporary track states. The filter uses whichever of the two is connected, so this is not a breaking change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UckMSEcXcYZMgokrPeSQTW
Ports both examples track finders and the combinatorial Kalman filter tests to `Acts::TrackStateCreatorBase` and deprecates `Acts::TrackStateCreator`. The examples track finding no longer creates a temporary track state per measurement on a surface. The chi2 is computed straight from the measurement container and only the selected measurements reach the track EDM. "Stay on seed" becomes a preselection which runs before anything is calibrated, rather than a filter on already built track states. Note the calibration reads through `MeasurementSubset::getMeasurement`, which takes an index into the underlying container, and not through `at`, which takes a position within the subset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UckMSEcXcYZMgokrPeSQTW
The two existing cases now exercise `TrackStateCreatorBase` directly. `Acts::TrackStateCreator` survives in this file only as the reference of the parity test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UckMSEcXcYZMgokrPeSQTW
Reworks the CRTP track state creator after review so that nothing in the new interface couples to `MeasurementSelector`, and moves the selection policy to whoever owns it. Core keeps only the mechanism. `TrackStateCreatorBase` populates the track EDM and offers customization points; it imposes no selection and knows nothing about geometry or eta binned cuts. What a creator hands back from `calibrate` is now pinned by `Acts::MeasurementConcept` instead of a concrete value type, so implementers keep their own measurement representation and Core reads it through Eigen maps without copying. `calculatePredictedChi2` is a customization point on the base with a default that dispatches a runtime measurement dimension onto a statically sized instantiation, so Eigen never allocates. `MeasurementSelector` and `MeasurementSelectorCuts` are deprecated. They keep their own private chi2, so their translation units differ from before only by the deprecation guards. The classic `TrackStateCreator` is removed; `CombinatorialKalmanFilterExtensions::createTrackStates` stays for downstream implementations of that signature, and connecting both it and `trackStateCreator` is now an error. The examples wire their cuts directly into their creator via a flat `TrackStateSelectionCuts` (chi2 measurement, chi2 outlier, max branches per surface). The geometry hierarchy and eta binning it replaces were never configured by anything in the repository. ODD ttbar pu200 `tracksummary_ckf.root` is identical to the previous behaviour across all 63 branches, including every measurement and outlier chi2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UckMSEcXcYZMgokrPeSQTW
A review pass over the new creator. No behaviour change: ODD ttbar pu200 `tracksummary_ckf.root` is still bit-identical to the baseline across all 63 branches, including the nested per-state chi2. - the CKF connected-creator guard was two ifs saying the same thing, now one `connected() == connected()` check - `createTrackState` guarded the predicted covariance while the chi2 path asserted on it and `addNonSourcelinkState` right next to it dereferences it unconditionally; go through the state accessor everywhere so the requirement is stated in one place - the examples base carried its own copy of `derived()`; inherit the base one instead, pulled in for unqualified lookup since the base is dependent - a stale comment claimed the seed restriction keys off seed deduplication when it is `stayOnSeed`, and the class doc still referred to the removed classic creator - drop includes, aliases and a `using namespace` that nothing uses, and trim the doc comments to what the surrounding code does not already say Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UckMSEcXcYZMgokrPeSQTW
github-actions
Bot
added
Component - Core
Contributor
Public API surface diff+29 added, 0 breaking. ➕ Added public APINew types / aliases / enums / variables / concepts (6)
New call signatures (incl. defaulted-arg overloads) (10)
New public data members (13)
|
It has been public since acts-project#3825 in January 2025 as the extension point outside the CKF, so downstream may well be using it. The repository deprecates first and removes in a separate `chore!` later; the removal here was a side effect of dropping the parity test that used it, not a decision that downstream had had its notice period. Restored verbatim from main with a `[[deprecated]]` attribute. Nothing in the tree includes it any more, so it is only compiled by the header self containment check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UckMSEcXcYZMgokrPeSQTW
andiwand
changed the title
- `predictedChi2` took its `Eigen::Ref<const ...>` parameters by value, which clang-tidy flags as a copy per invocation. Take them by const reference, matching `Acts::toString` in `StringHelpers.hpp` - doxygen cannot resolve an unqualified `\ref` to a concept, so the three references to `MeasurementConcept` and `StaticMeasurementConcept` failed the `FAIL_ON_WARNINGS` docs build. Qualify them with `Acts::` Every other failing job on this PR was cancelled by a runner shutdown, not by anything in the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WDnVtbVHMC9FJ9y5AgrrQg
|
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The CKF materialises a temporary track state for every measurement on a
surface, calibrates it and computes chi2 from it, only to throw almost all of
them away.
Acts::TrackStateCreatorBaseis a CRTP base which runs theselection on the measurements directly, so only the selected ones ever reach
the track EDM. What counts as a calibrated measurement is pinned by the new
Acts::MeasurementConcept, which the existing measurement proxies alreadysatisfy without a copy.
Extensions gain a
trackStateCreatordelegate without the scratch buffer fortemporary track states.
Acts::TrackStateCreatorandActs::MeasurementSelectorare deprecated, and the examples configure a flatTrackStateSelectionCutsinstead ofMeasurementSelector::Config.No behaviour change: ODD ttbar pu200
tracksummary_ckf.rootis identical.--- END COMMIT MESSAGE ---
Step 1 of the resurrection of #5264: exact behavioural parity, no new
features. Hole creation in the creator, brem branching onto previous surfaces
and the branch stopper rework are follow-ups.
Draft because the performance measurement is stale: an earlier revision gave
0.75 s → 0.56 s median CKF time per event (ODD ttbar pu200, n=30), but the
structure has changed since. A fresh number follows before this leaves draft.
The base owns the track EDM mechanics. A derived class provides
measurementRange(which measurements are on this surface),calibrate(thecalibrated form of one) and
selectMeasurements(which of them to keep). Thechi2 is a customization point too, with a default that reads the measurement
in place, so nothing is copied and Eigen never allocates.
Deprecations
Nothing is removed from Core, so this should not break a downstream build.
Acts::TrackStateCreatorruns its selection on temporary track states, soevery measurement on a surface has to pass through the track EDM first.
Derive from
Acts::TrackStateCreatorBaseActs::MeasurementSelectorandMeasurementSelectorCuts. Nothing ACTSships uses their geometry and eta binned cut lookup any more
extensions.createTrackStates, documented only. Exactly one of it andtrackStateCreatorhas to be connectedNothing in the tree calls any of them any more, so they are compiled but no
longer exercised by the tests. The
MeasurementSelectorPython bindings arekept so downstream configuration keeps working. A follow-up
chore!canremove all three together.
Examples
TrackFindingAlgorithm.Config.measurementSelectorCfgbecomestrackStateSelection, a flatTrackStateSelectionCuts. The old per-geometry,eta binned config has no equivalent, so scripts setting it need updating.
Verification
tracksummary_ckf.rootbit-identical tomainacross all 63 branches, including the nested
measurementChi2(126432values) and
outlierChi2(721 values)TrackStateCreatorTestscovering the selection edge cases and thetrack state contents;
CombinatorialKalmanFilterTestsnow runs through thenew creator