HCAL: bug fixed in DPGAnalysis/HcalTools for 2023 Run3 #40732

zhokin2 · 2023-02-09T06:25:01Z

PR description:

this is an additional PR to overcome serious warning seen only with command " scram b checker " and concerns only file plugin/[CMTRawAnalyzer.cc] (https://github.com/cms-sw/cmssw/blob/master/DPGAnalysis/HcalTools/plugins/CMTRawAnalyzer.cc)

PR validation:

scram build code-format

scram b checker

scram build code-checks

cmsbuild · 2023-02-09T06:30:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40732/34132

This PR adds an extra 76KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DPGAnalysis/HcalTools/plugins/CMTRawAnalyzer.cc modified in PR(s): [DQM][CLANG] Fix warnings reported by clang 14 #40721

cmsbuild · 2023-02-09T06:30:30Z

A new Pull Request was created by @zhokin2 (zhokin) for master.

It involves the following packages:

DPGAnalysis/HcalTools (dqm)

@emanueleusai, @cmsbuild, @syuvivida, @rvenditti, @micsucmed, @pmandrik can you please review it and eventually sign? Thanks.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

perrotta · 2023-02-09T06:36:10Z

DPGAnalysis/HcalTools/plugins/CMTRawAnalyzer.cc

@@ -8821,23 +8818,23 @@ void CMTRawAnalyzer::fillDigiAmplitude(HBHEDigiCollection::const_iterator& digiI
  pedestalaver4 /= c4;
  pedestalwaver9 = sqrt(pedestalwaver9 / TSsize);
  pedestalwaver4 = sqrt(pedestalwaver4 / c4);
-  if (ts_with_max_signal > -1 && ts_with_max_signal < 10)
+  if (ts_with_max_signal > 0 && ts_with_max_signal < TSsize)


Why here and in the following you have "> 0" and you have "> -1" in all other cases?

in principle, it does not matter, but -1 is more correct, Thank you !

perrotta · 2023-02-09T06:36:53Z

DPGAnalysis/HcalTools/plugins/CMTRawAnalyzer.cc

@@ -9662,7 +9659,7 @@ void CMTRawAnalyzer::fillDigiAmplitudeQIE11(QIE11DataFrame qie11df) {
  double amplitude345 = 0.;
  double ampl = 0.;
  double ampl3ts = 0.;
-  double amplmaxts = 0.;
+  //  double amplmaxts = 0.;


Please remove the commented out code with these useless variables, here and below

OK, these commented lines can be removed - thanks.

zhokin2 · 2023-02-09T07:45:59Z

tiny corrections of plugins/CMTRawAnalyzer.cc done with following git commit -m "HCAL: bug fixed in DPGAnalysis/HcalTools for 2023 Run3" ./ git push my-cmssw CMTRMTforRun3_CMSSW_13_0_X_2023-02-08-1100

________________________________ From: Andrea Perrotta ***@***.*** Sent: 09 February 2023 07:37 To: cms-sw/cmssw Cc: Alexandr Zhokin; Mention Subject: Re: [cms-sw/cmssw] HCAL: bug fixed in DPGAnalysis/HcalTools for 2023 Run3 (PR #40732) @perrotta commented on this pull request.

________________________________ In DPGAnalysis/HcalTools/plugins/CMTRawAnalyzer.cc<#40732 (comment)>:

@@ -8821,23 +8818,23 @@ void CMTRawAnalyzer::fillDigiAmplitude(HBHEDigiCollection::const_iterator& digiI

pedestalaver4 /= c4; pedestalwaver9 = sqrt(pedestalwaver9 / TSsize); pedestalwaver4 = sqrt(pedestalwaver4 / c4); - if (ts_with_max_signal > -1 && ts_with_max_signal < 10) + if (ts_with_max_signal > 0 && ts_with_max_signal < TSsize) Why here and in the following you have "> 0" and you have "> -1" in all other cases?

________________________________ In DPGAnalysis/HcalTools/plugins/CMTRawAnalyzer.cc<#40732 (comment)>:

@@ -9662,7 +9659,7 @@ void CMTRawAnalyzer::fillDigiAmplitudeQIE11(QIE11DataFrame qie11df) {

double amplitude345 = 0.; double ampl = 0.; double ampl3ts = 0.; - double amplmaxts = 0.; + // double amplmaxts = 0.; Please remove the commented out code with these useless variables, here and below — Reply to this email directly, view it on GitHub<#40732 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACHOUBDBH7LWRVNBG54X4I3WWSGBTANCNFSM6AAAAAAUWEPJCE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

cmsbuild · 2023-02-09T07:53:38Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40732/34133

This PR adds an extra 80KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DPGAnalysis/HcalTools/plugins/CMTRawAnalyzer.cc modified in PR(s): [DQM][CLANG] Fix warnings reported by clang 14 #40721

cmsbuild · 2023-02-09T07:54:00Z

Pull request #40732 was updated. @emanueleusai, @cmsbuild, @syuvivida, @rvenditti, @micsucmed, @pmandrik can you please check and sign again.

emanueleusai · 2023-02-09T16:40:19Z

type hcal

emanueleusai · 2023-02-09T16:40:30Z

type bug

emanueleusai · 2023-02-09T16:40:44Z

please test

perrotta · 2023-02-09T16:54:22Z

@zhokin2 there are still commented out lines to remove:

cmsbuild · 2023-02-09T19:07:59Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bd099d/30556/summary.html
COMMIT: bbebe9e
CMSSW: CMSSW_13_0_X_2023-02-09-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40732/30556/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 10 lines from the logs
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555852
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3555824
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 213 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

cmsbuild · 2023-02-10T10:10:07Z

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bd099d/30564/summary.html
COMMIT: 301c6e6
CMSSW: CMSSW_13_0_X_2023-02-09-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40732/30564/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

The relvals timed out after 4 hours.

Comparison Summary

Summary:

You potentially removed 19 lines from the logs
Reco comparison results: 3 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555852
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3555827
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 213 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

perrotta · 2023-02-10T15:12:01Z

@zhokin2 @abdoulline this PR is not in conflict with #40721 (which I think was the conclusion you reached in the thread ending with #40721 (comment))
If you closed this PR because of some different reason, instead, feel free to close it again

zhokin2 · 2023-02-10T15:20:22Z

@zhokin2 @abdoulline this PR is not in conflict with #40721 (which I think was the conclusion you reached in the thread ending with #40721 (comment)) If you closed this PR because of some different reason, instead, feel free to close it again

frankly, there is no idea what is the problem in runTheMatrix, because in the last edit, I just deleted three commented lines

perrotta · 2023-02-10T15:27:46Z

frankly, there is no idea what is the problem in runTheMatrix, because in the last edit, I just deleted three commented lines

There are no problems at all in runTheMatrix, as far as I can see. RelVals-INPUT time out quite often, independently on the PR. When it is clear, as in this case, that the timeour cannot be given by the PR changes we don't bother to re-run them. But if you like seeing the display all green, we can relaunch the tests here and see whether those timeouts disappear by themselves.

perrotta · 2023-02-10T15:28:03Z

please test

abdoulline · 2023-02-10T15:44:00Z

@perrotta I'm a bit confused... No conflict (in the sense that not the same lines are involved) but both PRs modify the same file CMTRawAnalyzer.cc ? -
You mean once this PR will be merged, it will not remove modifications done in #40721 ?

perrotta · 2023-02-10T15:59:03Z

@perrotta I'm a bit confused... No conflict, but PRs modify the same file CMTRawAnalyzer.cc ?

git can recognize that the modifications appeared in different parts of the file, far away "enough" according to some metrics, and considers them as independent.
In fact, #40721 removed some unused variable, while this PR touches other variables in other parts of the code. As such the modifications applied are independent on each other, and can be integrated independently.
If you feel more confortable, you can also rebase this PR on top of the latest IB: but looking at the code changes, I don't think it is necessary.

abdoulline · 2023-02-10T16:06:26Z

@perrotta OK, thanks. I see it better now.

cmsbuild · 2023-02-10T17:52:23Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bd099d/30585/summary.html
COMMIT: 301c6e6
CMSSW: CMSSW_13_0_X_2023-02-10-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40732/30585/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 13 lines to the logs
Reco comparison results: 5 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555972
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3555947
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 213 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

emanueleusai · 2023-02-11T01:19:14Z

+1

cmsbuild · 2023-02-11T01:19:39Z

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

perrotta · 2023-02-11T07:04:24Z

+1

The file touched by this PR is used for HCAL local monitoring, and doesn't touch any other workflow: therefore it can be merged in 13_0_X even before switching the master to 13_1_X, to make it already available also in 13_0_X for Run3 monitoring

HCAL: bug fixed in DPGAnalysis/HcalTools for 2023 Run3

adf7e65

cmsbuild added this to the CMSSW_13_0_X milestone Feb 9, 2023

cmsbuild added dqm-pending pending-signatures tests-pending orp-pending code-checks-pending labels Feb 9, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 9, 2023

perrotta reviewed Feb 9, 2023

View reviewed changes

HCAL: bug fixed in DPGAnalysis/HcalTools for 2023 Run3

bbebe9e

cmsbuild added code-checks-pending and removed code-checks-approved labels Feb 9, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 9, 2023

cmsbuild added bug-fix hcal labels Feb 9, 2023

cmsbuild added tests-started and removed tests-pending labels Feb 9, 2023

cmsbuild removed the tests-started label Feb 9, 2023

cmsbuild added tests-started and removed tests-pending labels Feb 10, 2023

cmsbuild added tests-rejected and removed tests-started labels Feb 10, 2023

abdoulline mentioned this pull request Feb 10, 2023

[DQM][CLANG] Fix warnings reported by clang 14 #40721

Merged

zhokin2 closed this Feb 10, 2023

perrotta reopened this Feb 10, 2023

cmsbuild added tests-started and removed tests-rejected labels Feb 10, 2023

cmsbuild added tests-approved and removed tests-started labels Feb 10, 2023

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Feb 11, 2023

cmsbuild added orp-approved and removed orp-pending labels Feb 11, 2023

cmsbuild merged commit 69a0aee into cms-sw:master Feb 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HCAL: bug fixed in DPGAnalysis/HcalTools for 2023 Run3 #40732

HCAL: bug fixed in DPGAnalysis/HcalTools for 2023 Run3 #40732

zhokin2 commented Feb 9, 2023

cmsbuild commented Feb 9, 2023

cmsbuild commented Feb 9, 2023

perrotta Feb 9, 2023

zhokin2 Feb 9, 2023

perrotta Feb 9, 2023

zhokin2 Feb 9, 2023

zhokin2 commented Feb 9, 2023 via email

cmsbuild commented Feb 9, 2023

cmsbuild commented Feb 9, 2023

emanueleusai commented Feb 9, 2023

emanueleusai commented Feb 9, 2023

emanueleusai commented Feb 9, 2023

perrotta commented Feb 9, 2023

cmsbuild commented Feb 9, 2023

cmsbuild commented Feb 10, 2023

perrotta commented Feb 10, 2023

zhokin2 commented Feb 10, 2023

perrotta commented Feb 10, 2023

perrotta commented Feb 10, 2023

abdoulline commented Feb 10, 2023 •

edited

perrotta commented Feb 10, 2023

abdoulline commented Feb 10, 2023 •

edited

cmsbuild commented Feb 10, 2023

emanueleusai commented Feb 11, 2023

cmsbuild commented Feb 11, 2023

perrotta commented Feb 11, 2023

HCAL: bug fixed in DPGAnalysis/HcalTools for 2023 Run3 #40732

HCAL: bug fixed in DPGAnalysis/HcalTools for 2023 Run3 #40732

Conversation

zhokin2 commented Feb 9, 2023

PR description:

PR validation:

cmsbuild commented Feb 9, 2023

cmsbuild commented Feb 9, 2023

perrotta Feb 9, 2023

Choose a reason for hiding this comment

zhokin2 Feb 9, 2023

Choose a reason for hiding this comment

perrotta Feb 9, 2023

Choose a reason for hiding this comment

zhokin2 Feb 9, 2023

Choose a reason for hiding this comment

zhokin2 commented Feb 9, 2023 via email

cmsbuild commented Feb 9, 2023

cmsbuild commented Feb 9, 2023

emanueleusai commented Feb 9, 2023

emanueleusai commented Feb 9, 2023

emanueleusai commented Feb 9, 2023

perrotta commented Feb 9, 2023

cmsbuild commented Feb 9, 2023

Comparison Summary

cmsbuild commented Feb 10, 2023

RelVals-INPUT

Comparison Summary

perrotta commented Feb 10, 2023

zhokin2 commented Feb 10, 2023

perrotta commented Feb 10, 2023

perrotta commented Feb 10, 2023

abdoulline commented Feb 10, 2023 • edited

perrotta commented Feb 10, 2023

abdoulline commented Feb 10, 2023 • edited

cmsbuild commented Feb 10, 2023

Comparison Summary

emanueleusai commented Feb 11, 2023

cmsbuild commented Feb 11, 2023

perrotta commented Feb 11, 2023

abdoulline commented Feb 10, 2023 •

edited

abdoulline commented Feb 10, 2023 •

edited