Run3-alca221 Cleanup the class in Calibration/HcalConnectivity #37080

bsunanda · 2022-02-27T17:45:58Z

PR description:

Cleanup the class in Calibration/HcalConnectivity

PR validation:

Use runTheMatrix test workflows

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

Nothing special

cmsbuild · 2022-02-27T17:53:01Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37080/28555

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

cmsbuild · 2022-02-27T18:04:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37080/28556

This PR adds an extra 12KB to repository

cmsbuild · 2022-02-27T18:04:55Z

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

It involves the following packages:

Calibration/HcalConnectivity (alca)

@cmsbuild, @malbouis, @tvami, @yuanchao, @francescobrivio can you please review it and eventually sign? Thanks.
@mmusich, @tocheng 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-02-27T18:17:57Z

@cmsbuild Please test

cmsbuild · 2022-02-27T21:03:53Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8bc494/22701/summary.html
COMMIT: bbdbf02
CMSSW: CMSSW_12_3_X_2022-02-27-0000/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37080/22701/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: 49
DQMHistoTests: Total histograms compared: 4001143
DQMHistoTests: Total failures: 8
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 4001113
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

tvami · 2022-02-27T21:38:54Z

Calibration/HcalConnectivity/src/HcalCableMapper.cc

+  const edm::Handle<HBHEDigiCollection> &hbhe = e.getHandle(tok_hbhe_);
+  const edm::Handle<HFDigiCollection> &hf = e.getHandle(tok_hf_);
+  const edm::Handle<HODigiCollection> &ho = e.getHandle(tok_ho_);

-  record(*hbhe);
-  record(*hf);
-  record(*ho);
+  record(*(hbhe.product()));
+  record(*(hf.product()));
+  record(*(ho.product()));
 }


The handle is not used for anything (no check in the validity) so one could just directly get the product from the event instead of the handle.

tvami · 2022-02-27T21:39:31Z

Calibration/HcalConnectivity/src/HcalCableMapper.cc

@@ -159,17 +158,13 @@ void HcalCableMapper::process(const PathSet &ps, const IdMap &im) {
 }

 void HcalCableMapper::analyze(edm::Event const &e, edm::EventSetup const &c) {


Suggested change

void HcalCableMapper::analyze(edm::Event const &e, edm::EventSetup const &c) {

void HcalCableMapper::analyze(edm::Event const &iEvent, edm::EventSetup const &iSetup) {

just to be in line with the naming in anywhere else in CMSSW

cmsbuild · 2022-02-28T05:14:00Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37080/28562

This PR adds an extra 16KB to repository

cmsbuild · 2022-02-28T05:14:23Z

Pull request #37080 was updated. @cmsbuild, @malbouis, @tvami, @yuanchao, @francescobrivio can you please check and sign again.

bsunanda · 2022-02-28T05:53:38Z

@cmsbuild Please test

cmsbuild · 2022-02-28T11:35:25Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8bc494/22703/summary.html
COMMIT: c8786a6
CMSSW: CMSSW_12_3_X_2022-02-27-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37080/22703/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test test-das-selected-lumis had ERRORS

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 4001207
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 4001183
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-02-28T12:41:24Z

@cmsbuild Please test

tvami · 2022-02-28T13:25:56Z

@bsunanda just one more thing: please squash the commits, now we have 4 commits changing a single file.

bsunanda · 2022-02-28T13:37:02Z

I am on my way back to India and will be wih no internet connection for a while. Can do that when I shall be in a stable condition.

…

_________I_______________________________ From: Tamas Vami ***@***.*** Sent: 28 February 2022 14:26 To: cms-sw/cmssw Cc: Sunanda Banerjee; Mention Subject: Re: [cms-sw/cmssw] Run3-alca221 Cleanup the class in Calibration/HcalConnectivity (PR #37080) @bsunanda<https://github.com/bsunanda> just one more thing: please squash the commits, now we have 4 commits changing a single file. — Reply to this email directly, view it on GitHub<#37080 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGMZOV3IXNBPXBDCBKYPZDU5NZXFANCNFSM5PPHIQHA>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

cmsbuild · 2022-02-28T19:01:26Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8bc494/22712/summary.html
COMMIT: c8786a6
CMSSW: CMSSW_12_3_X_2022-02-28-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37080/22712/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: 49
DQMHistoTests: Total histograms compared: 4001207
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 4001183
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

tvami · 2022-02-28T21:01:01Z

+alca

let's see what @cms-sw/orp-l2 thinks about the squashing

cmsbuild · 2022-02-28T21:01:22Z

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-02-28T21:15:22Z

+1

Cleanup the class in Calibration/HcalConnectivity

c43cfe2

cmsbuild added this to the CMSSW_12_3_X milestone Feb 27, 2022

cmsbuild added alca-pending code-checks-pending orp-pending pending-signatures tests-pending labels Feb 27, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Feb 27, 2022

Code check

bbdbf02

cmsbuild added code-checks-pending and removed code-checks-rejected labels Feb 27, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 27, 2022

cmsbuild added tests-started and removed tests-pending labels Feb 27, 2022

cmsbuild added tests-approved and removed tests-started labels Feb 27, 2022

tvami reviewed Feb 27, 2022

View reviewed changes

Take suggestions

c5f8040

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Feb 28, 2022

cmsbuild added the code-checks-pending label Feb 28, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 28, 2022

cmsbuild added tests-started and removed tests-pending labels Feb 28, 2022

cmsbuild added tests-rejected and removed tests-started labels Feb 28, 2022

cmsbuild added tests-started and removed tests-rejected labels Feb 28, 2022

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

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Feb 28, 2022

cmsbuild added orp-approved and removed orp-pending labels Feb 28, 2022

cmsbuild merged commit e871fd6 into cms-sw:master Feb 28, 2022

cmsbuild mentioned this pull request Mar 1, 2022

Revert "Revert "Add D88 DD4hep wf to relval_2026"" #37093

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run3-alca221 Cleanup the class in Calibration/HcalConnectivity #37080

Run3-alca221 Cleanup the class in Calibration/HcalConnectivity #37080

bsunanda commented Feb 27, 2022

cmsbuild commented Feb 27, 2022

cmsbuild commented Feb 27, 2022

cmsbuild commented Feb 27, 2022

bsunanda commented Feb 27, 2022

cmsbuild commented Feb 27, 2022

tvami Feb 27, 2022

tvami Feb 27, 2022

cmsbuild commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

bsunanda commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

bsunanda commented Feb 28, 2022

tvami commented Feb 28, 2022

bsunanda commented Feb 28, 2022 via email

cmsbuild commented Feb 28, 2022

tvami commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

perrotta commented Feb 28, 2022

		@@ -159,17 +158,13 @@ void HcalCableMapper::process(const PathSet &ps, const IdMap &im) {
		}

		void HcalCableMapper::analyze(edm::Event const &e, edm::EventSetup const &c) {

Run3-alca221 Cleanup the class in Calibration/HcalConnectivity #37080

Run3-alca221 Cleanup the class in Calibration/HcalConnectivity #37080

Conversation

bsunanda commented Feb 27, 2022

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 Feb 27, 2022

cmsbuild commented Feb 27, 2022

cmsbuild commented Feb 27, 2022

bsunanda commented Feb 27, 2022

cmsbuild commented Feb 27, 2022

Comparison Summary

tvami Feb 27, 2022

Choose a reason for hiding this comment

tvami Feb 27, 2022

Choose a reason for hiding this comment

cmsbuild commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

bsunanda commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

Unit Tests

Comparison Summary

bsunanda commented Feb 28, 2022

tvami commented Feb 28, 2022

bsunanda commented Feb 28, 2022 via email

cmsbuild commented Feb 28, 2022

Comparison Summary

tvami commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

perrotta commented Feb 28, 2022