RecHit-based rho producer for HLT #36157

swagata87 · 2021-11-17T18:19:11Z

PR description:

In an attempt to phase-out calotowers, this PR adds a new rho producer which is recHit-based. It takes the raw recHits of ECAL and HCAL as input and computes rho using fastjet. The new producer code is kept in RecoJets/JetProducers/plugins/ because other rho producer codes are also there. But this one is primarily aimed for HLT. According to current plan, egamma and muon HLT can use it in Run3, in order to phase out calotowers. Until Run2, calotower-based rho was used in egamma and muon HLT.

PR validation:

Ran egamma HLT after adding a module for the new rho producer. Relevant part of HLT config is this:

process.hltRhoNew = cms.EDProducer( "FixedGridRhoProducerFastjetFromRecHit",
    hbheRecHitsTag = cms.InputTag( "hltHbhereco" ),
    ebRecHitsTag = cms.InputTag( 'hltEcalRecHit','EcalRecHitsEB' ),
    eeRecHitsTag = cms.InputTag( 'hltEcalRecHit','EcalRecHitsEE' ),
    eThresHB = cms.vdouble(0.1, 0.2, 0.3, 0.3),
    eThresHE = cms.vdouble(0.1, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2),
    maxRapidity = cms.double( 2.5 ),
    gridSpacing = cms.double( 0.55 ),
    skipHCAL = cms.bool(False),
    skipECAL = cms.bool(False)
)

Then compared calotower-based rho and recHit-based rho, and found that they are fairly close.

Note that an exact match is neither expected, nor required. One reason of the small difference is different noise-cleaning cuts in calotower creation code.

Muon HLT team validated this producer independently, and found that their HLT performance remain very similar after moving to recHit-based rho. This is discussed in TSG meeting today: https://indico.cern.ch/event/1097086/contributions/4618454/attachments/2347696/4003696/20211117_TSG_MuonHLTIso_DropCaloTower_KPLee_v2.pdf

This PR is not a backport.
A backport to 12_1_X might be needed for HLT studies.

Thanks to Muon POG, in particular @KyeongPil-Lee, for validating the producer in the context of muon HLT.

With thanks to @Sam-Harper, for useful discussions.

swagata87 · 2021-11-17T18:21:22Z

assign hlt

the new producer code is primarily intended for HLT. All checks are made from HLT setup only.

cmsbuild · 2021-11-17T18:26:00Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36157/26707

This PR adds an extra 16KB to repository

cmsbuild · 2021-11-17T18:26:17Z

New categories assigned: hlt

@missirol,@Martin-Grunewald you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild · 2021-11-17T18:26:19Z

A new Pull Request was created by @swagata87 (Swagata Mukherjee) for master.

It involves the following packages:

RecoJets/JetProducers (reconstruction)

@jpata, @missirol, @cmsbuild, @Martin-Grunewald, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @jdolen, @yslai, @jdamgov, @ahinzmann, @nhanvtran, @gkasieczka, @clelange, @schoef, @mariadalfonso, @seemasharmafnal this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

missirol

A few small comments/suggestions. If any of these comments conflict with choices agreed upon in previous PRs, you can ignore them.

missirol · 2021-11-17T21:52:29Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+#include "FWCore/ParameterSet/interface/ParameterSet.h"
+#include "fastjet/tools/GridMedianBackgroundEstimator.hh"
+#include "FWCore/Framework/interface/Event.h"
+#include "DataFormats/Common/interface/View.h"


Suggested change

#include "DataFormats/Common/interface/View.h"

missirol · 2021-11-17T21:56:19Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+#include "DataFormats/Common/interface/View.h"
+#include "FWCore/Framework/interface/MakerMacros.h"
+#include "DataFormats/HcalRecHit/interface/HcalRecHitCollections.h"
+#include "TLorentzVector.h"


Suggested change

#include "TLorentzVector.h"

General comment: I was wondering if using TLorentzVector is the best choice here. I would have naively picked math::XYZTLorentzVector (used in reco::Candidate), or even just used 4 floats (px/py/pz/E), as that seems to be all that's needed below in this plugin.

