E/gamma T&P Offline DQM : 92X + VID + GenericTrigEventFlag Changes #19046

Sam-Harper · 2017-05-31T17:41:40Z

This a re-submission of #18969 which went all bit wrong and I have zero time to fix. I have copied over the PR text below.

This also modifies VID producers not to crash on missing object input collections. This was necessary and the reason why is discussed in the comments of the change. Finally it makes backwards compatible changes to GenericTriggerEventFlag so I dont need to pass in the EDProducer address and at the same time makes it movable by turning the last pointer to a std::unique_ptr

Original PR text:

This is an initial version of the E/gamma T&P offline DQM. This code is near feature complete and produces correct output (as far as I can tell) . However there are some layout and naming issues which I would like to solicit suggestions for before this PR is considered for integration.

The goal of this class is to be object agnostic, hence the templating.

The idea is very simple.

Select the event with a trigger. Require the tag to pass an HLT requirement & selection requirement. Require the probes to pass other selection requirements to entire the sample. Mass and opposite sign requirements are then applied.
There are then a series of histograms associated with each filter we wish to measure, one for all probe passing sample cuts and probes then passing the filter.
There are various levels of tag, probe kinematic and trigger selection: all t&p pairs (ie tag in barrel, passing tag trigger), all histograms of a filter (ie requiring Et to be > threshold, tag to pass an additional trigger, ie the single leg of a double legged trigger), and a specific histogram (ie requiring the probe to be in the endcap to measure the turn on in the endcap region only).

All the magic is in https://github.com/Sam-Harper/cmssw/blob/32a8b63ed2afd1a80771776d7c441b40cd3367bb/DQMOffline/Trigger/interface/HLTTagAndProbeEff.h

There are 3 main classes

HLTTagAndProbeEff : creates tag and probe pairs and then passes the pairs to the histograms.

HLTDQMHistColl : corresponds to a single filter for which we wish to measure the efficiency. Can apply additional selection to the tag & probe (ie restricting them to certain Et, eta ranges, requiring the tag to pass additional HLT requirements). Contains all the individual histograms which are of type HLTDQM1DHist and HLTDQM2DHist and automically fill themselves with the specified vs variable

RangeCutColl: ID selection is handled by edm::ValueMap this class is intended to cut on kinematic variables such as et, eta, phi. It allows us to restrict to a certain eta region (tags in the barrel, probes in HEP17, probes > Et threshold etc)

For the ID selection of tag & probe it was intended to use VID but I didnt get it working so made a temporary ID module to do that for the time being.

Suggestions on names and how to split them up are welcome. RangeCutColl and HLTDQMHist share the same mechanism to store the variable in question which is the sticking point.

Also suggestion on alternative ways to achieve the this are also welcome although performance/clarity gains would have to balanced against work required to make them.

cmsbuild · 2017-05-31T17:42:10Z

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

It involves the following packages:

CommonTools/TriggerUtils
DQMOffline/Trigger
PhysicsTools/SelectorUtils

@perrotta, @monttj, @dmitrijus, @cmsbuild, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@battibass, @mtosi, @jhgoh, @calderona, @HuguesBrun, @trocino, @rociovilar this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

slava77 · 2017-05-31T17:43:52Z

@cmsbuild please test

cmsbuild · 2017-05-31T17:44:12Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20261/console Started: 2017/05/31 19:44

cmsbuild · 2017-05-31T19:38:38Z

+1
Tested at: 877022b
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19046/20261/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
877022b
f3638a2
c6b7c56
bcc689f
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19046/20261/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19046/20261/git-merge-result

cmsbuild · 2017-05-31T19:38:41Z

Comparison job queued.

cmsbuild · 2017-05-31T22:10:09Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19046/20261/summary.html

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

/build/cmsbld/jenkins-workarea/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-19046/20434.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D10_GenSimHLBeamSpotFull14+DigiFullTrigger_2023D10+RecoFullGlobal_2023D10+HARVESTFullGlobal_2023D10
/build/cmsbld/jenkins-workarea/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-19046/23234.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D18_GenSimHLBeamSpotFull14+DigiFull_2023D18+RecoFullGlobal_2023D18+HARVESTFullGlobal_2023D18
/build/cmsbld/jenkins-workarea/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-19046/27434.0_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2023D17_GenSimHLBeamSpotFull14+DigiFull_2023D17+RecoFullGlobal_2023D17+HARVESTFullGlobal_2023D17

