-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 option to disable resetting L1 PS counters on ls change #37395
Conversation
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37395/29050
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37395/29052
|
A new Pull Request was created by @Sam-Harper (Sam Harper) 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 |
@cmsbuild please test |
I'm not sure, maybe because they wanted to have the same results when running a single lumi section wrt multiple lumisection. This might be important especially for debugging. Anyway, you are adding a new option so this PR is harmless (and thanks a lot for the debugging the problem of the L1 skim). |
+l1 |
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. 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) |
please abort |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dfaf4a/23645/summary.html Comparison SummarySummary:
|
I take it that these questions from the PR description won't be answered here by L1T. The integration of the PR can proceed regardless, as it simply adds a new option without changing defaults. I would suggest to integrate it soon, as I think it's best to backport it in time for |
+1 |
Hi @Sam-Harper , |
Hi Manfred, Thanks. I dont think this interfers with this software fix, in fact this software fix is a necessary change offline and it should be updated to now enable it by default. One potential issue though and something for the L1 team to consider is how to handle the versioning. The behaviour of the L1 hardware will change mid run and thus do we want a mechanism to properly emulate this automatically either via some sort of GT payload or checking the firmware version/date. On one hand, I dont like it'll be difficult to properly exactly emulate what the L1 is doing (without an automatic mechanism, you'll have to do it manually based on the runs you are using). On the other hand, prescale emulation is 1) rarely used and 2) imperfect in the sense that you need to run over all events L1 sees for a given seed to emulate it perfectly. And offline we have to change it anyways due to this. So I cant see a scenario where we need to exactly emulate the prescales as done offline, given to do this we need to take all data the L1 sees. So I would go with setting this option as default and adding a comment to that this changed on date X/X/X for the L1 firmware. However this is a trigger coordination call @silviodonato |
Dear @Sam-Harper @silviodonato @missirol @sanuvarghese all,
Thanks a lot in advance, |
Regarding 1., in my understanding all that's needed is in #40158 (no backports, no changes online). |
The new L1 firmware with the above mentioned update have been deployed online during the collision run 362673 (L1 + HLT key : collisions2022/v350) on 25/11/2022. See (CMSLITDPG-980) for all details. |
PR description:
This PR adds an option to disable resetting the prescale counters of the L1 trigger on lumi section change. This is needed for offline L1 reemulations where we only have ~23000 events per lumisection available (often much much less due to multithreading/job splitting) and this reset comes too soon and causes triggers to have a much higher effective prescale or not fire at all. This is based on the excellent debugging and investigations by @sanuvarghese
Also could experts just confirm why the L1 resets counters every lumisection? I'm just curious to why.
Finally as spotted by both Sanu and myself, we note that this code can not work with fractional prescales. What is the plan here?
PR validation:
Testing the L1 emulation workflow, prescale counts are now as expected.
if this PR is a backport please specify the original PR and why you need to backport that PR:
We'll need this in 12_3 as well, once we work out a plan for fixes.