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

Sam-Harper · 2020-12-31T11:13:22Z

PR description:

This is allows track isolation with L1 tracks to be produced for electrons for the E/g HLT. Part of this PR also introduces a typedef for a TTTrack<Ref_Phase2TrackerDigi_> to L1Track. This in my opinion should have been done a long time ago and such a typedef needs to exist centrally as people are already making their own. I dont care too much about the name.

Additionally in validation a "bug" was found with the track ambiguity solving for electrons in the HLT which will be fixed at a later date. Ie we seem to be picking the wrong track in a few cases and we should solve it.

Note, an example test config will not be provided for 11_3_X because of the difficulties of porting the HLT phase II between releases. A test config can be provided for 11_1_X on request but it'll be 50K lines of python

PR validation:

Tested in phase-II HLT setup and gives expected results.

cmsbuild · 2020-12-31T11:24:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32594/20622

This PR adds an extra 24KB to repository

cmsbuild · 2020-12-31T11:24:32Z

A new Pull Request was created by @Sam-Harper (Sam Harper) for master.

It involves the following packages:

DataFormats/L1TrackTrigger
RecoEgamma/EgammaHLTProducers
RecoEgamma/EgammaIsolationAlgos

@perrotta, @cmsbuild, @fwyzard, @kpedro88, @jmduarte, @Martin-Grunewald, @rekovic, @slava77, @jpata can you please review it and eventually sign? Thanks.
@sobhatta, @jainshilpi, @Fedespring, @lgray, @calderona, @HuguesBrun, @rovere, @sviret, @afiqaize, @varuns23, @cericeci, @ram1123 this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

fwyzard · 2020-12-31T13:48:14Z

RecoEgamma/EgammaHLTProducers/plugins/EgammaHLTEleL1TrackIsolProducer.cc

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


just a suggestion:

Suggested change

~EgammaHLTEleL1TrackIsolProducer() override;

~EgammaHLTEleL1TrackIsolProducer() override = default;

and drop the out-of-line implementation below.

thanks, done, of course this is better :)

fwyzard · 2020-12-31T13:52:42Z

allow @Sam-Harper test rights

fwyzard · 2020-12-31T13:52:47Z

please test

cmsbuild · 2020-12-31T16:41:38Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-863d8c/11910/summary.html
CMSSW: CMSSW_11_3_X_2020-12-31-1100/slc7_amd64_gcc900

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-863d8c/1325.81_TTbar_13_106Xv1NanoAODINPUT+TTbar_13_106Xv1NanoAODINPUT+NANOEDMMC2017_106XMiniAODv1+HARVESTNANOAODMC2017_106XMiniAODv1
/build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-863d8c/136.8523_RunJetHT2018C_nanoULremini+RunJetHT2018C_nanoULremini+NANOEDM2018_106Xv2+HARVESTNANOAOD2018_106Xv2

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2716967
DQMHistoTests: Total failures: 4
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2716941
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 156 log files, 37 edm output root files, 37 DQM output files

kpedro88 · 2020-12-31T18:13:53Z

RecoEgamma/EgammaIsolationAlgos/interface/EgammaL1TkIsolation.h

+//based on the work of Swagata Mukherjee and Giulia Sorrentino
+
+class EgammaL1TkIsolation {
+public:


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."

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

kpedro88 · 2020-12-31T18:19:20Z

RecoEgamma/EgammaIsolationAlgos/interface/EgammaL1TkIsolation.h

+    float minDEta;
+    float maxDZ;
+    explicit TrkCuts(const edm::ParameterSet& para);
+    static edm::ParameterSetDescription pSetDescript();


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.

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

cmssw/RecoEgamma/EgammaHLTProducers/plugins/EgammaHLTEleL1TrackIsolProducer.cc

Line 64 in 68e0824

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

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

That line would just need to change to:

EgammaL1TkIsolation::fillPSetDescription(desc);

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

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

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

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)

kpedro88 · 2020-12-31T18:20:45Z

