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

Dqm single electron jet #19290

Merged
merged 55 commits into from
Jul 25, 2017
Merged

Dqm single electron jet #19290

merged 55 commits into from
Jul 25, 2017

Conversation

parbol
Copy link
Contributor

@parbol parbol commented Jun 16, 2017

This PR adds the monitoring for the singleElectron_PFJet triggers used to collect fake-rate control sample. It uses the code proposed in: #19119 by the top PAG, in the corresponding SusyMonitoring configuration files.

defranchis and others added 30 commits May 27, 2017 10:36
adding variable binning + multiplicity histograms
Update top hlt dqm code to include PV cuts to be used by higgs montoring
@fwyzard
Copy link
Contributor

fwyzard commented Jul 2, 2017

@parbol, this PR has automatically been moved to CMSSW 9.3.x .
After fixing the merge conflict, can you make a new PR for CMSSW 9.2.x ?

@davidlange6
Copy link
Contributor

@parbol while waiting for your DQM review - there is now a base class appropriate for your new modules available at https://github.com/cms-sw/cmssw/blob/master/DQMOffline/Trigger/plugins/TriggerDQMBase.h. Please have a look and adapt your module. In addition to removing code duplication, you will have addressed a number of the "usual" comments on PRs like this one likely resulting in a faster review.

(also your PR is not mergeable)

fwyzard added a commit to fwyzard/cmssw that referenced this pull request Jul 11, 2017
@fwyzard fwyzard mentioned this pull request Jul 11, 2017
@fwyzard
Copy link
Contributor

fwyzard commented Jul 12, 2017

@dmitrijus do you have any comments for this PR ?
@davidlange6 do you have any comments for this PR ?

I plan to include it in a second "merged" PR to fix the various conflicts.


TopMonitor::~TopMonitor()
{
if (num_genTriggerEventFlag_) num_genTriggerEventFlag_.reset();
Copy link
Contributor

@dmitrijus dmitrijus Jul 13, 2017

Choose a reason for hiding this comment

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

You don't need to explicitly reset the unique_ptr. It will be done automatically once unique_ptr will be destroyed.

@dmitrijus
Copy link
Contributor

+1
Made a small comment for the code, but it can be merged.

@cmsbuild
Copy link
Contributor

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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@fwyzard
Copy link
Contributor

fwyzard commented Jul 13, 2017

Please do not rebase this.
As soon as @dmitrijus (done) and @davidlange6 let me know that the PR is fine (apart from the conflict) I will fix it in a separate PR.

@fwyzard
Copy link
Contributor

fwyzard commented Jul 23, 2017

@davidlange6 do you have any comments for this PR, apart from the needed rebase ?

@davidlange6
Copy link
Contributor

hi @fwyzard - i don't really know what should be reviewed - can you identify the commits? if its just the 3 from @parbol - then it would be easy to proceed from a rebase removing the other 80% of content

fwyzard added a commit to fwyzard/cmssw that referenced this pull request Jul 24, 2017
Conflicts:
	DQMOffline/Trigger/python/SusyMonitoring_Client_cff.py
	DQMOffline/Trigger/python/SusyMonitoring_cff.py
@fwyzard
Copy link
Contributor

fwyzard commented Jul 24, 2017

hi @davidlange6,
once rebased on top of the current 9.3.x IB, this PR touches only python files.

I have done a minimal clean up, splitting two large python files, and pushed it to #19891 .

@fwyzard
Copy link
Contributor

fwyzard commented Jul 24, 2017

(please keep this PR open and as-is until #19891 is merged)

@cmsbuild cmsbuild merged commit ba1b6d5 into cms-sw:master Jul 25, 2017
@fwyzard
Copy link
Contributor

fwyzard commented Jul 25, 2017

@parbol , after merging this PR (via #19891) we are seeing errors like

----- Begin Fatal Exception 25-Jul-2017 14:24:05 CEST-----------------------
An exception of category 'FatalRootError' occurred while
   [0] Processing  stream end Run run: 297227 stream: 3
   [1] Calling method for module TopMonitor/'susyEle17CaloIdMJet30_ele'
   Additional Info:
      [a] Fatal Root Error: @SUB=Merge
Cannot merge histograms - limits are inconsistent:
 first: elePtEta_1_denominator (9, 0.000000, 400.000000), second: elePtEta_1_denominator (10, 0.000000, 400.000000)

----- End Fatal Exception -------------------------------------------------

Looking at DQMOffline/Trigger/python/susyHLTEleCaloJets_cff.py (where I moved the definition of the SUSY electron_jet cross-trigger plots) I see that multiple modules re-use the same folders:

susyEle8CaloJet30_jet.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/JetMonitor')
susyEle8CaloJet30_ele.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/ElectronMonitor')
susyEle8CaloJet30_all.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/GlobalMonitor')
susyEle8CaloIdMJet30_jet.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/JetMonitor')
susyEle8CaloIdMJet30_ele.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/ElectronMonitor')
susyEle8CaloIdMJet30_all.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/GlobalMonitor')
susyEle12CaloJet30_jet.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/JetMonitor')
susyEle12CaloJet30_ele.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/ElectronMonitor')
susyEle12CaloJet30_all.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/GlobalMonitor')
susyEle17CaloIdMJet30_jet.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/JetMonitor')
susyEle17CaloIdMJet30_ele.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/ElectronMonitor')
susyEle17CaloIdMJet30_all.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/GlobalMonitor')
susyEle23CaloJet30_jet.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/JetMonitor')
susyEle23CaloJet30_ele.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/ElectronMonitor')
susyEle23CaloJet30_all.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/GlobalMonitor')
susyEle23CaloIdMJet30_jet.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/JetMonitor')
susyEle23CaloIdMJet30_ele.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/ElectronMonitor')
susyEle23CaloIdMJet30_all.FolderName = cms.string('HLT/SusyHLTOffline/SusyMonitor/EleJet/GlobalMonitor')

Is it intended ?
Won't this cause the different modules to fill the same histograms multiple times (and possibly lead to conflicts if the binning is different) ?

@fwyzard
Copy link
Contributor

fwyzard commented Jul 25, 2017

If the aim is to use different folders (and/or the same binning), can you make a PR to fix it, based on the latest 9.3.x IB ?

@fwyzard fwyzard mentioned this pull request Jul 25, 2017
@fwyzard
Copy link
Contributor

fwyzard commented Jul 25, 2017

@parbol, can you check if #19912 fixes this in the intended way ?

fwyzard added a commit to fwyzard/cmssw that referenced this pull request Jul 28, 2017
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 added a commit to fwyzard/cmssw that referenced this pull request Sep 3, 2017
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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet