Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E/gamma HLT L1 Track Isolation Producer & TTTrack typedef: 11_3_X #32594

Merged
merged 5 commits into from Jan 14, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions DataFormats/L1TrackTrigger/interface/L1Track.h
@@ -0,0 +1,6 @@
#include "DataFormats/L1TrackTrigger/interface/TTTrack.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the include guard omitted on purpose? Other headers in this space seem to include it and I can't think of a good reason to not have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that was an accident, thanks

#include "DataFormats/L1TrackTrigger/interface/TTTypes.h"
#include <vector>

using L1Track = TTTrack<Ref_Phase2TrackerDigi_>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was pointed out by Slava that this name is generic enough and might be more safer in a namespace, to avoid potential future symbol conficts. This is not strictly a reco file, so I let someone else give their suggestion as to what this could be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure on this, I honestly have no preference, perhaps @rekovic could comment on what the L1 prefers here?

For what its worth on this point, I agree. I would have stuck it in whatever namespace TTrack is in but its in none.

Copy link
Contributor

@slava77 slava77 Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC there is an l1t namespace.
Perhaps we can place it there?
The full name l1t::L1Track is a bit redundant though.

BTW, are there any other cases of TTTrack instances? How would those be named with a typedef in a similar pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont believe there are any other such instances after having a brief but not exhaustive look. I'm fine with l1t::L1Track, we could also have l1t::TTTrack but given most code using a TTTrack is likely in the l1t namespace, its probably a bad idea. Last option would be l1t::Phase2Track which is a bit longer but also acceptable to me. I really have little preference here so if people have strong feelings I'm happy to change it to whatever.

I'll give it a little while to see if Vladimir comments.

Copy link
Contributor

@slava77 slava77 Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should clarify one point: my main concern in the earlier discussion with @jpata was a possible conflict in compiled symbols. With a bit more thought, a typedef is just a synonym (it does not define a new type or compiled symbol) and a conflict if any will be at compile time and will be easily manageable.
So, perhaps there is no problem after all.

using L1TrackCollection = std::vector<L1Track>;
@@ -0,0 +1,95 @@
// Author: Sam Harper (RAL/CERN)
// L1 track isolation producer for the HLT
// A quick primer on how the E/gamma HLT works w.r.t to ID variables
// 1. the supercluster is the primary object
// 2. superclusters get id variables associated to them via association maps keyed
// to the supercluster
// However here we also need to read in electron objects as we need to solve for which
// GsfTrack associated to the supercluster to use for the isolation
// The electron producer solves for this and assigns the electron the best GsfTrack
// which we will use for the vz of the electron
// One thing which Swagata Mukherjee pointed out is that we have to be careful of
// is getting a bad GsfTrack with a bad vertex which will give us a fake vz which then
// leads to a perfectly isolated electron as that random vz is not a vertex

#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/MakerMacros.h"
#include "FWCore/Framework/interface/global/EDProducer.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"

#include "DataFormats/EgammaCandidates/interface/Electron.h"
#include "DataFormats/EgammaCandidates/interface/ElectronFwd.h"
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidate.h"
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidateFwd.h"
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidateIsolation.h"
#include "DataFormats/TrackReco/interface/TrackFwd.h"
#include "DataFormats/TrackReco/interface/Track.h"

#include "RecoEgamma/EgammaIsolationAlgos/interface/EgammaL1TkIsolation.h"

#include <memory>

class EgammaHLTEleL1TrackIsolProducer : public edm::global::EDProducer<> {
public:
explicit EgammaHLTEleL1TrackIsolProducer(const edm::ParameterSet&);
~EgammaHLTEleL1TrackIsolProducer() override;
Copy link
Contributor

@fwyzard fwyzard Dec 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a suggestion:

Suggested change
~EgammaHLTEleL1TrackIsolProducer() override;
~EgammaHLTEleL1TrackIsolProducer() override = default;

and drop the out-of-line implementation below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, done, of course this is better :)

void produce(edm::StreamID sid, edm::Event&, const edm::EventSetup&) const override;
static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);

private:
const edm::EDGetTokenT<reco::RecoEcalCandidateCollection> ecalCandsToken_;
const edm::EDGetTokenT<reco::ElectronCollection> elesToken_;
const edm::EDGetTokenT<L1TrackCollection> l1TrksToken_;
EgammaL1TkIsolation isolAlgo_;
};

EgammaHLTEleL1TrackIsolProducer::EgammaHLTEleL1TrackIsolProducer(const edm::ParameterSet& config)
: ecalCandsToken_(consumes<reco::RecoEcalCandidateCollection>(config.getParameter<edm::InputTag>("ecalCands"))),
elesToken_(consumes<reco::ElectronCollection>(config.getParameter<edm::InputTag>("eles"))),
l1TrksToken_(consumes<L1TrackCollection>(config.getParameter<edm::InputTag>("l1Tracks"))),
isolAlgo_(config.getParameter<edm::ParameterSet>("isolCfg")) {
produces<reco::RecoEcalCandidateIsolationMap>();
}

EgammaHLTEleL1TrackIsolProducer::~EgammaHLTEleL1TrackIsolProducer() {}

