ZDC trigger primitive emulation and HCAL look up table generation #42818
Conversation
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42818/36937
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
cmsbuild
added
code-checks-rejected
code-checks-pending
and removed
code-checks-pending
code-checks-rejected
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42818/36941
|
|
A new Pull Request was created by @Michael-Krohn for master. It involves the following packages:
@perrotta, @consuegs, @Dr15Jones, @bsunanda, @epalencia, @civanch, @mdhildreth, @cmsbuild, @makortel, @saumyaphor4252, @aloeliger can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@Michael-Krohn I want to make sure I understand what is going on in this PR: In an attempt to avoid having to sort the ZDC LUT out in L1 DB/O2O this is now making different/new HCAL/ZDC TPs in HCAL where this LUT is more readily accessible, and then the |
|
@aloeliger Yes, you essentially have that right. However, I think you're overlooking a bit how complicated the system would become if we left the emulation in the L1 software as it is. The ZDC LUTs are not stored in any database on the HCAL side. For HCAL, we only store the conditions that are used to construct the LUTs and then when we want to emulate data the HCAL software grabs the conditions for the exact run number from the DB to construct the LUTs on the fly and perform the ZDC TP calculations. This code that calculates the LUTs on the fly can also be run in a special mode to produce the physical LUT files that are loaded on the detector in the HCAL uHTRs. This ensures that the LUT calculations between the detector and emulation are identical. The version of the code that produced the ZDC TPs in the L1 area had no way to create the LUTs, so we would need to have the LUTs generated in the HCAL area and then passed to the L1 team for them to handle the storing of the LUTs. We would also have to ensure that the code that creates the LUTs in HCAL and the code that reads the LUTs in L1 area never gets out of sync. From the HCAL side, we believe this could cause some major headaches in the future so we would prefer to use the already existing code in the HCAL framework that syncs the LUTs/conditions online and offline. And produces the emulated ZDC TPs in the identical way that the rest of the HCAL TPs are produced |
|
@Michael-Krohn Okay, that makes sense to me to offload the TP generation into HCAL specific code, my primary complaint is more about duplication of emulators. If we are going to do TP generation in HCAL, we still don't need 2 ZDC emulators at L1. As a first pass modification, couldn't we just move the |
|
@aloeliger ahh I understand your complaint now. I think you are correct that we could merge these together so that there is only one ZDC emulator at L1. I will let Chris comment on it as he is the one who wrote both sets of code |
|
@aloeliger @Michael-Krohn @cfmcginn do fully understand the complaint of having emulator duplication and I do agree we should indeed avoid it. On the other hand, being so close to the data taking, I would not remove a fully operation emulator completely. If you agree maybe we can simply move to the test folder the actual producer and keep running the new mapper |
|
If it were just me, we could do that, I'm the easy one to please in this scenario, but that is unlikely to be accepted by the release experts. We have releases and tags with the first draft emulator available, so this can be used for studies if the original emulator is needed. Or even reverted to in the worst case scenario. But I agree with @Michael-Krohn in this situation, if the TP generation exists more readily in HCAL software, then perhaps it is better to do it there (this has been a bit murky in this situation, the uHTR is pulling double duty as TP generator and trigger card, so the conceptual lines were a bit more difficult to draw). @Michael-Krohn Given that we do have an existing emulator, which has been at least slightly sanity checked, how hard would it be to check the new mapper against the emulator for consistency? If it is consistent, I don't think there should be any barrier to adopting it. |
|
In preparing this PR, I think we did perform the check that you are suggesting. And in fact, we found an issue in the old emulator (L1ZDCTProducer), which made the validation a little hacky but let me explain. The issue we found is that the L1ZDCTProducer does not handle the out-of-time pileup (OOTPU) LUTs that are used in the uHTR to calculate the ZDC TPs. It only applies the input LUTs. So we modified the L1ZDCTProducer to apply the OOTPU LUTs properly and we then compared that emulation with the emulation from this PR and found perfect agreement across a few 10s of thousands of events. @cfmcginn made a plot, that he might still have, showing this. Sorry that that's a little convoluted, but let me know if that doesn't make sense |
OOTPU handling was not part of L1TZDCProducer, this description is correct; after making a few basic modifications to the L1TZDCProducer to incorporate the OOTPU handling and the required LUT, L1TZDCProducer with uncalibrated TPs perfectly reproduces L1TZDCMapper w/ Michael's input calibrated ZDC TPs. Two files are attached, run over O(10k) events, showing this agreement - you can find the necessary modification to the L1TZDCProducer to achieve this here: master...cfmcginn:cmssw:l1tZDCProducerHack_MapperAgree_20230923
|
|
ok, so if I understand correctly both version of emulator are now fully validated. If this is the case great. I would then move to the newest version as main emulator and keep the old version (renamed if needed) just as a standalone producer in the test folder. I don't believe anybody can object to have a producer in the test folder indeed. |
@Michael-Krohn @icali @cfmcginn I would think it preferable just to edit the @Michael-Krohn the one thing I would like to ask is that we only end up with 1 in-production ZDC emulator making the agreed on ZDC sums, not two emulators in the same place. Would it be possible to make that change for this PR (making the |
|
@aloeliger I'm happy doing that. Let me talk to @cfmcginn since he is responsible for writing both implementations in the L1 code and my contribution is within the HCAL code. |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c867e6/35837/summary.html Comparison SummarySummary:
|
|
+l1
|
|
+alca
|
|
+db |
|
+1 |
cmsbuild
added
geometry-approved
fully-signed
and removed
geometry-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. @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
This PR performs the calculation of ZDC trigger primitives by emulating the calculation within HCAL uHTRs and is needed for the upcoming heavy ion data taking. The calculation pulls the relevant conditions from CondDB to calculate the look-up-tables (LUTs) on the fly and convert the ADC to energy in each ZDC trigger channel. The emulated ZDC digis are added to the simHcalTriggerPrimitiveDigis collection which are then passed to the L1Trigger/L1TZDC package and then on to the uGT. There are two ZDC digis that are passed to the uGT. These are the energy sums across each of the plus and minus sides of the ZDC detector. Individual digis are also created for the 18 individual ZDC channels (9 per side). These digis are not passed further up the chain to L1, but can be used to study possible new ZDC triggers in the future. The ZDC digis correspond to new HcalTrigTowerDetId's that have been defined with ieta=+/-42. The algorithm that calculates the ZDC energy sums matches the algorithm that is in the uHTR firmware.
The PR mimics the logic that was done in this PR#42634 (#42634), except that the ZDC trigger primitive calculation is done within the existing HCAL software, which enables the easy access to on detector conditions, as well as verifying that the LUTs calculation are identical on-detector and off-detector.
PR validation:
A basic technical test was performed: runTheMatrix.py -l limited -i all --ibeos
If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
This PR is not a backport, however we would like to submit a backport to 13_2_X