New HLT DEBUG object: EgTrigSumObj : CMSSW_11_3_X #32486

Sam-Harper · 2020-12-14T20:53:30Z

PR description:

This PR introduces a new trigger object, EgTrigSumObj.

The purpose of this is for easing debugging, validation and trigger development. In a nutshell, this object packs all E/gamma ID reco and ID quantities into a single object which can be easily analysed offline.

The reason this is necessary is that the HLT builds the e/gamma object bit by bit for CPU efficiency reasons. Therefore you have to associate the GsfTrack, the pixel seeds , the id variables to the supercluster when you want to analyse this. This class does all this for you.

At the same time, the producer of the class can also filter PFClusters, calo hits and tracks for further analysis offline. This greatly helps ID tuning as well as validation. The class may evolve in the future with filtering of the pixel hits, gsf tracks and superclusters in the plans, however it is usable now.

This is has been the core of the E/gamma Phase-II development (a separate class for which another PR will be made once this merges handles the phase-II specific things).

The goal of this class once merged is to add it to the HLTDEBUG format for RelVals and also in any future Phase-II re-HLT. Ultimately it may make it into other data tiers once it is stable but that depends on many other things.

Finally the observant reader will notice that this has a lot of overlap with the scouting egamma format, both store very similar information. However right now they serve different purposes (this is a validation / debugging / trigger dev format) and scouting has an analysis format with strong space constraints. This does not preclude in the future that they merge but right now that is not in the plan.

PR validation:

To get it to output, the following snippet can be added to an HLT workflow

process.load("RecoEgamma.EgammaHLTProducers.hltEgammaHLTExtra_cfi")
process.hltEgHLTOut = cms.EndPath(process.hltEgammaHLTExtra)
process.schedule.append(process.hltEgHLTOut)
process.FEVTDEBUGHLToutput.outputCommands.append("keep *_hltEgammaHLTExtra_*_*")

Happily used for phase-II HLT development.

cmsbuild · 2020-12-14T21:00:37Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32486/20392

This PR adds an extra 64KB to repository
Found files with invalid states:
- RecoEgamma/EgammaHLTProducers/python/hltEgammaHLTExtraL1Seeded_cfi.py:
  - Added: 5ab6257
  - Deleted: d1262dd
- RecoEgamma/EgammaHLTProducers/python/hltEgammaHLTExtraUnseeded_cfi.py:
  - Added: 5ab6257
  - Deleted: d1262dd

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-checks:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32486/20392/code-checks.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32486/20392/code-checks.patch | patch -p1
You can also run scram build code-checks to apply code checks directly

cmsbuild · 2020-12-15T08:49:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32486/20409

This PR adds an extra 68KB to repository
Found files with invalid states:
- RecoEgamma/EgammaHLTProducers/python/hltEgammaHLTExtraL1Seeded_cfi.py:
  - Added: 3ea8c94
  - Deleted: 51f3658
- RecoEgamma/EgammaHLTProducers/python/hltEgammaHLTExtraUnseeded_cfi.py:
  - Added: 3ea8c94
  - Deleted: 51f3658

cmsbuild · 2020-12-15T08:49:59Z

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

It involves the following packages:

DataFormats/EgammaReco
RecoEgamma/EgammaHLTProducers

@perrotta, @Martin-Grunewald, @slava77, @cmsbuild, @fwyzard, @jpata can you please review it and eventually sign? Thanks.
@sobhatta, @jainshilpi, @rovere, @lgray, @calderona, @HuguesBrun, @Fedespring, @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

Martin-Grunewald · 2020-12-15T09:04:20Z

please test

cmsbuild · 2020-12-15T11:56:50Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c18dd3/11666/summary.html
CMSSW: CMSSW_11_3_X_2020-12-14-2300/slc7_amd64_gcc900

Comparison Summary

Summary:

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

Martin-Grunewald · 2020-12-16T14:27:19Z

+1

slava77 · 2020-12-17T15:14:40Z

DataFormats/EgammaReco/interface/EgTrigSumObj.h

