Adapt Alpaka-based PF Clustering to read Hcal thresholds from GT #43574

jsamudio · 2023-12-14T21:36:21Z

PR description:

This is a follow up to #43130. It is the adaptation of the Alpaka-based PFRecHitSoAProducer and PFClusterSoAProducer to read Hcal thresholds from GT similar to #43025. Included are some fixes to naming conventions that were missed in the initial implementation of #43130.

PR validation:

PR was validated using local testing of cluster-level comparison and PF Jet Response comparison as in #43130. The results are consistent between Alpaka and Legacy for the entire variety of configurations: Alpaka CPU and GPU backends with usePFThresholdsFromDB set to true or false.

The basic suite of tests, runTheMatrix.py -l limited -i all --ibeos, has also been passed.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

PR is not a backport.

@hatakeyamak @swagata87

cmsbuild · 2023-12-14T21:36:44Z

cms-bot internal usage

cmsbuild · 2023-12-14T21:43:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43574/38207

This PR adds an extra 52KB to repository

cmsbuild · 2023-12-14T21:43:31Z

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

It involves the following packages:

DataFormats/ParticleFlowReco (reconstruction)
RecoParticleFlow/PFClusterProducer (reconstruction)
RecoParticleFlow/PFRecHitProducer (reconstruction)

@cmsbuild, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks.
@felicepantaleo, @seemasharmafnal, @missirol, @ebrondol, @lgray, @mmarionncern, @sameasy, @rovere, @hatakeyamak this is something you requested to watch as well.
@sextonkennedy, @rappoccio, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

swagata87 · 2023-12-15T04:35:45Z

type pf

jfernan2 · 2023-12-15T12:01:12Z

assign heterogeneous

cmsbuild · 2023-12-15T12:01:37Z

New categories assigned: heterogeneous

@fwyzard,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

fwyzard · 2023-12-15T12:21:27Z

DataFormats/ParticleFlowReco/interface/PFRecHitSoA.h

