Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

event_.put(std::move(jets_[0]), "Jet");
event_.put(std::move(taus_[0]), "Tau");
for (int i = 1; i < 6; ++i) {
event_.put(std::move(muons_[i]), "Muon" + std::to_string(i + 1));
event_.put(std::move(muonShowers_[i]), "MuonShower" + std::to_string(i + 1));
event_.put(std::move(egammas_[i]), "EGamma" + std::to_string(i + 1));
event_.put(std::move(etsums_[i]), "EtSum" + std::to_string(i + 1));
event_.put(std::move(zdcsums_[i]), "ZDCSum" + std::to_string(i + 1));
event_.put(std::move(jets_[i]), "Jet" + std::to_string(i + 1));
event_.put(std::move(taus_[i]), "Tau" + std::to_string(i + 1));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace l1t {
muonShowers_.begin(), muonShowers_.end(), [] { return std::make_unique<MuonShowerBxCollection>(); });
std::generate(egammas_.begin(), egammas_.end(), [] { return std::make_unique<EGammaBxCollection>(); });
std::generate(etsums_.begin(), etsums_.end(), [] { return std::make_unique<EtSumBxCollection>(); });
std::generate(zdcsums_.begin(), zdcsums_.end(), [] { return std::make_unique<EtSumBxCollection>(); });
std::generate(jets_.begin(), jets_.end(), [] { return std::make_unique<JetBxCollection>(); });
std::generate(taus_.begin(), taus_.end(), [] { return std::make_unique<TauBxCollection>(); });
};
Expand All @@ -36,6 +37,7 @@ namespace l1t {
};
inline EGammaBxCollection* getEGammas(const unsigned int copy) override { return egammas_[copy].get(); };
inline EtSumBxCollection* getEtSums(const unsigned int copy) override { return etsums_[copy].get(); };
inline EtSumBxCollection* getZDCSums(const unsigned int copy) override { return zdcsums_[copy].get(); };
inline JetBxCollection* getJets(const unsigned int copy) override { return jets_[copy].get(); };
inline TauBxCollection* getTaus(const unsigned int copy) override { return taus_[copy].get(); };

