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

Offline DQM for Top HLT #18981

Closed
wants to merge 34 commits into from
Closed

Conversation

defranchis
Copy link
Contributor

A few features are not yet implemented and will be added in the upcoming days

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @defranchis (Matteo Defranchis) for master.

It involves the following packages:

DQMOffline/Trigger

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

eleJet_jet.eleSelection = cms.string('pt>50 & abs(eta)<2.1')
eleJet_jet.jetSelection = cms.string('pt>30 & abs(eta)<2.4')
eleJet_jet.numGenericTriggerEventPSet.hltPaths = cms.vstring('HLT_Ele30_eta2p1_WPTight_Gsf_CentralPFJet35_EleCleaned_v*')
eleJet_jet.denGenericTriggerEventPSet.hltPaths = cms.vstring('HLT_Ele30_eta2p1_WPTight_Gsf_v*',
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure those triggers are available in 2017 HLT menu ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HLT_Ele30_eta2p1_WPTight_Gsf_CentralPFJet35_EleCleaned_v1 is in /dev/CMSSW_9_1_0/HLT
https://its.cern.ch/jira/browse/CMSHLT-1268

eleJet_jet.numGenericTriggerEventPSet.hltPaths = cms.vstring('HLT_Ele30_eta2p1_WPTight_Gsf_CentralPFJet35_EleCleaned_v*')
eleJet_jet.denGenericTriggerEventPSet.hltPaths = cms.vstring('HLT_Ele30_eta2p1_WPTight_Gsf_v*',
'HLT_Ele35_WPTight_Gsf_v*',
'HLT_Ele38_WPTight_Gsf_v*',
Copy link
Contributor

Choose a reason for hiding this comment

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

38 and 40 are monitored by EGM tag-n-probe
while 35 is not monitored (?) and 30 seems to not be available, so far

Copy link
Contributor

Choose a reason for hiding this comment

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

35 GeV was an oversight and will be added to EGM tag-n-probe. Thanks for the spot

eleJet_ele.eleSelection = cms.string('pt>25 & abs(eta)<2.1')
eleJet_ele.jetSelection = cms.string('pt>50 & abs(eta)<2.4')
eleJet_ele.numGenericTriggerEventPSet.hltPaths = cms.vstring('HLT_Ele30_eta2p1_WPTight_Gsf_CentralPFJet35_EleCleaned_v*')
eleJet_ele.denGenericTriggerEventPSet.hltPaths = cms.vstring('HLT_PFJet60_v*')
Copy link
Contributor

Choose a reason for hiding this comment

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

who is monitoring this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The BTV and SMP groups will take care of the HLT_PFJet* paths

topDiElectronHLTValidation.eleSelection = cms.string('pt>20 & abs(eta)<2.5 & (dr03TkSumPt+dr03EcalRecHitSumEt+dr03HcalTowerSumEt)/pt < 0.1')
topDiElectronHLTValidation.muoSelection = cms.string('pt>20 & abs(eta)<2.4 & (pfIsolationR04.sumChargedHadronPt+pfIsolationR04.sumPhotonEt+pfIsolationR04.sumNeutralHadronEt)/pt < 0.12')
topDiElectronHLTValidation.jetSelection = cms.string('pt>30 & abs(eta)<2.5')
topDiElectronHLTValidation.numGenericTriggerEventPSet.hltPaths = cms.vstring(['HLT_Ele12_Ele23_CaloIdL_TrackIdL_IsoVL_DZ_v*'])
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure this path is available in 2017 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is in /dev/CMSSW_9_1_0/HLT

Copy link
Contributor

Choose a reason for hiding this comment

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

which JIRA ticket ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry I actually replied to the wrong comment. I have to ask the responsibles for the di-lepton

Copy link
Contributor

Choose a reason for hiding this comment

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

;)

@mtosi
Copy link
Contributor

mtosi commented May 29, 2017

what about HLT_Ele30_eta2p1_WPTight_Gsf_CentralPFJet35_EleCleaned ?

@@ -0,0 +1,541 @@
#include "DQMOffline/Trigger/plugins/TopMonitor.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should update the name of this class
unless you are going to add observables really TOP dependent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In principle top dependent variables can be added. I would keep it as it is for the moment...

, ls_binning_ ( getHistoLSPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("lsPSet") ) )
, phi_binning_ ( getHistoPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("phiPSet") ) )
, pt_binning_ ( getHistoPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("ptPSet") ) )
, eta_binning_ ( getHistoPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("etaPSet") ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be variable binning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to add the variable binning in the next days

Copy link
Contributor

Choose a reason for hiding this comment

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

please, we have to converge on this PR before the end of this week
thanks !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confident we will

, met_binning_ ( getHistoPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("metPSet") ) )
, ls_binning_ ( getHistoLSPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("lsPSet") ) )
, phi_binning_ ( getHistoPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("phiPSet") ) )
, pt_binning_ ( getHistoPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("ptPSet") ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be variable binning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

, phi_binning_ ( getHistoPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("phiPSet") ) )
, pt_binning_ ( getHistoPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("ptPSet") ) )
, eta_binning_ ( getHistoPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("etaPSet") ) )
, HT_binning_ ( getHistoPSet (iConfig.getParameter<edm::ParameterSet>("histoPSet").getParameter<edm::ParameterSet> ("htPSet") ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be variable binning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

verbose = cms.untracked.uint32(0), # Set to 2 for all messages
resolution = cms.vstring(),
efficiency = cms.vstring(
"effic_metME 'efficiency vs MET; MET [GeV]; efficiency' metME_numerator metME_denominator",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can play w/ the python

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but I think it would make the code less easily understandable for others who might want to edit or use it

Copy link
Contributor

Choose a reason for hiding this comment

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

please change all cms.EDAnalyzer("DQMGenericClient" instances to cms.EDProducer("DQMGenericClient" to adapt to #18751

The tests and integration will fail otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry because I'm reverse engineering these changes I think I got them wrong
cms.EDAnalyzer("DQMGenericClient",

to

DQMEDHarvester("DQMGenericClient",

and you need to have

from DQMServices.Core.DQMEDHarvester import DQMEDHarvester

in your config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Sam. How can I get relevant updates for that? I would like to keep testing my code with my current setup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sam-Harper done!

@cmsbuild
Copy link
Contributor

Pull request #18981 was updated. @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please check and sign again.

@cmsbuild
Copy link
Contributor

Pull request #18981 was updated. @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please check and sign again.

@cmsbuild
Copy link
Contributor

Pull request #18981 was updated. @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please check and sign again.

@cmsbuild
Copy link
Contributor

Pull request #18981 was updated. @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please check and sign again.

@cmsbuild
Copy link
Contributor

Pull request #18981 was updated. @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please check and sign again.

@mtosi
Copy link
Contributor

mtosi commented May 31, 2017

@defranchis we are done, aren't we ?

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2017

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

Comparison Summary:

  • You potentially added 140 lines to the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1872323
  • DQMHistoTests: Total failures: 34695
  • DQMHistoTests: Total nulls: 130
  • DQMHistoTests: Total successes: 1837325
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@mtosi
Copy link
Contributor

mtosi commented Jun 12, 2017

@dmitrijus, if you are fine w/ this, ca you please sign it ?
thanks !

@mtosi
Copy link
Contributor

mtosi commented Jun 12, 2017

I still do not see
HLT_PFHT430_SixJet40
HLT_PFHT350

@defranchis
Copy link
Contributor Author

@mtosi both paths are monitored in TopMonitoring_cff.py

@mtosi
Copy link
Contributor

mtosi commented Jun 12, 2017

what about
HLT_HT300PT30_QuadJet_75_60_45_40_TripeCSV_p07_v*
HLT_HT300PT30_QuadJet_75_60_45_40_v*
?

jira ticket https://its.cern.ch/jira/browse/CMSHLT-1352
points to this PR (?!?!?!)

@defranchis
Copy link
Contributor Author

defranchis commented Jun 12, 2017

There is an open PR to this branch defranchis#18 . As we discussed, the idea is to include those changes in #19119 instead

@mtosi
Copy link
Contributor

mtosi commented Jun 12, 2017

fine,
what is needed is to be in a CMSSW PR ....

@dmitrijus
Copy link
Contributor

+1

@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

@dmitrijus
Copy link
Contributor

(close if changes are included elsewhere)

@davidlange6
Copy link
Contributor

shall we close this in favor of #19119?

@defranchis
Copy link
Contributor Author

defranchis commented Jun 13, 2017

we should merge this one first, then #19119

@defranchis
Copy link
Contributor Author

@davidlange6 but actually we could also close this in favour of #19119 as you suggested

@davidlange6
Copy link
Contributor

davidlange6 commented Jun 13, 2017 via email

@fwyzard
Copy link
Contributor

fwyzard commented Jun 14, 2017

The paths

HLT_PFHT380_SixJet32_v*
HLT_PFHT430_SixJet40_v

are not being monitored, and will not be included in the HLT menu.

In fact this also invalidates all the monitoring that use them as reference...

@defranchis
Copy link
Contributor Author

@fwyzard I will add them in #19119

@davidlange6
Copy link
Contributor

seems this is superseded by #19119

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

10 participants