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

Restructure TrackletProcessorDisplaced #297

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

cgsavard
Copy link

@cgsavard cgsavard commented Nov 6, 2024

PR description:

This PR restructures TrackletProcessorDisplaced to look like TrackletProcessor which was adapted to better follow the firmware implementation. The new structure is explained in this presentation here and requires the addition of a new TripletEngineUnit script (mimicking the TrackletEngineUnit in the TrackletProcessor case) tasked with creating triplets from different stub combinations.

In addition to this restructuring, a bug in the wiring map was fixed which was causing duplicate stub memories to be passed into the same TPD instance.

PR validation:

No changes were made which affect the performance of the tracking. I have checked the make sure that the performance (efficiency, fake rate, duplicate rate, etc) is the same before and after these changes.

Copy link

@aehart aehart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the extremely nit-picky comments above, this is good to go, as far as I'm concerned. I'll go ahead and approve it, and unless @tomalin objects, we can merge it once the comments are resolved.

@cgsavard
Copy link
Author

Comments above were addressed, we can let @tomalin merge when he has also taken a look.

@aehart aehart changed the base branch from L1TK-dev-14_0_0_pre2 to L1TK-dev-14_2_0_pre4 December 11, 2024 17:09
@aehart aehart force-pushed the restructure_TPD_PR branch from 4378c34 to 9d75886 Compare December 11, 2024 17:09
@aehart
Copy link

aehart commented Dec 11, 2024

I took the liberty of changing the target branch to the new default branch (L1TK-dev-14_2_0_pre4). I think this was the cause of the failing CI jobs.

I also rebased the PR branch onto the new default branch and added a commit re-adding the L1Trigger/TrackFindingTracklet/data/ directory. Hopefully this all allows the CI to pass again.

@tomalin tomalin self-requested a review December 11, 2024 19:18
@@ -0,0 +1,2721 @@
AllInnerStubs: AS_D1PHIA_DM [42]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is also changed by #301 . Why do we have two PRs changing the same file?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this reflects the course of development. The changes here are a minor bug fix to the wiring, while #301 includes more substantial changes.

@@ -666,7 +667,8 @@ namespace trklet {
int chisqphifactbits_{14};
int chisqzfactbits_{14};

std::array<unsigned int, N_SEED> teunits_{{5, 2, 5, 3, 3, 2, 3, 2, 0, 0, 0, 0}}; //teunits used by seed
std::array<unsigned int, N_SEED> teunits_{{5, 2, 5, 3, 3, 2, 3, 2, 0, 0, 0, 0}}; //teunits used by seed
std::array<unsigned int, N_SEED> trpunits_{{0, 0, 0, 0, 0, 0, 0, 0, 10, 10, 10, 10}}; //trpunits used by seed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a "trp"?

Copy link
Author

@cgsavard cgsavard Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indicates the number of triplet engine units, distinct from tracklet engine units

@tomalin
Copy link
Collaborator

tomalin commented Dec 13, 2024

This needs rebasing, now I've merged your other PR, since of them changed the same wiring map files, so we now have a git conflict.

@aehart aehart force-pushed the restructure_TPD_PR branch from 84293ed to 3af747e Compare December 13, 2024 23:08
@tomalin
Copy link
Collaborator

tomalin commented Dec 16, 2024

@cgsavard Have you checked that your PR to update the wiring map in central CMSSW cms-data repo still corresponds to the wiring here, giving you/Andrew have mucked around with the wiring a bit to resolve the conflicts between your two PRs?

@tomalin tomalin merged commit 8baea33 into L1TK-dev-14_2_0_pre4 Dec 16, 2024
1 check passed
tschuh pushed a commit that referenced this pull request Jan 9, 2025
Changed geometry for latest CMSSW release version

Run combined modules by default (#265)

* Make combined modules default

* tweak

* Improve USEHYBRID ifdef range

* Fix compiler error for pure Tracklet algo

* Move fitpattern.txt refs, so only used for pure Tracklet algo

* code format

Bin TQ MVA output (#272)

* add TQ MVA output binning scheme

* remove code no longer used

* move logistic sigmoid into TTTrack class

* fix for proper precision after MVA conversion in track word

* label all MVA variables before logistic sigmoid with 'Pre'

* formatting

* fix filling of track word before running TQ MVA

* switch to post-sigmoid mva bins

* remove any onnx instances

* fix MVA1 initialization

* bin TQ MVA in pre-sigmoided bins in KFout

* move pre-sigmoid bins to track quality class

* change minimum bin

squash

c++20 and merge fixes

code-format

run scripts update.

run script clean up.

rebase fix

TrackFindingProcessor for hybrid adapted.

script fix

little fixes.

furthrt little fixes

TQ outputs altered.

TM mux order configurable

New wiring for TPD (#301)

* new wiring for TPD

* fix seed 6 duplicates

* code formatting

Restructure TrackletProcessorDisplaced (#297)

* initial add of restructured TPD

* clean up new TPD code

* remove unnecessary includes

* remove magic numbers

auto code format (#303)

Eliminated MatchCalculator vs MonteCarlo ambiguity
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