Run3-hcx328 Replace cout with LogVerbatim and update the cfg's in Geometry/HcalCommonData #36372

bsunanda · 2021-12-06T13:41:26Z

PR description:

Replace cout with LogVerbatim and update the cfg's in Geometry/HcalCommonData

PR validation:

Use the runTheMatrix test workflows and cfg's in the test directory

if this PR is a backport please specify the original PR and why you need to backport that PR:

Nothing special

…mmonData

cmsbuild · 2021-12-06T14:03:35Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36372/27144

This PR adds an extra 44KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36372/27144/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36372/27144/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

bsunanda · 2021-12-06T14:06:17Z

@cmsbuild Please test

cmsbuild · 2021-12-06T14:13:43Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36372/27146

This PR adds an extra 44KB to repository

cmsbuild · 2021-12-06T14:14:08Z

A new Pull Request was created by @bsunanda (Sunanda Banerjee) for master.

It involves the following packages:

Geometry/HcalCommonData (geometry)

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth can you please review it and eventually sign? Thanks.
@fabiocos 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

cmsbuild · 2021-12-06T17:10:14Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6d7e3c/21015/summary.html
COMMIT: 2d40ff7
CMSSW: CMSSW_12_2_X_2021-12-06-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36372/21015/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: 13 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3250608
DQMHistoTests: Total failures: 183
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3250402
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 41 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

cvuosalo · 2021-12-06T20:59:56Z

In the comparison test workflow 10042.0 ZMM 2017 had several unexpected differences. How can any change in this PR cause those differences?

cvuosalo · 2021-12-06T21:10:59Z

Geometry/HcalCommonData/src/HcalLayerDepthMap.cc

@@ -117,8 +121,8 @@ int HcalLayerDepthMap::getDepth(
      depth = itr->second;
  }
 #ifdef EDM_ML_DEBUG
-  std::cout << "getDepth::Input " << subdet << ":" << ieta << ":" << iphi << ":" << zside << ":" << layer << " Output "
-            << depth << std::endl;
+  edm::LogVerbatim("HCalGeom") << "getDepth::Input " << subdet << ":" << ieta << ":" << iphi << ":" << zside << ":"


Suggested change

edm::LogVerbatim("HCalGeom") << "getDepth::Input " << subdet << ":" << ieta << ":" << iphi << ":" << zside << ":"

edm::LogVerbatim("HCalGeom") << "Debug info -- getDepth::Input " << subdet << ":" << ieta << ":" << iphi << ":" << zside << ":"

cvuosalo · 2021-12-06T21:11:26Z

Geometry/HcalCommonData/src/HcalLayerDepthMap.cc

@@ -128,31 +132,31 @@ int HcalLayerDepthMap::getDepth16(const int subdet, const int iphi, const int zs
  if (isValid(subdet, iphi, zside))
    depth = dep16C_;
 #ifdef EDM_ML_DEBUG
-  std::cout << "getDepth16::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth << std::endl;
+  edm::LogVerbatim("HCalGeom") << "getDepth16::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth;


Suggested change

edm::LogVerbatim("HCalGeom") << "getDepth16::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth;

edm::LogVerbatim("HCalGeom") << "Debug info -- getDepth16::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth;

cvuosalo · 2021-12-06T21:11:53Z

Geometry/HcalCommonData/src/HcalLayerDepthMap.cc

 #endif
  return depth;
 }

 int HcalLayerDepthMap::getDepthMin(const int subdet, const int iphi, const int zside) const {
  int depth = (isValid(subdet, iphi, zside)) ? depthMin_ : -1;
 #ifdef EDM_ML_DEBUG
-  std::cout << "getDepthMin::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth << std::endl;
+  edm::LogVerbatim("HCalGeom") << "getDepthMin::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth;


Suggested change

edm::LogVerbatim("HCalGeom") << "getDepthMin::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth;

edm::LogVerbatim("HCalGeom") << "Debug info -- getDepthMin::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth;

cvuosalo · 2021-12-06T21:12:10Z

Geometry/HcalCommonData/src/HcalLayerDepthMap.cc

 #endif
  return depth;
 }

 int HcalLayerDepthMap::getDepthMax(const int subdet, const int iphi, const int zside) const {
  int depth = (isValid(subdet, iphi, zside)) ? depthMax_ : -1;
 #ifdef EDM_ML_DEBUG
-  std::cout << "getDepthMax::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth << std::endl;
+  edm::LogVerbatim("HCalGeom") << "getDepthMax::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth;


Suggested change

edm::LogVerbatim("HCalGeom") << "getDepthMax::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth;

edm::LogVerbatim("HCalGeom") << "Debug info -- getDepthMax::Input " << subdet << ":" << iphi << ":" << zside << " Output " << depth;

cvuosalo · 2021-12-06T21:17:31Z

To explain my suggestions above:

As I mentioned in issue #34294, some of the debug messages from HcalLayerDepthMap.cc can be needlessly alarming when the depth is reported as -1. The message looks like an error to the non-expert. To make more clear that the message is just debugging information and not an error, I am suggesting that explanatory text be added to each message, as shown above.

cvuosalo · 2021-12-07T21:15:17Z

Comparison differences in reco quantities for workflow 10042.0 have appeared again. These differences don't appear in recent tests in other PRs, but I don't see any change in this PR that could cause these differences.
Does anyone have any idea what could cause these differences?

bsunanda · 2021-12-12T20:05:51Z

@cvuosalo Please approve this PR

cvuosalo · 2021-12-13T20:51:57Z

@bsunanda What is the explanation of the comparison differences? If they are caused by this PR, then the PR needs to be changed to prevent such differences.

Comparison differences in reco quantities for workflow 10042.0 have appeared again. These differences don't appear in recent tests in other PRs, but I don't see any change in this PR that could cause these differences. Does anyone have any idea what could cause these differences?

bsunanda · 2021-12-14T14:44:11Z

@cvuosalo This PR only modifies the print statements from out to LogVerbatim and use uniform token name "HCalGeom" rather than a mixture of "HCalGeom" and "HcalGeom". Please approve this PR

cvuosalo · 2021-12-14T19:46:03Z

Let's try the comparison test again.

cvuosalo · 2021-12-14T19:46:13Z

@cmsbuild please test

cmsbuild · 2021-12-14T22:39:56Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6d7e3c/21276/summary.html
COMMIT: e4291a1
CMSSW: CMSSW_12_3_X_2021-12-14-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36372/21276/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: 3250719
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3250691
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

cvuosalo · 2021-12-15T21:44:28Z

The comparison differences have disappeared in the latest test. They were not related to this PR but may have briefly been in the IB used for previous tests.

cvuosalo · 2021-12-15T21:44:34Z

+1

cmsbuild · 2021-12-15T21:44:54Z

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-12-16T01:19:08Z

please test
just to double check possible differences

cmsbuild · 2021-12-16T09:24:54Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6d7e3c/21300/summary.html
COMMIT: e4291a1
CMSSW: CMSSW_12_3_X_2021-12-15-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36372/21300/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: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 3250719
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3250691
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

qliphy · 2021-12-17T02:35:58Z

+1
tests are stable now without essential differences.

Replace cout with LogVerbatim and update the cfg's in Geometry/HcalCo…

eacf563

…mmonData

cmsbuild added this to the CMSSW_12_2_X milestone Dec 6, 2021

cmsbuild added code-checks-pending geometry-pending orp-pending pending-signatures tests-pending labels Dec 6, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Dec 6, 2021

Code check

2d40ff7

cmsbuild added code-checks-pending and removed code-checks-rejected labels Dec 6, 2021

cmsbuild added tests-started and removed tests-pending labels Dec 6, 2021

bsunanda mentioned this pull request Dec 6, 2021

[Low priority] Change cout to LogTrace in HcalLayerDepthMap.cc #34294

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 6, 2021

cmsbuild added tests-approved and removed tests-started labels Dec 6, 2021

cvuosalo reviewed Dec 6, 2021

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Dec 7, 2021

cmsbuild added tests-started and removed tests-approved labels Dec 14, 2021

cmsbuild added tests-approved and removed tests-started labels Dec 14, 2021

cmsbuild added fully-signed geometry-approved and removed geometry-pending pending-signatures labels Dec 15, 2021

cmsbuild added tests-started and removed tests-approved labels Dec 16, 2021

cmsbuild added tests-approved and removed tests-started labels Dec 16, 2021

cmsbuild added orp-approved and removed orp-pending labels Dec 17, 2021

cmsbuild merged commit a6fdbe7 into cms-sw:master Dec 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run3-hcx328 Replace cout with LogVerbatim and update the cfg's in Geometry/HcalCommonData #36372

Run3-hcx328 Replace cout with LogVerbatim and update the cfg's in Geometry/HcalCommonData #36372

bsunanda commented Dec 6, 2021

cmsbuild commented Dec 6, 2021

bsunanda commented Dec 6, 2021

cmsbuild commented Dec 6, 2021

cmsbuild commented Dec 6, 2021

cmsbuild commented Dec 6, 2021

cvuosalo commented Dec 6, 2021

cvuosalo Dec 6, 2021

cvuosalo Dec 6, 2021

cvuosalo Dec 6, 2021

cvuosalo Dec 6, 2021

cvuosalo commented Dec 6, 2021

cvuosalo commented Dec 7, 2021

bsunanda commented Dec 12, 2021

cvuosalo commented Dec 13, 2021

bsunanda commented Dec 14, 2021

cvuosalo commented Dec 14, 2021

cvuosalo commented Dec 14, 2021

cmsbuild commented Dec 14, 2021

cvuosalo commented Dec 15, 2021

cvuosalo commented Dec 15, 2021

cmsbuild commented Dec 15, 2021

qliphy commented Dec 16, 2021

cmsbuild commented Dec 16, 2021

qliphy commented Dec 17, 2021

	edm::LogVerbatim("HCalGeom") << "getDepth::Input " << subdet << ":" << ieta << ":" << iphi << ":" << zside << ":"
	edm::LogVerbatim("HCalGeom") << "Debug info -- getDepth::Input " << subdet << ":" << ieta << ":" << iphi << ":" << zside << ":"

Run3-hcx328 Replace cout with LogVerbatim and update the cfg's in Geometry/HcalCommonData #36372

Run3-hcx328 Replace cout with LogVerbatim and update the cfg's in Geometry/HcalCommonData #36372

Conversation

bsunanda commented Dec 6, 2021

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Dec 6, 2021

bsunanda commented Dec 6, 2021

cmsbuild commented Dec 6, 2021

cmsbuild commented Dec 6, 2021

cmsbuild commented Dec 6, 2021

Comparison Summary

cvuosalo commented Dec 6, 2021

cvuosalo Dec 6, 2021

Choose a reason for hiding this comment

cvuosalo Dec 6, 2021

Choose a reason for hiding this comment

cvuosalo Dec 6, 2021

Choose a reason for hiding this comment

cvuosalo Dec 6, 2021

Choose a reason for hiding this comment

cvuosalo commented Dec 6, 2021

cvuosalo commented Dec 7, 2021

bsunanda commented Dec 12, 2021

cvuosalo commented Dec 13, 2021

bsunanda commented Dec 14, 2021

cvuosalo commented Dec 14, 2021

cvuosalo commented Dec 14, 2021

cmsbuild commented Dec 14, 2021

Comparison Summary

cvuosalo commented Dec 15, 2021

cvuosalo commented Dec 15, 2021

cmsbuild commented Dec 15, 2021

qliphy commented Dec 16, 2021

cmsbuild commented Dec 16, 2021

Comparison Summary

qliphy commented Dec 17, 2021