Expand All @@ -47,6 +49,7 @@ namespace l1t {
std::array<std::unique_ptr<MuonShowerBxCollection>, 6> muonShowers_;
std::array<std::unique_ptr<EGammaBxCollection>, 6> egammas_;
std::array<std::unique_ptr<EtSumBxCollection>, 6> etsums_;
std::array<std::unique_ptr<EtSumBxCollection>, 6> zdcsums_;
std::array<std::unique_ptr<JetBxCollection>, 6> jets_;
std::array<std::unique_ptr<TauBxCollection>, 6> taus_;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "EventFilter/L1TRawToDigi/plugins/implementations_stage2/MuonUnpacker.h"
#include "EventFilter/L1TRawToDigi/plugins/implementations_stage2/EGammaUnpacker.h"
#include "EventFilter/L1TRawToDigi/plugins/implementations_stage2/EtSumUnpacker.h"
#include "EventFilter/L1TRawToDigi/plugins/implementations_stage2/ZDCUnpacker.h"
#include "EventFilter/L1TRawToDigi/plugins/implementations_stage2/JetUnpacker.h"
#include "EventFilter/L1TRawToDigi/plugins/implementations_stage2/TauUnpacker.h"

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added addition EtSum collection for ZDC unpacker

Please improve the comment (or remove it).

prod.produces<JetBxCollection>("Jet");
prod.produces<TauBxCollection>("Tau");
prod.produces<GlobalAlgBlkBxCollection>();
Expand All @@ -64,6 +66,7 @@ namespace l1t {
prod.produces<MuonShowerBxCollection>("MuonShower" + std::to_string(i));
prod.produces<EGammaBxCollection>("EGamma" + std::to_string(i));
prod.produces<EtSumBxCollection>("EtSum" + std::to_string(i));
prod.produces<EtSumBxCollection>("ZDCSum" + std::to_string(i));
prod.produces<JetBxCollection>("Jet" + std::to_string(i));
prod.produces<TauBxCollection>("Tau" + std::to_string(i));
}
Expand All @@ -80,6 +83,8 @@ namespace l1t {
static_pointer_cast<l1t::stage2::EGammaUnpacker>(UnpackerFactory::get()->make("stage2::EGammaUnpacker"));
auto etsum_unp =
static_pointer_cast<l1t::stage2::EtSumUnpacker>(UnpackerFactory::get()->make("stage2::EtSumUnpacker"));
auto zdc_unp =
static_pointer_cast<l1t::stage2::ZDCUnpacker>(UnpackerFactory::get()->make("stage2::ZDCUnpacker"));
auto jet_unp = static_pointer_cast<l1t::stage2::JetUnpacker>(UnpackerFactory::get()->make("stage2::JetUnpacker"));
auto tau_unp = static_pointer_cast<l1t::stage2::TauUnpacker>(UnpackerFactory::get()->make("stage2::TauUnpacker"));

Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I would also change the name of this method.

jet_unp->setJetCopy(amc - 1);
tau_unp->setTauCopy(amc - 1);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood it was link 71 (0x8E)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Contributor

@bundocka bundocka Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (amc == 1) { // only unpack first uGT board for the external signal inputs (single copy)
res[24] = ext_unp;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
#include "FWCore/MessageLogger/interface/MessageLogger.h"
#include "EventFilter/L1TRawToDigi/plugins/UnpackerFactory.h"

#include "L1Trigger/L1TCalorimeter/interface/CaloTools.h"

#include "L1TObjectCollections.h"

#include "L1TStage2Layer2Constants.h"
#include "ZDCUnpacker.h"

namespace l1t {
namespace stage2 {
ZDCUnpacker::ZDCUnpacker() : ZDCSumCopy_(0) {}

bool ZDCUnpacker::unpack(const Block& block, UnpackerCollections* coll) {
using namespace l1t::stage2::layer2;
Copy link
Contributor

@bundocka bundocka Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dinyar dinyar Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


LogDebug("L1T") << "Block ID = " << block.header().getID() << " size = " << block.header().getSize();

int nBX = int(
ceil(block.header().getSize() / demux::nOutputFramePerBX)); // Since there 6 frames per demux output event
// expect the first four frames to be the first 4 EtSum objects reported per event (see CMS IN-2013/005)

// Find the central, first and last BXs
int firstBX = -(ceil((double)nBX / 2.) - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int firstBX = -(ceil((double)nBX / 2.) - 1);
int firstBX = (nBX / 2) - nBX + 1;

int lastBX;
if (nBX % 2 == 0) {
lastBX = ceil((double)nBX / 2.);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just checked that nBX is even, so there is no need to cast to double and call ceil.
Just

Suggested change
lastBX = ceil((double)nBX / 2.);
lastBX = nBX / 2;

} else {
lastBX = ceil((double)nBX / 2.) - 1;
Copy link
Contributor

@fwyzard fwyzard Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here nBX is odd, so this is equivalent to

Suggested change
lastBX = ceil((double)nBX / 2.) - 1;
lastBX = nBX / 2;

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, this whole block can be simplified to

Suggested change
int lastBX;
if (nBX % 2 == 0) {
lastBX = ceil((double)nBX / 2.);
} else {
lastBX = ceil((double)nBX / 2.) - 1;
}
int lastBX = nBX / 2;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the new code in this PR.
A separate PR can be made to fix the other instances.


auto res_ = static_cast<L1TObjectCollections*>(coll)->getZDCSums(ZDCSumCopy_);
res_->setBXRange(firstBX, lastBX);

LogDebug("L1T") << "nBX = " << nBX << " first BX = " << firstBX << " lastBX = " << lastBX;

// Loop over multiple BX and fill EtSums collection
for (int bx = firstBX; bx <= lastBX; bx++) {
// ZDC -
int iFrame = (bx - firstBX) * demux::nOutputFramePerBX;

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.. )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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+

Copy link
Contributor

@dinyar dinyar Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

zdcm.setP4(l1t::CaloTools::p4Demux(&zdcm));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


LogDebug("L1T") << "ZDC -: pT " << zdcm.hwPt() << " bx " << bx;

res_->push_back(bx, zdcm);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll ask this again here since I asked on the email chain. @elfontan Is there an expected order of these sums at the GT emulator? Do these need to be in a specific order?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// ZDC +
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

zdcp.setP4(l1t::CaloTools::p4Demux(&zdcp));

LogDebug("L1T") << "ZDC +: pT " << zdcp.hwPt() << " bx " << bx;

res_->push_back(bx, zdcp);

}

return true;
}
} // namespace stage2
} // namespace l1t

DEFINE_L1T_UNPACKER(l1t::stage2::ZDCUnpacker);
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#ifndef L1T_PACKER_STAGE2_ZDCUnpacker_H
#define L1T_PACKER_STAGE2_ZDCUnpacker_H

#include "EventFilter/L1TRawToDigi/interface/Unpacker.h"

namespace l1t {
namespace stage2 {
class ZDCUnpacker : public Unpacker {
public:
ZDCUnpacker();
~ZDCUnpacker() override{};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
~ZDCUnpacker() override{};
~ZDCUnpacker() override = default;


bool unpack(const Block& block, UnpackerCollections* coll) override;

virtual inline void setZDCSumCopy(const unsigned int copy) { ZDCSumCopy_ = copy; };

private:
unsigned int ZDCSumCopy_;
};
} // namespace stage2
} // namespace l1t

#endif