Conversation
Adds a cut on the time compatibility of the two space points forming a doublet. Both times are corrected for the time of flight from the origin so they are directly comparable, and the doublet is rejected when they differ by more than `timeCutNSigma` times their combined resolution. The cut is added as a fifth axis to the existing `boost::mp11` template dispatch in `create()`, so that no time related code is generated when `useTime` is disabled. This keeps seeding without time information unaffected, and avoids reading the `Time` and `VarianceT` columns from containers that do not provide them at all. The two new configuration fields are deliberately placed in existing padding of `Config`: `useTime` after `interactionPointCut` (3 byte hole) and `timeCutNSigma` after `helixCutTolerance` (4 byte hole). `Config` therefore stays 72 bytes and no existing member changes offset. Keeping them together at the end instead grows the struct to 80 bytes, which shifts `experimentCuts` and the `DerivedConfig` members and changes the generated code of the time-less seeding, even though it stays semantically identical. Verified by compiling this translation unit and its counterpart from main, both with -ffunction-sections, and comparing every instantiation with `useTime=false` against its counterpart from main: all 97 are identical in machine code and relocations (33397 bytes), including all 32 instantiations of the `createDoubletsImpl` hot loop. Seeding without time information therefore executes exactly the same instructions as before. The added instantiations cost compile time and binary size of this translation unit only. The cut is placed after the deltaR, deltaZ and collision region checks so that the square roots are not evaluated for candidates that are rejected by the cheaper cuts anyway. Co-authored-by: Yanqi Wang <184178084+wangyanqi191025@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a cut on the time compatibility of the three space points of a triplet. The times are corrected for the time of flight from the origin, and the chi2 of the three corrected times with respect to their mean is compared to `timeChi2Max`. The cut lives in the triplet seed finder rather than in the seed filter, so that incompatible triplets are rejected while they are formed instead of after they have been collected and sorted into seed candidates. As for the doublet finder, the cut is a new axis of the `boost::mp11` template dispatch in `create()`, so no time related code is generated when `useTime` is disabled. It is applied in both the pixel and the strip path, in each case after the cheap cotTheta compatibility check and before the curvature computation and the strip coordinate transformation. The pixel path needs the space point container to look up the bottom and top space points, so `createPixelTripletTopCandidates` now takes it, like the strip path already did. The function is inlined into its caller, so this costs nothing. `useTime` is placed in the padding after `sortedByCotTheta` and does not change the layout of `Config`. `timeChi2Max` does: `Config` has no four-byte hole and no tail padding, so it grows from 32 to 36 bytes and the `DerivedConfig` members shift by four. Comparing every instantiation with `useTime=false` against its counterpart from main shows no semantic change: instruction counts and control flow are identical, and the only differences are the displacement immediates of the shifted members, plus the size operand of the sized deallocation in the destructor. Unlike `DoubletSeedFinder`, bit identical output is not achievable here without leaving the new option out of `Config`. Co-authored-by: Yanqi Wang <184178084+wangyanqi191025@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Wires the new `useTime` options of `DoubletSeedFinder` and `TripletSeedFinder` through `GridTripletSeedingAlgorithm`, the python bindings and `addGridTripletSeeding`. `SpacePointMaker` already computed the time variance of a measurement but dropped it on the floor. It is now stored, and the `VarianceT` column is created alongside the `Time` column that was already there. The time columns of the core space point container are only created and filled when one of the two cuts is enabled, so that seeding without time does not pay for two extra float columns per space point. When a cut is enabled and the input space points do not provide time and time variance, the algorithm now fails with a clear message instead of reading columns that do not exist. Two deviations from the original proposal in acts-project#5524, both easy to revert: - one pair of options for the doublet finders rather than separate ones for the bottom and the top finder, since the two configurations only differ in their deltaR range. Separate flags can be reintroduced if asymmetric tuning turns out to be useful. - the triplet options live in `SeedFinderConfigArg` rather than in `SeedFilterConfigArg`, because the triplet time cut now sits in the triplet seed finder and no longer in the seed filter. Co-authored-by: Yanqi Wang <184178084+wangyanqi191025@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
Component - Core
pbutti
marked this pull request as ready for review
September 16, 2026 15:27
paulgessinger
left a comment
paulgessinger
left a comment
Member
There was a problem hiding this comment.
lgtm, but i'll let @andiwand weight in.
Contributor
Public API surface diff+5 added, 0 breaking. ➕ Added public APINew public data members (5)
|
Contributor
andiwand
reviewed
Sep 17, 2026
Removed detailed comments on time compatibility cut and replaced with a concise version.
Renamed 'timeCutNSigma' to 'timeCutNVariance' and updated comment.
Restore the TopDoublets return type of createPixelTripletTopCandidates and return its result from createTripletTopCandidates, fixing -Werror=return-type build failures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Correct the time differences for the straight line distance between the space points instead of the distance of each space point from the origin, which biased the cuts for vertices displaced along z. In the triplet cut, transport the bottom and top times to the middle space point and compute the chi2 with respect to the inverse variance weighted mean. Also fix the examples for the timeCutNSigma -> timeCutNVariance rename. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rename timeCutNVariance back to timeCutNSigma so the doublet time cut is configured in units of sigma everywhere. The squared value is computed once in DerivedConfig, so the cut is still applied on the combined time variance without a square root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This branch has not been deployed
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
3 participants
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.
This PR implements the comments on PR #5524 building on the commits by Yangqi.
This brings in the Core changes to the Seeding to include time information in the GridTriplet seeder.
-- Doublet finder is byte-identical when using useTime=false
-- Triplet finder: no semantic change. Indentical instructions and control flow with useTime=false
-- c=1 and using fastHypoth instead of sqrt
We miss unitTests for these finders. I can add them if you want. I did not run with useTime=true. This PR is to make sure to comply with the requests to have it available and check that useTime=false doesn't impact on ITK. Finally the seeding with time impact also the strip seeding. I can switch that off or use a usePixelTime / useStripTime to steer it. Suggestions?