Adding ZDC Unpacker to L1RawtoDigi #42733

matt2275 · 2023-09-06T16:30:25Z

PR description:

Adding a ZDC unpacker for the L1 Emulation

This is a work in progress Andrew David Loeliger and Dinyar Rabady are more expect in this area and will help solve the issues with the PR

All file changes are in EventFilter/L1TRawToDigi/plugins/implementations_stage2/
2 files were created
ZDCUnpacker.cc
ZDCUnpacker.h

3 Files Changed:
GTSetup.cc
GTCollections.cc
GTCollections.h

Currently there is a complier error I'm not sure how to resolve:
/afs/cern.ch/user/m/mnickel/private/ZDC_Trig/New_WF/CMSSW_13_2_0_pre3/src/EventFilter/L1TRawToDigi/plugins/implementations_stage2/GTCollections.h:40:33: error: 'l1t::EtSumBxCollection* l1t::stage2::GTCollections::getZDCSums(unsigned int)' marked 'override', but does not override
40 | inline EtSumBxCollection* getZDCSums(const unsigned int copy) override { return zdcsums_[copy].get(); };

PR validation:

No validation has been done yet.

Added Et sum for ZDC unpacker in registerProducts

added zdc to GTCollections

added ZDCSum to GTCollections

Updating names due to GTCollection changes

Updated names due to GTCollection changes

Merging CMS-SW master to ZDCUnpacker

bundocka · 2023-09-06T16:35:56Z

EventFilter/L1TRawToDigi/plugins/implementations_stage2/ZDCUnpacker.cc

+    ZDCUnpacker::ZDCUnpacker() : ZDCSumCopy_(0) {}
+
+    bool ZDCUnpacker::unpack(const Block& block, UnpackerCollections* coll) {
+      using namespace l1t::stage2::layer2;


this should just be l1t::stage2 (ZDC is not part of Calo Layer 2)

I addressed this in my commit, Fixing Issues for PR 42733 (First Attempt). I had to change demux::nOutputFramePerBX to layer2::demux::nOutputFramePerBX to solve compiler errors. The ZDC data has 6 frames for BX which is the value of nOutputFramePerBX but this may be coincident.

Yeah, I was unsure whether to flag the use of demux::nOutputFramePerBX in my first pass. I'd say this could technically change and become inconsistent with the number of input frames to the uGT, but in practice I have a hard time imagining how that would do. (So the bottom line from my side was that it's probably reasonable to leave it as you have it. If I remember correctly we just put the literal 6 in the muon unpacker which is also not nice.. ) @bundocka may have better thought through input than me, though.

demux::nOutputFramePerBX == zdc::nOutputFramePerBX is a coincidence, I would not recommend using the demux constant, but instead create zdc::nOutputFramePerBX, for clarity. as Dinyar said, this should be fine as it is, and it shouldn't change, but the two numbers are technically unrelated so they shouldn't use the same constant.

I tell a lie, it is not a coincidence at all :) 6bx is the maximum length allowed for the GT input packet. the input links are clocked at 240 MHz. 240 / 6 = 40 MHz.

To add the zdc::nOutputFramePerBX variable. should I add it to the L1TStage2Layer2Constants.cc file or create a new constants file only for zdc. I could also add it to the ZDCUnpacker.h but I don't like that since I believe the variable will also be used by the zdc packer in the future.

I would have the variable be this in all cases

const unsigned int l1t::stage2::zdc::nOutputFramePerBX = 6;

I would be tempted to put it somewhere like GTSetup.h, since it is a general parameter for the input to the GT. (Then technically this parameter should be used throughout the muon and layer 2 unpackers)
https://github.com/cms-sw/cmssw/blob/master/EventFilter/L1TRawToDigi/plugins/implementations_stage2/GTSetup.h

cmsbuild · 2023-09-06T16:37:43Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42733/36824

ERROR: Build errors found during clang-tidy run.

EventFilter/L1TRawToDigi/plugins/implementations_stage2/ZDCUnpacker.cc:33:61: error: no member named 'getZDCSums' in 'l1t::stage2::L1TObjectCollections'; did you mean 'getEtSums'? [clang-diagnostic-error]
      auto res_ = static_cast<L1TObjectCollections*>(coll)->getZDCSums(ZDCSumCopy_);
                                                            ^~~~~~~~~~
                                                            getEtSums
--
EventFilter/L1TRawToDigi/plugins/implementations_stage2/GTCollections.h:40:69: error: only virtual member functions can be marked 'override' [clang-diagnostic-error]
      inline EtSumBxCollection* getZDCSums(const unsigned int copy) override { return zdcsums_[copy].get(); };
                                                                    ^~~~~~~~~
