refactor: Managing Seed Candidates with ad-hoc container #1764

CarloVarni · 2022-12-20T14:07:35Z

Rewriting some part of the seeding codes, in particular how we collect the triplets candidates given a specific middle space point.

As of now these candidates are stored (and passed throughout the code) as std::pair<float, std::unique_ptr<InternalSeed<external_spacepoint_t>>>. Replacing the lower quality candidate with a new higher-quality candidate is not trivial if this has to happen multiple times during execution.

This is now handled by a specific class (CandidatesForSpM<InternalSpacePoint<external_spacepoint_t>>), which stores triplets candidates in a vector sorted as min heap tree. The first elements of this vector is always guaranteed to be the one with the lowest weight (i.e. quality) even after removals and/or additions of other candidates.

Inserting and removing elements from the stored collection is a log(N) operation.

Also taken a look around and made a few optimization changes:

Do not create Internal Seeds anymore (do we actually need them at all in Acts??? And why they have their own class if they are nothing more then a Seed<InternalSpacePoint<external_spacepoint_t>>?)
Avoid redundant or unnecessary sorting of vectors
Modularization of SeedFinder methods to make code cleaner and more readable by humans

Both default and orthogonal seedings have been adapted.
No physics performance changes are expected from this.

Still have to investigate on timings w.r.t. default code

/cc @noemina @LuisFelipeCoelho @paulgessinger @tboldagh @gagnonlg

github-actions · 2022-12-20T14:51:14Z

📊 Physics performance monitoring for `90764f0`

Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu

IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

codecov · 2022-12-20T15:20:57Z

Codecov Report

Merging #1764 (90764f0) into main (75d5271) will decrease coverage by 0.17%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1764      +/-   ##
==========================================
- Coverage   49.77%   49.61%   -0.17%     
==========================================
  Files         406      407       +1     
  Lines       22521    22596      +75     
  Branches    10286    10301      +15     
==========================================
  Hits        11210    11210              
- Misses       4132     4207      +75     
  Partials     7179     7179

Impacted Files	Coverage Δ
...ore/include/Acts/Seeding/CandidatesForMiddleSp.hpp	`0.00% <0.00%> (ø)`
...ore/include/Acts/Seeding/CandidatesForMiddleSp.ipp	`0.00% <0.00%> (ø)`
Core/include/Acts/Seeding/IExperimentCuts.hpp	`0.00% <ø> (ø)`
Core/include/Acts/Seeding/SeedFilter.hpp	`0.00% <ø> (ø)`
Core/include/Acts/Seeding/SeedFilter.ipp	`0.00% <0.00%> (ø)`
Core/include/Acts/Seeding/SeedFinder.hpp	`0.00% <ø> (ø)`
Core/include/Acts/Seeding/SeedFinder.ipp	`0.00% <0.00%> (ø)`
Core/include/Acts/Seeding/SeedFinderUtils.ipp	`0.00% <0.00%> (ø)`
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

tboldagh

Just a few comments concerning the readability of the code (naming). I tis clearly inherited but maybe it could be improved.
One technical suggestion about use of quality ordered set. It may be that I have misunderstood things though.

Core/include/Acts/Seeding/CandidatesForSpM.hpp

tboldagh

Had a quick look. I realy have (again) some better naming suggestions and questions. Besides that it all looks good.

Core/include/Acts/Seeding/CandidatesForMiddleSp.hpp

Core/include/Acts/Seeding/SeedFilter.hpp

Core/include/Acts/Seeding/SeedFilter.ipp

Core/include/Acts/Seeding/SeedFinderUtils.hpp

Core/include/Acts/Seeding/CandidatesForMiddleSp.hpp

SYCL and CUDA compilation working

CarloVarni · 2023-01-03T21:49:00Z

On a positive side, the effect of these changes is a slightly faster code. The plot is for the default seeding, but the same consideration applies to the orthogonal seeding.

Sample: tt-bar PU200 (50 events)
Script: full_chain_itk.py

paulgessinger · 2023-01-16T09:25:49Z

@tboldagh, @noemina can one of you review this and hit "approve" if we can merge?

noemina

Great code! I really like it. I have left a few comments to correct typos and add lines to the documentation.
The major is to better explain the logic in the bubbledw method.

Core/include/Acts/Seeding/CandidatesForMiddleSp.hpp

Core/include/Acts/Seeding/CandidatesForMiddleSp.ipp

Core/include/Acts/Seeding/SeedFilter.ipp

Core/include/Acts/Seeding/CandidatesForMiddleSp.hpp

CarloVarni · 2023-01-16T13:09:50Z

Hi @noemina I should have addressed you comments. In particular I've extended the documentation on the bubbledw method. Hopefully it is now clearer what it does

CarloVarni · 2023-01-17T09:02:12Z

thanks @noemina for the feedback. All threads are resolved. However the CI fails due to the codecov test, so I'm afraid that the automerge will not work

noemina · 2023-01-17T09:02:32Z

Great! All good from my side.

paulgessinger

Assuming @noemina meant to approve this PR.

I suspect the codecov might resolve when this branch is updated to current main.

noemina

Of course!