Comparison Summary:

You potentially added 130 lines to the logs
Reco comparison results: 1709 differences found in the comparisons
DQMHistoTests: Total files compared: 23
DQMHistoTests: Total histograms compared: 1786106
DQMHistoTests: Total failures: 28212
DQMHistoTests: Total nulls: 105
DQMHistoTests: Total successes: 1757616
DQMHistoTests: Total skipped: 173
DQMHistoTests: Total Missing objects: 0
Checked 94 log files, 14 edm output root files, 23 DQM output files

slava77 · 2017-06-01T18:27:01Z

PhysicsTools/SelectorUtils/interface/VersionedIdProducer.h

+  if(!physicsObjectsHandle.isValid()){
+    edm::LogWarning("InvalidCollection")<<
+      "physics objects collection is invalid, no VID info will be written";
+    return;


If this producer can not run, why is it in the configuration?
I think that this "graceful fail" should be removed and the issue resolved at configuration level.

Also, simple return is bad: the producer should still put the products in the event.

Yes, I was wondering about how to do this.

It all come from the fact I dont know how to have this included in the pp DQM sequence and not HI sequence (it failed in HI tests). Also speaking as somebody who occasionally has to hack things around to get to run in unusual work flows, having modules which fail gracefully when their inputs are not present is kinda nice. However I admit thats a very weak reason.

So how can I detect that gedGsfElectrons are not present (either in the input file or a producer to make it) or disable them from HI DQM?

cmsbuild · 2017-06-02T05:30:40Z

Pull request #19046 was updated. @perrotta, @monttj, @dmitrijus, @cmsbuild, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

Sam-Harper · 2017-06-02T05:37:37Z

Okay VID is back to exactly how it was. Now its properly removed from HI DQM sequences instead. Yes this way is the correct way to do it thanks Slava (and sorry Mia, I didnt understand you before, I think this is what you were telling me). Not entirely sure why I didnt do this in the first place, I blame the lack of sleep recently!

Btw I'm traveling shortly for the weekend so its unlikely I'll be able to respond to any further changes till monday. However I think we're good though, the changes here are just for the new E/g DQM, even if they are not perfect, they produce acceptable results and we can fine tune for the next train. And the only changes to existing code is the GenericTriggerEvent flag which actually fixes an issue (bare -> smart pointer) and adds a backwards compatible change.

mtosi · 2017-06-02T05:48:59Z

thanks !!!! (yes, this is what I was suggesting :P ) can we start the integration test, please ? ps: enjoy your weekend !

…

On Fri, Jun 2, 2017 at 7:37 AM, Sam-Harper ***@***.***> wrote: Okay VID is back to exactly how it was. Now its properly removed from HI DQM sequences instead. Yes this way is the correct way to do it thanks Slava (and sorry Mia, I didnt understand you before, I think this is what you were telling me). Not entirely sure why I didnt do this in the first place, I blame the lack of sleep recently! Btw I'm traveling shortly for the weekend so its unlikely I'll be able to respond to any further changes till monday. However I think we're good though, the changes here are just for the new E/g DQM, even if they are not perfect, they produce acceptable results and we can fine tune for the next train. And the only changes to existing code is the GenericTriggerEvent flag which actually fixes an issue (bare -> smart pointer) and adds a backwards compatible change. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19046 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEt5877fgxNcwo0CjD4cxQU12nhyUFrkks5r_5-igaJpZM4Nr9kY> .

perrotta · 2017-06-02T08:53:27Z

please test

cmsbuild · 2017-06-02T08:53:43Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20299/console Started: 2017/06/02 10:54

cmsbuild · 2017-06-02T10:35:50Z

+1
Tested at: 9636bcf
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19046/20299/summary.html

cmsbuild · 2017-06-02T10:35:53Z

Comparison job queued.

cmsbuild · 2017-06-02T13:39:15Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19046/20299/summary.html

Comparison Summary:

You potentially added 18 lines to the logs
Reco comparison results: 1637 differences found in the comparisons
DQMHistoTests: Total files compared: 23
DQMHistoTests: Total histograms compared: 1837605
DQMHistoTests: Total failures: 24118
DQMHistoTests: Total nulls: 65
DQMHistoTests: Total successes: 1813249
DQMHistoTests: Total skipped: 173
DQMHistoTests: Total Missing objects: 0
Checked 94 log files, 14 edm output root files, 23 DQM output files

slava77 · 2017-06-02T17:01:18Z

+1

for #19046 9636bcf

reco is minimally affected: only somewhat tangentially in CommonTools/TriggerUtils/interface/GenericTriggerEventFlag; no changes expected in reco or miniAOD
jenkins tests pass and comparisons show no differences

dmitrijus · 2017-06-06T13:14:48Z

+1

mtosi · 2017-06-06T14:03:35Z

we have "pending signature", could we move on, please ?
thanks !

davidlange6 · 2017-06-07T06:17:55Z

CommonTools/TriggerUtils/src/GenericTriggerEventFlag.cc

+GenericTriggerEventFlag::GenericTriggerEventFlag( const edm::ParameterSet & config, edm::ConsumesCollector & iC):
+  GenericTriggerEventFlag(config,iC,false)
+{
+  if ( config.exists( "andOrL1" ) ) {


Hi @Sam-Harper - given the GenericTriggerEventFlag(config,iC,false) call, is this needed?

Hi David,

Many thanks for the review. So I think this not strictly needed but it does simplify things for me.

I wanted to leave the original public constructor:
https://github.com/Sam-Harper/cmssw/blob/9636bcf9626f001993f6d941c1b598a8b7eb3a13/CommonTools/TriggerUtils/interface/GenericTriggerEventFlag.h#L157-L169

exactly as is. Which means all the l1 setup gets done outside the private constructor (which is now tagged with an extra argument of a bool). A dirty little secret, that bool is only there to identify the private contructor,

https://github.com/Sam-Harper/cmssw/blob/9636bcf9626f001993f6d941c1b598a8b7eb3a13/CommonTools/TriggerUtils/src/GenericTriggerEventFlag.cc#L130-L135

could easily just be at the location you mention and inside the config.exists("andOrL1") statement. But I put it in the private constructor simply because I didnt like having an unused variable.

As written, it is simpler to have all the L1 construction happening in the public constructors and a shared private constructor which does the rest of the setup. Note, the false only means it is an error that stage-1 is selected, stage-2 can be setup.

ok, it seemed like it was just repeating things to me. Thanks to for the explanation.

fwyzard · 2017-06-08T12:12:43Z

tracked by #19142

This PR includes a backport of the following PRs: - cms-sw#18172 - cms-sw#18950 - cms-sw#18959 - cms-sw#18968 - cms-sw#18971 - cms-sw#19023 - cms-sw#19046 - cms-sw#19078 - cms-sw#19119 - cms-sw#19178 - cms-sw#19290 - cms-sw#19293 - cms-sw#19294 - cms-sw#19490 - cms-sw#19499 - cms-sw#19577 - cms-sw#19585 - cms-sw#19596 - cms-sw#19599 - cms-sw#19627 - cms-sw#19689 - cms-sw#19694 - cms-sw#19703 - cms-sw#19781 - cms-sw#19794 plus the older ones, contained in DQMOffline/Trigger and HLTriggerOffline. It synchronises with CMSSW_9_3_X - DQMServices/ClientConfig - DQMOffline/Configuration - DQMOffline/Trigger - HLTriggerOffline/Btag - HLTriggerOffline/Higgs - HLTriggerOffline/SUSYBSM - HLTriggerOffline/Tau - HLTriggerOffline/Top

Sam-Harper added 9 commits May 26, 2017 16:52

initial version

32a8b63

refactor for readability and fill descriptions

cc642e9

updating to use VID and generic trigger event

e8e3b72

allowing VID to gracefully fail if the input collection is invalid

4a42e67

initial version

fb1a3f4

refactor for readability and fill descriptions

4ff545f

updating to use VID and generic trigger event

9dffa8b

allowing VID to gracefully fail if the input collection is invalid

6f31866

merging in python changes

877022b

cmsbuild added this to the CMSSW_9_2_X milestone May 31, 2017

cmsbuild added analysis-pending comparison-pending dqm-pending orp-pending pending-signatures reconstruction-pending tests-pending labels May 31, 2017

cmsbuild added tests-started and removed tests-pending labels May 31, 2017

cmsbuild added tests-approved and removed tests-started labels May 31, 2017

cmsbuild added comparison-available and removed comparison-pending labels May 31, 2017

slava77 reviewed Jun 1, 2017

View reviewed changes

cmsbuild added the tests-pending label Jun 2, 2017

cmsbuild added tests-started and removed tests-pending labels Jun 2, 2017

cmsbuild added tests-approved and removed tests-started labels Jun 2, 2017

cmsbuild added comparison-available and removed comparison-pending labels Jun 2, 2017

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jun 2, 2017

cmsbuild added dqm-approved and removed dqm-pending labels Jun 6, 2017

davidlange6 reviewed Jun 7, 2017

View reviewed changes

davidlange6 merged commit 76d3430 into cms-sw:master Jun 7, 2017

mtosi mentioned this pull request Jun 8, 2017

HLT DQM #19142

Open

fwyzard mentioned this pull request Jul 28, 2017

backport HLT DQM for menu v2.1 to CMSSW 9.2.x #19961

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E/gamma T&P Offline DQM : 92X + VID + GenericTrigEventFlag Changes #19046

E/gamma T&P Offline DQM : 92X + VID + GenericTrigEventFlag Changes #19046

Sam-Harper commented May 31, 2017

cmsbuild commented May 31, 2017

slava77 commented May 31, 2017

cmsbuild commented May 31, 2017 •

edited

cmsbuild commented May 31, 2017

cmsbuild commented May 31, 2017

cmsbuild commented May 31, 2017

slava77 Jun 1, 2017

Sam-Harper Jun 1, 2017

cmsbuild commented Jun 2, 2017

Sam-Harper commented Jun 2, 2017

mtosi commented Jun 2, 2017 via email

perrotta commented Jun 2, 2017

cmsbuild commented Jun 2, 2017 •

edited

cmsbuild commented Jun 2, 2017

cmsbuild commented Jun 2, 2017

cmsbuild commented Jun 2, 2017

slava77 commented Jun 2, 2017

dmitrijus commented Jun 6, 2017

mtosi commented Jun 6, 2017

davidlange6 Jun 7, 2017

Sam-Harper Jun 7, 2017

davidlange6 Jun 7, 2017

fwyzard commented Jun 8, 2017

E/gamma T&P Offline DQM : 92X + VID + GenericTrigEventFlag Changes #19046

E/gamma T&P Offline DQM : 92X + VID + GenericTrigEventFlag Changes #19046

Conversation

Sam-Harper commented May 31, 2017

cmsbuild commented May 31, 2017

slava77 commented May 31, 2017

cmsbuild commented May 31, 2017 • edited

cmsbuild commented May 31, 2017

cmsbuild commented May 31, 2017

cmsbuild commented May 31, 2017

slava77 Jun 1, 2017

Choose a reason for hiding this comment

Sam-Harper Jun 1, 2017

Choose a reason for hiding this comment

cmsbuild commented Jun 2, 2017

Sam-Harper commented Jun 2, 2017

mtosi commented Jun 2, 2017 via email

perrotta commented Jun 2, 2017

cmsbuild commented Jun 2, 2017 • edited

cmsbuild commented Jun 2, 2017

cmsbuild commented Jun 2, 2017

cmsbuild commented Jun 2, 2017

slava77 commented Jun 2, 2017

dmitrijus commented Jun 6, 2017

mtosi commented Jun 6, 2017

davidlange6 Jun 7, 2017

Choose a reason for hiding this comment

Sam-Harper Jun 7, 2017

Choose a reason for hiding this comment

davidlange6 Jun 7, 2017

Choose a reason for hiding this comment

fwyzard commented Jun 8, 2017

cmsbuild commented May 31, 2017 •

edited

cmsbuild commented Jun 2, 2017 •

edited