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
L1T DQM Plots for Kalman Filter for BMTF - CMSSW_10_2_X #23389
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23389/4911 |
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 |
please test |
The tests are being triggered in jenkins. |
@@ -16,5 +16,6 @@ | |||
summaryTitle = cms.untracked.string("Summary of comparison between BMTF muons and BMTF emulator muons"), | |||
ignoreBin = cms.untracked.vint32(ignoreBinsDeStage2Bmtf), | |||
verbose = cms.untracked.bool(False), | |||
kalman = cms.untracked.bool(False), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed since false is the default.
DQM/L1TMonitor/src/L1TStage2BMTF.cc
Outdated
bmtf_hwEta->setTitle("kbmtf_hwEta"); | ||
bmtf_hwLocalPhi->setTitle("kbmtf_hwLocalPhi"); | ||
bmtf_hwGlobalPhi->setTitle("kbmtf_hwGlobalPhi"); | ||
kmtf_hwPt = ibooker.book1D("kmtf_hwPt", "HW p_{T}", 521, -0.5, 520.5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To have a bmtf_hwPt and a kmtf_hwPt in the dame directory will cause confusion. I would rather reuse the bmtf_hwPt also for the kalman hw pt and eventually book that one with a different x axis range if you really want that.
DQM/L1TMonitor/src/L1TStage2BMTF.cc
Outdated
bmtf_hwEta_bx->setTitle("kbmtf_hwEta_bx;HW #eta; BX"); | ||
bmtf_hwLocalPhi_bx->setTitle("kbmtf_hwLocalPhi_bx;HW Local #phi; BX"); | ||
bmtf_hwPt_bx->setTitle("kbmtf_hwPt_bx;HW p_{T}; BX"); | ||
kmtf_hwQual_bx = ibooker.book2D("kmtf_hwQual_bx" , "HW Quality vs BX" , 13, -0.5, 12.5, 5, -2.5, 2.5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To have a bmtf_hwQual_bx and a kmtf_hwQual_bx in the dame directory will cause confusion. I would rather reuse the bmtf_hwQual_bx also for the kalman hw qual and eventually book that one with a different x axis range if you really want that.
DQM/L1TMonitor/src/L1TStage2BMTF.cc
Outdated
@@ -74,6 +75,28 @@ void L1TStage2BMTF::bookHistograms(DQMStore::IBooker &ibooker, const edm::Run& i | |||
bmtf_hwQual_bx = ibooker.book2D("bmtf_hwQual_bx" , "HW Quality vs BX" , 20, -0.5, 19.5, 5, -2.5, 2.5); | |||
bmtf_hwQual_bx->setTitle("; HW Quality; BX"); | |||
|
|||
if (kalman) { | |||
ibooker.setCurrentFolder(monitorDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed.
DQM/L1TMonitor/src/L1TStage2BMTF.cc
Outdated
|
||
edm::Handle<l1t::RegionalMuonCandBxCollection> bmtfMuon; | ||
eve.getByToken(bmtfToken, bmtfMuon); | ||
|
||
if (kalman) { | ||
for(int itBX=bmtfMuon->getFirstBX(); itBX<=bmtfMuon->getLastBX(); ++itBX) { | ||
for(l1t::RegionalMuonCandBxCollection::const_iterator itMuon = bmtfMuon->begin(itBX); itMuon != bmtfMuon->end(itBX); ++itMuon) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to do this in separate for loops. Just place it in the for loop where all the other variables are filled and there inside an if (kalman)
statement.
@@ -178,6 +242,13 @@ void L1TStage2RegionalMuonCandComp::bookHistograms(DQMStore::IBooker& ibooker, c | |||
muColl2TrkAddr = ibooker.book2D("muTrkAddrColl2", (muonColl2Title+" mismatching muon track address"+trkAddrIgnoreText).c_str(), 10, -0.5, 9.5, 16, -0.5, 15.5); | |||
muColl2TrkAddr->setAxisTitle("key", 1); | |||
muColl2TrkAddr->setAxisTitle("value", 2); | |||
if (kalman) { | |||
ibooker.setCurrentFolder(monitorDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to set the monitoring directory again.
@@ -45,6 +44,9 @@ class L1TStage2RegionalMuonCandComp : public DQMEDAnalyzer { | |||
bool ignoreBadTrkAddr; | |||
std::vector<int> ignoreBin; | |||
bool verbose; | |||
bool kalman; | |||
|
|||
int nbins=17; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this variable. It does not need to be a class member.
@@ -52,45 +55,91 @@ void L1TStage2RegionalMuonCandComp::bookHistograms(DQMStore::IBooker& ibooker, c | |||
} | |||
|
|||
// Subsystem Monitoring and Muon Output | |||
if(!kalman){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version with less code duplication:
int nbins = 17;
if (kalman) {
nbins += 2;
}
summary = ibooker.book1D("summary", (summaryTitle+trkAddrIgnoreText).c_str(), nbins, 1, nbins+1);
summary->setBinLabel(BXRANGEGOOD, "BX range match", 1);
...
summary->setBinLabel(TRACKADDRBAD, "track address mismatch", 1);
if (kalman) {
summary->setBinLabel(DXYBAD, "DXY mismatch", 1);
summary->setBinLabel(PT2BAD, "P_{T}2 mismatch", 1);
}
The same can be done for the booking of errorSummaryNum
and errorSummaryDen
.
} | ||
|
||
if(!kalman) { | ||
ibooker.setCurrentFolder(monitorDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to set the monitoring directory again.
errorSummaryDen->setBinLabel(RNMUON, "# muon collections", 1); | ||
for (int i = RMUON; i <= RTRACKADDR; ++i) { | ||
if(!kalman){ | ||
ibooker.setCurrentFolder(monitorDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to set the monitoring directory again.
This PR requires the Kalman BMTF unpacker and the Kalman emulator. @panoskatsoulis please provide two PRs for those as well. |
-1 Tested at: 8f78536 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: RelVals
When I ran the RelVals I found an error in the following worklfows: runTheMatrix-results/7.3_CosmicsSPLoose_UP17+CosmicsSPLoose_UP17+DIGICOS_UP17+RECOCOS_UP17+ALCACOS_UP17+HARVESTCOS_UP17/step3_CosmicsSPLoose_UP17+CosmicsSPLoose_UP17+DIGICOS_UP17+RECOCOS_UP17+ALCACOS_UP17+HARVESTCOS_UP17.log136.731 step3 runTheMatrix-results/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2/step3_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2.log136.788 step3 runTheMatrix-results/136.788_RunSinglePh2017B+RunSinglePh2017B+HLTDR2_2017+RECODR2_2017reHLT_skimSinglePh_Prompt+HARVEST2017/step3_RunSinglePh2017B+RunSinglePh2017B+HLTDR2_2017+RECODR2_2017reHLT_skimSinglePh_Prompt+HARVEST2017.log150.0 step3 runTheMatrix-results/150.0_HydjetQ_B12_5020GeV_2018+HydjetQ_B12_5020GeV_2018+DIGIHI2018+RECOHI2018+HARVESTHI2018/step3_HydjetQ_B12_5020GeV_2018+HydjetQ_B12_5020GeV_2018+DIGIHI2018+RECOHI2018+HARVESTHI2018.log1306.0 step3 runTheMatrix-results/1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15/step3_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15.log136.85 step3 runTheMatrix-results/136.85_RunEGamma2018A+RunEGamma2018A+HLTDR2_2018+RECODR2_2018reHLT_skimEGamma_Prompt_L1TEgDQM+HARVEST2018_L1TEgDQM/step3_RunEGamma2018A+RunEGamma2018A+HLTDR2_2018+RECODR2_2018reHLT_skimEGamma_Prompt_L1TEgDQM+HARVEST2018_L1TEgDQM.log1330.0 step3 runTheMatrix-results/1330.0_ZMM_13+ZMM_13+DIGIUP15+RECOUP15_L1TMuDQM+HARVESTUP15_L1TMuDQM/step3_ZMM_13+ZMM_13+DIGIUP15+RECOUP15_L1TMuDQM+HARVESTUP15_L1TMuDQM.log10042.0 step3 runTheMatrix-results/10042.0_ZMM_13+ZMM_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017/step3_ZMM_13+ZMM_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017.log10024.0 step3 runTheMatrix-results/10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017/step3_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+HARVESTFull_2017+ALCAFull_2017.log25202.0 step3 runTheMatrix-results/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/step3_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25.log10224.0 step3 runTheMatrix-results/10224.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017PU_GenSimFull+DigiFullPU_2017PU+RecoFullPU_2017PU+HARVESTFullPU_2017PU/step3_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017PU_GenSimFull+DigiFullPU_2017PU+RecoFullPU_2017PU+HARVESTFullPU_2017PU.log11624.0 step3 runTheMatrix-results/11624.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2019_GenSimFull+DigiFull_2019+RecoFull_2019+HARVESTFull_2019+ALCAFull_2019/step3_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2019_GenSimFull+DigiFull_2019+RecoFull_2019+HARVESTFull_2019+ALCAFull_2019.log10824.0 step3 runTheMatrix-results/10824.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+HARVESTFull_2018+ALCAFull_2018/step3_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+HARVESTFull_2018+ALCAFull_2018.log250202.181 step4 runTheMatrix-results/250202.181_TTbar_13UP18+TTbar_13UP18+PREMIXUP18_PU25+DIGIPRMXLOCALUP18_PU25+RECOPRMXUP18_PU25+HARVESTUP18_PU25/step4_TTbar_13UP18+TTbar_13UP18+PREMIXUP18_PU25+DIGIPRMXLOCALUP18_PU25+RECOPRMXUP18_PU25+HARVESTUP18_PU25.log |
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
+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:
|
Hi @fabiocos |
@dtsitson |
+1 |
@rekovic @dtsitson sorry, regardless of the temporary import outside the official CMSSW that are done in online DQM at P5, the release should be the place where to have the master store of the updates. In any case, DQM updates can be managed even later, while we shouiild have the unpacker possibly frozen by now. @jfernan2 @dmitrijus could you please check and sign it in case for DQM? |
@dtsitson as far as I can see the L1TStage2BMTF directory is now duplicated into a corresponding Kalman version, and this adds some 1.7 MB of memory occupancy. Is this duplication to be considered as temporary, for the commissioning of this code, or permanent? |
+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) |
+1 |
@dtsitson do you want to run this at P5? Is there a 10_1_X backport PR? |
New DQM Plots for Kalman Filter. The whole task is based on DQM for Kalman BMTF