void EgammaHLTEleL1TrackIsolProducer::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
edm::ParameterSetDescription desc;
desc.add<edm::InputTag>("ecalCands", edm::InputTag("hltEgammaCandidates"));
desc.add<edm::InputTag>("eles", edm::InputTag("hltEgammaGsfElectrons"));
desc.add<edm::InputTag>("l1Tracks", edm::InputTag("TTTracksFromTrackletEmulation", "Level1TTTracks"));
desc.add("isolCfg", EgammaL1TkIsolation::pSetDescript());
descriptions.add("hltEgammaHLTEleL1TrackIsolProducer", desc);
}
void EgammaHLTEleL1TrackIsolProducer::produce(edm::StreamID sid,
edm::Event& iEvent,
const edm::EventSetup& iSetup) const {
auto ecalCands = iEvent.getHandle(ecalCandsToken_);
auto eles = iEvent.getHandle(elesToken_);
auto l1Trks = iEvent.getHandle(l1TrksToken_);

auto recoEcalCandMap = std::make_unique<reco::RecoEcalCandidateIsolationMap>(ecalCands);

for (size_t candNr = 0; candNr < ecalCands->size(); candNr++) {
reco::RecoEcalCandidateRef recoEcalCandRef(ecalCands, candNr);
reco::ElectronRef eleRef;
for (size_t eleNr = 0; eleNr < eles->size(); eleNr++) {
if ((*eles)[eleNr].superCluster() == recoEcalCandRef->superCluster()) {
eleRef = reco::ElectronRef(eles, eleNr);
break;
}
}

float isol =
eleRef.isNonnull() ? isolAlgo_.calIsol(*eleRef->gsfTrack(), *l1Trks).second : std::numeric_limits<float>::max();

recoEcalCandMap->insert(recoEcalCandRef, isol);
}
iEvent.put(std::move(recoEcalCandMap));
}

#include "FWCore/Framework/interface/MakerMacros.h"
DEFINE_FWK_MODULE(EgammaHLTEleL1TrackIsolProducer);
58 changes: 58 additions & 0 deletions RecoEgamma/EgammaIsolationAlgos/interface/EgammaL1TkIsolation.h
@@ -0,0 +1,58 @@
#ifndef RECOEGAMMA_EGAMMAISOLATIONALGOS_EGAMMAL1TKISOLATION_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by convention, we don't use allcaps for this, but rather the same capitalization as in the filenames.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

#define RECOEGAMMA_EGAMMAISOLATIONALGOS_EGAMMAL1TKISOLATION_H

#include "DataFormats/L1TrackTrigger/interface/L1Track.h"
#include "DataFormats/TrackReco/interface/TrackBase.h"
#include "DataFormats/TrackReco/interface/TrackFwd.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"

//author S. Harper (RAL/CERN)
//based on the work of Swagata Mukherjee and Giulia Sorrentino

