Split HGCal layer cluster producer #41589

OlivieFranklova · 2023-05-08T13:30:07Z

PR description:

The whole purpose of this pull request was to change the workflow for clustering. We split clustering for the silicon and scintillator parts. Firstly, layer clusters are created for all three types of recHits (EE, HSci, HSi). For the Silicon types, the Silicon specialization of CLUE is used, while for the Scintillator, the Scintillator specialization of CLUE is used. Then, all three clusters that were created are merged together. Because of this change, lots of other changes were made. All changes are described below.

BH was renamed to HSci

FH was renamed to HSi

Clustering

HGCalClusteringAlgoBase

Method getEventSetup was changed in HGCalClusteringAlgoBase.h. New implementation accept RecHitTools with setted Geometry, so the geometry is not set in getEventSetup but must be set before calling this method.
Thanks to that we removed edm::ConsumesCollector iC from constructor because it was used only for setting geometry. This was removed also from all derived classes.

HGCalCLUEAlgo

We rewrite CLUEAlgo that it can be specify by template if it is silicon or scintillator case. New CLUEAlgo is templated by tile and strategy. Strategy is new class which determines how should algorithm behaves. We have two strategy types: HGCalSiliconStrategy, HGCalScintillatorStrategy

Method distance2 was removed from CLUEAlgo and now is counted in tiles.
The CellsOnLayer structure was changed to use only dim1 and dim2 vector instead of x, y, eta, phi, isSi, with isSi being unnecessary due to specialization by strategy.
New specializations of calculateLocalDensity was added - one for silicon and one for scintillator.
Density was removed from CLUEAlgo and according to it from other files

populate

According to strategy method determine if will fill dim1 and dim2 vector by x and y or eta and phi.

makeClusters

The method decides which delta to use based on the strategy.

getClusters

Position is counted in HGCalLayerClusterProducer so in the method getClusters position is set to math::XYZPoint(0.f, 0.f, 0.f);.

calculateLocalDensity

Original function only decide by strategy which specification of calculateLocalDensity should call
Implementation was splitted to silicon one and scintillator one. Each of the implementation has their own function.

calculateDistanceToHigher

The method decides which bindId to use based on the strategy.
thanks to changes in tiles this method could be rewrite so the implementation is basically the same for both silicon and scintillator strategy.

now we can use HGCalSiCLUEAlgo, HGCalSciCLUEAlgo and HFNoseCLUEAlgo

Layer Cluster Algo Factory

New types of CLUE settings were added to HGCalLayerClusterPluginFactory.cc one for Silicon and one for Scintillator

DEFINE_EDM_VALIDATED_PLUGIN(HGCalLayerClusterAlgoFactory,HGCalSiCLUEAlgo, "SiCLUE");
DEFINE_EDM_VALIDATED_PLUGIN(HGCalLayerClusterAlgoFactory, HGCalSciCLUEAlgo, "SciCLUE");

Layer Tiles

Layer Tiles were split to Silicon and Scintillator one. So changes were made in *Constants.h files. And we created new structures PhiWrapper and NoPhiWrapper which can determine if the type is silicon or scinitillator.

Changes in constants

HGCalTilesConstants was splitted to HGCalSiliconTilesConstants and HGCalScintillatorTilesConstants
X and Eta was renamed to Dim1 -> now we set minDim1 and maxDim1
Y and Phi was renamed to Dim2 -> now we set minDim2 and maxDim2
Only one tileSize, nColumns and nRows is in the constants
HFNoseTilesConstants is now treated like silicon

HGCalLayerTiles.h

HGCalLayerTiles is now templated by TilesConstants and by Wrapper - PhiWrapper for HGCalScintillatorTilesConstants and NoPhiWrapper for both HGCalSiliconTilesConstants and HFNoseTilesConstants
Method fill accepts only Dim1 and Dim2 vector
New method getDim1Bin is now used instead of getXBin and getEtaBin.
New method getDim2Bin is now used instead of getYBin and getPhiBin. This method uses wrapping.

Distance is now counted in HGCalLayerTiles and not in CLUEAlgo

Method searchBox uses wrapping so searchBoxEtaPhi was removed.
Methods getGlobalBinEtaPhi and getGlobalBinByBinEtaPhi were removed.

now we can use HGCalSiliconLayerTiles, HGCalScintillatorLayerTiles and HFNoseLayerTiles

Layer Cluster Producer