Suppressed 871 warnings (871 in non-user code).
--
EventFilter/L1TRawToDigi/plugins/implementations_stage2/GTCollections.h:40:69: error: only virtual member functions can be marked 'override' [clang-diagnostic-error]
      inline EtSumBxCollection* getZDCSums(const unsigned int copy) override { return zdcsums_[copy].get(); };
                                                                    ^~~~~~~~~
Suppressed 977 warnings (977 in non-user code).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

missirol · 2023-09-06T16:37:55Z

EventFilter/L1TRawToDigi/plugins/implementations_stage2/GTCollections.cc

@@ -9,13 +9,15 @@ namespace l1t {
      event_.put(std::move(muonShowers_[0]), "MuonShower");
      event_.put(std::move(egammas_[0]), "EGamma");
      event_.put(std::move(etsums_[0]), "EtSum");
+      event_.put(std::move(zdcsums_[0]), "ZDCSum");


Please change this string to EtSumZDC, here and elsewhere in this PR (see #42707 (comment)).

This was addressed in Fixing Issues for PR 42733 (First Attempt). I did not update the zdcsums since I thought changing to etsumzdcs would be confusing but I can change that as well

missirol · 2023-09-06T16:39:19Z

EventFilter/L1TRawToDigi/plugins/implementations_stage2/GTSetup.cc

@@ -55,6 +56,7 @@ namespace l1t {
      prod.produces<MuonShowerBxCollection>("MuonShower");
      prod.produces<EGammaBxCollection>("EGamma");
      prod.produces<EtSumBxCollection>("EtSum");
+      prod.produces<EtSumBxCollection>("ZDCSum"); // added addition EtSum collection for ZDC  unpacker 


added addition EtSum collection for ZDC unpacker

Please improve the comment (or remove it).

missirol · 2023-09-06T16:40:55Z

EventFilter/L1TRawToDigi/plugins/implementations_stage2/GTSetup.cc

@@ -97,6 +102,7 @@ namespace l1t {
      muon_unp->setMuonCopy(amc - 1);
      egamma_unp->setEGammaCopy(amc - 1);
      etsum_unp->setEtSumCopy(amc - 1);
+      zdc_unp->setZDCSumCopy(amc - 1);


Suggested change

zdc_unp->setZDCSumCopy(amc - 1);

zdc_unp->setEtSumZDCCopy(amc - 1);

I would also change the name of this method.

aloeliger · 2023-09-06T16:44:28Z

@fwyzard & @Martin-Grunewald if you haven't seen this, I wanted to let you know this exists.

I see @missirol has already found it.

missirol · 2023-09-06T16:46:20Z

Notifying also @mmusich as @cms-sw/hlt-l2.

Currently there is a complier error I'm not sure how to resolve:
/afs/cern.ch/user/m/mnickel/private/ZDC_Trig/New_WF/CMSSW_13_2_0_pre3/src/EventFilter/L1TRawToDigi/plugins/implementations_stage2/GTCollections.h:40:33: error: 'l1t::EtSumBxCollection* l1t::stage2::GTCollections::getZDCSums(unsigned int)' marked 'override', but does not override
40 | inline EtSumBxCollection* getZDCSums(const unsigned int copy) override { return zdcsums_[copy].get(); };

I would guess one needs to add a method here:

cmssw/EventFilter/L1TRawToDigi/plugins/implementations_stage2/L1TObjectCollections.h

Line 28 in 6303322

aloeliger · 2023-09-06T17:42:15Z

@dinyar In case you were unaware that this has been opened, I also wanted you to know that this is available on github for review.

dinyar · 2023-09-06T18:50:53Z

EventFilter/L1TRawToDigi/plugins/implementations_stage2/ZDCUnpacker.cc

+        uint32_t raw_data = block.payload().at(iFrame +1); // ZDC - info is found on frame 1 of each bx
+
+        l1t::EtSum zdcm{l1t::EtSum::kZDCM};
+        zdcm.setHwPt(raw_data & 0xFFFF);


Just to make sure: the sum has 16 bits? (I couldn't find this documented anywhere, but maybe I just don't know where this is officially documented.. )

The sum should be 10 bits if I'm not wrong but @jmmans can comment as for what Herbert says it is an exact copy of what the uHTR sends. As far as I know frame 1 is ZDC- and frame 2 is ZDC+

Ok, in that case if I'm not miscounting:

Suggested change

zdcm.setHwPt(raw_data & 0xFFFF);

zdcm.setHwPt(raw_data & 0x3FF);

(but as mentioned below I would put that into a named constant)

To close this comment, I will update the bit mask and put it into a variable. Are we sure bit shifting is not required to extract the zdc info (i.e. rawdata >> 8) ?

@matt2275 My understanding of Herbert on the email was the uGT stores these in the same word format that ZDC hands them off to uGT. Does ZDC store bits unrelated to the overall HW Pt that will need to be masked or shifted away?

I don't think there is any data other than the energy count (even so, it should always be masked to the know width).
ps. I found some doc, a search for ZDC gives quite a lot of useful info for this PR:
https://github.com/cms-l1-globaltrigger/mp7_ugt_legacy/blob/master/doc/mp7_ugt_firmware_specification/pdf/gt-mp7-firmware-specification.pdf

dinyar · 2023-09-06T18:53:14Z

EventFilter/L1TRawToDigi/plugins/implementations_stage2/ZDCUnpacker.cc

+        raw_data = block.payload().at(iFrame +2); // ZDC + info is found on frame 2 of each bx
+
+        l1t::EtSum zdcp{l1t::EtSum::kZDCP};
+        zdcp.setHwPt(raw_data & 0xFFFF);


I would suggest to put the mask into a variable given that it's used in these two places and also in a future packer..

dinyar · 2023-09-06T19:03:35Z

EventFilter/L1TRawToDigi/plugins/implementations_stage2/ZDCUnpacker.cc

+
+        l1t::EtSum zdcm{l1t::EtSum::kZDCM};
+        zdcm.setHwPt(raw_data & 0xFFFF);
+        zdcm.setP4(l1t::CaloTools::p4Demux(&zdcm));


Just to check here: my understanding is that this function computes the four vector with the implicit understanding that the LSB is 0.5 GeV. Is that the case for the ZDC sums too? (Again this is based on me not finding documentation, so may be a moot point.)

it is correct your interpretation but in ZDC we should use the numerical expression and not the conversion in energy as it is going to be wrong, in my understanding. @cfmcginn can comment.

Right, I think the uGT emulator always uses the hw values anyway and gets the scales from the menu XML. The physical pT should, however be correct because that's what people usually look at in e.g. the ntuples.

what about the ZDC L1 candidate used in the object map ?

Yes, absolutely. If someone accesses the physical pT value of that object they would get a value of hw_code*0.5 GeV. I'm not entirely sure from Ivan's answer whether this is correct.

dinyar · 2023-09-06T19:04:33Z

EventFilter/L1TRawToDigi/plugins/implementations_stage2/GTSetup.cc

@@ -116,6 +122,7 @@ namespace l1t {
        res[16] = tau_unp;
        res[18] = tau_unp;
        res[20] = etsum_unp;
+        res[22] = zdc_unp;


I suppose it's confirmed that ZDC inputs come on link 11 of the uGT?

I understood it was link 71 (0x8E)

I was not sure of the link. If the link is 71, then it should be res[142]?

If the logical link ID is 71, then yes I think res[142] should be correct.
Cross referencing the GT input data format [1] with the unpacker [2] I think supports this.

[1] https://twiki.cern.ch/twiki/bin/viewauth/CMS/GlobalTriggerInputDataFormat
[2]

cmssw/EventFilter/L1TRawToDigi/plugins/implementations_stage2/GTSetup.cc

Line 114 in 440318c

res[0] = muon_unp;

(I think for res[X], even X is Rx and odd X is Tx)

Yeah, my understanding is also that input ports get even numbers, so it would be 142 if it's link 71.

Addressing some of the issues raised by cms-sw

aloeliger · 2023-09-15T07:56:31Z

please test

cmsbuild · 2023-09-15T11:53:06Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49d39c/34776/summary.html
COMMIT: 7422859
CMSSW: CMSSW_13_3_X_2023-09-14-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42733/34776/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 6 lines from the logs
ROOTFileChecks: Some differences in event products or their sizes found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3348648
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3348623
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 214 log files, 167 edm output root files, 50 DQM output files
TriggerResults: no differences found

fwyzard · 2023-09-15T12:02:40Z

Sorry @aloeliger , I'm not able to follow part of your last comment.

Run a technical check to make sure that the gt emulator can be run off of this unpacked collection

Rerunning the L1 analysis chain through to ntuples, and running GT off the ZDC unpacker, matt was also able to get the chain to run without crashing and examine the output, which is in agreement with the unpacker output from before

does this last point mean that

the ZDC input to the uGT was unpacked from the uGT raw data using the module introduced in this PR (Adding ZDC Unpacker to L1RawtoDigi #42733)
the resulting collection was to the uGT emulator (which now includes L1T ZDC and uGT Emulator to CMSSW_13_3_X #42634/L1T ZDC and uGT Emulator Backport #42706)
the results of the uGT emulator were compared with the uGT results from the data

?

aloeliger · 2023-09-15T12:19:20Z

Sorry @aloeliger , I'm not able to follow part of your last comment.

Run a technical check to make sure that the gt emulator can be run off of this unpacked collection

Rerunning the L1 analysis chain through to ntuples, and running GT off the ZDC unpacker, matt was also able to get the chain to run without crashing and examine the output, which is in agreement with the unpacker output from before

does this last point mean that

the ZDC input to the uGT was unpacked from the uGT raw data using the module introduced in this PR (Adding ZDC Unpacker to L1RawtoDigi #42733)

the resulting collection was to the uGT emulator (which now includes L1T ZDC and uGT Emulator to CMSSW_13_3_X #42634/L1T ZDC and uGT Emulator Backport #42706)

the results of the uGT emulator were compared with the uGT results from the data

?

@fwyzard

It means ZDC sums were unpacked from uGT input as is done at HLT (to my understanding) and the resultant sums are not appreciably different than they were in the last step, so it is our understanding that HLT should be able to run this module this way.

Edit:

Forgot to mention, the uGT emulator runs on this and we can produce sensible L1Ntuples from the output of the GT emulator. This signals to me that the GT emulation should be working well enough to run HLT sensibly.

fwyzard · 2023-09-15T13:17:31Z

OK, so if I understand correctly you are able to run these two workflows

read the uGT results from data, and fill the ntuples with these original results
unpack all uGT inputs (including ZDC), run the uGT emulator on them, and fill the ntuples with the emulated results

Assuming that's correct, do you have a way to compare event by event the original and emulated results ?

If they are identical, everything is fine.

aloeliger · 2023-09-15T13:26:15Z

+l1

at this point I haven't heard any show stoppers

@cms-sw/orp-l2 when this is done, a new version of the software is likely needed urgently. I can discuss this at any point Monday if needed.

I think it worth noting that I am signing this commit from the Acropolis of Athens :)

cmsbuild · 2023-09-15T13:26:42Z

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. @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

mandrenguyen · 2023-09-15T13:35:33Z

@matt2275 Can you please prepare the 13_2_X backport?

antoniovilela · 2023-09-15T13:40:57Z

OK, so if I understand correctly you are able to run these two workflows
1. read the uGT results from data, and fill the ntuples with these original results

2. unpack all uGT inputs (including ZDC), run the uGT emulator on them, and fill the ntuples with the emulated results
Assuming that's correct, do you have a way to compare event by event the original and emulated results ?

If they are identical, everything is fine.

@matt2275 @aloeliger
Given the urgency and validation performed so far, we will merge this PR. Please continue with the event-by-event validation proposed.

antoniovilela · 2023-09-15T13:41:32Z

+1

matt2275 · 2023-09-15T13:56:04Z

To do the backport should I do a similar procedure as this: #42781 but add EventFilter/L1TRawtoDigi instead of L1Trigger/L1TGlobal?

mandrenguyen · 2023-09-15T15:37:03Z

I suppose, although I'm not sure sure what procedure was followed for this other PR.

Normally one should grab the latest CMSSW_13_2_X integration build, which at this moment you would do as follows:
cmsrel CMSSW_13_2_X_2023-09-15-1100
cd CMSSW_13_2_X_2023-09-15-1100/src
cmsenv

Ideally you would use the git-cherry-pick command with the hash of each commit, but you have many of them.
In case you don't manage, I guess since the changes are modest perhaps in this case it would be enough to then do:
git cms-addpkg EventFilter/L1TRawToDigi
and then add your changes by hand very carefully

kpedro88 · 2023-09-15T15:39:33Z

Repeated cherry-pick is not necessary. There are backporting instructions here: http://cms-sw.github.io/tutorial-resolve-conflicts.html#backporting-a-pr

To make a backport simpler, you can first make a copy of your 13_3_X branch and squash all of your commits down to one commit, and then just rebase that single commit.

fwyzard · 2023-09-15T15:44:12Z

We should really move the default action in CMSSW to "Squash and merge" ...

mmusich · 2023-09-15T15:45:09Z

To make a backport simpler, you can first make a copy of your 13_3_X branch and squash all of your commits down to one commit, and then just rebase that single commit.

on a similar tone, I am not sure why this PR was accepted with 18 commits (most of them with uninformative messages). A minimum squash would have made life simpler.

mandrenguyen · 2023-09-15T15:58:30Z

I just went ahead and made PR in 13_2_X after rebase/squash:
#42800

@matt2275 (and others) please check.

antoniovilela · 2023-09-15T19:00:12Z

We should really move the default action in CMSSW to "Squash and merge" ...

Seems like something to consider.
@rappoccio

matt2275 added 12 commits September 3, 2023 18:18

Adding ZDCUnpacker files

000f129

Update GTSetup.cc for ZDCUnpacker

94a8788

Update ZDCUnpacker.cc to fix double variable declarations

a398a77

Update ZDCUnpacker.cc to clarify ZDC data format

a7910cc

Update GTSetup.cc

345bb42

Added Et sum for ZDC unpacker in registerProducts

Update GTCollections.h

f2b08ff

added zdc to GTCollections

Update GTCollections.cc

b5df094

added ZDCSum to GTCollections

Update ZDCUnpacker.cc

80b4e2e

Updating names due to GTCollection changes

Update ZDCUnpacker.h

579f7e3

Updated names due to GTCollection changes

Update GTSetup.cc

eff61c7

Update GTSetup.cc

76e8be8

Merge pull request #1 from matt2275/master

440318c

Merging CMS-SW master to ZDCUnpacker

cmsbuild added this to the CMSSW_13_3_X milestone Sep 6, 2023

cmsbuild added l1-pending pending-signatures tests-pending orp-pending code-checks-pending labels Sep 6, 2023

bundocka reviewed Sep 6, 2023

View reviewed changes

cmsbuild added code-checks-rejected and removed code-checks-pending labels Sep 6, 2023

missirol reviewed Sep 6, 2023

View reviewed changes

dinyar reviewed Sep 6, 2023

View reviewed changes

Fixing Issues for PR 42733 (First Attempt)

44d6172

Addressing some of the issues raised by cms-sw

cmsbuild added code-checks-pending and removed code-checks-rejected labels Sep 6, 2023

cmsbuild added tests-started and removed tests-pending labels Sep 15, 2023

cmsbuild added tests-approved and removed tests-started labels Sep 15, 2023

cmsbuild added l1-approved fully-signed and removed l1-pending pending-signatures labels Sep 15, 2023

cmsbuild added orp-approved and removed orp-pending labels Sep 15, 2023

cmsbuild merged commit aa4c5ab into cms-sw:master Sep 15, 2023

mandrenguyen mentioned this pull request Sep 15, 2023

[13_2_X] Adding ZDC Unpacker to L1RawtoDigi #42800

Merged

missirol mentioned this pull request Nov 7, 2023

Include ZDC in L1T re-emulation workflows #43214

Open

	zdc_unp->setZDCSumCopy(amc - 1);
	zdc_unp->setEtSumZDCCopy(amc - 1);

	zdcm.setHwPt(raw_data & 0xFFFF);
	zdcm.setHwPt(raw_data & 0x3FF);

Adding ZDC Unpacker to L1RawtoDigi #42733

Adding ZDC Unpacker to L1RawtoDigi #42733

Conversation

matt2275 commented Sep 6, 2023

PR description:

PR validation:

bundocka Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinyar Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloeliger commented Sep 6, 2023

missirol commented Sep 6, 2023

aloeliger commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinyar Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bundocka Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloeliger commented Sep 15, 2023

cmsbuild commented Sep 15, 2023

Comparison Summary

fwyzard commented Sep 15, 2023 • edited Loading

aloeliger commented Sep 15, 2023 • edited Loading

fwyzard commented Sep 15, 2023

aloeliger commented Sep 15, 2023

cmsbuild commented Sep 15, 2023

mandrenguyen commented Sep 15, 2023

antoniovilela commented Sep 15, 2023

antoniovilela commented Sep 15, 2023

matt2275 commented Sep 15, 2023 • edited Loading

mandrenguyen commented Sep 15, 2023

kpedro88 commented Sep 15, 2023

fwyzard commented Sep 15, 2023

mmusich commented Sep 15, 2023

mandrenguyen commented Sep 15, 2023

antoniovilela commented Sep 15, 2023

bundocka Sep 6, 2023 •

edited

Loading

dinyar Sep 6, 2023 •

edited

Loading

dinyar Sep 6, 2023 •

edited

Loading

bundocka Sep 6, 2023 •

edited

Loading

fwyzard commented Sep 15, 2023 •

edited

Loading

aloeliger commented Sep 15, 2023 •

edited

Loading

matt2275 commented Sep 15, 2023 •

edited

Loading