class EgammaL1TkIsolation {
public:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMS code rule 4.25 (http://cms-sw.github.io/cms_coding_rules.html): "Each class may have only one each of public, protected, and private sections, which must be declared in that order."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be pedantic, per 4.26, the order of functions in the .h and .cc should also be the same

private:
struct TrkCuts {
float minPt;
float minDR2;
float maxDR2;
float minDEta;
float maxDZ;
explicit TrkCuts(const edm::ParameterSet& para);
static edm::ParameterSetDescription pSetDescript();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nicer to call this fillPSetDescription(), following the naming scheme from https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp#A_Plugin_Module_Using_Another_He (also for the instance below). Even better if it follows the pattern with edm::ParameterSetDescription& iDesc provided as an argument rather than a return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this makes it cumbersome to use later on, eg here:

desc.add("isolCfg", EgammaL1TkIsolation::pSetDescript());

However hypothetically (though unlikely) somebody may wish to have this functionality and so I have provided both interfaces.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That line would just need to change to:

EgammaL1TkIsolation::fillPSetDescription(desc);

(and then fillPSetDescription() would call desc.add(...))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still two lines and less clear than before? I dont see the problem with makePSetDescription() given I also supply a fillPSetDescription()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@makortel please clarify how strongly the recommended pattern should be enforced

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the makePSetDescription() that returns a edm::ParameterSetDescription works fine here.

(for non-helper-plugin use cases I think the naming convention of ...PSetDescription is more important than whether the ParameterSetDescription is passed as an out-argument or returned)

};

TrkCuts barrelCuts_, endcapCuts_;

public:
explicit EgammaL1TkIsolation(const edm::ParameterSet& para);
EgammaL1TkIsolation(const EgammaL1TkIsolation&) = default;
~EgammaL1TkIsolation() = default;
EgammaL1TkIsolation& operator=(const EgammaL1TkIsolation&) = default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not totally clear why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to telegraph my intentions. It also shows at a glance that the author likely has some clue about the pitfalls of c++ copies and didnt simply lack that knowledge. There is far to much copy unsafe code in CMSSW.

Copy link
Contributor Author

@Sam-Harper Sam-Harper Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And on 4.26. I do like to group methods of sub classes and not split them but I guess the consequences of 4.25 + 4.26 mean I can no longer do that. Fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to telegraph my intentions.

4.13: For a class, definition of any of the following requires definition of all five: destructor, copy constructor, copy assignment operator, move constructor, and move assignment operator. (*)

If I interpret your comment and 4.13 at face value, you should then also then explicitly define (as defaults) the move constructor and move assignment operators, which are currently implicit, or make them all implicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well its a rather pedantic interpretation of 4.13 isnt it? 4.13 clearly is intended to avoid the situation where a custom destructor is needed and thus any attempt to move, copy or assign the class will almost certainty cause problems if the other 4 arent defined. Given that adding or removing these lines doesnt functionally change anything, does it really count as a definition.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting from C++ Core Guidelines C.21

Declaring any copy/move/destructor function, even as =default or =delete, will suppress the implicit declaration of a move constructor and move assignment operator.

Therefore it would be better to define all five or none of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already removed them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and thank you for the clarification here, I agree my original proposal was incorrect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Matti, I learned something new here as well.


static edm::ParameterSetDescription pSetDescript();

std::pair<int, double> calIsol(const reco::TrackBase& trk, const L1TrackCollection& l1Tks) const;

std::pair<int, double> calIsol(const double objEta,
const double objPhi,
const double objZ,
const L1TrackCollection& l1Tks) const;

//little helper function for the two calIsol functions for it to directly return the pt
template <typename... Args>
double calIsolPt(Args&&... args) const {
return calIsol(std::forward<Args>(args)...).second;
}

private:
static bool passTrkSel(const L1Track& trk,
const double trkPt,
const TrkCuts& cuts,
const double objEta,
const double objPhi,
const double objZ);
};

#endif
72 changes: 72 additions & 0 deletions RecoEgamma/EgammaIsolationAlgos/src/EgammaL1TkIsolation.cc
@@ -0,0 +1,72 @@
#include "RecoEgamma/EgammaIsolationAlgos/interface/EgammaL1TkIsolation.h"
#include "DataFormats/TrackReco/interface/Track.h"
#include "DataFormats/Math/interface/deltaR.h"

EgammaL1TkIsolation::TrkCuts::TrkCuts(const edm::ParameterSet& para) {
minPt = para.getParameter<double>("minPt");
auto sq = [](double val) { return val * val; };
minDR2 = sq(para.getParameter<double>("minDR"));
maxDR2 = sq(para.getParameter<double>("maxDR"));
minDEta = para.getParameter<double>("minDEta");
maxDZ = para.getParameter<double>("maxDZ");
}

edm::ParameterSetDescription EgammaL1TkIsolation::TrkCuts::pSetDescript() {
edm::ParameterSetDescription desc;
desc.add<double>("minPt", 2.0);
desc.add<double>("maxDR", 0.3);
desc.add<double>("minDR", 0.01);
desc.add<double>("minDEta", 0.003);
desc.add<double>("maxDZ", 0.7);
return desc;
}

EgammaL1TkIsolation::EgammaL1TkIsolation(const edm::ParameterSet& para)
: barrelCuts_(para.getParameter<edm::ParameterSet>("barrelCuts")),
endcapCuts_(para.getParameter<edm::ParameterSet>("endcapCuts")) {}

edm::ParameterSetDescription EgammaL1TkIsolation::pSetDescript() {
edm::ParameterSetDescription desc;
desc.add("barrelCuts", TrkCuts::pSetDescript());
desc.add("endcapCuts", TrkCuts::pSetDescript());
return desc;
}

std::pair<int, double> EgammaL1TkIsolation::calIsol(const reco::TrackBase& trk, const L1TrackCollection& tracks) const {
return calIsol(trk.eta(), trk.phi(), trk.vz(), tracks);
}

std::pair<int, double> EgammaL1TkIsolation::calIsol(const double objEta,
const double objPhi,
const double objZ,
const L1TrackCollection& tracks) const {
double ptSum = 0.;
int nrTrks = 0;

const TrkCuts& cuts = std::abs(objEta) < 1.5 ? barrelCuts_ : endcapCuts_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic number 1.5 should be named constant (or obtained from geometry...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made it configurable as its more a parameter than a strict geometry based variable


for (auto& trk : tracks) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const auto&

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ta missed that.

const float trkPt = trk.momentum().perp();
if (passTrkSel(trk, trkPt, cuts, objEta, objPhi, objZ)) {
ptSum += trkPt;
nrTrks++;
}
}
return {nrTrks, ptSum};
}

bool EgammaL1TkIsolation::passTrkSel(const L1Track& trk,
const double trkPt,
const TrkCuts& cuts,
const double objEta,
const double objPhi,
const double objZ) {
if (trkPt > cuts.minPt && std::abs(objZ - trk.z0()) < cuts.maxDZ) {
const float trkEta = trk.eta();
const float dEta = trkEta - objEta;
const float dR2 = reco::deltaR2(objEta, objPhi, trkEta, trk.phi());
return dR2 >= cuts.minDR2 && dR2 <= cuts.maxDR2 && std::abs(dEta) >= cuts.minDEta;
}

return false;
}