Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imath removal, removal of non-combined modules (VMR, TE, TC, PR, ME, MP), and new combined TB module #304

Open
wants to merge 23 commits into
base: L1TK-dev-14_2_0_pre4
Choose a base branch
from

Conversation

aryd
Copy link

@aryd aryd commented Jan 3, 2025

PR description:

This PR has code developed over a long time that includes the removal of the iMath code, removal of the non-combined modules and new TB code that combines TB for multiple seeding combinations. Plus many other cleanups.

PR validation:

This code is running and producing tracks. Need to check that test vectors are OK with HLS.

number of events = 100

efficiency for |eta| < 1.0 = 92.33 +- 0.76
efficiency for 1.0 < |eta| < 1.75 = 93.96 +- 0.93
efficiency for 1.75 < |eta| < 2.40 = 94.51 +- 1.26
combined efficiency for |eta| < 2.40 = 93.14 +- 0.54 = 2076.00/2229.00

efficiency for pt > 2.00 = 93.14 +- 0.54
efficiency for 2.00 < pt < 8.0 = 92.79 +- 0.63
efficiency for pt > 8.0 = 94.18 +- 1.00
efficiency for pt > 40.0 = 85.71 +- 5.91

TP/event (pt > 2.00) = 156.71
TP/event (pt > 3.0) = 52.16
TP/event (pt > 10.0) = 4.63
tracks/event (no pt cut)= 173.70
tracks/event (pt > 2.00) = 162.58
tracks/event (pt > 3.0) = 57.50
tracks/event (pt > 10.0) = 5.93

Percentage fake tracks (pt > 2.00) = 2.20%
Percentage duplicate tracks (pt > 2.00)= 1.63%

z0 resolution = 0.10cm at |eta| = 0.05
z0 resolution = 0.41cm at |eta| = 1.95

@aryd aryd changed the title Multiple m ps rebase2 squashed imath removal, removal of non-combined modules (VMR, TE, TC, PR, ME, MP), and new combined TB module Jan 3, 2025
@aryd aryd marked this pull request as ready for review January 3, 2025 22:01
* Removed superfluous continue.

* Switch to Stub::rvalue().

* Switch to Stub::rvalue().
@tschuh
Copy link

tschuh commented Jan 29, 2025

I had a look at the code generating the track and stub streams and it looks like you are messing up the stub channel assignment per seed type. That may lead to stub losses (seed type 3 projects only in 4 layers, stubs send on channel >6 won't be used) and wrong interpretation of stub data (e.g. ps barrel stub data interpreted as ps endcap stub). I would recommend to make the ChannelAssigment class available in TrackFit and to use it to make the correct assignments.

aehart and others added 10 commits February 12, 2025 15:57
* Added back triplet seeds to the seed map for now.

* Added separate FM memories for triplet seeds for now.

* Consider TPROJ memories unpaged.

* Added warnings when no projection memory exists.

* Replaced magic numbers with seed enum.

* Added an N_SEED_DISPLACED constant.

* Added wiring based on latest baseline wiring.

* Removed obsolete settings and methods.
@aryd
Copy link
Author

aryd commented Feb 17, 2025

After spending considerable time trying to understand the data organization in the ChannelAssignment, TrackStream, and StubStream classes I convinced myself that there was no problem with how was filling this in FitTrack.cc and Sector.cc. There were two issues; the first was easy, we had change the radial position to be stored in 11 bits using an offset. However, the KF code expects the corrected radial position that uses 12 bits. The issue that I struggled a lot with resolving was that in the L1FPGATrackProducer where the StreamsTrack object is filled it assumed that the tracks were stored in the track collection in order of the seed number. But as we now find tracks using two track builders that finds tracks from different seeds this is no longer the case. I wrote a patch that fixes this. But ultimately I think this should be fixed in some cleaner way.

The CI integration test is successful now - I hope we can merge this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants