-
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
[HGCAL trigger] Updated trigger towers (+minor updates) #30883
Conversation
The code-checks are being triggered in jenkins. |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30883/17263
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
35b8a30
to
a7fa581
Compare
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30883/17264
|
A new Pull Request was created by @jbsauvan (Jean-Baptiste Sauvan) for master. It involves the following packages: L1Trigger/L1THGCal @cmsbuild, @rekovic, @benkrikler, @kpedro88 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
void buildTowerMap2D(const std::vector<edm::Ptr<T>>& ptrs, l1t::HGCalTowerMapBxCollection& towerMaps) { | ||
std::unordered_map<int, l1t::HGCalTowerMap> towerMapsTmp = newTowerMaps(); | ||
|
||
for (auto ptr : ptrs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better as const auto& ptr
towerMapsTmp[layer].addEt(towerGeometryHelper_.getTriggerTower(*ptr), etEm, etHad); | ||
} | ||
|
||
/* store towerMaps in the persistent collection */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the towerMapsTmp
needed at all? can't this algorithm just fill towerMaps
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have one tower map per trigger layer, so not every layer. It was easier to fill in a map
rather than in a vector
of tower maps.
std::vector<edm::Ptr<l1t::HGCalTriggerCell>> trigCellVec; | ||
for (unsigned i = 0; i < unclTCsCollHandle->size(); ++i) { | ||
edm::Ptr<l1t::HGCalCluster> ptr(unclTCsCollHandle, i); | ||
for (auto itTC : ptr->constituents()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const auto&
} | ||
|
||
void HGCalHistoClusteringImpl::finalizeClusters(std::vector<l1t::HGCalMulticluster>& multiclusters_in, | ||
std::vector<l1t::HGCalCluster>& rejected_clusters_in, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const?
const HGCalTriggerGeometryBase& triggerGeometry) const { | ||
for (auto& tc : rejected_clusters_in) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const auto&
@@ -133,6 +145,10 @@ void HGCalHistoClusteringImpl::finalizeClusters(std::vector<l1t::HGCalMulticlust | |||
multicluster.saveHOverE(); | |||
|
|||
multiclusters_out.push_back(0, multicluster); | |||
} else { | |||
for (auto& tc : multicluster.constituents()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const auto&
@@ -75,6 +78,17 @@ DEFINE_EDM_PLUGIN(HGCalTriggerNtupleFactory, HGCalTriggerNtupleHGCDigis, "HGCalT | |||
|
|||
HGCalTriggerNtupleHGCDigis::HGCalTriggerNtupleHGCDigis(const edm::ParameterSet& conf) : HGCalTriggerNtupleBase(conf) { | |||
is_Simhit_comp_ = conf.getParameter<bool>("isSimhitComp"); | |||
digiBXselect_ = conf.getParameter<std::vector<unsigned int>>("digiBXselect"); | |||
|
|||
if (*std::max_element(digiBXselect_.begin(), digiBXselect_.end()) >= kDigiSize_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for safety, check if the vector is non-empty first
@@ -50,7 +50,8 @@ | |||
eeSimHits = cms.InputTag('g4SimHits:HGCHitsEE'), | |||
fhSimHits = cms.InputTag('g4SimHits:HGCHitsHEfront'), | |||
bhSimHits = cms.InputTag('g4SimHits:HcalHits'), | |||
isSimhitComp = cms.bool(False) | |||
isSimhitComp = cms.bool(False), | |||
digiBXselect = cms.vuint32([2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can just be cms.vuint32(2)
please test |
The tests are being triggered in jenkins.
|
+1 |
Comparison job queued. |
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30883/17388
|
Pull request #30883 was updated. @cmsbuild, @rekovic, @benkrikler, @kpedro88 can you please check and sign again. |
please test |
The tests are being triggered in jenkins.
|
+1 |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+upgrade |
Kind reminder @cms-sw/l1-l2 |
merge |
PR description:
PR validation:
Workflows tested:
23234.0
,27034.0
,28234.0