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.

Sign up for GitHub

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

Jump to bottom

Adding revised module for lepton(s) + HT trigger DQM #19577

Merged
merged 5 commits into from Jul 18, 2017
Merged

Adding revised module for lepton(s) + HT trigger DQM #19577

merged 5 commits into from Jul 18, 2017

Conversation

heller3
Copy link
Contributor

@heller3 heller3 commented Jul 5, 2017

Adds a module that makes efficiency histograms for the following triggers:
HLT_Ele15_HT600
HLT_Mu15_HT600
HLT_Ele15_HT450
HLT_Ele50_HT450
HLT_Mu15_HT450
HLT_Mu50_HT450
HLT_DoubleEle8_CaloIdM_TrackIdM_Mass8_DZ_PFHT350
HLT_DoubleEle8_CaloIdM_TrackIdM_Mass8_PFHT350
HLT_DoubleMu4_Mass8_DZ_PFHT350
HLT_DoubleMu4_Mass8_PFHT350
HLT_Mu8_Ele8_CaloIdM_TrackIdM_Mass8_PFHT350_DZ
HLT_Mu8_Ele8_CaloIdM_TrackIdM_Mass8_PFHT350

Each one has monitoring of efficiency as a function of lepton pT, eta and phi, as well as HT and N_PV.
This PR supersedes #19305

This module was tested on the following sample:
/store/relval/CMSSW_9_2_4/RelValTTbarLepton_13/GEN-SIM-DIGI-RAW
in release CMSSW_9_2_4

@cmsbuild cmsbuild added this to the CMSSW_9_3_X milestone Jul 5, 2017
@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2017

A new Pull Request was created by @heller3 for master.

It involves the following packages:

DQMOffline/Trigger

@vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @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

@heller3 heller3 mentioned this pull request Jul 5, 2017
Closed
@dmitrijus
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 6, 2017 •
edited

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 6, 2017

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 6, 2017

-1

Tested at: e53ce31

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
60d8503
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19577/21211/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19577/21211/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19577/21211/summary.html

I found follow errors while testing this PR

Failed tests: Build

  • Build:

I found an error when building:

gmake[1]: Target 'PostBuild' not remade because of errors.
gmake[1]: Leaving directory '/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_3_X_2017-07-05-2300'
config/SCRAM/GMake/Makefile.rules:2058: recipe for target 'src' failed
gmake: *** [src] Error 2
gmake: Target 'all' not remade because of errors.
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
60d8503
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19577/21211/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19577/21211/git-merge-result

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 6, 2017

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

@fwyzard
Copy link
Contributor

fwyzard commented Jul 12, 2017

@davidlange6 this PR is signed by DQM, and I think all comments have been addressed.

Do you have any objections if I include it in a second "merged" PR to fix the various conflicts ?

@cmsbuild
Copy link
Contributor

+1
Tested at: 9eff198
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19577/21389/summary.html

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

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • You potentially added 10 lines to the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 2015567
  • DQMHistoTests: Total failures: 29451
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1985950
  • DQMHistoTests: Total skipped: 166
  • DQMHistoTests: Total Missing objects: 0
  • Checked 93 log files, 14 edm output root files, 23 DQM output files

davidlange6
davidlange6 reviewed Jul 13, 2017
View reviewed changes
DQMOffline/Trigger/plugins/LepHTMonitor.cc

