Phase2-hg302 Bug fix for the corners and replacing cout with message logger services #37330

bsunanda · 2022-03-24T06:16:04Z

PR description:

Bug fix for the corners and replacing cout with message logger services. The corner calculation for the rotated layers was wrong. The corner calculation is useful for firework studies

PR validation:

Use the runTheMatrix test workflows

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

Nothing special - may need to bckport the bug fixes in HGCal geometry

cmsbuild · 2022-03-24T06:22:26Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37330/28974

This PR adds an extra 48KB 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-37330/28974/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37330/28974/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2022-03-24T06:37:01Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37330/28975

This PR adds an extra 48KB to repository

cmsbuild · 2022-03-24T06:37:22Z

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

It involves the following packages:

Geometry/HGCalCommonData (geometry, upgrade)
Geometry/HGCalGeometry (geometry, upgrade)

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob 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

bsunanda · 2022-03-24T06:41:30Z

@cmsbuild Please test

perrotta · 2022-03-24T06:52:27Z

@bsunanda could you please quickly summarize in the PR description which is the bug that you aim fixing with this PR?

cmsbuild · 2022-03-24T17:48:46Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9b2b9c/23343/summary.html
COMMIT: 0acf376
CMSSW: CMSSW_12_4_X_2022-03-23-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37330/23343/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: 3517 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3695650
DQMHistoTests: Total failures: 51426
DQMHistoTests: Total nulls: 63
DQMHistoTests: Total successes: 3644139
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 45 edm output root files, 49 DQM output files
TriggerResults: no differences found

bsunanda · 2022-03-28T05:24:27Z

@cvuosalo @srimanob Please sign this PR. It is worthwhile to backport the bug fixing part to 12_3_X

srimanob · 2022-03-28T16:11:38Z

@cmsbuild please test

This is to wipe out unexpected failure comparison.

srimanob · 2022-03-28T16:13:44Z

Hi @bsunanda
As you mentioned in the PR description, The corner calculation for the rotated layers was wrong., do we expect to see change(s) in PR test result, or what do you expect to see? Thanks for clarification.

In the future, I would suggest not to couple between bug-fix and technical PR (i.e. message logger replacement) together. This decouple PRs will keep a clear between bug-fix PR and technical PR.

srimanob · 2022-03-28T16:13:54Z

type bug-fix

bsunanda · 2022-03-29T00:48:30Z

When there is a bug fix, it is expected to change the predictions.

perrotta · 2022-03-29T05:35:57Z

ping bot

perrotta · 2022-03-29T05:41:01Z

When there is a bug fix, it is expected to change the predictions.

@bsunanda it would help (and speed up) the review process if, when a bug is reported and a PR to fix it is submitted, you list:

where is that bug, and how did you fix it
what was the effect of that bug
what is the difference in outputs expected after the fix

Only saying that "when there is a bug fix, it is expected to change the predictions" does not help in judging whether the effects of the fix are as wanted, or if there are any unplanned and unwanted side effects instead.

bsunanda · 2022-03-29T08:28:58Z

The bug was in the treatment of rotated layers. In computing the corner coordinates, it first retrieves the cell position in the local frame of the wafer, then computes the corner coordinates in that local frame and finally apply the transformation to global frame. In the earlier version computation of the cell position in the local frame had an error (some rotation was applied for rotated layers). Clearly this used to lead to wrong results. Now this is avoided by inhibiting rotation for this computation. The implication of this on the output should be there, but cannot judge how much it will be

cmsbuild · 2022-03-29T12:06:51Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9b2b9c/23482/summary.html
COMMIT: 0acf376
CMSSW: CMSSW_12_4_X_2022-03-28-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37330/23482/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: 48
DQMHistoTests: Total histograms compared: 3589460
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3589436
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 200 log files, 45 edm output root files, 48 DQM output files
TriggerResults: no differences found

cvuosalo · 2022-03-30T18:44:44Z

+1

srimanob · 2022-04-03T01:45:05Z

+Upgrade

This is a bug fix PR for the rotated layers of HGCAL. We need to follow up from the validation result from the next round of Phase-2 validation.
FYI @cms-sw/pdmv-l2

cmsbuild · 2022-04-03T01:45:27Z

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)

perrotta · 2022-04-03T07:29:00Z

Geometry/HGCalGeometry/interface/HGCalGeometry.h

@@ -71,6 +71,7 @@ class HGCalGeometry final : public CaloSubdetectorGeometry {
                  CaloSubdetectorGeometry::IVec& dinsVector) const override;

  GlobalPoint getPosition(const DetId& id) const;


having only one getPosition method with the bool debug = false as default would have done the same without duplication

perrotta · 2022-04-03T07:31:12Z

+1

Effect of the bug are not visible in the bot test outputs: they would be probably observed in a larger scale validation campaign

Bug fix for the corners and replacing cout with message logger services

e37c6ca

cmsbuild added this to the CMSSW_12_4_X milestone Mar 24, 2022

cmsbuild added code-checks-pending geometry-pending orp-pending pending-signatures tests-pending upgrade-pending labels Mar 24, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Mar 24, 2022

Code check

0acf376

cmsbuild added code-checks-pending and removed code-checks-rejected labels Mar 24, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 24, 2022

cmsbuild added tests-started and removed tests-pending labels Mar 24, 2022

cmsbuild added tests-approved and removed tests-started labels Mar 24, 2022

cmsbuild mentioned this pull request Mar 28, 2022

Phase2-hgx303 Rearrange some of the parameters in the proper class in view of V17 geometry #37374

Merged

cmsbuild added tests-started and removed tests-approved labels Mar 28, 2022

cmsbuild added the bug-fix label Mar 28, 2022

cmsbuild added tests-approved and removed tests-started labels Mar 29, 2022

cmsbuild added geometry-approved and removed geometry-pending labels Mar 30, 2022

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels Apr 3, 2022

perrotta reviewed Apr 3, 2022

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Apr 3, 2022

cmsbuild merged commit 109548e into cms-sw:master Apr 3, 2022

bsunanda mentioned this pull request Apr 5, 2022

Run4-hgx306 Backport the bugfix to HGCal geometry from the PR's 37330 and 37460 #37466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phase2-hg302 Bug fix for the corners and replacing cout with message logger services #37330

Phase2-hg302 Bug fix for the corners and replacing cout with message logger services #37330

bsunanda commented Mar 24, 2022 •

edited

cmsbuild commented Mar 24, 2022

cmsbuild commented Mar 24, 2022

cmsbuild commented Mar 24, 2022

bsunanda commented Mar 24, 2022

perrotta commented Mar 24, 2022

cmsbuild commented Mar 24, 2022

bsunanda commented Mar 28, 2022

srimanob commented Mar 28, 2022

srimanob commented Mar 28, 2022

srimanob commented Mar 28, 2022

bsunanda commented Mar 29, 2022

perrotta commented Mar 29, 2022

perrotta commented Mar 29, 2022

bsunanda commented Mar 29, 2022

cmsbuild commented Mar 29, 2022

cvuosalo commented Mar 30, 2022

srimanob commented Apr 3, 2022

cmsbuild commented Apr 3, 2022

perrotta Apr 3, 2022

perrotta commented Apr 3, 2022

		@@ -71,6 +71,7 @@ class HGCalGeometry final : public CaloSubdetectorGeometry {
		CaloSubdetectorGeometry::IVec& dinsVector) const override;

		GlobalPoint getPosition(const DetId& id) const;

Phase2-hg302 Bug fix for the corners and replacing cout with message logger services #37330

Phase2-hg302 Bug fix for the corners and replacing cout with message logger services #37330

Conversation

bsunanda commented Mar 24, 2022 • edited

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 Mar 24, 2022

cmsbuild commented Mar 24, 2022

cmsbuild commented Mar 24, 2022

bsunanda commented Mar 24, 2022

perrotta commented Mar 24, 2022

cmsbuild commented Mar 24, 2022

Comparison Summary

bsunanda commented Mar 28, 2022

srimanob commented Mar 28, 2022

srimanob commented Mar 28, 2022

srimanob commented Mar 28, 2022

bsunanda commented Mar 29, 2022

perrotta commented Mar 29, 2022

perrotta commented Mar 29, 2022

bsunanda commented Mar 29, 2022

cmsbuild commented Mar 29, 2022

Comparison Summary

cvuosalo commented Mar 30, 2022

srimanob commented Apr 3, 2022

cmsbuild commented Apr 3, 2022

perrotta Apr 3, 2022

Choose a reason for hiding this comment

perrotta commented Apr 3, 2022

bsunanda commented Mar 24, 2022 •

edited