@@ -14,6 +14,7 @@ namespace reco {
  using PFRecHitsNeighbours = Eigen::Matrix<int32_t, 8, 1>;
  GENERATE_SOA_LAYOUT(PFRecHitSoALayout,
                      SOA_COLUMN(uint32_t, detId),
+                      SOA_COLUMN(uint32_t, denseId),


I see where this is stored (PFRecHitProducerKernel.dev.cc lines 100 and 118), but where is it used ?

Used to set the threshold for the PFRecHit later in PFClusterSoAProducerKernel.dev.cc.

fwyzard · 2023-12-15T12:33:09Z

RecoParticleFlow/PFClusterProducer/interface/PFClusterParamsSoA.h

-                      SOA_SCALAR(float, seedPt2ThresholdEB),
-                      SOA_SCALAR(float, seedPt2ThresholdEE),
-                      SOA_COLUMN(float, seedEThresholdEB_vec),
-                      SOA_COLUMN(float, seedEThresholdEE_vec),
-                      SOA_COLUMN(float, topoEThresholdEB_vec),
-                      SOA_COLUMN(float, topoEThresholdEE_vec),
+                      SOA_SCALAR(float, seedPt2ThresholdHB),
+                      SOA_SCALAR(float, seedPt2ThresholdHE),
+                      SOA_COLUMN(float, seedEThresholdHB_vec),
+                      SOA_COLUMN(float, seedEThresholdHE_vec),
+                      SOA_COLUMN(float, topoEThresholdHB_vec),
+                      SOA_COLUMN(float, topoEThresholdHE_vec),


are these variable specific to HCAL ?

fwyzard · 2023-12-15T12:33:53Z

RecoParticleFlow/PFClusterProducer/plugins/LegacyPFClusterProducer.cc

@@ -65,6 +70,7 @@ class LegacyPFClusterProducer : public edm::stream::EDProducer<> {
    desc.add<edm::InputTag>("PFRecHitsLabelIn");
    desc.add<edm::ESInputTag>("pfClusterParams");
    desc.add<edm::InputTag>("recHitsSource");
+    desc.add<bool>("usePFThresholdsFromDB", "True");


shouldn't this be

Suggested change

desc.add<bool>("usePFThresholdsFromDB", "True");

desc.add<bool>("usePFThresholdsFromDB", true);

?

fwyzard · 2023-12-15T12:34:52Z

RecoParticleFlow/PFClusterProducer/plugins/LegacyPFClusterProducer.cc

@@ -180,12 +186,18 @@ class LegacyPFClusterProducer : public edm::stream::EDProducer<> {
  const edm::ESGetToken<reco::PFClusterParamsHostCollection, JobConfigurationGPURecord> pfClusParamsToken_;
  const edm::EDPutTokenT<reco::PFClusterCollection> legacyPfClustersToken_;
  const edm::EDGetTokenT<reco::PFRecHitCollection> recHitsLabel_;
+  const edm::ESGetToken<HcalPFCuts, HcalPFCutsRcd> hcalCutsToken_;
+  bool cutsFromDB;


Suggested change

bool cutsFromDB;

const bool cutsFromDB_;

fwyzard · 2023-12-15T12:35:53Z

RecoParticleFlow/PFClusterProducer/plugins/LegacyPFClusterProducer.cc

+        hcalCutsToken_(esConsumes<HcalPFCuts, HcalPFCutsRcd>(edm::ESInputTag("", "withTopo"))) {
+    cutsFromDB = config.getParameter<bool>("usePFThresholdsFromDB");


Suggested change

hcalCutsToken_(esConsumes<HcalPFCuts, HcalPFCutsRcd>(edm::ESInputTag("", "withTopo"))) {

cutsFromDB = config.getParameter<bool>("usePFThresholdsFromDB");

hcalCutsToken_(esConsumes<HcalPFCuts, HcalPFCutsRcd>(edm::ESInputTag("", "withTopo"))),

cutsFromDB_(config.getParameter<bool>("usePFThresholdsFromDB")) {

fwyzard · 2023-12-15T12:37:49Z

RecoParticleFlow/PFClusterProducer/plugins/LegacyPFClusterProducer.cc

+  if (cutsFromDB) {
+    paramPF = &setup.getData(hcalCutsToken_);
+  }


This can be a local variable, it doesn't need to be a member variable:

Suggested change

if (cutsFromDB) {

paramPF = &setup.getData(hcalCutsToken_);

}

HcalPFCuts const* paramPF = cutsFromDB_ ? &setup.getData(hcalCutsToken_) : nullptr;

mmusich · 2023-12-15T12:39:20Z

RecoParticleFlow/PFRecHitProducer/plugins/alpaka/PFRecHitTopologyESProducer.cc

+      if constexpr (std::is_same_v<CAL, HCAL>)
+        desc.add<bool>("usePFThresholdsFromDB", "True");
+      else  // only needs to be true for HBHE
+        desc.add<bool>("usePFThresholdsFromDB", "False");


Suggested change

if constexpr (std::is_same_v<CAL, HCAL>)

desc.add<bool>("usePFThresholdsFromDB", "True");

else // only needs to be true for HBHE

desc.add<bool>("usePFThresholdsFromDB", "False");

if constexpr (std::is_same_v<CAL, HCAL>)

desc.add<bool>("usePFThresholdsFromDB", true);

else // only needs to be true for HBHE

desc.add<bool>("usePFThresholdsFromDB", false);

fwyzard · 2023-12-15T12:39:55Z

RecoParticleFlow/PFClusterProducer/plugins/alpaka/PFClusterSoAProducerKernel.dev.cc

-        rhENormInv = 0.;
-        printf("Rechit %d has invalid layer %d!\n", i, pfRecHits[i].layer());
+      if (topology.cutsFromDB()) {
+        rhENormInv = (1. / topology[pfRecHits[i].denseId()].noiseThreshold());


Suggested change

rhENormInv = (1. / topology[pfRecHits[i].denseId()].noiseThreshold());

rhENormInv = (1.f / topology[pfRecHits[i].denseId()].noiseThreshold());

should avoid to do the division in double precision.

Can you check if the results are OK ?

Validation looks okay when I make this change.

fwyzard · 2023-12-15T12:40:22Z

RecoParticleFlow/PFClusterProducer/plugins/alpaka/PFClusterSoAProducerKernel.dev.cc

-        printf("Rechit %d has invalid layer %d!\n", i, pfRecHits[i].layer());
+
+      if (topology.cutsFromDB()) {
+        rhENormInv = (1. / topology[pfRecHits[i].denseId()].noiseThreshold());


Suggested change

rhENormInv = (1. / topology[pfRecHits[i].denseId()].noiseThreshold());

rhENormInv = (1.f / topology[pfRecHits[i].denseId()].noiseThreshold());

fwyzard · 2023-12-15T12:46:34Z

From the @cms-sw/heterogeneous-l2 point of view, the only comment I have is about using 1.f / ... instead of 1. / ..., to avoid doing the division in double precisions - unless the validation shows that it is necessary to do so.

The others are general C++ and CMSSW comments.

cmsbuild · 2023-12-15T19:55:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43574/38226

This PR adds an extra 52KB to repository

cmsbuild · 2023-12-15T19:55:59Z

Pull request #43574 was updated. @makortel, @cmsbuild, @fwyzard, @jfernan2, @mandrenguyen can you please check and sign again.

fwyzard · 2023-12-16T11:55:30Z

+heterogeneous

fwyzard · 2023-12-16T11:55:39Z

please test

cmsbuild · 2023-12-16T14:47:01Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f64c79/36530/summary.html
COMMIT: a24b7e4
CMSSW: CMSSW_14_0_X_2023-12-16-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/43574/36530/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 22 lines from the logs
Reco comparison results: 9 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3429858
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3429830
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

jfernan2 · 2023-12-18T08:44:42Z

+1

cmsbuild · 2023-12-18T08:45:08Z

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

antoniovilela · 2023-12-19T15:39:57Z

+1

jsamudio added 3 commits December 13, 2023 13:22

First implementation to use DB thresholds for PFRecHitProducer

8e69475

Use PF thresholds from DB in PFClusterSoAProducer

2ebe42e

Update configuration to run alpaka-based PF clustering

5813b58

cmsbuild added this to the CMSSW_14_0_X milestone Dec 14, 2023

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending code-checks-pending labels Dec 14, 2023

jsamudio mentioned this pull request Dec 14, 2023

Follow up to the Alpaka Implementation of PFClusterProducer #43501

Open

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 14, 2023

cmsbuild added the pf label Dec 15, 2023

swagata87 mentioned this pull request Dec 15, 2023

Overview of implementations to consume HCalPFCuts from DB #43312

Open

cmsbuild added the heterogeneous-pending label Dec 15, 2023

fwyzard reviewed Dec 15, 2023

View reviewed changes

mmusich reviewed Dec 15, 2023

View reviewed changes

fwyzard reviewed Dec 15, 2023

View reviewed changes

Implement suggestions from PR review

a24b7e4

cmsbuild added code-checks-pending and removed code-checks-approved labels Dec 15, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 15, 2023

cmsbuild added tests-started heterogeneous-approved and removed tests-pending heterogeneous-pending labels Dec 16, 2023

cmsbuild added tests-approved and removed tests-started labels Dec 16, 2023

cmsbuild added reconstruction-approved fully-signed and removed reconstruction-pending pending-signatures labels Dec 18, 2023

cmsbuild added orp-approved and removed orp-pending labels Dec 19, 2023

cmsbuild merged commit a04ade0 into cms-sw:master Dec 19, 2023
11 checks passed

This was referenced Dec 19, 2023

[14.0.X] Updated root to tip of branch v6-30-00-patches cms-sw/cmsdist#8896

Closed

Materialize terror symbols cms-sw/root#191

Closed

First PyTorch tests for TorchScript inference CPU/CUDA #43475

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt Alpaka-based PF Clustering to read Hcal thresholds from GT #43574

Adapt Alpaka-based PF Clustering to read Hcal thresholds from GT #43574

jsamudio commented Dec 14, 2023

cmsbuild commented Dec 14, 2023 •

edited

Loading

cmsbuild commented Dec 14, 2023

cmsbuild commented Dec 14, 2023

swagata87 commented Dec 15, 2023

jfernan2 commented Dec 15, 2023

cmsbuild commented Dec 15, 2023

fwyzard Dec 15, 2023

jsamudio Dec 15, 2023

fwyzard Dec 15, 2023

jsamudio Dec 15, 2023

fwyzard Dec 15, 2023

fwyzard Dec 15, 2023

fwyzard Dec 15, 2023

fwyzard Dec 15, 2023

mmusich Dec 15, 2023

fwyzard Dec 15, 2023

jsamudio Dec 15, 2023

fwyzard Dec 15, 2023

fwyzard commented Dec 15, 2023

cmsbuild commented Dec 15, 2023

cmsbuild commented Dec 15, 2023

fwyzard commented Dec 16, 2023

fwyzard commented Dec 16, 2023

cmsbuild commented Dec 16, 2023

jfernan2 commented Dec 18, 2023

cmsbuild commented Dec 18, 2023

antoniovilela commented Dec 19, 2023

	desc.add<bool>("usePFThresholdsFromDB", "True");
	desc.add<bool>("usePFThresholdsFromDB", true);

		hcalCutsToken_(esConsumes<HcalPFCuts, HcalPFCutsRcd>(edm::ESInputTag("", "withTopo"))) {
		cutsFromDB = config.getParameter<bool>("usePFThresholdsFromDB");

	rhENormInv = (1. / topology[pfRecHits[i].denseId()].noiseThreshold());
	rhENormInv = (1.f / topology[pfRecHits[i].denseId()].noiseThreshold());

Adapt Alpaka-based PF Clustering to read Hcal thresholds from GT #43574

Adapt Alpaka-based PF Clustering to read Hcal thresholds from GT #43574

Conversation

jsamudio commented Dec 14, 2023

PR description:

PR validation:

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

cmsbuild commented Dec 14, 2023 • edited Loading

cmsbuild commented Dec 14, 2023

cmsbuild commented Dec 14, 2023

swagata87 commented Dec 15, 2023

jfernan2 commented Dec 15, 2023

cmsbuild commented Dec 15, 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

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

fwyzard commented Dec 15, 2023

cmsbuild commented Dec 15, 2023

cmsbuild commented Dec 15, 2023

fwyzard commented Dec 16, 2023

fwyzard commented Dec 16, 2023

cmsbuild commented Dec 16, 2023

Comparison Summary

jfernan2 commented Dec 18, 2023

cmsbuild commented Dec 18, 2023

antoniovilela commented Dec 19, 2023

cmsbuild commented Dec 14, 2023 •

edited

Loading