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

Update tdr barrel regionizer emulator #116

Closed

Conversation

jmitrevs
Copy link

@jmitrevs jmitrevs commented May 5, 2023

PR description:

This updates the tdr barrel emulator. It generally matches results with the firmware in:

https://gitlab.cern.ch/cms-cactus/phase2/firmware/correlator-common/-/merge_requests/134

The one issue that remains is that the processing delays are not 100% accurately modeled, so data coming from different fibers can appear in different order in the output between the emulator and the simulation. (The TDR emulator was never 100% accurate on this.) This includes an attempt to model this better, but it's not quite there.

PR validation:

This is mainly tested manually comparing emulator and simulator using scripts in in correlator-layer1. I will comment on the related PR once I make it.

@jmitrevs jmitrevs changed the title Update tdr barrel regionizer Update tdr barrel regionizer emulator May 5, 2023
@jmitrevs jmitrevs marked this pull request as draft May 8, 2023 11:38
@gpetruc
Copy link
Collaborator

gpetruc commented May 11, 2023

At a quick look it seems ok to me.
Since this will have to be ported also to cms-l1t-offline and the master branch of central cmssw , I would suggest you to squash all into a single commit - that would make the rebase to cmssw master easier.

@jmitrevs
Copy link
Author

For squashing into a single commit, you mean that when this is merged, you will use the "Squash and Merge" option, or is there something I need to do before?

@gpetruc
Copy link
Collaborator

gpetruc commented May 12, 2023

I think it's best if you squash the commits locally and force-push the new commit here.
That way, you can then also open a PR to cms-l1t-offline with the same branch, and a PR to cmssw master with a rebase of that commit on the latest CMSSW 13_2_X IB

@jmitrevs
Copy link
Author

I may have a solution to the order problem I am still seeing, so I may want to include a few more commits before squashing. It should be within the next few days.

@jmitrevs jmitrevs force-pushed the tdr_regionizer_timing_rewrite branch from 8f79baf to 29af0a6 Compare May 31, 2023 19:41
@jmitrevs jmitrevs force-pushed the tdr_regionizer_timing_rewrite branch from 29af0a6 to 71bc3b9 Compare May 31, 2023 20:06
@jmitrevs jmitrevs marked this pull request as ready for review May 31, 2023 20:24
@jmitrevs
Copy link
Author

I fixed the order issue and squashed this commit.

@jmitrevs
Copy link
Author

jmitrevs commented Jun 2, 2023

I pushed another commit here as requested in the review in cms-sw#41838. If you agree with the response to that request, we should cherry-pick the answer. If not, let me know what is the best way to fix it.

@gpetruc
Copy link
Collaborator

gpetruc commented Jun 3, 2023 via email

@gpetruc
Copy link
Collaborator

gpetruc commented Jun 13, 2023

Ok, the PRs to upstream are not yet merged but they are fully signed so I can merge this.
To stay better in sync with cms-l1t-offline in fact I'll merge the branch used to make cms-l1t-offline#1134 which has the cherry-pick of the last commit instead of the original one

@gpetruc
Copy link
Collaborator

gpetruc commented Jun 13, 2023

effectively merged.

@gpetruc gpetruc closed this Jun 13, 2023
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.

2 participants