missirol · 2021-11-17T21:56:33Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+#include "Geometry/CaloGeometry/interface/CaloGeometry.h"
+#include "Geometry/CaloGeometry/interface/CaloSubdetectorGeometry.h"
+#include "Geometry/Records/interface/CaloGeometryRecord.h"
+#include "DataFormats/Math/interface/Vector3D.h"


Suggested change

#include "DataFormats/Math/interface/Vector3D.h"

missirol · 2021-11-17T21:57:18Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+  descriptions.add("hltFixedGridRhoProducerFastjetFromRecHit", desc);
+}
+
+FixedGridRhoProducerFastjetFromRecHit::~FixedGridRhoProducerFastjetFromRecHit() {}


Suggested change

FixedGridRhoProducerFastjetFromRecHit::~FixedGridRhoProducerFastjetFromRecHit() {}

FixedGridRhoProducerFastjetFromRecHit::~FixedGridRhoProducerFastjetFromRecHit() = default;

missirol · 2021-11-17T21:58:40Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+  desc.add<std::vector<double> >("eThresHE", {0.1, 0.2, 0.2, 0.2, 0.2, 0.2, 0.2});
+  desc.add<double>("maxRapidity", 2.5);
+  desc.add<double>("gridSpacing", 0.55);
+  descriptions.add("hltFixedGridRhoProducerFastjetFromRecHit", desc);


Suggested change

descriptions.add("hltFixedGridRhoProducerFastjetFromRecHit", desc);

descriptions.addWithDefaultLabel(desc);

