[HGCAL trigger] Mainly seeding and frontend Concentrator updates #28385

jbsauvan · 2019-11-12T17:51:31Z

PR description:

This PR includes several updates, which have been done mainly for the choice of the concentrator algorithms to be implemented in hardware.

Add cluster seeding in the x-y space. Can now choose to seed in the (default) r-phi space or x-y space
Optimization of the cluster shape variables used for cluster identification
Add framework for cluster energy interpretations and implement the EM interpretation
Update trigger geometry and Concentrator to deal with a "decentralized" concentrator architecture
Add energy compression for SuperTriggerCells
Add combinations of concentrator algorithms

Depends on external cms-data/L1Trigger-L1THGCal#16

Right after this PR is merged, an other one will follow which integrates the HFNose into the HGCAL TPG.
(FYI @mariadalfonso)

PR validation:

Standard code checks and formatting.

scram b code-checks
scram b code-format

Test V9, V10, V11 workflows (though the HGCAL trigger is still disconnected for V11).

runTheMatrix.py -w upgrade -l 20034.0 --maxSteps=2
runTheMatrix.py -w upgrade -l 20434.0 --maxSteps=2
runTheMatrix.py -w upgrade -l 22034.0 --maxSteps=2

Additionally each of these commits have been validated by looking at their impact on standard object distributions, and have also been validated with performance studies.

cmsbuild · 2019-11-12T17:51:59Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-11-12T17:59:51Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28385/12716

This PR adds an extra 220KB to repository

cmsbuild · 2019-11-12T18:00:20Z

A new Pull Request was created by @jbsauvan (Jean-Baptiste Sauvan) for master.

It involves the following packages:

DataFormats/L1THGCal
L1Trigger/L1THGCal
L1Trigger/L1THGCalUtilities

@cmsbuild, @rekovic, @benkrikler, @kpedro88 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @amarini, @rovere, @lgray this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

kpedro88 · 2019-11-12T19:22:53Z

please test with cms-data/L1Trigger-L1THGCal#16

cmsbuild · 2019-11-12T19:23:19Z

The tests are being triggered in jenkins.
Tested with other pull request(s) cms-data/L1Trigger-L1THGCal#16
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3442/console Started: 2019/11/12 20:26

kpedro88 · 2019-11-12T19:34:27Z

L1Trigger/L1THGCal/interface/backend/HGCalHistoSeedingImpl.h

@@ -55,6 +57,54 @@ class HGCalHistoSeedingImpl {
  };
  using Histogram = HistogramT<Bin>;

+  class Navigator {
+  public:
+    enum AxisType { Bounded, Circular };


better to use enum class

kpedro88 · 2019-11-12T19:42:39Z

L1Trigger/L1THGCal/plugins/backend/HGCalBackendLayer2Processor3DClustering.cc

    } else {
      throw cms::Exception("HGCTriggerParameterError") << "Unknown Multiclustering type '" << typeMulticluster << "'";
    }
+
+    for (auto interpretationPset : conf.getParameter<std::vector<edm::ParameterSet>>("energy_interpretations")) {


const auto&

kpedro88 · 2019-11-12T19:43:24Z

L1Trigger/L1THGCal/plugins/backend/HGCalTriggerClusterInterpretationEM.cc

+  scale_corrections_coeff_ = conf.getParameter<std::vector<double>>("scale_correction_coeff");
+  dr_bylayer_ = conf.getParameter<std::vector<double>>("dr_bylayer");
+
+  if (scale_corrections_coeff_.size() != 2) {


magic number 2 should be named constant

kpedro88 · 2019-11-12T19:43:55Z

L1Trigger/L1THGCal/plugins/backend/HGCalTriggerClusterInterpretationEM.cc

+        }
+      }
+    }
+    energy += scale_corrections_coeff_.at(1) * fabs(cluster3d.eta()) + scale_corrections_coeff_.at(0);


use std::abs() rather than fabs()

kpedro88 · 2019-11-12T19:45:16Z

