-
Notifications
You must be signed in to change notification settings - Fork 27
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
Adding EMTF++ Emulator for Phase-2 #1184
Adding EMTF++ Emulator for Phase-2 #1184
Conversation
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. Attempts to compile this PR succeeded!
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found no issues with the code checks!
I found issues with the headers. These issues likely arise from improper build files. The following is the stderr of the header checking:
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found 94 files that did not meet formatting requirements:
Please run
|
Hi @omiguelc , there are a few formatting issues to be fixed. In any case, is there any chance that this PR can be split in several smaller ones? |
Howdy, @epalencia, The smallest I can make the pull request is by breaking it into 3 parts:
I don't believe there's a way to break the last PR into small bits. It's a whole new package. Is this good with you? |
@omiguelc, I think that even if in terms on size there is no a huge gain, it would still help to split this in 3 PRs |
Sounds good, will split up in 3 then. |
I've made the first PR here: #1186 I have a few questions:
Cheers, |
I thought that the 3 potential PRs were independent (modifying different set of files). If this is not the case, then it is probably better to leave as it is with just 1 PR. |
They are independent in the sense that they belong to different packages. Only the last PR would require the first two PR. |
If the big PR requires the first two, then I think that, to gain time, we should go with just this one PR. @aloeliger, is this ok with you? |
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. Attempts to compile this PR succeeded!
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found no issues with the code checks!
I found no issues with the headers!
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found 93 files that did not meet formatting requirements:
Please run
|
@omiguelc, could you please fix these format issues? |
@epalencia, I've formatted the code. |
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. Attempts to compile this PR succeeded!
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found no issues with the code checks!
I found no issues with the headers!
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found 1 files that did not meet formatting requirements:
Please run
|
This file isn't something I modified but if needed I can add-pkg it and format it. |
@omiguelc & @watson-ij Thanks for the changes, there are still two questions though:
|
Howdy Andrew, Thanks for the comments, however I believe @watson-ij already took care of this in his latest changes. See: 0ec883b Let us know if there's anything else that need to be done. Thanks, |
Okay, then if that's done, I'd like to this along as quickly as possible. I will redo the quick checks from our end and ping Enrique and then I think that we don't need to delay opening this to CMSSW |
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. Attempts to compile this PR succeeded!
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found no issues with the code checks!
I found no issues with the headers!
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found 2 files that did not meet formatting requirements:
Please run
|
@watson-ij & @omiguelc Could you please fix the formatting issues introduced into |
Howdy @aloeliger, Done. By open this PR up to central CMSSW, do you mean to open a new PR in CMSSW_13_3_0_pre3? Cheers, |
@omiguelc Please open the PR to the main branch. If for any reason it needs to be backported, that will happen concurrently or after the PR to main. |
Another question @aloeliger. I have 2 data files under L1Trigger/L1TMuonEndCapPhase2, these are my NN protobuf files. What repository should I commit these too for phase2? |
Do not commit it to CMSSW or here, or it will have to be removed from the history. They need to be committed to the data externals, under the repository |
Understood. I didn't commit here, but I knew at some point it would have to be added somewhere. Thanks for the clarification. I shall proceed with the PR. |
I've created the new PR in the main branch of CMSSW: cms-sw#43766 |
@omiguelc We'll keep it open until it's merged in CMSSW (please port any changes back here if the PR to CMSSW is a different branch), or until someone tells us they need it ASAP. The menu team is starting to talk about examining this stuff, so that may happen. |
Thanks for all the help, @aloeliger. I'm using the same branch I employed for this PR in the other one. If I understand correctly, the changes I make moving forward should show up in both PRs. |
@omiguelc you'll also need to change the gemrechitsproducer to use the sim digis, Im travelling at the moment but can make a commit on Monday |
@omiguelc In order to start constructing a protoytype branch for the menu team to validate, this PR is being merged immediately. |
6030604
into
cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3
PR description:
This PR is enormous. It was inevitable. It contains the latest version of the EMTF++ emulator. The emulator is used to study the Phase-2 upgrade of EMTF. Other projects are waiting to integrate the emulator into their code.
Initially, the emulator was designed for CMSSW 12.5.0pre3 and updated to CMSSW 13.3.0pre3. In this version of CMSSW, the GEM group renamed the ME0 chambers to GE0. However, this was not propagated to the L1Trigger packages, resulting in a few errors. I integrated GE0 to the L1Trigger in CMSSW 13.3.0pre3, thanks to Ian Watson. I had to use a version of his fix intended for CMSSW 12.5.0pre3 for it to work. I checked the latest releases of CMSSW, and these fixes aren't there yet. Here's a link to Ian's fork containing the fix: https://github.com/watson-ij/cmssw/tree/2023-07-ge0-for-l1t-1253p3/L1Trigger
Besides this, there weren't any other major issues when moving to CMSSW 13.3.0pre3.
Cheers,
Osvaldo