Phase-2 L1T: update Correlator layer 1 barrel "TDR" regionizer emulator #41838
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41838/35737
|
|
A new Pull Request was created by @gpetruc (Giovanni Petrucciani) for master. It involves the following packages:
@epalencia, @AdrianoDee, @srimanob, @cmsbuild, @aloeliger can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b256da/32923/summary.html Comparison SummarySummary:
|
| inline unsigned int l1ct::tdr_regionizer::Buffer<l1ct::TkObjEmu>::nextObjClk(unsigned int ndup) { | ||
| assert(ndup == 1); |
There was a problem hiding this comment.
I'm a little confused by this. You take ndup as a parameter... but then assert it has to be 1 or crash? Why is this done this way?
There was a problem hiding this comment.
Basically it's that ndup != 1 is not implemented yet. In particular, it hasn't been implemented in the corresponding firmware, and there will likely never be a need for it. This function is the template specialization for TkObjEmu, which is a bit special since two objects are sent in three transfers, unlike the others which are one object per transfer. The call signature is the same as for the general case, which uses ndup.
There was a problem hiding this comment.
Would it be a good idea to throw a more detailed exception that makes these assumptions/explanations clear?
There was a problem hiding this comment.
Cherry-picked here the commit that throws the exception.
For the reviewers: it's a std::exception and not a cms::Exception since this emulator code is also compiled standalone outside CMSSW.
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-approved
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41838/35771
|
|
Pull request #41838 was updated. @epalencia, @AdrianoDee, @srimanob, @cmsbuild, @aloeliger can you please check and sign again. |
|
@cmsbuild, please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b256da/32974/summary.html Comparison SummarySummary:
|
|
+l1 |
|
Hello @AdrianoDee, can you have a look at this PR? Thanks, Giovanni |
|
+upgrade |
cmsbuild
added
fully-signed
upgrade-approved
and removed
pending-signatures
upgrade-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
This PR updates the emulator of the "TDR" regionizer for the Barrel region of the Correlator Layer 1.
It is the forward to the CMSSW master of cms-l1t-offline#1134, which contains two PRs from @jmitrevs:
This emulator is not yet enabled in the default configuration of the correlator trigger, so no physics changes are expected from this PR, but is run in integration tests and comparisons with the firmware so we would like to have it integrated in CMSSW (and then we may perform the switch of the default configuration to enable this emulator in a later PR)
As a side comment, a separate PR which will add the fillDescriptions to all the Correlator Layer 1 emulators will arrive in the next weeks: it's some amount of work and for the sake of validation I'd rather keep it factorized from changes to the emulator algorithms themselves.
PR validation:
This PR was tested on
CMSSW_13_2_X_2023-05-31-1100(el8)runTheMatrix.py -l 23234.0(runs the standard L1T config, so it does not run this emulator)L1Trigger/Phase2L1ParticleFlow/test/make_l1ct_binaryFiles_cfg.py(used for correlator trigger integration, runs this emulator among other custom firmware-friendly configurations)The original code in 12_5_2_patch1 was validated against the firmware by @jmitrevs.