Refactor Seeding to avoid repeated Finders instantiations #1690
Labels
Impact - Minor
Nuissance bug and/or affects only a single module
Improvement
Changes to an existing feature
Long Term
Tracking issues for longer-term projects and developments
Needs Decision
Needs decision or further information
Stale
Milestone
Comments
tboldagh
added
Improvement
kodiakhq bot
pushed a commit
that referenced
this issue
Dec 14, 2022
… every event (#1693) This PR is a technical refactor of the SeedFinder so that it does not need to be reinstantiated in every event. Instead it would be made only once, properly configured and then only its `createSeedsForGroup` called for events. BREAKING CHANGE The arguments to SeedFinder constructor have changed and the arguments needed when calling it's main method. On the way, the SeedFinderConfig object was slimmed from those quantities that can't be filled w/o knowledge about the event (i.e. B field). Python binding for those properties that are overwritten anayways: `highland` and `maxScatteringAngle2`, `pTPerHelixRadius`, `minHelixDiameter2` and `pT2perRadius` are removed. @LuisFelipeCoelho - please check me on this. Tested on standalone ODD full chain. It is marginally faster now. The OrthogonalSeedFinder should be likely refactored in the same way? For later PR. It addresses issue #1690 (not closing it)
CarloVarni
pushed a commit
to CarloVarni/acts
that referenced
this issue
Dec 22, 2022
… every event (acts-project#1693) This PR is a technical refactor of the SeedFinder so that it does not need to be reinstantiated in every event. Instead it would be made only once, properly configured and then only its `createSeedsForGroup` called for events. BREAKING CHANGE The arguments to SeedFinder constructor have changed and the arguments needed when calling it's main method. On the way, the SeedFinderConfig object was slimmed from those quantities that can't be filled w/o knowledge about the event (i.e. B field). Python binding for those properties that are overwritten anayways: `highland` and `maxScatteringAngle2`, `pTPerHelixRadius`, `minHelixDiameter2` and `pT2perRadius` are removed. @LuisFelipeCoelho - please check me on this. Tested on standalone ODD full chain. It is marginally faster now. The OrthogonalSeedFinder should be likely refactored in the same way? For later PR. It addresses issue acts-project#1690 (not closing it)
|
This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
At ACTS dev. weekly 22.11.22 we discussed and agreed that SeedFinder(s) should be refactored so that clients (example algorithm in ACTS or ATLAS Athena or ... ) do not need to reinstantiate it for every event.
In practice this means that seed finding method should get part Options object that changes from event to event in the methods that process the data rather than at construction.
Suitable Option objects were introduced in PR #1630.
The changes can happen in steps, one for each specific Seeding algorithm.
FI @LuisFelipeCoelho @stephenswat
The text was updated successfully, but these errors were encountered: