[L1T] Phase-2, correlator developments #39397

cecilecaillol · 2022-09-15T09:56:29Z

PR description:

This PR includes the latest devlopments to the Phase-2 L1 correlator. The local PR is cms-l1t-offline#1031

We would like this PR to be backported to 12_5 for our Phase-2 MC production

PR validation:

code-checks, code-format, objects validated in our local branch

cmsbuild · 2022-09-15T10:02:27Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39397/32102

This PR adds an extra 76KB to repository

cmsbuild · 2022-09-15T10:02:51Z

A new Pull Request was created by @cecilecaillol for master.

It involves the following packages:

L1Trigger/Phase2L1GMT (l1)
L1Trigger/Phase2L1ParticleFlow (upgrade, l1)

@rekovic, @epalencia, @cmsbuild, @AdrianoDee, @srimanob, @cecilecaillol can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @beaucero, @trtomei this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

cecilecaillol · 2022-09-15T10:13:09Z

please test

cmsbuild · 2022-09-15T14:36:28Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-faae7c/27563/summary.html
COMMIT: 7701e6a
CMSSW: CMSSW_12_6_X_2022-09-14-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39397/27563/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3618326
DQMHistoTests: Total failures: 28
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3618276
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 212 log files, 49 edm output root files, 51 DQM output files
TriggerResults: no differences found

cecilecaillol · 2022-09-15T16:05:50Z

+l1

cecilecaillol · 2022-09-20T07:29:35Z

urgent
(needed for L1 Phase2 MC production in 12_5)

srimanob · 2022-09-27T16:00:19Z

L1Trigger/Phase2L1ParticleFlow/test/make_l1ct_patternFiles_cfg.py

+process.source = cms.Source("PoolSource",
+    fileNames = cms.untracked.vstring('file:inputs110X.root'),
+    inputCommands = cms.untracked.vstring("keep *", 
+            "drop l1tPFClusters_*_*_*",


Is this correct, not *_l1tPFClusters_*_* ? I assume l1tPFClusters is a module name, not type.

it's a type name: https://github.com/cms-sw/cmssw/blob/master/DataFormats/L1TParticleFlow/interface/PFCluster.h

srimanob · 2022-09-27T16:08:39Z

L1Trigger/Phase2L1ParticleFlow/interface/egamma/pftkegsorter_ref.h

@@ -5,12 +5,10 @@
 #include <vector>

 #include "DataFormats/L1TParticleFlow/interface/layer1_emulator.h"
+#include "L1Trigger/Phase2L1ParticleFlow/interface/dbgPrintf.h"

 #ifdef CMSSW_GIT_HASH


Just my curiosity, why do we need CMSSW_GIT_HASH to control ParameterSet header file? I search on CMSSW git, I see only L1T uses this style.

@gpetruc Can you address this question?

Because I also need to compile the same files from outside CMSSW within Vivado when compiling the IP cores for the firmware.

AdrianoDee · 2022-09-27T13:53:31Z

L1Trigger/Phase2L1ParticleFlow/interface/regionizer/multififo_regionizer_elements_ref.icc

@@ -53,10 +53,10 @@ void l1ct::multififo_regionizer::RegionBuffer<T>::initFifos(unsigned int nfifos)
    for (auto& t : queues_.back().second)
      t.clear();
  }
-  if (!(nfifos == 1 || nfifos == 2 || nfifos == 4 || nfifos == 6 || nfifos == 8 || nfifos == 12)) {
+  if (!(nfifos == 1 || nfifos == 2 || nfifos == 3 || nfifos == 4 || nfifos == 6 || nfifos == 8 || nfifos == 12)) {


Very minor. give this is used twice could be worth to define bool isGood = (nfifos == 1 || nfifos == 2 || nfifos == 3 || nfifos == 4 || nfifos == 6 || nfifos == 8 || nfifos == 12); and the use the assert as assert(isGood);.

AdrianoDee · 2022-09-27T13:56:15Z

L1Trigger/Phase2L1ParticleFlow/plugins/L1TCorrelatorLayer1Producer.cc

@@ -76,6 +77,8 @@ class L1TCorrelatorLayer1Producer : public edm::stream::EDProducer<> {
  const std::string regionDumpName_;
  bool writeRawHgcalCluster_;
  std::fstream fRegionDump_;
+  std::vector<edm::ParameterSet> patternWriterConfigs_;


AdrianoDee · 2022-09-27T14:45:09Z

L1Trigger/Phase2L1ParticleFlow/interface/L1TCorrelatorLayer1PatternFileWriter.h

+  const unsigned int tfTimeslices_ = 3, tfLinksFactor_ = 1;    // not really configurable in current architecture
+  const unsigned int hgcTimeslices_ = 3, hgcLinksFactor_ = 4;  // not really configurable in current architecture
+  const unsigned int gctTimeslices_ = 1, gctSectors_ = 3;      // not really configurable in current architecture
+  const unsigned int gctLinksEcal_ = 1, gctLinksHad_ = 2;      // could be made configurable later
+  const unsigned int gmtTimeslices_ = 3, gmtLinksFactor_ = 1;  // not really configurable in current architecture
+  const unsigned int gttTimeslices_ = 1, gttLinksFactor_ = 1;  // not really configurable in current architecture


Could be constexprs.

non-static data member cannot be 'constexpr', were you suggesting a more complex change than just switching const to constepr?

Sorry no, I had missed that, not intended to add complexity

AdrianoDee · 2022-09-27T14:45:40Z

L1Trigger/Phase2L1ParticleFlow/interface/L1TCorrelatorLayer1PatternFileWriter.h

+  enum class Partition { Barrel, HGCal, HGCalNoTk, HF };
+
+  Partition partition_;
+  const unsigned int tmuxFactor_ = 6;  // not really configurable in current architecture


Could be constexpr.

AdrianoDee · 2022-09-27T17:07:24Z

L1Trigger/Phase2L1ParticleFlow/src/L1TCorrelatorLayer1PatternFileWriter.cc

+      outputBoard_(-1),
+      outputLinkEgamma_(-1),


Do you actually want these to be always constant? If so I would move their initialization to the class header (and put them as constexpr). If not and you want to have a default value, a fillDescription may be added to the class. You can do it starting from a an existing cfi file and following this.

@gpetruc Can you comment?

No, they're not always constant, they're filled later in the constructor depending on which detector region is implemented.

A fillDescriptions could be written, but as you can see from the code below in the constructor it will end end up probably being pretty complex as the set of parameters needed depends on the values of the parameters.

AdrianoDee · 2022-09-27T17:09:16Z

L1Trigger/Phase2L1ParticleFlow/src/L1TCorrelatorLayer1PatternFileWriter.cc

+L1TCorrelatorLayer1PatternFileWriter::L1TCorrelatorLayer1PatternFileWriter(const edm::ParameterSet& iConfig,
+                                                                           const l1ct::Event& eventTemplate)
+    : partition_(parsePartition(iConfig.getParameter<std::string>("partition"))),
+      writeInputs_(iConfig.existsAs<std::string>("inputFileName") &&


These iConfig.existsAs could be avoided with a fillDescription(). I would change them adding two input parameters (such as writeInputs and writeOutputs) to actually turn these features on, rather than relying on some other parameter.

Having a lot of parameters that used conditionally you would simply need to fill them with dummy values when they are not actually used.

I do prefer to not have the parameter at all when it's not used, instead of having it filled with dummy values, so that whenever one enables something that requires those parameters but fails to specify them, the code fails cleanly with a 'missing parameter' error (instead of having to rely on the code to check whether the parameter values are dummy or not, which isn't always obvious)

Hmm, that will not wash with ConfDB: the top-level parameters of a plugin are treated fixed, and one can not have additional/different parameters depending on the value of some other parameter.
Does this ever need to run as part of the L1T emulator of phase-2 within HLT?

This class is only used to produce text files dumps of L1T data to use in board tests - something that one would never want to run in production or in HLT

(cherry picked from commit f51be45)

cmsbuild · 2022-09-28T08:03:43Z

Pull request #39397 was updated. @rekovic, @epalencia, @cmsbuild, @AdrianoDee, @srimanob, @cecilecaillol can you please check and sign again.

cmsbuild · 2022-09-28T09:48:10Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39397/32281

This PR adds an extra 24KB to repository

cmsbuild · 2022-09-28T09:48:32Z

Pull request #39397 was updated. @rekovic, @epalencia, @cmsbuild, @AdrianoDee, @srimanob, @cecilecaillol can you please check and sign again.

cecilecaillol · 2022-09-28T09:50:42Z

please test

cmsbuild · 2022-09-28T14:22:38Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-faae7c/27811/summary.html
COMMIT: b07a309
CMSSW: CMSSW_12_6_X_2022-09-27-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39397/27811/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3624368
DQMHistoTests: Total failures: 23
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3624323
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 212 log files, 49 edm output root files, 51 DQM output files
TriggerResults: no differences found

AdrianoDee · 2022-09-28T15:57:26Z

+upgrade

minor comments addressed;
would be needed for the upcoming MC production (and has been back ported to 12_5_X).

cecilecaillol · 2022-09-28T19:55:17Z

+l1

cmsbuild · 2022-09-28T19:55:38Z

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

perrotta · 2022-09-30T04:49:43Z

+1

cmsbuild added this to the CMSSW_12_6_X milestone Sep 15, 2022

cmsbuild added code-checks-pending l1-pending orp-pending pending-signatures tests-pending upgrade-pending labels Sep 15, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 15, 2022

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

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

cmsbuild added l1-approved and removed l1-pending labels Sep 15, 2022

cecilecaillol mentioned this pull request Sep 16, 2022

[12_5_X][L1T] Backport of: Phase-2, correlator developments #39417

Merged

cmsbuild added the urgent label Sep 20, 2022

cmsbuild mentioned this pull request Sep 27, 2022

Tracker Traits and Enabling Phase2 for Inner Tracker Reconstruction on GPU #38761

Merged

8 tasks

srimanob reviewed Sep 27, 2022

View reviewed changes

AdrianoDee reviewed Sep 27, 2022

View reviewed changes

ccaillol added 3 commits September 28, 2022 09:54

L1T correlator developments phase-2

277eca7

(cherry picked from commit f51be45)

fix BuildFile

c6fbf7e

fix loops and l1t module names

e342da4

ccaillol added 2 commits September 28, 2022 10:22

Merged l1t-1031v3 from repository cecilecaillol with cms-merge-topic

425b3a0

upgrade's comments

b07a309

cmsbuild added code-checks-pending and removed code-checks-approved labels Sep 28, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 28, 2022

cmsbuild added tests-started and removed tests-pending labels Sep 28, 2022

cmsbuild added tests-approved and removed tests-started labels Sep 28, 2022

cmsbuild added upgrade-approved and removed upgrade-pending labels Sep 28, 2022

cmsbuild added fully-signed l1-approved and removed l1-pending pending-signatures labels Sep 28, 2022

cmsbuild added orp-approved and removed orp-pending labels Sep 30, 2022

cmsbuild merged commit a706cc5 into cms-sw:master Sep 30, 2022

perrotta mentioned this pull request Sep 30, 2022

[L1T] phase-2, update trackjet simulation #38337

Merged

cmsbuild mentioned this pull request Sep 30, 2022

Add helper functions for alpaka kernels #39546

Merged

[L1T] Phase-2, correlator developments #39397

[L1T] Phase-2, correlator developments #39397

Conversation

cecilecaillol commented Sep 15, 2022

PR description:

PR validation:

cmsbuild commented Sep 15, 2022

cmsbuild commented Sep 15, 2022

cecilecaillol commented Sep 15, 2022

cmsbuild commented Sep 15, 2022

Comparison Summary

cecilecaillol commented Sep 15, 2022

cecilecaillol commented Sep 20, 2022

srimanob Sep 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srimanob Sep 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdrianoDee Sep 27, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

cecilecaillol commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

Comparison Summary

AdrianoDee commented Sep 28, 2022

cecilecaillol commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

perrotta commented Sep 30, 2022

srimanob Sep 27, 2022 •

edited

srimanob Sep 27, 2022 •

edited

AdrianoDee Sep 27, 2022 •

edited