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

Add Phase-2 Level-1 LLP trigger emulator: TOoLLiP [13_3_0_pre3] #1195

Conversation

ddiaz006
Copy link

PR description:

PR validation:

In progress

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -j 8

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 code-checks && scram b -k -j 8 code-checks

I found no issues with the headers!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 check-headers

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 1 files that did not meet formatting requirements:

  • L1Trigger/L1TTrackMatch/plugins/L1TrackJetClustering.h

Please run scram b code-format to auto-apply code formatting

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 code-format-all

@EmyrClement
Copy link

@ddiaz006 thanks for this PR! For my understanding, you are reusing/generalizing the jet ID class that was originally written for the b tag ID so that it can calculate an ID from any NN (assuming the same/similar input features) by either running tensorflow directly or the "HLS4ML emulation"? I think this makes sense, though I would like to see a quick validation that the b tag scores are the same before/after this PR to make sure they are unaffected.

@aloeliger aloeliger added Phase-2 Pertains to phase-2 development Emulator Development Emulator development PR Physics Affecting A PR expected to affect Physics content of the trigger labels Jan 17, 2024
@aloeliger
Copy link

@ddiaz006 Just a gentle ping on the review comments.

@aloeliger aloeliger changed the base branch from phase2-l1t-integration-13_3_0_pre3 to phase2-l1t-integration-14_0_0_pre3 February 5, 2024 15:43
@aloeliger aloeliger changed the base branch from phase2-l1t-integration-14_0_0_pre3 to phase2-l1t-integration-13_3_0_pre3 February 13, 2024 12:49
@aloeliger aloeliger added the Needs Rebase PR should be rebased to newer branch label Feb 13, 2024
@aloeliger
Copy link

Hi @ddiaz006, I'm going around starting to figure out how to update all PRs to our new IB

For this PR I would recommend:

  1. First, make a backup of this branch and it's commits pushed to github under a different name
  2. checkout CMSSW_14_0_0_pre3 via cmsrel CMSSW_14_0_0_pre3 && cd CMSSW_14_0_0_pre3/src/ && cmsenv && git cms-init
  3. Get a copy of this branch locally via git cms-checkout-topic -u ddiaz006:TOoLLiP_1330pre3_2
  4. Perform an interactive rebase via git rebase from-CMSSW_14_0_0_pre3 --interactive
  5. In the interactive rebase, please drop all commits that were not originally part of this pull request. In my tests, there seemed to be some earlier commits from track trigger commits that get caught up in this somehow?
  6. You can either then push the resulting branch to the current PR branch, or make a new one and make a PR from that branch to cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3

Alternatively, because you have relatively few commits, you could just cherry-pick commits by doing something like:

  1. First, make a backup of this branch and it's commits pushed to github under a different name
  2. checkout CMSSW_14_0_0_pre3 via cmsrel CMSSW_14_0_0_pre3 && cd CMSSW_14_0_0_pre3/src/ && cmsenv && git cms-init
  3. Fetch your changes git fetch my-cmssw
  4. Cherry pick the 4 necessary commits via git cherry-pick, the 4 commands needed (in order) I think are: git cherry-pick 9eaa43f096c1d45995bc647ef04e8017de403b9d git cherry-pick 1e4a6d6040201bc79be960fc81d2816b2d04987a git cherry-pick 666efd5f97c5b85e2e5340d26f3e7a0a1374613f and git cherry-pick fc0fa3d7f251aab0249caf6ddb1d503283022322
  5. You can either then push the resulting branch to the current PR branch, or make a new one and make a PR from that branch to cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3

@EmyrClement
Copy link

@ddiaz006 In addition to the rebase above, can you also add python configuration for running your tagger, as is done for the b jets here? You can then add the tagger to the L1 sequence (see example here for the b tagger) and to the event content (see here).

@EmyrClement
Copy link

I tried running the tagger, but found the code crashes in this section:

ap_fixed<16, 6> modelResult = -1;
//for (int i = 0; i < 1; i++) {
// modelResult[i] = -1;
//}
modelRef_->prepare_input(modelInput);
modelRef_->predict();
modelRef_->read_result(modelResult);

The problem is modelResults is an ap_fixed<> but modelRef_->read_result(modelResult); requires modelResults to be an array of ap_fixed<> with size 1.

I guess this wasn't caught in the tests by trigger doctor (or in master) as the producer isn't being run in the L1 sequence.

@aloeliger
Copy link

@ddiaz006 Any updates on rebasing this PR?

@ddiaz006
Copy link
Author

Thank you for your instructions @aloeliger. I have rebased to CMSSW_14_0_0_pre3 and created a new PR: #1213. This one can be closed and I will update the other.

@epalencia epalencia closed this Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Emulator Development Emulator development PR Needs Rebase PR should be rebased to newer branch Phase-2 Pertains to phase-2 development Physics Affecting A PR expected to affect Physics content of the trigger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants