L1TObjects Ratio Timing Plots - L1T & Online DQM - CMSSW - 102X #22524
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22524/3832 |
|
A new Pull Request was created by @dtsitson for master. It involves the following packages: DQM/L1TMonitor @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @vanbesien can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| @@ -103,6 +103,11 @@ class L1TObjectsTiming : public DQMEDAnalyzer { | |||
| std::vector<MonitorElement*> etsum_eta_phi_METHF_lastbunch; | |||
| std::vector<MonitorElement*> etsum_eta_phi_MHT_lastbunch; | |||
| std::vector<MonitorElement*> etsum_eta_phi_MHTHF_lastbunch; | |||
|
|
|||
| MonitorElement* denominator; | |||
There was a problem hiding this comment.
These denominators are only used for the muons. There are denominators missing for jets, egamma, taus, and etsums.
| @@ -134,6 +140,9 @@ void L1TObjectsTiming::bookHistograms(DQMStore::IBooker& ibooker, const edm::Run | |||
| } | |||
|
|
|||
| if(algoBitFirstBxInTrain_ > -1 && algoBitLastBxInTrain_ > -1) { | |||
| ibooker.setCurrentFolder(monitorDir_); | |||
| denominator_isolated = ibooker.book2D("denominator_isolated","Denominator for Isolated Bunch for L1TObjects",25, -2.5, 2.5, 25, -3.2, 3.2); | |||
There was a problem hiding this comment.
Maybe align the histogram titles with the numerator histograms.
| for (int itBX = MuonBxCollection->getFirstBX(); itBX <= MuonBxCollection->getLastBX(); ++itBX) { | ||
| for (l1t::MuonBxCollection::const_iterator Muon = MuonBxCollection->begin(itBX); Muon != MuonBxCollection->end( | ||
| itBX); ++Muon) { | ||
| denominator->Fill(Muon->eta(), Muon->phi()); |
There was a problem hiding this comment.
This line can be moved just before line 318 to get rid of the two for loops.
| # sequences | ||
| l1tObjectsTimingClient = cms.Sequence( | ||
| l1tObjectsRatioPlots | ||
| + l1tMuonFirstBunchRatioPlots |
There was a problem hiding this comment.
Can you try to group the four groups per object into one? They are in different subdirectories but it may be possible to give a histogram title like e.g.
"Isolated_bunch/Ratio_L1TTau_BX_LastBunch_0 'Ratio for L1TTau last bunch for BX=0' Isolated_bunch/tau_eta_phi_bx_lastbunch_0 Isolated_bunch/den_tau_eta_phi_bx_lastbunch_0"
It works for sure for the input histograms. For the histogram that is created I am not sure.
| l1tMuonFirstBunchRatioPlots = DQMEDHarvester("DQMGenericClient", | ||
| subDirs = cms.untracked.vstring(l1tobjectstimingDqmDir+'L1TMuon/timing/First_bunch/'), | ||
| efficiency = cms.vstring( | ||
| "Ratio_L1TMuons_BX_FirstBunch_minus2 'Ratio for L1TMuons first bunch for BX=-2' muons_eta_phi_bx_firstbunch_minus2 den_muons_eta_phi_bx_firstbunch_minus2", |
There was a problem hiding this comment.
The denominator plot does not exist.
| "Ratio_L1TMuons_BX_0 'Ratio for L1TMuons for BX=0' L1TMuon/timing/muons_eta_phi_bx_0 denominator", | ||
| "Ratio_L1TMuons_BX_plus1 'Ratio for L1TMuons for BX=+1' L1TMuon/timing/muons_eta_phi_bx_plus1 denominator", | ||
| "Ratio_L1TMuons_BX_plus2 'Ratio for L1TMuons for BX=+2' L1TMuon/timing/muons_eta_phi_bx_plus2 denominator", | ||
| "Ratio_L1TJet_BX_minus2 'Ratio for L1TJet for BX=-2' L1TJet/timing/jet_eta_phi_bx_minus2 denominator", |
There was a problem hiding this comment.
You still need separate denominators for the muons, jet, taus, egamma, and etsums.
| # L1 event info DQM client | ||
| from DQM.L1TMonitorClient.L1TStage2EventInfoClient_cfi import * | ||
|
|
||
| # CaloLayer2 client | ||
| from DQM.L1TMonitorClient.L1TStage2CaloLayer2Client_cff import * | ||
|
|
||
| # BMTF client | ||
| from DQM.L1TMonitorClient.L1TStage2BMTFClient_cff import * |
There was a problem hiding this comment.
Please revert these changes since they have nothing to do with the timing.
| # uGMT client | ||
| from DQM.L1TMonitorClient.L1TStage2uGMTClient_cff import * | ||
|
|
||
| # uGT client | ||
| from DQM.L1TMonitorClient.L1TStage2uGTClient_cff import * | ||
|
|
||
| # L1 event info DQM client EMTF | ||
| from DQM.L1TMonitorClient.L1TStage2EMTFEventInfoClient_cfi import * |
There was a problem hiding this comment.
Please revert these changes since they have nothing to do with the timing.
| # define sequences | ||
| # | ||
|
|
||
| # L1T monitor client sequence (system clients and quality tests) | ||
| l1TStage2Clients = cms.Sequence( | ||
| l1tStage2EventInfoClient | ||
| + l1tStage2uGTCaloLayer2CompClient | ||
| + l1tStage2BmtfClient |
There was a problem hiding this comment.
Please revert these changes since they have nothing to do with the timing.
| + l1tStage2uGMTClient | ||
| + l1tStage2uGTClient | ||
| + l1tStage2EMTFEventInfoClient |
There was a problem hiding this comment.
Please revert the removal of this module.
…m-CMSSW_10_1_0_pre2 I also fixed the second part of the algorithm trigger timing plots dividing two different histograms for unprescaled and prescaled trigger bits .
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22524/3934 |
|
Pull request #22524 was updated. @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @vanbesien can you please check and sign again. |
| @@ -75,9 +75,16 @@ class L1TStage2uGT: public DQMEDAnalyzer { | |||
| int algoBitLastBxInTrain_; | |||
| const std::string algoNameFirstBxInTrain_; | |||
| const std::string algoNameLastBxInTrain_; | |||
|
|
|||
|
|
|||
| int algoBitUnpre_; | |||
There was a problem hiding this comment.
These two variables do not need to be class members.
| int algoBitUnpre_; | ||
| int algoBitDis_; | ||
| std::vector<std::string> unprescaledAlgoShortList_ ; | ||
| std::vector<std::string> disabledAlgoShortList_; |
There was a problem hiding this comment.
Better call the variables prescaledAlgo*** instead of disabledAlgo*** since not all algos in the list will be disabled.
| @@ -1,10 +1,11 @@ | |||
| import FWCore.ParameterSet.Config as cms | |||
|
|
|||
| from DQMServices.Core.DQMEDAnalyzer import DQMEDAnalyzer | |||
| l1tStage2uGT = DQMEDAnalyzer('L1TStage2uGT', | |||
| l1tStage2uGT = cms.EDAnalyzer("L1TStage2uGT", | |||
There was a problem hiding this comment.
Please revert the changes in these two lines.
DQM/L1TMonitor/src/L1TStage2uGT.cc
Outdated
| @@ -20,7 +20,12 @@ L1TStage2uGT::L1TStage2uGT(const edm::ParameterSet& params): | |||
| algoBitFirstBxInTrain_(-1), | |||
| algoBitLastBxInTrain_(-1), | |||
| algoNameFirstBxInTrain_(params.getUntrackedParameter<std::string>("firstBXInTrainAlgo","")), | |||
| algoNameLastBxInTrain_(params.getUntrackedParameter<std::string>("lastBXInTrainAlgo","")) | |||
| algoNameLastBxInTrain_(params.getUntrackedParameter<std::string>("lastBXInTrainAlgo","")), | |||
| algoBitUnpre_(-1), | |||
There was a problem hiding this comment.
No need to make these two variables class members. They are only needed in the constructor and can be declared locally there.
DQM/L1TMonitor/src/L1TStage2uGT.cc
Outdated
| // Prescaled and Unprescaled Algo Trigger Bits | ||
| prescaled_first_collision_run_ = ibooker.book2D("prescaled_first_collision_run_", "uGT: Prescaled Algorithm Trigger Bits (First bunch) vs. BX Number In Event", 5, -2.5, 2.5, numAlgs, -0.5, numAlgs_d-0.5); | ||
| prescaled_first_collision_run_->setAxisTitle("Bunch Crossing Number In Event", 1); | ||
| prescaled_first_collision_run_->setAxisTitle("Algorithm Trigger Bits (First bunch in train)", 2); |
There was a problem hiding this comment.
Please label the y axis bins with the algo name + the algo bit.
E.g.: L1_SingleMu25 (23)
Think about what happens if an algo name given in the configuration does not exist. Does the algorithm still work in such a case? Only existing algos should end up in the histogram.
DQM/L1TMonitor/src/L1TStage2uGT.cc
Outdated
| for(unsigned int algo=0; algo<disabledAlgoBit_.size(); algo++) { | ||
| if(disabledAlgoBit_.at(algo) != -1 && itr2->getAlgoDecisionInitial(disabledAlgoBit_.at(algo))) { | ||
| prescaled_first_collision_run_->Fill(ibx, disabledAlgoBit_.at(algo)); | ||
| prescaled_first_collision_run_->setBinLabel(disabledAlgoBit_.at(algo),disabledAlgoShortList_.at(algo),2); |
There was a problem hiding this comment.
You can do this already when you book the histogram. No need to do it every time you fill.
DQM/L1TMonitor/src/L1TStage2uGT.cc
Outdated
| @@ -284,6 +354,82 @@ void L1TStage2uGT::analyze(const edm::Event& evt, const edm::EventSetup& evtSetu | |||
| } // selecting LastCollisionInTrain | |||
| } // end of uGTAlgs = 1 | |||
|
|
|||
|
|
|||
| for(auto itr = uGtAlgs->begin(0); itr != uGtAlgs->end(0); ++itr) { | |||
There was a problem hiding this comment.
Can these be integrated in the existing for loops? At least the first two loops are common as far as I can see.
DQM/L1TMonitor/src/L1TStage2uGT.cc
Outdated
| for(int ibx = uGtAlgs->getFirstBX(); ibx <= uGtAlgs->getLastBX(); ++ibx) { | ||
| for(auto itr2 = uGtAlgs->begin(ibx); itr2 != uGtAlgs->end(ibx); ++itr2) { | ||
| for(unsigned int algo=0; algo<disabledAlgoBit_.size(); algo++) { | ||
| if(disabledAlgoBit_.at(algo) != -1 && itr2->getAlgoDecisionInitial(disabledAlgoBit_.at(algo))) { |
There was a problem hiding this comment.
Sort out the algo bits with -1 already in the dqmBeginRun. You can make a new vector of algo bits containing only the existing ones and use that one from that time onwards. That way the setting of bin labels should be easily done as well when booking the histograms.
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22524/4014 Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22524/4014/git-diff.patch You can run |
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22524/4401 |
|
Pull request #22524 was updated. @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @vanbesien can you please check and sign again. |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
unhold @thomreis please let us know when you are satisfied |
|
Thanks @fabiocos |
|
+1 |
|
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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
cmsbuild
added
dqm-approved
fully-signed
and removed
dqm-pending
pending-signatures
labels
|
+1 |
My task in JIRA is : Timing Plot Improvements
The whole work is to produce ratio timing plots for any L1T Objects for each BX for first,isolated and last bunch. After I added numerators and denominators to L1TObjectsTiming module, I made a new module for inserting ratio plots in the DQM/L1TMonitorClient directory L1TObjectsTimingClient_cff and I modified the L1TStage2MonitorClient_cff module in order to produce ratio plots.