missirol · 2021-11-17T22:22:06Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+  if (!skipHCAL_) {
+    for (const auto &hit : iEvent.get(hbheRecHitsTag_)) {
+      if (passedHcalNoiseCut(hit)) {
+        TLorentzVector hitp4(0, 0, 0, 0);


See comment above on use of TLorentzVector.

missirol · 2021-11-17T22:22:29Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+    for (const auto &hit : iEvent.get(hbheRecHitsTag_)) {
+      if (passedHcalNoiseCut(hit)) {
+        TLorentzVector hitp4(0, 0, 0, 0);
+        getHitP4(hit.id(), hit.energy(), hitp4, iSetup.getData(caloGeometryToken_));


Suggested change

getHitP4(hit.id(), hit.energy(), hitp4, iSetup.getData(caloGeometryToken_));

getHitP4(hit.id(), hit.energy(), hitp4, caloGeometry);

missirol · 2021-11-17T22:22:49Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+  if (!skipECAL_) {
+    for (const auto &hit : iEvent.get(ebRecHitsTag_)) {
+      if (passedEcalNoiseCut(hit, &thresholds)) {
+        TLorentzVector hitp4(0, 0, 0, 0);


See comment above on use of TLorentzVector.

missirol · 2021-11-17T22:23:15Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+
+    for (const auto &hit : iEvent.get(eeRecHitsTag_)) {
+      if (passedEcalNoiseCut(hit, &thresholds)) {
+        TLorentzVector hitp4(0, 0, 0, 0);


See comment above on use of TLorentzVector.

missirol · 2021-11-17T22:27:40Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+  std::shared_ptr<const CaloCellGeometry> cellGeom =
+      subDetGeom != nullptr ? subDetGeom->getGeometry(detId) : std::shared_ptr<const CaloCellGeometry>();
+  if (cellGeom != nullptr) {
+    const auto &gpPos = cellGeom->repPos();


Suggested change

std::shared_ptr<const CaloCellGeometry> cellGeom =

subDetGeom != nullptr ? subDetGeom->getGeometry(detId) : std::shared_ptr<const CaloCellGeometry>();

if (cellGeom != nullptr) {

const auto &gpPos = cellGeom->repPos();

if (subDetGeom != nullptr) {

const auto &gpPos = subDetGeom->getGeometry(detId)->repPos();

This is a guess, just to highlight that maybe things could be simplified (..but maybe I'm missing something). Mainly, I didn't get why shared_ptr was needed.

cmsbuild · 2021-11-18T12:40:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36157/26722

This PR adds an extra 16KB to repository

cmsbuild · 2021-11-18T12:40:38Z

Pull request #36157 was updated. @jpata, @missirol, @cmsbuild, @Martin-Grunewald, @slava77 can you please check and sign again.

VinInn · 2021-11-19T09:39:01Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+    double thispx = thispt * cos(gpPos.phi());
+    double thispy = thispt * sin(gpPos.phi());
+    double thispz = thispt * sinh(gpPos.eta());
+    hitp4.SetPxPyPzE(thispx, thispy, thispz, hitE);


should it not be corrected for beam spot (or identified signal vertex) position?

For this rho producer, we are just aiming for a proxy of pile-up. As long as we can construct a variable whose value increases with pile-up, we are fine. So I think it's okay to not correct the recHit positions for beamspot.

I have addressed the other comments.

VinInn · 2021-11-19T09:43:25Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+      if (passedHcalNoiseCut(hit)) {
+        math::XYZTLorentzVector hitp4(0, 0, 0, 0);
+        getHitP4(hit.id(), hit.energy(), hitp4, caloGeometry);
+        inputs.push_back(fastjet::PseudoJet(hitp4.Px(), hitp4.Py(), hitp4.Pz(), hitp4.E()));


suggested change
inputs.emplace_back(hitp4.Px(), hitp4.Py(), hitp4.Pz(), hitp4.E());
similar below

there is no need of a LorentzVector or similar.
one single double hitp4[4]; suffice;

VinInn · 2021-11-19T09:47:36Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+                                                     math::XYZTLorentzVector &hitp4,
+                                                     const CaloGeometry &caloGeometry) {
+  const CaloSubdetectorGeometry *subDetGeom = caloGeometry.getSubdetectorGeometry(detId);
+  if (subDetGeom != nullptr) {


is this really something that can happen?
should not the job just stop? (and tested much earlier)?

VinInn · 2021-11-19T09:48:32Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+    double thispz = thispt * sinh(gpPos.eta());
+    hitp4.SetPxPyPzE(thispx, thispy, thispz, hitE);
+  } else {
+    if (detId.rawId() != 0)


again: is this really something that can happen?
in particular once the hit paid the noise cut!

jpata

A few additional comments inline.

jpata · 2021-11-19T14:59:28Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+
+void FixedGridRhoProducerFastjetFromRecHit::getHitP4(const DetId &detId,
+                                                     float hitE,
+                                                     math::XYZTLorentzVector &hitp4,


As suggested by Vincenzo, this function could just return a fixed-size array, rather than modify an input TLorentzVector.

jpata · 2021-11-19T15:00:15Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+  const CaloSubdetectorGeometry *subDetGeom = caloGeometry.getSubdetectorGeometry(detId);
+  if (subDetGeom != nullptr) {
+    const auto &gpPos = subDetGeom->getGeometry(detId)->repPos();
+    double thispt = hitE / cosh(gpPos.eta());


these could all be const

jpata · 2021-11-19T15:00:42Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+private:
+  void produce(edm::Event &, const edm::EventSetup &) override;
+  void getHitP4(const DetId &detId, float hitE, math::XYZTLorentzVector &hitp4, const CaloGeometry &caloGeometry);
+  bool passedHcalNoiseCut(const HBHERecHit &hit);


Suggested change

bool passedHcalNoiseCut(const HBHERecHit &hit);

bool passedHcalNoiseCut(const HBHERecHit &hit) const;

jpata · 2021-11-19T15:00:52Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+  void produce(edm::Event &, const edm::EventSetup &) override;
+  void getHitP4(const DetId &detId, float hitE, math::XYZTLorentzVector &hitp4, const CaloGeometry &caloGeometry);
+  bool passedHcalNoiseCut(const HBHERecHit &hit);
+  bool passedEcalNoiseCut(const EcalRecHit &hit, const EcalPFRecHitThresholds &thresholds);


Suggested change

bool passedEcalNoiseCut(const EcalRecHit &hit, const EcalPFRecHitThresholds &thresholds);

bool passedEcalNoiseCut(const EcalRecHit &hit, const EcalPFRecHitThresholds &thresholds) const;

jpata · 2021-11-19T15:02:17Z

RecoJets/JetProducers/plugins/FixedGridRhoProducerFastjetFromRecHit.cc

+
+private:
+  void produce(edm::Event &, const edm::EventSetup &) override;
+  void getHitP4(const DetId &detId, float hitE, math::XYZTLorentzVector &hitp4, const CaloGeometry &caloGeometry);


Suggested change

void getHitP4(const DetId &detId, float hitE, math::XYZTLorentzVector &hitp4, const CaloGeometry &caloGeometry);

std::array<double, 4> getHitP4(const DetId &detId, float hitE, const CaloGeometry &caloGeometry) const;

(or float, in case everything is already float, I didn't check)

cmsbuild · 2021-11-23T21:17:21Z

Pull request #36157 was updated. @jpata, @missirol, @cmsbuild, @Martin-Grunewald, @slava77 can you please check and sign again.

missirol · 2021-11-23T21:21:21Z

please test

cmsbuild · 2021-11-24T00:23:39Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5d7a34/20721/summary.html
COMMIT: 3d014b7
CMSSW: CMSSW_12_2_X_2021-11-23-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36157/20721/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: 42
DQMHistoTests: Total histograms compared: 3247025
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3247003
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

missirol · 2021-11-24T10:08:06Z

+hlt

Reviewed on demand; aside from the intended use case for this plugin, there is nothing in its implementation which is explicitly specific to HLT (no dependence on HLT packages).
There is a certain level of assumption in some methods concerning the validity of pointers, and length of arrays/vectors [1] [2]; I can't really judge this, so it is left to the review of RECO experts.

jpata · 2021-11-24T12:45:43Z

I can't really judge this, so it is left to the review of RECO experts.

Since this code is not running in a test, we can only perform a visual inspection, which may not be very helpful. Was this PR actually executed in an HLT setup to test that it runs?

missirol · 2021-11-24T13:22:21Z

Since this code is not running in a test, we can only perform a visual inspection, which may not be very helpful.

I agree; I was hoping that people more familiar than me with some of the methods in this PR could tell a priori that certain calls (valid of pointers, etc) are always valid for the use case at hand; I understand it might not be obvious to guarantee that. I'm just trying to exclude issues like the one in #35488.

Was this PR actually executed in a HLT setup to test that it runs?

Swagata can answer this, of course; my understanding is yes (and this was used for some results presented in TSG). I don't know which tests were done with the latest version of the plugin (post-PR review).

jpata · 2021-11-24T13:25:00Z

Right. I don't know either. I think it would be good to run a new test on the code as it is currently in the PR.

swagata87 · 2021-11-24T21:04:17Z

Was this PR actually executed in an HLT setup to test that it runs?

yes, before making this PR the producer was run independently in Egamma HLT and muon HLT.

I think it would be good to run a new test on the code as it is currently in the PR.

indeed, as several parts were changed it makes sense to re-run and make sure things are still okay.

Egamma HLT was run using the latest version of the code. Calotower-based rho and recHit-based rho was compared. Here are the plots. Entries of histogram indicates number of events on which HLT was run.

QCD_Pt-30To50_EMEnriched
TTbar
WJetsToLNu

swagata87 · 2021-11-25T16:01:04Z

Muon POG has now made rate & timing studies using the recHit based rho producer.
For the timing study they used updated version of the code from this PR, commit 503d8b4. This is not the latest commit, but the only change made after this was adding some packages in BuildFile. So I guess this might resolve the concern related to validity of pointers.
here is slides from @KyeongPil-Lee: https://swmukher.web.cern.ch/swmukher/20211124_MuonHLTIsoStudy_rechitBasedRho_KPLee_v1.pdf

missirol · 2021-11-26T10:29:29Z

So I guess this might resolve the concern related to validity of pointers.

Thanks, Swagata. I don't have any further comments.

jpata · 2021-11-26T12:19:27Z

+reconstruction

adds a new FixedGridRhoProducerFastjetFromRecHit that is not enabled by default, intended for HLT
no reco changes (the new code is not running in tests)
according to private tests reported above, the code works as intended

cmsbuild · 2021-11-26T12:19:53Z

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

perrotta · 2021-11-26T21:44:16Z

+1

swagata87 added 3 commits November 17, 2021 09:36

new rho producer for HLT

9710cd0

code cleanup

0a95f54

added supporting text

da552c9

cmsbuild added this to the CMSSW_12_2_X milestone Nov 17, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Nov 17, 2021

cmsbuild added the hlt-pending label Nov 17, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 17, 2021

missirol reviewed Nov 17, 2021

View reviewed changes

addressed review comments

e4ae3d2

cmsbuild added code-checks-pending and removed code-checks-approved labels Nov 18, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 18, 2021

VinInn reviewed Nov 19, 2021

View reviewed changes

jpata reviewed Nov 19, 2021

View reviewed changes

Addressed comments from Vincenzo and Joosep

73db4a6

cmsbuild added the code-checks-approved label Nov 23, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 23, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 24, 2021

cmsbuild added hlt-approved and removed hlt-pending labels Nov 24, 2021

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Nov 26, 2021

cmsbuild added orp-approved and removed orp-pending labels Nov 26, 2021

cmsbuild merged commit a41acbb into cms-sw:master Nov 26, 2021

swagata87 mentioned this pull request Jan 14, 2023

[EGM HLT@Phase2] Phase out caloTowers #40525

Merged

swagata87 mentioned this pull request Nov 3, 2023

Migrate RecHit-based rho producer to use HCal noise cuts from GT #43176

Merged

swagata87 mentioned this pull request Jan 3, 2024

Calotower phase-out from PF code #43612

Merged

	FixedGridRhoProducerFastjetFromRecHit::~FixedGridRhoProducerFastjetFromRecHit() {}
	FixedGridRhoProducerFastjetFromRecHit::~FixedGridRhoProducerFastjetFromRecHit() = default;

	descriptions.add("hltFixedGridRhoProducerFastjetFromRecHit", desc);
	descriptions.addWithDefaultLabel(desc);

	getHitP4(hit.id(), hit.energy(), hitp4, iSetup.getData(caloGeometryToken_));
	getHitP4(hit.id(), hit.energy(), hitp4, caloGeometry);

	bool passedHcalNoiseCut(const HBHERecHit &hit);
	bool passedHcalNoiseCut(const HBHERecHit &hit) const;

	bool passedEcalNoiseCut(const EcalRecHit &hit, const EcalPFRecHitThresholds &thresholds);
	bool passedEcalNoiseCut(const EcalRecHit &hit, const EcalPFRecHitThresholds &thresholds) const;

	void getHitP4(const DetId &detId, float hitE, math::XYZTLorentzVector &hitp4, const CaloGeometry &caloGeometry);
	std::array<double, 4> getHitP4(const DetId &detId, float hitE, const CaloGeometry &caloGeometry) const;

RecHit-based rho producer for HLT #36157

RecHit-based rho producer for HLT #36157

Conversation

swagata87 commented Nov 17, 2021

PR description:

PR validation:

swagata87 commented Nov 17, 2021

cmsbuild commented Nov 17, 2021

cmsbuild commented Nov 17, 2021

cmsbuild commented Nov 17, 2021

missirol left a 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 18, 2021

cmsbuild commented Nov 18, 2021

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

VinInn Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

jpata left a 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 23, 2021

missirol commented Nov 23, 2021

cmsbuild commented Nov 24, 2021

Comparison Summary

missirol commented Nov 24, 2021

jpata commented Nov 24, 2021 • edited Loading

missirol commented Nov 24, 2021

jpata commented Nov 24, 2021

swagata87 commented Nov 24, 2021

swagata87 commented Nov 25, 2021

missirol commented Nov 26, 2021

jpata commented Nov 26, 2021

cmsbuild commented Nov 26, 2021

perrotta commented Nov 26, 2021

VinInn Nov 19, 2021 •

edited

Loading

jpata commented Nov 24, 2021 •

edited

Loading