if(!pass_id) return false;

}catch(...){
Copy link
Contributor

Choose a reason for hiding this comment

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

please avoid try catch. (I'm not really sure what possible issues you are looking to avoid here and above so am not sure what alternative to suggest)

Copy link
Contributor

Choose a reason for hiding this comment

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

none of the called methods throws, as far as I can see from their code; I'll simply remove the try and the catch blocks.

davidlange6
davidlange6 reviewed Jul 13, 2017
View reviewed changes
DQMOffline/Trigger/plugins/LepHTMonitor.cc
int npv=0;
//Vertex
edm::Handle<reco::VertexCollection> VertexCollection;
if(theVertexCollectionTag_.label() != ""){
Copy link
Contributor

Choose a reason for hiding this comment

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

please do these string comparisons once and store them (eg, in the constructor?)

Copy link
Contributor

Choose a reason for hiding this comment

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

to avoid adding extra variables, I propose to use if (not theVertexCollectionTag_.label().empty()) which is just a pointer comparison, and should be as fast a checking a bool.

davidlange6
davidlange6 reviewed Jul 13, 2017
View reviewed changes
DQMOffline/Trigger/plugins/LepHTMonitor.cc
for(const auto &electron: *ElectronCollection){
const auto el = ElectronCollection->ptrAt(index);
bool pass_id = (*ele_id_decisions)[el];
if(isGood(electron, VertexCollection->front().position(),
Copy link
Contributor

Choose a reason for hiding this comment

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

the indentation here is confusing - please try to use a few spaces to indent for loops/ifs (perhaps your editor can do this for you) and avoid the '}' to be at the end of a line when you have multiline loops [eg, line 406]

Copy link
Contributor

Choose a reason for hiding this comment

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

it also doesn't help to mix tabs and spaces - they look OK on the terminal but not so much on GitHub

davidlange6
davidlange6 reviewed Jul 13, 2017
View reviewed changes
DQMOffline/Trigger/plugins/LepHTMonitor.h
@@ -0,0 +1,142 @@
#ifndef LepHTMonitor_H
Copy link
Contributor

Choose a reason for hiding this comment

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

please include the subsystem and package name in the include guard

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

@davidlange6
Copy link
Contributor

otherwise how many histogram bins are added?

@heller3
Copy link
Contributor Author

heller3 commented Jul 13, 2017

@davidlange6 This module adds 107 histogram bins per trigger, x3 copies for numerator, denominator, and efficiency. Is that what you mean?
If necessary it can be slimmed.

@fwyzard
Copy link
Contributor

fwyzard commented Jul 13, 2017

How many bins in total ?

@heller3
Copy link
Contributor Author

heller3 commented Jul 13, 2017

@fwyzard Total bins: 12 triggers x 107 bins x 3 sets of hists = 3852 bins total

fwyzard added a commit to fwyzard/cmssw that referenced this pull request Jul 13, 2017
@fwyzard
Merge pull request cms-sw#19577 from heller3/LepHTMonitoring
97ebe39
fwyzard added a commit to fwyzard/cmssw that referenced this pull request Jul 13, 2017
@fwyzard
Merge pull request cms-sw#19577 from heller3:LepHTMonitoring
2d500f5
@fwyzard fwyzard mentioned this pull request Jul 13, 2017
Merged
@fwyzard
Copy link
Contributor

fwyzard commented Jul 13, 2017

I think I have addressed all comments, as well as resolved the conflicts, in #19731

@fwyzard
Copy link
Contributor

fwyzard commented Jul 13, 2017

Please keep this PR open for the time being, without making any further changes to it.

@heller3
Copy link
Contributor Author

heller3 commented Jul 13, 2017

@fwyzard I need to change one wrong trigger path in LepHTMonitor_cff.py: HLT_DoubleMu4_Mass8_PFHT350 should be HLT_DoubleMu8_Mass8_PFHT350, on lines 151 and 152
(the version without DZ cut has pT threshold of 8 instead of 4, for dimuon+HT only)

Do you want me to do it or is it easier if you do?

@fwyzard
Copy link
Contributor

fwyzard commented Jul 14, 2017 •
edited

@heller3 please wait for #19731 to be merged, then make a new PR with the update.

@davidlange6 davidlange6 merged commit 9eff198 into cms-sw:master Jul 18, 2017
fwyzard added a commit to fwyzard/cmssw that referenced this pull request Jul 28, 2017
@fwyzard
backport HLT DQM for menu v2.1 to CMSSW 9.2.x
71bfa51 
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
@fwyzard fwyzard mentioned this pull request Jul 28, 2017
Closed
fwyzard added a commit to fwyzard/cmssw that referenced this pull request Sep 3, 2017
@fwyzard
backport HLT DQM for menu v2.1 to CMSSW 9.2.x
20188a1 
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
mtosi pushed a commit to mtosi/cmssw that referenced this pull request Oct 17, 2017
@fwyzard @mtosi
backport HLT DQM for menu v2.1 to CMSSW 9.2.x
2befd4c 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fwyzard fwyzard fwyzard left review comments

@mtosi mtosi mtosi left review comments

@Sam-Harper Sam-Harper Sam-Harper left review comments

@davidlange6 davidlange6 davidlange6 left review comments

Assignees
No one assigned
Labels
comparison-available dqm-approved fully-signed orp-pending tests-approved
Projects
None yet
Milestone
CMSSW_9_3_X
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@heller3 @cmsbuild @dmitrijus @mtosi @Sam-Harper @lathomas @fwyzard @davidlange6