Creating Layer Clusters is now made by creating layerClusters specialized for each collection of recHits (HGCEERecHits, HGCHEFRecHits, HGCHEBRecHits) and then all three layerCLusters are merged together. This change was created because now we can specialize implementatation by silicon or scintillator and we can reduce if-else branching.

HGCalLayerClusterProducer

New implementation of this producer can consume only one collection of recHits.
Algorithm for clustering is created by HGCalLayerClusterAlgoFactory: HFNoseCLUE for HGCHFNoseRecHits collection, SiCLUE for HGCEERecHits and HGCHEFRecHits, SciCLUE for HGCHEBRecHits.

Produce

Method Produce sets geometry for RecHitsTools and than calls algos getEventSetup, populate, makeClusters, getClusters.

In this new implementation position for clusters is counted in HGCalLayerClusterProducer and not in CLUEAlgo
Method Produce then counts position and time.
LayerClusterMask is counted only for HFNose because for other types of recHits is counted in MergeClusterProducer

calculatePosition

Position is counted from hitsAndFraction from clusters. For counting position is used Geometry (to get specific position) which could cause slowlynes, this will be remove in the future.

MergeClusterProducer

This new producer consumes hgcalLayerClusters and timeLayerClusters for 3 types of RecHits EE, HSi and HSci that were made by hgcalLayerClusterProducer.
The producer merge layerClusters and put them to event.
Then producer creates layerClusterMask and put it into event.
Then timeLayerCLusters are merged with correct id and put it into event.
Output of MergeClusterProducer is same as output of old usage of LayerClusterProducer.

Change in configuration files

In old implementation only hgcalLayerCluster was called in the new one we firs want to call hgcalLayerCluster for all 3 types of recHits and then we want to call mergeLayerClusters to merge them together.

hgcalLocalRecoTask = cms.Task(
  HGCalRecHit,
  HGCalUncalibRecHit,
  hgcalLayerClustersEE,
  hgcalLayerClustersHSi,
  hgcalLayerClustersHSci,
  hgcalMergeLayerClusters,
  particleFlowClusterHGCal,
  particleFlowRecHitHGC
)

changes are made in these files:
- HLTrigger/.../hgcalLocalRecoTask_cfi.py
- HLTrigger/.../HLTHgcalTiclPFClusteringForEgammaUnseededTask_cfi.py
- HLTrigger/.../HLT_IsoMu24_FromL1TkMuon_cfi.py,

Change in hgcalLayerClusters_cff.py

We create 3 types of hgcalLayerClusters one for each type of recHits.
For each type we need to set specific detector, recHits and type of CLUEAlgo SiCLUE or SciCLUE

Filtering

Filtering by algo_number was changed to filtering by a vector of algo_numbers. This change was made due to changing the setting of algo_id. In the previous implementation, algo_id was always set in the HGCalLayerCLusterProducer for hgcal_mixed because we ran the producer for HSi, HSci, and EE types of recHits at one time. In the new implementation, we run the HGCalLayerClusterProducer once for each type of recHits, and we set algo_id accordingly: hgcal_had for HSci and Hsi, and hgcal_em for EE. Then, the results are merged by the MergeClusterProducer. We want to have the same behavior as the previous implementation, so we want to make filtering by both hgcal_had and hgcal_em.

Changes in filtering are in these files: RecoHGCal/.../ClusterFilterByAlgo.h, RecoHGCal/.../ClusterFilterByAlgoAndSize.h, RecoHGCal/.../ClusterFilterByAlgoAndSizeAndLayerRange.h
The default option is changed from number 9 - hfnose to a vector of hgcal_had and hgcal_em. The change is made in RecoHGCal/.../FilteredLayerClusterProducer.cc.
Thanks to changing the default option, we can remove setting algo_number to 8 - hgcal_mixed (now to 7 and 6 - hgcal_had, hgcal_em) in configuration files for filtering.
We also remove hgcal_mixed from CaloCluster.h.
HgcalClusterFilterMask was renamed to TICLClusterFilterMask.

PR validation:

Pull request was tased by scram b runtests and runTheMatrix.py -l limited -i all --ibeos. Test EtaPhiSearchInTile_t.cpp was changed according to changes in HGCalLayerTiles. We also used a couple of other workflows to test whether the clusters remained unchanged (23224, 25234, 25294,23834)

cmsbuild · 2023-05-08T13:39:10Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41589/35468

This PR adds an extra 352KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Validation/HGCalValidation/src/HGVHistoProducerAlgo.cc modified in PR(s): [DQM] [CLANG] Fix warnings reported by llvm16 in CLANG IBs #41588

