implements LLP displaced jet trigger in uGT emulator for CMSSW 12 3 x #36858
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36858/28099
|
|
A new Pull Request was created by @cavana (Richard Cavanaugh) for master. It involves the following packages:
@epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-56b28c/22137/summary.html CMS StaticAnalyzer warnings: There are 1 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-56b28c/22137/llvm-analysis/legacy-mod-sa.txt for details. Comparison SummarySummary:
|
|
+l1 |
cmsbuild
added
fully-signed
l1-approved
and removed
l1-pending
pending-signatures
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, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
| // note that this check is only valid when hwQual contains just the single LLP DISP bit, which is consistent | ||
| // with the uGT firmware expectations. If hwQual is later defined to contain more quality bits, then | ||
| // the below sanity check will have to be changed to include the extra quality bits. | ||
| if (cand.hwQual() > 1) { |
There was a problem hiding this comment.
Were is this hwQual for the L1Candidate set?
Wouldn't it be safer and better portable to check the explicit LLP DISP bit instead?
|
The value for hwQual is set by the software emulator for the L1 Calo
Trigger.
The data structure word for a L1 jet object, as sent from the L1 Calo
Trigger to the uGT, has reserved 4 bits (at the end of the jet word) that
are identified as "quality bits". Until now, those bits had never been
used by CMS and the uGT had never implemented those bits in firmware.
Recently, the L1 Calo Trigger now uses 1 of those 4 quality bits to tag a
L1 jet object as possibly being a long-lived-particle or displaced jet (LLP
DISP). When the uGT group implemented that LLP DISP bit into the UTM
library, it was implemented as an individual bit, separate and distinct
from the original "quality bits" (effectively reducing the number of
available quality bits from 4 now to 3).
The current situation is that (for the historical reasons mentioned above)
the emulator for the L1 Calo Trigger uses the least significant bit of
hwQual to send the LLP DISP bit to the uGT emulator, whereas the UTM
library and the uGT firmware explicitly separate out 1 of the 4 quality
bits and call it the LLP DISP bit with its own explicit definition. The
sanity check in the software emulator for the uGT simply recognises the
actual way those two different groups decided to define and use this "new"
bit for L1 jets objects, and the comment prudently warns developers to
continue to monitor future evolutions of the "quality" bits for L1 Calo
objects and how those bits relate to the bit definitions used by the UTM
and uGT.
…On Thu, Feb 3, 2022 at 5:07 AM Andrea Perrotta ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In L1Trigger/L1TGlobal/src/CaloCondition.cc
<#36858 (comment)>:
> @@ -525,6 +525,25 @@ const bool l1t::CaloCondition::checkObjectParameter(const int iCondition,
LogDebug("L1TGlobal") << "\t\t l1t::Candidate failed isolation requirement" << std::endl;
return false;
}
+
+ // sanity check LLP DISP ( bit check ) with two-state LUT = {0,1}
+ // note that this check is only valid when hwQual contains just the single LLP DISP bit, which is consistent
+ // with the uGT firmware expectations. If hwQual is later defined to contain more quality bits, then
+ // the below sanity check will have to be changed to include the extra quality bits.
+ if (cand.hwQual() > 1) {
Were is this hwQual for the L1Candidate set?
Wouldn't it be safer and better portable to check the explicit LLP DISP
bit?
—
Reply to this email directly, view it on GitHub
<#36858 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABQ3MBD7K6BV7IS6MUT23FLUZJOXXANCNFSM5NKQPZ4Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Thank you @cavana for the clear explanation |
|
Thanks @perrotta , your suggestion is a good one. I will change the code
to use LogError, instead of LogDebug.
…On Thu, Feb 3, 2022 at 12:06 PM Andrea Perrotta ***@***.***> wrote:
Thank you @cavana <https://github.com/cavana> for the clear explanation
Then, wouldn't it be more appropiate a LogError there, instead of LogDebug,
to warn future developers who may not dare enabling the LogDebug?
—
Reply to this email directly, view it on GitHub
<#36858 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABQ3MBHNHONLO3YELQRGRTTUZK733ANCNFSM5NKQPZ4Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Thank you @perrotta and my apologies for causing extra work for this PR. I just consulted with the Calo Trigger emulator people and we agreed that the "sanity" check for hwQual() should be for 3 bits, instead of just the single LSB bit (as I originally coded). I have committed that change, which also reverts back to LogDebug. I very much appreciate your extra pair of eyes; your review was important in clarifying better the interface between the Calo Trigger emulator and the Global Trigger emulator. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36858/28229
|
|
Pull request #36858 was updated. @epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please check and sign again. |
Thank you @cavana About using
You know the details of your code, then you must judge which one has to be preferred. Please notice that would you had left the LogDebug as it was at the beginning you would have never noticed that |
|
@perrotta Indeed, I currently prefer to leave it as LogDebug, as that is historically consistent with the rest of the legacy uGT emulator code. If, in the future, the uGT group decides it should be elevated to LogError, I would like to consistently do that for all other bit-wise "sanity checks" in the emulator code. In which case, it would be appropriate to have a separate PR for that. |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-56b28c/22325/summary.html CMS StaticAnalyzer warnings: There are 1 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-56b28c/22325/llvm-analysis/legacy-mod-sa.txt for details. Comparison SummarySummary:
|
|
+l1 |
cmsbuild
added
fully-signed
l1-approved
and removed
l1-pending
pending-signatures
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, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
This PR implements the long-lived particle (LLP) displaced (DISP) jet trigger in the uGT emulator. There are no changes to the output. This PR does not depend on any other PRs, but it does rely on the external UTM library 0.10, which is already included in the CMSSW_12_3_0_pre3 release and the CMSSW_12_3_X master.
PR validation:
This PR was validated in the following ways:
(1) all code changes were verified to successfully compile with the CMSSW_12_3_X master
(2) all code changes successfully passed "scram build code-format" checks
(2) test vectors were produced (with the code changes to the emulator) and compared with the uGT firmware. Perfect agreement was observed between the emulator and the firmware.
(3) the runTheMatrix workflow successfully passed all tests.
if this PR is a backport please specify the original PR and why you need to backport that PR:
This PR is not a backport.
Before submitting your pull requests, make sure you followed this checklist: