Modification on r-phi distribution plots and fix on under/overflow bins in GEM onlineDQM #36230

quark2 · 2021-11-24T08:26:45Z

PR description:

Some requests on r-phi distributions are applied; finer binning on phi direction, more plots for digi.

Also, the under/overflow bins of 1D histograms are not displayed well. It has been fixed.

PR validation:

Tests are done and one can check again by runTheMatrix workflows

@jshlee @watson-ij

cmsbuild · 2021-11-24T08:34:50Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36230/26852

This PR adds an extra 24KB to repository

cmsbuild · 2021-11-24T08:35:22Z

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

It involves the following packages:

DQM/GEM (dqm)

@emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please review it and eventually sign? Thanks.
@watson-ij this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

jshlee · 2021-11-24T08:37:27Z

please test

jfernan2 · 2021-11-24T08:38:38Z

please test

sroychow · 2021-11-24T10:35:05Z

DQM/GEM/plugins/GEMRecHitSource.cc


        // Filling of RecHit occupancy
        mapTotalRecHit_layer_.Fill(key3, chamber, eId.ieta());

        // Filling of R-Phi occupancy
        Float_t fR = fRadiusMin_ + (fRadiusMax_ - fRadiusMin_) * (eId.ieta() - 0.5) / stationInfo.nNumEtaPartitions_;
-        mapRecHitWheel_layer_.Fill(key3, fPhi, fR);
+        Float_t fPhiShift = (fPhi >= stationInfo.fMinPhi_ ? fPhi : fPhi + 2 * 3.141592);


Please use M_PI instead of 3.14...

Hi @sroychow,

Thanks for your advice. All 3.141592 have been replaced. Please let me know if there is any missed.

Best regards,
Byeonghak Ko

sroychow · 2021-11-24T10:55:14Z

DQM/GEM/plugins/GEMDQMHarvester.cc

-Int_t GEMDQMHarvester::assessOneBin(Float_t fAll, Float_t fNumOcc, Float_t fNumWarn, Float_t fNumErr) {
-  if (fNumErr > 0.05 * fAll)  // The error status criterion
+Int_t GEMDQMHarvester::assessOneBin(
+    std::string strName, Int_t nIdxX, Int_t nIdxY, Float_t fAll, Float_t fNumOcc, Float_t fNumWarn, Float_t fNumErr) {


What is the role of strName here? It is not used in this function.

Yes, they are not used currently, but I let them leave for future usage.

cmsbuild · 2021-11-24T11:05:36Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36230/26856

This PR adds an extra 28KB to repository

cmsbuild · 2021-11-24T11:06:02Z

Pull request #36230 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please check and sign again.

sroychow · 2021-11-24T11:12:39Z

@cmsbuild please test

cmsbuild · 2021-11-24T14:19:55Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f04352/20731/summary.html
COMMIT: 020e5f2
CMSSW: CMSSW_12_2_X_2021-11-23-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36230/20731/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3247025
DQMHistoTests: Total failures: 114
DQMHistoTests: Total nulls: 83
DQMHistoTests: Total successes: 3246806
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 108.283 KiB( 41 files compared)
DQMHistoSizes: changed ( 10024.0,... ): 1.969 KiB GEM/RecHits
DQMHistoSizes: changed ( 11634.0,... ): 3.938 KiB GEM/RecHits
DQMHistoSizes: changed ( 23234.0,... ): 7.875 KiB GEM/RecHits
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
DQMHistoSizes: changed ( 35034.0,... ): 19.688 KiB GEM/RecHits
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

quark2 · 2021-11-24T15:53:40Z

Hi @jfernan2,

Yep, you are right... I made a confusion between the two variables. Thanks for pointing it out!

Best regards,
Byeonghak Ko

jfernan2 · 2021-11-24T15:55:30Z

please test

cmsbuild · 2021-11-24T16:02:21Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36230/26867

This PR adds an extra 16KB to repository

cmsbuild · 2021-11-24T16:02:45Z

Pull request #36230 was updated. @emanueleusai, @ahmad3213, @jfernan2, @rvenditti, @pbo0, @pmandrik can you please check and sign again.

cmsbuild · 2021-11-24T19:04:16Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f04352/20740/summary.html
COMMIT: acc4357
CMSSW: CMSSW_12_2_X_2021-11-23-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36230/20740/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3247025
DQMHistoTests: Total failures: 45
DQMHistoTests: Total nulls: 82
DQMHistoTests: Total successes: 3246876
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 108.287 KiB( 41 files compared)
DQMHistoSizes: changed ( 10024.0,... ): 1.969 KiB GEM/RecHits
DQMHistoSizes: changed ( 11634.0,... ): 3.938 KiB GEM/RecHits
DQMHistoSizes: changed ( 23234.0,... ): 7.875 KiB GEM/RecHits
DQMHistoSizes: changed ( 35034.0,... ): 19.688 KiB GEM/RecHits
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

jfernan2 · 2021-11-24T19:13:19Z

+1

cmsbuild · 2021-11-24T19:13:42Z

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2021-11-25T00:44:24Z

+1

quark2 · 2021-11-25T06:43:06Z

Hi @jfernan2,

It is already merged, but I have a question about its backport, to make sure. The version to the backport is still 12_0_X, isn't it?
Thanks!

Best regards,
Byeonghak Ko

perrotta

There were a few comments I supposed to have sent yesterday, but they remained pending in my laptop instead. I list them now here, because there is quite likely a bug to be fixed in them, together with some suggestion for possible code performance improvement (which I understand is not the priority for this code, but let me list them anihow, and you'll decide whether to implement them or not: several other improvements are also possible in the parts of the code not touched by this PR).

Besides them, please also consider and fix the dead initialization issue reported by the static analyzer in line 221 of DQM/GEM/plugins/GEMDQMHarvester.cc

Since this one have been merged already, please implement at least the bug fix and the resolution of the issue reported by SA in a separate PR

perrotta · 2021-11-24T20:54:06Z

DQM/GEM/plugins/GEMRecHitSource.cc


        // Filling of RecHit (iEta)
        mapRecHitOcc_ieta_.Fill(key3, eId.ieta());

        // Filling of RecHit (phi)
-        Float_t fPhiDeg = fPhi * 180.0 / 3.141592;
+        Float_t fPhiDeg = fPhi * 180.0 / M_PI;
+        fPhiDeg = (fPhiDeg >= -0.5 ? fPhiDeg : fPhiDeg + 360);


BUG
The histo mapRecHitOcc_phi_ goes between "-5" and "355", and it is filled with fPhiDeg that goes from "-0.5" to "360"
I think it would be far simpler (and less error prone) mapping everything between 0 and 360.

Ahh, thanks. It's a silly bug... Btw is there any nice internal function (ROOT or gcc or anything) for this task?

But I don't get your upper bound. The range of fPhiDeg before line 214 is in [-180, 180] (except one of 180 and -180), and line 214 shifts values in [-180, -5] to [180, 355] (if the wrong value -0.5 is replaced by -5). Am I corrent?

perrotta · 2021-11-24T20:59:04Z

DQM/GEM/plugins/GEMDQMHarvester.cc

+  Int_t assessOneBin(
+      std::string strName, Int_t nIdxX, Int_t nIdxY, Float_t fAll, Float_t fNumOcc, Float_t fNumWarn, Float_t fNumErr);
+
+  Float_t fCutErr_, fCutWarnErr_, fCutWarn_;

  Float_t fReportSummary_;


This is only used inside GEMDQMHarvester::drawSummaryHistogram, no need to make it a class member

Hmm, you're right. It would be better to let it be local.

perrotta · 2021-11-24T21:01:09Z

DQM/GEM/plugins/GEMDQMHarvester.cc

+  Int_t assessOneBin(
+      std::string strName, Int_t nIdxX, Int_t nIdxY, Float_t fAll, Float_t fNumOcc, Float_t fNumWarn, Float_t fNumErr);
+
+  Float_t fCutErr_, fCutWarnErr_, fCutWarn_;


These floats are only used inside GEMDQMHarvester::assessOneBin: I think they should be made configurable, but if you don't you can make them local constexpr in that method

Yep, that's a good idea to make them configurable. Thanks!

perrotta · 2021-11-24T21:01:32Z

DQM/GEM/plugins/GEMDQMHarvester.cc

+  Int_t assessOneBin(
+      std::string strName, Int_t nIdxX, Int_t nIdxY, Float_t fAll, Float_t fNumOcc, Float_t fNumWarn, Float_t fNumErr);
+
+  Float_t fCutErr_, fCutWarnErr_, fCutWarn_;

  Float_t fReportSummary_;
  std::string strOutFile_;


All these string can also made const

Or I'll remove it. I think it is a legacy.

Ahh, I thought you mentioned strOutFile_ . I'll make strDirSummary_ and strDirRecHit_ and strDirStatus_ const, then. Thanks!

perrotta · 2021-11-24T21:07:15Z

DQM/GEM/plugins/GEMDigiSource.cc

@@ -26,8 +26,14 @@ void GEMDigiSource::bookHistograms(DQMStore::IBooker& ibooker, edm::Run const&,

  nBXMin_ = -10;


All these do not need to be class member: make them local const variable

nBXMin_ and nBXMax_ could be const (but not local since they are used in another method). I'll do that.

But fRadiusMin_ and fRadiusMax_ are more complicated. They depend on the geometry (GE11 and GE21 and ME0 have different radius ranges) and I have a plan to set these ranges from the configuration directly. So they cannot be const.

jfernan2 · 2021-11-25T08:26:04Z

It is already merged, but I have a question about its backport, to make sure. The version to the backport is still 12_0_X, isn't it? Thanks!

Yes, in current data taking we are using 12_0_X, but from January it is expected to switch to 12_2_X (current master). Please consider merging this PR and the changes requested by @perrotta in a single PR if you plan to backport. Thanks

quark2 · 2021-11-25T18:07:09Z

Hi @jfernan2,

Thanks! I've made an update for the comments of @perrotta. I'll also make a backport PR soon.

Best regards,
Byeonghak Ko

Displaying under/overflow bins, modification on r-phi plots

1cd5cfd

cmsbuild added this to the CMSSW_12_2_X milestone Nov 24, 2021

cmsbuild added code-checks-pending dqm-pending orp-pending pending-signatures tests-pending labels Nov 24, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 24, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 24, 2021

sroychow reviewed Nov 24, 2021

View reviewed changes

3.141592 -> M_PI (Suvanka)

020e5f2

sroychow reviewed Nov 24, 2021

View reviewed changes

cmsbuild added code-checks-pending tests-pending and removed tests-started code-checks-approved labels Nov 24, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 24, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 24, 2021

cmsbuild added code-checks-pending tests-pending labels Nov 24, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 24, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 24, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 24, 2021

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Nov 24, 2021

cmsbuild added orp-approved and removed orp-pending labels Nov 25, 2021

cmsbuild merged commit d69b215 into cms-sw:master Nov 25, 2021

perrotta reviewed Nov 25, 2021

View reviewed changes

quark2 mentioned this pull request Nov 25, 2021

Modification on r-phi distribution plots and fix on under/overflow bins in GEM onlineDQM (continued) #36253

Merged

quark2 added a commit to quark2/cmssw that referenced this pull request Nov 25, 2021

Backport of cms-sw#36230 and cms-sw#36253 to 12_0_X

822704b

quark2 mentioned this pull request Nov 25, 2021

Modification on r-phi distribution plots and fix on under/overflow bins in GEM onlineDQM, a backport to 12_0_X #36254

Merged

		@@ -26,8 +26,14 @@ void GEMDigiSource::bookHistograms(DQMStore::IBooker& ibooker, edm::Run const&,

		nBXMin_ = -10;

Modification on r-phi distribution plots and fix on under/overflow bins in GEM onlineDQM #36230

Modification on r-phi distribution plots and fix on under/overflow bins in GEM onlineDQM #36230

Conversation

quark2 commented Nov 24, 2021

PR description:

PR validation:

cmsbuild commented Nov 24, 2021

cmsbuild commented Nov 24, 2021

jshlee commented Nov 24, 2021

jfernan2 commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Nov 24, 2021

cmsbuild commented Nov 24, 2021

sroychow commented Nov 24, 2021

cmsbuild commented Nov 24, 2021

Comparison Summary

quark2 commented Nov 24, 2021

jfernan2 commented Nov 24, 2021

cmsbuild commented Nov 24, 2021

cmsbuild commented Nov 24, 2021

cmsbuild commented Nov 24, 2021

Comparison Summary

jfernan2 commented Nov 24, 2021

cmsbuild commented Nov 24, 2021

qliphy commented Nov 25, 2021

quark2 commented Nov 25, 2021 • edited Loading

perrotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quark2 Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfernan2 commented Nov 25, 2021

quark2 commented Nov 25, 2021

quark2 commented Nov 25, 2021 •

edited

Loading

quark2 Nov 25, 2021 •

edited

Loading