+}
+
+namespace reco {
+  class EgTrigSumObj {


Perhaps EgammaTriggerSummary?
(just to be more readable and not go to even shorter EgTrSumOb)

I would prefer EgTrigSumObj as EgammaTriggerSummary suggests its a summary of the entire trigger rather than a specific e/gamma object. I could also go to EgHLTSumObj or EgHLTObj. However its not something I feel super strong.

EgammaHLTSummaryObject ?

could do, I mean I'm trying to have a relatively short name. I think "EgHLT" should be an acceptable abbreviation which anybody using this class will figure out. I do concede on reflection "Sum" is a bit less clear. Obj I would imagine is also fairly clear.

With the summary, I'm trying to get across that its an object which is produced at the end of the HLT rather than an object which is part of the HLT decision process. Now this is important to get right as there will be very soon be a MuHLTSumObj, a TauHLTSumObj, a BTagHLTSumObj and a JetHLTSumObj (although jet & btag will likely merge).

So I do appreciate suggestions here, I know I'm terrible at names.

I could live with
EgHLTSummaryObject
MuHLTSummaryObject
TauHLTSummaryObject
JetHLTSummaryObject

but I would prefer something a bit shorter but not to the point thats confusing.

Thoughts?

I'd still use Muon and Egamma, these are common class naming patterns in CMSSW and it's better to be consistent.

With the summary, I'm trying to get across that its an object which is produced at the end of the HLT rather than an object which is part of the HLT decision process.

Looking at existing patterns,
isn't a TriggerObject from DataFormats/HLTReco/interface/TriggerObject.h supposed to cover the final objects as well?
Then, EgammaTriggerObject seems like a natural extension.

But if the endpoint needs to be included, berhaps EgammaTriggerResultObject ?

given the trigger/HLT specificity, does this have to be in the reco namespace?

Also, given the ideas for other variants, wouldn't it be better to inherit from smth like TriggerObject for some common interface?

apparently #32486 (comment)
and #32486 (comment) were ignored.
It would be nice to at least have a comment why they were not considered.

sorry they were not ignored, just missed (my bad, I need to fix my email filters better.)

apparently #32486 (comment)
and #32486 (comment) were ignored.
It would be nice to at least have a comment why they were not considered.

Sorry Slava, I also missed them during the review.
Removing the "+1" while waiting for Sam making the update

this indeed might be better.

What do you think @fwyzard, @Martin-Grunewald

EgammaTriggerObject is fine with me especially if you extend TriggerObject.

slava77 · 2020-12-28T15:06:32Z

DataFormats/EgammaReco/interface/EgTrigSumObj.h

+    float eta_;
+    float phi_;
+    bool hasPixelMatch_;
+    std::unordered_map<std::string, float> vars_;


do we have any other examples of unordered_map streamed in CMSSW?
@cms-sw/core-l2 do we have good support for this in root?

do we have any other examples of unordered_map streamed in CMSSW?

It seems we do have a few

cmssw/DataFormats/BTauReco/interface/DeepBoostedJetFeatures.h

Line 57 in 00360a8

std::unordered_map<std::string, std::vector<float>> feature_map_;

cmssw/DataFormats/L1THGCal/interface/HGCalClusterT.h

Lines 195 to 196 in 00360a8

std::unordered_map<uint32_t, edm::Ptr<C>> constituents_;

std::unordered_map<uint32_t, double> constituentsFraction_;

cmssw/DataFormats/L1THGCal/interface/HGCalTowerMap.h

Line 31 in 00360a8

std::unordered_map<unsigned short, l1t::HGCalTower> towerMap_;

@cms-sw/core-l2 do we have good support for this in root?

Good question, @pcanal (@Dr15Jones) could you comment?

How many elements is the map expected to have?

Traditional alternative would be a sorted std::vector<std::pair<std::string, float>> (or separate std::vector<std::string> and std::vector<float>, those would allow to make varNames() trivial).

do we have any other examples of unordered_map streamed in CMSSW?

It seems we do have a few

IIRC, DeepBoostedJetFeatures is effectively transient (although it is not declared as such): it is not saved.

I'm not familiar with the L1THGCal cases.

We've recently gone through a case with problems of streaming a usual std::map in cms-sw/cmsdist#6388 (comment)
If this data is important it may be worth to pick a different type, like a suggested vector<pair

thanks for the spot, I had not considered this. I used std::unorderedmap simply for convenience but we get better data stability with a sorted std::vector then thats not a problem.

It will contain on average a maximum of ~about 20, possibly 30 entries I believe but I can check.

On the splitting it to std::string and std::vector, so I personally dislike having two vectors coupled to each other as in we need to ensure they are the same length, kept up to date etc. This is not overly difficult and I'm happy to go with yours and others suggestions on what would be the best practice to create a set of read only key,value pairs in a class.

perrotta · 2020-12-28T15:15:26Z

DataFormats/EgammaReco/src/classes_def.xml

+  <class name="edm::Wrapper<edm::ValueMap<reco::SuperClusterRef> >" />
+
+  <class name="reco::EgTrigSumObj" ClassVersion="10">
+   <version ClassVersion="10" checksum="2636406548"/>


Use 3 as the initial version number of a class which has never been stored before

ah thank you I did not know that, much appreicated!

perrotta · 2020-12-28T16:08:04Z

In general, even if there is some overlap I agree that the scouting class and this debug one can remain separated, as they serve different purposes and will probaly need to be updated and maintainent independently.

By the way @Sam-Harper , do you have some script or simple recipe to test that this new code is actually working?

Sam-Harper · 2021-02-05T14:57:54Z

okay I've now done all the renaming (I had some other ideas how to do this but ultimate decided not to do them), I squished all the commits to avoid modifying packages in Reco and make the backport easier

Sam-Harper · 2021-02-05T14:58:31Z

Oh and I named it trigger::EgammaObject (the trigger::EgammaTriggerObject seemed a bit redundant)

cmsbuild · 2021-02-05T15:03:07Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32486/21028

This PR adds an extra 28KB to repository

cmsbuild · 2021-02-05T15:03:32Z

Pull request #32486 was updated. @Martin-Grunewald, @cmsbuild, @fwyzard can you please check and sign again.

perrotta · 2021-02-05T15:05:05Z

please test

cmsbuild · 2021-02-05T18:58:44Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c18dd3/12737/summary.html
COMMIT: 9ae839c
CMSSW: CMSSW_11_3_X_2021-02-05-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/32486/12737/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: 37
DQMHistoTests: Total histograms compared: 2751765
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2751742
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

Sam-Harper · 2021-02-08T18:25:05Z

@Martin-Grunewald , could you have a look at the new solution please and say if you're happy with it. Its now pure HLT :) Thanks!

Martin-Grunewald · 2021-02-09T07:40:10Z

+1

cmsbuild · 2021-02-09T07:40:34Z

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)

silviodonato · 2021-02-09T09:56:01Z

+1

cmsbuild added this to the CMSSW_11_3_X milestone Dec 14, 2020

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

cmsbuild added code-checks-rejected and removed code-checks-pending labels Dec 14, 2020

Sam-Harper force-pushed the EgTrigSum_1120pre7 branch from 41fca5d to 8ebda55 Compare December 15, 2020 08:41

cmsbuild added code-checks-pending and removed code-checks-rejected labels Dec 15, 2020

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

cmsbuild added tests-started and removed tests-pending labels Dec 15, 2020

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

cmsbuild added hlt-approved and removed hlt-pending labels Dec 16, 2020

slava77 reviewed Dec 17, 2020

View reviewed changes

slava77 reviewed Dec 28, 2020

View reviewed changes

perrotta reviewed Dec 28, 2020

View reviewed changes

Sam-Harper force-pushed the EgTrigSum_1120pre7 branch from 92f056d to 9ae839c Compare February 5, 2021 14:56

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved reconstruction-pending tests-approved labels Feb 5, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 5, 2021

cmsbuild added tests-started and removed tests-pending labels Feb 5, 2021

cmsbuild added tests-approved and removed tests-started labels Feb 5, 2021

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Feb 9, 2021

cmsbuild added orp-approved and removed orp-pending labels Feb 9, 2021

cmsbuild merged commit 454c63e into cms-sw:master Feb 9, 2021

Sam-Harper mentioned this pull request Feb 14, 2021

New HLT DEBUG object: EgTrigSumObj : CMSSW_11_1_X #32910

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New HLT DEBUG object: EgTrigSumObj : CMSSW_11_3_X #32486

New HLT DEBUG object: EgTrigSumObj : CMSSW_11_3_X #32486

Sam-Harper commented Dec 14, 2020

cmsbuild commented Dec 14, 2020

cmsbuild commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

Martin-Grunewald commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

Martin-Grunewald commented Dec 16, 2020

slava77 Dec 17, 2020

Sam-Harper Jan 8, 2021

fwyzard Jan 8, 2021

Sam-Harper Jan 8, 2021

slava77 Jan 8, 2021

slava77 Jan 11, 2021

Sam-Harper Jan 11, 2021

perrotta Jan 11, 2021

Sam-Harper Jan 11, 2021

Martin-Grunewald Jan 11, 2021

slava77 Dec 28, 2020

makortel Dec 28, 2020

slava77 Dec 28, 2020

Sam-Harper Jan 8, 2021

perrotta Dec 28, 2020

Sam-Harper Jan 8, 2021

perrotta commented Dec 28, 2020

Sam-Harper commented Feb 5, 2021

Sam-Harper commented Feb 5, 2021

cmsbuild commented Feb 5, 2021

cmsbuild commented Feb 5, 2021

perrotta commented Feb 5, 2021

cmsbuild commented Feb 5, 2021

Sam-Harper commented Feb 8, 2021

Martin-Grunewald commented Feb 9, 2021

cmsbuild commented Feb 9, 2021

silviodonato commented Feb 9, 2021

	std::unordered_map<uint32_t, edm::Ptr<C>> constituents_;
	std::unordered_map<uint32_t, double> constituentsFraction_;

New HLT DEBUG object: EgTrigSumObj : CMSSW_11_3_X #32486

New HLT DEBUG object: EgTrigSumObj : CMSSW_11_3_X #32486

Conversation

Sam-Harper commented Dec 14, 2020

PR description:

PR validation:

cmsbuild commented Dec 14, 2020

cmsbuild commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

Martin-Grunewald commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

Comparison Summary

Martin-Grunewald commented Dec 16, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrotta commented Dec 28, 2020

Sam-Harper commented Feb 5, 2021

Sam-Harper commented Feb 5, 2021

cmsbuild commented Feb 5, 2021

cmsbuild commented Feb 5, 2021

perrotta commented Feb 5, 2021

cmsbuild commented Feb 5, 2021

Comparison Summary

Sam-Harper commented Feb 8, 2021

Martin-Grunewald commented Feb 9, 2021

cmsbuild commented Feb 9, 2021

silviodonato commented Feb 9, 2021