Intergration of TOoLLiP_v3. - #51057
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51057/49508 |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51057/49561 |
|
Pull request #51057 was updated. |
|
please test |
|
A new Pull Request was created by @Brainz22 for master. It involves the following packages:
@BenjaminRS, @quinnanm can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
-1 Failed Tests: RelVals Failed RelVals |
|
I've seen the error happen when the model binary cannot be found. Probably because PR#10591 has not been yet merged in this case? |
|
please test with cms-sw/cmsdist#10591 |
|
Just checking, is this PR intended for Phase 2 (20_1_X) or Run 3 legacy (17_0_X) or both? |
|
This is a Phase 2 upgrade @makortel, but does it hurt to have a working version on both 20_1_x and 17_0_X? |
That would be up to @cms-sw/l1-l2. The 17_0_X will need a separate backport PR as usual (and you can use the same topic branch as in this PR, as long as it is not rebased to anything newer than 17_0_0_pre2). |
|
Hi Matti and Russell - we will just have this Phase 2 PR going to 20_1_X. |
|
please test with cms-sw/cmsdist#10591 |
|
+1 Size: This PR adds an extra 16KB to repository The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see more details here: Comparison SummarySummary:
|
|
Milestone for this pull request has been moved to CMSSW_20_1_X. Please open a backport if it should also go in to CMSSW_20_0_X. |
|
REMINDER @sextonkennedy, @ftenchini, @mandrenguyen: This PR was tested with cms-sw/cmsdist#10591, please check if they should be merged together |
| @@ -31,7 +31,7 @@ class JetId { | |||
|
|
|||
| void setNNVectorVar(); | |||
| float EvaluateNN(); | |||
| ap_fixed<16, 6> EvaluateNNFixed(); | |||
| ap_fixed<14, 8, AP_TRN, AP_SAT, 0> EvaluateNNFixed(); | |||
There was a problem hiding this comment.
Should
also be changed to
<14, 8, AP_TRN, AP_SAT, 0> or it is ok to keep that as <16, 6>?
There was a problem hiding this comment.
Thanks for the catch. I checked things and I found no difference in going from <16, 6> to <14, 8, AP_TRN, AP_SAT, 0>. However, I updated the precision accordingly to keep things consistent.
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51057/50874 |
|
Pull request #51057 was updated. @BenjaminRS, @cmsbuild, @quinnanm can you please check and sign again. |
PR description:
This PR integrates TOoLLiP_v3 into CMSSW, which involves the newest development of the Level-1 long-lived-particle jet trigger. The official TOoLLiP development repository is found here.
TOoLLiP/TOoLLiP_v3and linked here, e.g. ROC curves, HLS firmwareTOoLLiP_v3folder, Python to HLS conversion scripts, and layer tracing, and model plots.The PR to pass in the correct TOoLLiP version on
cms-sw/cmsdistcan be found herePR validation:
perfNano.rootwill appeat at$PWD. This ntuple contains the branchscPuppiL1TTOOLLIPJet_llpTagScoreas defined by the lines here.TOoLLiP_v3emulation checks have been performed on signal and minbias samples, where inputs are taken from cmssw directly and used to testqkerasandHLSmodels. Their predictions are compared against the emulation predictions. Plot are shown below (original_scoreaxis has distribution ofqkerasmodel predictions):The
qkerasmodel outputs differ fromhls(and thus,cmssw) outputs due to arithmetic precision and quantization mismatches in the shift fromqkerastohls. This conclusion can be inferred from the spread in thehlsandqkerasmodel final layer outputs:No backports needed. The workflow works fine for CMSSW_17_0_0_pre1.