CarloVarni · 2023-01-17T12:07:01Z

@paulgessinger I'm afraid codecov is stubborn and doesn't want to succeed.

CarloVarni added the 🚧 WIP Work-in-progress label Dec 20, 2022

CarloVarni added Component - Core Affects the Core module Improvement Changes to an existing feature labels Dec 20, 2022

CarloVarni added the Component - Plugins Affects one or more Plugins label Dec 20, 2022

CarloVarni added this to the next milestone Dec 21, 2022

CarloVarni added the Component - Documentation Affects the documentation label Dec 21, 2022

tboldagh reviewed Dec 22, 2022

View reviewed changes

CarloVarni marked this pull request as ready for review January 2, 2023 11:27

CarloVarni removed the 🚧 WIP Work-in-progress label Jan 2, 2023

CarloVarni requested a review from tboldagh January 2, 2023 13:23

tboldagh suggested changes Jan 2, 2023

View reviewed changes

Carlo Varni and others added 17 commits January 3, 2023 20:17

Managing Seed Candidates with ad-hoc container

7a4fb29

change method name

75184a9

added protection

f11e7f1

clang format applied

317287a

typo

5faed24

using size

f368bfa

add algorithm

3945c5b

documentation

6621521

cuda and sycl cutPerMiddleSP method

f3eb1d6

more cuda and sycl

be833a9

should solve sycl

fc4bc26

documentation

04811da

SeedFinder in Sycl (#22)

78ac59f

SYCL and CUDA compilation working

clang tidy

8606048

implement first round of comments

7b166eb

some optimization and changes

6217508

Using size_t and central storage

a080f33

format as usual

04894bb

Carlo Varni added 2 commits January 10, 2023 09:23

Specify move and copy operations

3f88f52

format

0b3fffe

CarloVarni assigned noemina Jan 10, 2023

LuisFelipeCoelho self-requested a review January 16, 2023 09:35

LuisFelipeCoelho approved these changes Jan 16, 2023

View reviewed changes

LuisFelipeCoelho added the automerge label Jan 16, 2023

Merge branch 'main' into ManagingSeedCandidates

c1fcc13

noemina reviewed Jan 16, 2023

View reviewed changes

LuisFelipeCoelho removed the automerge label Jan 16, 2023

Resolve typos

f8f2e1c

Carlo Varni added 2 commits January 16, 2023 14:45

Core

37076a1

format

b8e88e6

Merge branch 'main' into ManagingSeedCandidates

97bf199

paulgessinger approved these changes Jan 17, 2023

View reviewed changes

paulgessinger added the automerge label Jan 17, 2023

noemina approved these changes Jan 17, 2023

View reviewed changes

Merge branch 'main' into ManagingSeedCandidates

5e61e5d

Merge branch 'main' into ManagingSeedCandidates

90764f0

kodiakhq bot merged commit 200aa2e into acts-project:main Jan 17, 2023

github-actions bot removed the automerge label Jan 17, 2023

paulgessinger modified the milestones: next, v23.0.0 Jan 18, 2023

LuisFelipeCoelho mentioned this pull request Feb 9, 2023

fix: Pass By Reference function and Extend object to binned group #1850

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Managing Seed Candidates with ad-hoc container #1764

refactor: Managing Seed Candidates with ad-hoc container #1764

CarloVarni commented Dec 20, 2022 •

edited

github-actions bot commented Dec 20, 2022 •

edited

codecov bot commented Dec 20, 2022 •

edited

tboldagh left a comment

tboldagh left a comment

CarloVarni commented Jan 3, 2023

paulgessinger commented Jan 16, 2023

noemina left a comment

CarloVarni commented Jan 16, 2023

CarloVarni commented Jan 17, 2023

noemina commented Jan 17, 2023

paulgessinger left a comment

noemina left a comment

CarloVarni commented Jan 17, 2023

refactor: Managing Seed Candidates with ad-hoc container #1764

refactor: Managing Seed Candidates with ad-hoc container #1764

Conversation

CarloVarni commented Dec 20, 2022 • edited

github-actions bot commented Dec 20, 2022 • edited

📊 Physics performance monitoring for 90764f0

Vertexing

Seeding

CKF

Ambiguity resolution

Truth tracking (Kalman Filter)

Truth tracking (GSF)

codecov bot commented Dec 20, 2022 • edited

Codecov Report

tboldagh left a comment

Choose a reason for hiding this comment

tboldagh left a comment

Choose a reason for hiding this comment

CarloVarni commented Jan 3, 2023

paulgessinger commented Jan 16, 2023

noemina left a comment

Choose a reason for hiding this comment

CarloVarni commented Jan 16, 2023

CarloVarni commented Jan 17, 2023

noemina commented Jan 17, 2023

paulgessinger left a comment

Choose a reason for hiding this comment

noemina left a comment

Choose a reason for hiding this comment

CarloVarni commented Jan 17, 2023

CarloVarni commented Dec 20, 2022 •

edited

github-actions bot commented Dec 20, 2022 •

edited

📊 Physics performance monitoring for `90764f0`

codecov bot commented Dec 20, 2022 •

edited