RecoEgamma/EgammaIsolationAlgos/src/EgammaL1TkIsolation.cc

+  double ptSum = 0.;
+  int nrTrks = 0;
+
+  const TrkCuts& cuts = std::abs(objEta) < 1.5 ? barrelCuts_ : endcapCuts_;


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

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

kpedro88 · 2020-12-31T18:20:54Z

RecoEgamma/EgammaIsolationAlgos/src/EgammaL1TkIsolation.cc

+
+  const TrkCuts& cuts = std::abs(objEta) < 1.5 ? barrelCuts_ : endcapCuts_;
+
+  for (auto& trk : tracks) {


const auto&

ta missed that.

swagata87 · 2021-01-03T18:12:40Z

Hi,
just wanted to point out a couple of minor things about the typedef

typedef of TTTrack<Ref_Phase2TrackerDigi_> is also available under namespace l1t in DataFormats/L1TCorrelator/interface/, inside L1CaloTkTau.h, TkEGTau.h etc. The typedef name is different though. They use this:
typedef TTTrack<Ref_Phase2TrackerDigi_> L1TTTrackType;
typedef std::vector L1TTTrackCollection;
is it an option to just use this typedef?

but I understand that having the typedef in DataFormats/L1TrackTrigger/interface/L1Track.h is much better, as it is more central/generic place.

but another L1Track.h exists under DataFormats/L1CSCTrackFinder/interface/
there, inside namespace csc, they have a class called L1Track,
while this PR introduces the following typedef:
using L1Track = TTTrack<Ref_Phase2TrackerDigi_>;
so to avoid any situation of ambiguity in future, may be it is safer to use a different name of the typedef?

I had a quick chat with @guitargeek , and he suggested some things. I request him to write his suggestions here. Thanks!

guitargeek · 2021-01-03T18:46:48Z

Okay, sure! My suggestion would be to adapt the names of the previously existing aliases in DataFormats/L1TrackTrigger/interface/L1Track.h, which would mean to rename the file to L1TTTrack.h (or even L1TrackTriggerTrack.h for more clarity).

The content of the file would then be:

using L1TTTrack = TTTrack<Ref_Phase2TrackerDigi_>;
using L1TTTrackCollection = std::vector<L1Track>;

The file can then also be included in the https://github.com/cms-sw/cmssw/blob/master/DataFormats/L1TCorrelator/interface files to ensure consistency.

fwyzard · 2021-01-03T19:10:07Z

typedef of TTTrack<Ref_Phase2TrackerDigi_> is also available under namespace l1t in DataFormats/L1TCorrelator/interface/, inside L1CaloTkTau.h, TkEGTau.h etc. The typedef name is different though. They use this:
typedef TTTrack<Ref_Phase2TrackerDigi_> L1TTTrackType;

In general, the class or type names used in CMS software do not have Type appended to them (unless they are used to handle types, e.g. BranchType)

Also, L1TTTrack (short for L1 Track Trigger Track ?) is more cumbersome than just L1Track.

typedef std::vector L1TTTrackCollection;
is it an option to just use this typedef?

Just add L1Track as proposed here, and adapt he existing code to use it.

but another L1Track.h exists under DataFormats/L1CSCTrackFinder/interface/
there, inside namespace csc, they have a class called L1Track

Well, yes, that is what namespaces are for: to distinguish csc::L1Track from L1Track.

cmsbuild · 2021-01-05T17:03:51Z

Pull request #32594 was updated. @perrotta, @cmsbuild, @fwyzard, @kpedro88, @jmduarte, @Martin-Grunewald, @rekovic, @srimanob, @slava77, @jpata can you please check and sign again.

jpata · 2021-01-06T11:06:52Z

@cmsbuild please test

cmsbuild · 2021-01-06T13:35:41Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-863d8c/11987/summary.html
COMMIT: 58356aa
CMSSW: CMSSW_11_3_X_2021-01-05-2300/slc7_amd64_gcc900

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2716967
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2716938
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 156 log files, 37 edm output root files, 37 DQM output files

jpata · 2021-01-07T14:24:34Z

DataFormats/L1TrackTrigger/interface/L1Track.h

+#include "DataFormats/L1TrackTrigger/interface/TTTypes.h"
+#include <vector>
+
+using L1Track = TTTrack<Ref_Phase2TrackerDigi_>;


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.

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.

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?

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.

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.

jpata · 2021-01-11T09:26:11Z

+reconstruction

adds EgammaHLTEleL1TrackIsolProducer and a related EgammaL1TkIsolation helper class
tests pass with no difference, none is expected, as per previous discussions, this code does not run in the tests
the validation was done by the authors and reported in the description to be successful

Martin-Grunewald · 2021-01-12T15:03:04Z

+1

rekovic · 2021-01-13T12:55:05Z

+1

srimanob · 2021-01-13T14:09:27Z

+Upgrade

cmsbuild · 2021-01-13T14:09:51Z

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

qliphy · 2021-01-14T00:11:23Z

+1

Sam-Harper added 3 commits December 31, 2020 11:18

initial commit of E/gamma L1 track isolation

5c7da44

code checks & format

0aeba85

removing debug statement

68e0824

cmsbuild added this to the CMSSW_11_3_X milestone Dec 31, 2020

cmsbuild added code-checks-pending hlt-pending l1-pending orp-pending pending-signatures reconstruction-pending tests-pending upgrade-pending labels Dec 31, 2020

Sam-Harper force-pushed the L1TrkIso_1120pre7 branch from 1c75342 to 68e0824 Compare December 31, 2020 11:17

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 31, 2020

fwyzard reviewed Dec 31, 2020

View reviewed changes

cmsbuild added tests-started allow-Sam-Harper and removed tests-pending labels Dec 31, 2020

cmsbuild added tests-approved and removed tests-started labels Dec 31, 2020

kpedro88 reviewed Dec 31, 2020

View reviewed changes

cmsbuild added the code-checks-approved label Jan 5, 2021

jpata mentioned this pull request Jan 6, 2021

HGCal ID variables #32519

Merged

cmsbuild added tests-started and removed tests-pending labels Jan 6, 2021

cmsbuild added tests-approved and removed tests-started labels Jan 6, 2021

jpata reviewed Jan 7, 2021

View reviewed changes

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jan 11, 2021

Sam-Harper mentioned this pull request Jan 12, 2021

E/gamma HLT L1 Track Isolation Producer & TTTrack typedef: 11_1_X #32644

Merged

cmsbuild added hlt-approved and removed hlt-pending labels Jan 12, 2021

cmsbuild added l1-approved and removed l1-pending labels Jan 13, 2021

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels Jan 13, 2021

cmsbuild added orp-approved and removed orp-pending labels Jan 14, 2021

cmsbuild merged commit 82f177e into cms-sw:master Jan 14, 2021

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


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

		for (auto& trk : tracks) {

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

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

Conversation

Sam-Harper commented Dec 31, 2020

PR description:

PR validation:

cmsbuild commented Dec 31, 2020

cmsbuild commented Dec 31, 2020

fwyzard Dec 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwyzard commented Dec 31, 2020

fwyzard commented Dec 31, 2020

cmsbuild commented Dec 31, 2020

Comparison Summary

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

swagata87 commented Jan 3, 2021

guitargeek commented Jan 3, 2021

fwyzard commented Jan 3, 2021

cmsbuild commented Jan 5, 2021

jpata commented Jan 6, 2021

cmsbuild commented Jan 6, 2021

Comparison Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 Jan 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 Jan 7, 2021 • edited

Choose a reason for hiding this comment

jpata commented Jan 11, 2021

Martin-Grunewald commented Jan 12, 2021

rekovic commented Jan 13, 2021

srimanob commented Jan 13, 2021

cmsbuild commented Jan 13, 2021

qliphy commented Jan 14, 2021

fwyzard Dec 31, 2020 •

edited

slava77 Jan 7, 2021 •

edited

slava77 Jan 7, 2021 •

edited