refactor!: Refactored SeedFinder to avoid the need to recreate it for every event #1693
Conversation
tboldagh
added
Improvement
Codecov Report
@@ Coverage Diff @@
## main #1693 +/- ##
==========================================
+ Coverage 49.19% 49.29% +0.09%
==========================================
Files 398 395 -3
Lines 21852 21809 -43
Branches 9917 9925 +8
==========================================
Hits 10751 10751
+ Misses 4222 4179 -43
Partials 6879 6879
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
📊 Physics performance monitoring for b556a13Full report VertexingVertexing vs. muIVF seededIVF truth_smearedIVF truth_estimatedIVF orthogonalSeedingSeeding seededSeeding truth_estimatedSeeding orthogonalCKFCKF seededCKF truth_smearedCKF truth_estimatedCKF orthogonalAmbiguity resolutionseededTruth tracking (Kalman Filter)Truth trackingTruth tracking (GSF)Truth tracking |
|
I also agree that we should remove from the python binding the parameters that are overwritten: |
…of-options-till-seed-making
…of-options-till-seed-making
Thanks. There is one thing I am unsure about. That is the protections agains units conversion are aimplemented following ACTS sprit. I.e. there are exceptions now, but in principle they can be avoided i.e. toInternalUnits could become no-op method if config is in internal units already. |
|
@tboldagh I am not sure why we have two different units. The units from the config are mostly the same as in Athena configuration, if we change it it could be a bit confusing, but I don't have a strong opinion about that. Maybe we could discuss that in the meeting |
It seems that all stuff is sorted out. Can you approve this PR. I think this is needed to move forward. |
… 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)
…ct (#1757) This PR applies similar changes to these in #1693 . As a result the SeedFinderOrthogonal does not need to be recreated for every event. Closes #1690 BREAKING CHANGE Method to crate OthogonalSeedFinder and createSeeds are modified. The former does not take SeedOptions while the later does now.
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
createSeedsForGroupcalled 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:
highlandandmaxScatteringAngle2,pTPerHelixRadius,minHelixDiameter2andpT2perRadiusare 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)