L1Trigger/L1THGCal/plugins/geometries/HGCalTriggerGeometryV9Imp2.cc

      l1tModulesMapping_(conf.getParameter<edm::FileInPath>("L1TModulesMapping")),
      l1tLinksMapping_(conf.getParameter<edm::FileInPath>("L1TLinksMapping")) {
+  unsigned ntc_per_wafer = 48;


kpedro88 · 2019-11-12T19:55:44Z

L1Trigger/L1THGCal/src/backend/HGCalShowerShape.cc

+    for (const auto& id_tc : triggerCells) {
+      if (!pass(*id_tc.second, c3d))
+        continue;
+      tc_energy_z.emplace_back(std::make_pair(id_tc.second->energy(), id_tc.second->position().z()));


don't need make_pair with emplace_back

kpedro88 · 2019-11-12T19:56:48Z

L1Trigger/L1THGCal/src/concentrator/HGCalConcentratorSuperTriggerCellImpl.cc

 void HGCalConcentratorSuperTriggerCellImpl::createAllTriggerCells(
-    std::unordered_map<unsigned, SuperTriggerCell>& STCs, std::vector<l1t::HGCalTriggerCell>& trigCellVecOutput) const {
+    std::unordered_map<unsigned, SuperTriggerCell>& STCs, std::vector<l1t::HGCalTriggerCell>& trigCellVecOutput) {


why is this no longer const?

I cannot recall exactly why. Maybe it was needed to be non-const in an intermediate version. Anyway I added the const back.

kpedro88 · 2019-11-12T19:59:43Z

L1Trigger/L1THGCal/src/concentrator/HGCalConcentratorSuperTriggerCellImpl.cc

+  if (triggerTools_.isSilicon(c.detId())) {
+    thickness = triggerTools_.thicknessIndex(c.detId(), true);
+  } else if (triggerTools_.isScintillator(c.detId())) {
+    thickness = 3;


use HGCalTriggerTools::kScintillatorPseudoThicknessIndex_

Actually, since this pattern of "check if silicon, use thickness index, otherwise for scintillator use pseudo index" is repeated various times in the trigger code, it seems like it should just all be absorbed into the thicknessIndex() tool function

I followed your suggestion and absorbed this in the trigger tool function.

kpedro88 · 2019-11-12T20:02:18Z

L1Trigger/L1THGCalUtilities/python/hgcalTriggerChains.py

+        ntuple_sequence = cms.Sequence(tmpseq)
+        for vfe,truth_prod,backend1,backend2,selector,ntuple in self.truth_chain:
+            truth_prod_name = '{0}{1}'.format(vfe, truth_prod)
+            backend1_name = '{0}{1}{2}'.format(vfe, truth_prod, backend1)


could just chain these together to reduce duplication, e.g.

backend1_name = truth_prod_name+'{0}'.format(backend1)

(etc.)

kpedro88 · 2019-11-12T20:03:26Z

L1Trigger/L1THGCalUtilities/test/testHGCalL1T_multialgo_with_truth_cfg.py

+process.load('FWCore.MessageService.MessageLogger_cfi')
+process.load('Configuration.EventContent.EventContent_cff')
+process.load('SimGeneral.MixingModule.mixNoPU_cfi')
+process.load('Configuration.Geometry.GeometryExtended2023D35Reco_cff')


2023 -> 2026

cmsbuild · 2019-11-12T20:26:31Z

-1

Tested at: 5c7ffa5

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-815f79/3442/summary.html

I found follow errors while testing this PR

Failed tests: ClangBuild

Clang:

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

cmsbuild · 2019-11-12T20:26:37Z

Comparison not run due to Build errors/Fireworks only changes/No short matrix requested (RelVals and Igprof tests were also skipped)

kpedro88 · 2019-11-12T20:43:08Z

Clang complained about:

/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-11-12-1100/src/L1Trigger/L1THGCal/src/backend/HGCalHistoSeedingImpl.cc:527:26: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-11-12-1100/src/L1Trigger/L1THGCal/src/backend/HGCalHistoSeedingImpl.cc:529:26: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-11-12-1100/src/L1Trigger/L1THGCal/src/backend/HGCalHistoSeedingImpl.cc:530:26: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]

(I actually haven't seen that one before...)

jbsauvan · 2019-11-14T08:14:50Z

Thanks @kpedro88
I've implemented the corrections. I still have to validate them, then I'll push the commit.

cmsbuild · 2019-11-21T12:35:50Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-815f79/3572/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2936360
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2936017
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

kpedro88 · 2019-11-21T19:25:39Z

+upgrade

kpedro88 · 2019-11-21T19:25:57Z

@rekovic @benkrikler please check and review or sign

rekovic · 2019-11-22T13:49:15Z

+1

cmsbuild · 2019-11-22T13:49:47Z

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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2019-11-22T14:03:13Z

L1Trigger/L1THGCal/interface/concentrator/HGCalConcentratorBestChoiceImpl.h

-  static constexpr uint32_t kWaferOffset_ = 3;
-  static constexpr uint32_t kWaferMask_ = 0x7;
-  static constexpr uint32_t kLinkMask_ = 0x7;
+  static const unsigned kNDataSize_ = 128;


@jbsauvan @rekovic why was constexpr removed?

Thanks for noticing. Looks like I had modified these lines when const was used and when I rebased I missed the constexpr. I will change back to constexpr

@jbsauvan please propose it in a small update PR, I will merge this one for the time being

ok, will do

fabiocos · 2019-11-22T20:53:58Z

+1

cmsbuild added this to the CMSSW_11_0_X milestone Nov 12, 2019

cmsbuild added code-checks-pending comparison-pending l1-pending orp-pending pending-signatures tests-pending upgrade-pending labels Nov 12, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 12, 2019

cmsbuild mentioned this pull request Nov 12, 2019

Add mappings for decentralized ECON architecture cms-data/L1Trigger-L1THGCal#16

Merged

cmsbuild added requires-external tests-started and removed tests-pending labels Nov 12, 2019

kpedro88 reviewed Nov 12, 2019

View reviewed changes

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Nov 12, 2019

jbsauvan mentioned this pull request Nov 13, 2019

List of integrations PFCal-dev/cmssw#44

Open

jbsauvan added a commit to jbsauvan/cmssw that referenced this pull request Nov 13, 2019

Comments PR cms-sw#28385

f77381c

cmsbuild added tests-approved and removed tests-started labels Nov 21, 2019

cmsbuild added comparison-available and removed comparison-pending labels Nov 21, 2019

cmsbuild added upgrade-approved and removed upgrade-pending labels Nov 21, 2019

fabiocos mentioned this pull request Nov 22, 2019

Finalization of CMSSW_11_0_X open development #28455

Closed

9 tasks

cmsbuild added fully-signed l1-approved and removed l1-pending pending-signatures labels Nov 22, 2019

fabiocos reviewed Nov 22, 2019

View reviewed changes

smuzaffar mentioned this pull request Nov 22, 2019

Update tag for L1Trigger-L1THGCal to V01-00-15 cms-sw/cmsdist#5374

Merged

cmsbuild added orp-approved and removed orp-pending labels Nov 22, 2019

cmsbuild merged commit eb40634 into cms-sw:master Nov 22, 2019

cmsbuild mentioned this pull request Nov 23, 2019

Delay constructing transition wrappers #28459

Merged

jbsauvan mentioned this pull request Nov 24, 2019

[HGCAL trigger] Revert const back to constexpr #28460

Merged

jbsauvan added a commit to PFCal-dev/cmssw that referenced this pull request Dec 10, 2019

Comments PR cms-sw#28385

2a41870

jbsauvan mentioned this pull request Dec 10, 2019

Comments PR #28385 PFCal-dev/cmssw#413

Merged

jbsauvan added a commit to PFCal-dev/cmssw that referenced this pull request Dec 10, 2019

Comments PR cms-sw#28385

eec6193

jbsauvan added a commit to PFCal-dev/cmssw that referenced this pull request Dec 15, 2019

Comments PR cms-sw#28385

2a32b48

jbsauvan added a commit to PFCal-dev/cmssw that referenced this pull request Dec 15, 2019

Comments PR cms-sw#28385

27b8fae

jbsauvan mentioned this pull request Nov 29, 2021

Archived Integrations hgc-tpg/cmssw#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HGCAL trigger] Mainly seeding and frontend Concentrator updates #28385

[HGCAL trigger] Mainly seeding and frontend Concentrator updates #28385

jbsauvan commented Nov 12, 2019

cmsbuild commented Nov 12, 2019

cmsbuild commented Nov 12, 2019

cmsbuild commented Nov 12, 2019

kpedro88 commented Nov 12, 2019

cmsbuild commented Nov 12, 2019 •

edited

kpedro88 Nov 12, 2019

kpedro88 Nov 12, 2019

kpedro88 Nov 12, 2019

kpedro88 Nov 12, 2019

kpedro88 Nov 12, 2019

kpedro88 Nov 12, 2019

kpedro88 Nov 12, 2019

jbsauvan Nov 14, 2019

kpedro88 Nov 12, 2019

kpedro88 Nov 12, 2019

jbsauvan Nov 14, 2019

kpedro88 Nov 12, 2019

kpedro88 Nov 12, 2019

cmsbuild commented Nov 12, 2019

cmsbuild commented Nov 12, 2019

kpedro88 commented Nov 12, 2019

jbsauvan commented Nov 14, 2019

cmsbuild commented Nov 21, 2019

kpedro88 commented Nov 21, 2019

kpedro88 commented Nov 21, 2019

rekovic commented Nov 22, 2019

cmsbuild commented Nov 22, 2019

fabiocos Nov 22, 2019

jbsauvan Nov 22, 2019

fabiocos Nov 22, 2019

jbsauvan Nov 22, 2019

fabiocos commented Nov 22, 2019

[HGCAL trigger] Mainly seeding and frontend Concentrator updates #28385

[HGCAL trigger] Mainly seeding and frontend Concentrator updates #28385

Conversation

jbsauvan commented Nov 12, 2019

PR description:

PR validation:

cmsbuild commented Nov 12, 2019

cmsbuild commented Nov 12, 2019

cmsbuild commented Nov 12, 2019

kpedro88 commented Nov 12, 2019

cmsbuild commented Nov 12, 2019 • edited

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

cmsbuild commented Nov 12, 2019

cmsbuild commented Nov 12, 2019

kpedro88 commented Nov 12, 2019

jbsauvan commented Nov 14, 2019

cmsbuild commented Nov 21, 2019

kpedro88 commented Nov 21, 2019

kpedro88 commented Nov 21, 2019

rekovic commented Nov 22, 2019

cmsbuild commented Nov 22, 2019

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

fabiocos commented Nov 22, 2019

cmsbuild commented Nov 12, 2019 •

edited