cmsbuild · 2023-05-08T13:39:47Z

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

It involves the following packages:

DataFormats/CaloRecHit (reconstruction)
DataFormats/HGCalReco (upgrade, reconstruction)
Fireworks/Calo (visualization)
HLTrigger/Configuration (hlt)
RecoHGCal/TICL (upgrade, reconstruction)
RecoLocalCalo/Configuration (reconstruction)
RecoLocalCalo/HGCalRecProducers (upgrade, reconstruction)
RecoParticleFlow/PFClusterProducer (reconstruction)
SimCalorimetry/HGCalAssociatorProducers (upgrade, simulation)
Validation/HGCalValidation (dqm)

@alja, @civanch, @Dr15Jones, @missirol, @makortel, @micsucmed, @emanueleusai, @Martin-Grunewald, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob, @clacaputo, @syuvivida, @pmandrik, @mandrenguyen, @rvenditti can you please review it and eventually sign? Thanks.
@felicepantaleo, @forthommel, @Martin-Grunewald, @bsunanda, @pfs, @thomreis, @seemasharmafnal, @youyingli, @mmarionncern, @sethzenz, @missirol, @silviodonato, @SohamBhattacharya, @lgray, @apsallid, @beaucero, @vandreev11, @rovere, @cseez, @hatakeyamak, @ebrondol, @edjtscott, @alja, @sobhatta, @lecriste, @wang0jin, @sameasy 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

SohamBhattacharya · 2023-05-22T12:24:40Z

Do we need this PR in 13_1 for the next Phase-2 MC campaign?

Ciao @srimanob good question. I believe we can re-run HLT on top of the centrally produced samples w/o the need of having this PR backported. This PR is preparatory to bring in the possibility of running the clustering step on GPU. This will come with another PR in the next few months. This will make the possible backport of this PR even less useful, IMHO.

But I let @SohamBhattacharya and @beaucero comment further.

Hi, if we can re-HLT on 13_1 samples w/o backporting this PR, that should be fine.

HLTrigger/Configuration/python/HLT_75e33/modules/hgcalLayerClustersEEL1Seeded_cfi.py

civanch · 2023-05-29T09:36:17Z

+1

mandrenguyen · 2023-06-01T05:00:48Z

+reconstruction

cmsbuild · 2023-06-01T05:01:30Z

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 · 2023-06-01T06:55:28Z

@cmsbuild please test
(lates test results have expired by now)

cmsbuild · 2023-06-01T10:58:18Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dca5ff/32919/summary.html
COMMIT: 651e4d6
CMSSW: CMSSW_13_2_X_2023-05-31-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41589/32919/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 14 lines from the logs
ROOTFileChecks: Some differences in event products or their sizes found
Reco comparison results: 177 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3186957
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3186932
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 168.456 KiB( 47 files compared)
DQMHistoSizes: changed ( 23234.0,... ): 28.076 KiB HGCAL/HGCalValidator
Checked 207 log files, 159 edm output root files, 48 DQM output files
TriggerResults: found differences in 1 / 46 workflows

rappoccio · 2023-06-01T13:29:20Z

+1

perrotta

A few comments that you can take into account for a possible cleanup of the code

perrotta · 2023-06-01T10:24:36Z

RecoLocalCalo/HGCalRecProducers/plugins/HGCalCLUEAlgo.cc

+      float delta;
+
+      if constexpr (std::is_same_v<STRATEGY, HGCalSiliconStrategy>)
+        delta = delta_c;


delta_c could be more conveniently computed locally within the scope of the if

perrotta · 2023-06-01T10:24:50Z

RecoLocalCalo/HGCalRecProducers/plugins/HGCalCLUEAlgo.cc

+      if constexpr (std::is_same_v<STRATEGY, HGCalSiliconStrategy>)
+        delta = delta_c;
+      else
+        delta = delta_r;


delta_r could be more conveniently computed locally within the scope of the if

perrotta · 2023-06-01T10:34:10Z

RecoLocalCalo/HGCalRecProducers/plugins/MergeClusterProducer.cc

+#include "DataFormats/Common/interface/OrphanHandle.h"
+
+#include "FWCore/Framework/interface/Event.h"
+#include "FWCore/Framework/interface/ESHandle.h"


Suggested change

#include "FWCore/Framework/interface/ESHandle.h"

This does not seem to be needed (maybe there are a few more include that could be also removed)

perrotta · 2023-06-01T10:46:57Z