Run3-Sim101F Avoid compilation warnings in some of the codes of Validation/RecoTau #36608

bsunanda · 2021-12-28T17:29:22Z

PR description:

Avoid compilation warnings in some of the codes of Validation/RecoTau

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

cmsbuild · 2021-12-28T17:38:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36608/27582

This PR adds an extra 20KB to repository

cmsbuild · 2021-12-28T17:38:41Z

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

It involves the following packages:

Validation/RecoTau (dqm)

@emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @pmandrik, @pbo0, @rvenditti can you please review it and eventually sign? Thanks.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

bsunanda · 2021-12-29T02:10:02Z

@cmsbuild Please test

cmsbuild · 2021-12-29T06:12:46Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54a768/21494/summary.html
COMMIT: ac80bd8
CMSSW: CMSSW_12_3_X_2021-12-28-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36608/21494/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: 43
DQMHistoTests: Total histograms compared: 3461692
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3461664
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
Checked 181 log files, 42 edm output root files, 43 DQM output files
TriggerResults: no differences found

jfernan2 · 2021-12-30T21:15:21Z

+1

cmsbuild · 2021-12-30T21:15:43Z

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)

makortel · 2021-12-30T21:17:43Z

Validation/RecoTau/plugins/DQMFileLoader.h

@@ -21,7 +21,7 @@
 #include <vector>
 #include <string>

-class TauDQMFileLoader : public edm::EDAnalyzer {
+class TauDQMFileLoader : public edm::one::EDAnalyzer<edm::one::WatchRuns> {
  typedef dqm::legacy::DQMStore DQMStore;
  typedef dqm::legacy::MonitorElement MonitorElement;


Is the direct use of DQMStore safe in non-legacy modules?

I believe it is not. I leave the author of the code to make use of DQM codes``

Then, to me, it would make more sense to leave these modules as legacy and migrate them properly (or remove) later rather than enabling unsafe code.

makortel · 2022-01-04T16:57:57Z

Validation/RecoTau/plugins/DQMHistNormalizer.cc

@@ -27,14 +27,15 @@

 using namespace std;

-class DQMHistNormalizer : public edm::EDAnalyzer {
+class DQMHistNormalizer : public edm::one::EDAnalyzer<edm::one::WatchRuns> {
 public:
  typedef dqm::legacy::DQMStore DQMStore;
  typedef dqm::legacy::MonitorElement MonitorElement;


This module appears to use DQMStore directly too.

makortel · 2022-01-04T16:58:03Z

Validation/RecoTau/plugins/DQMHistPlotter.h

@@ -24,7 +24,7 @@
 #include <vector>
 #include <map>

-class TauDQMHistPlotter : public edm::EDAnalyzer {
+class TauDQMHistPlotter : public edm::one::EDAnalyzer<edm::one::WatchRuns> {
  typedef dqm::legacy::DQMStore DQMStore;
  typedef dqm::legacy::MonitorElement MonitorElement;


makortel · 2022-01-04T16:58:10Z

Validation/RecoTau/plugins/DQMSimpleFileSaver.h

@@ -19,7 +19,7 @@

 #include <string>

-class TauDQMSimpleFileSaver : public edm::EDAnalyzer {
+class TauDQMSimpleFileSaver : public edm::one::EDAnalyzer<> {
 public:
  typedef dqm::legacy::DQMStore DQMStore;
  typedef dqm::legacy::MonitorElement MonitorElement;


bsunanda · 2022-01-04T17:24:38Z

@cmsbuild Please test

cmsbuild · 2022-01-04T17:30:41Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36608/27632

This PR adds an extra 12KB to repository

cmsbuild · 2022-01-04T17:31:06Z

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

cmsbuild · 2022-01-04T20:30:26Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54a768/21531/summary.html
COMMIT: 8f13cf7
CMSSW: CMSSW_12_3_X_2022-01-04-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36608/21531/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 4 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54a768/21531/llvm-analysis/legacy-mod-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 9 differences found in the comparisons
DQMHistoTests: Total files compared: 43
DQMHistoTests: Total histograms compared: 3461692
DQMHistoTests: Total failures: 10
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3461659
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 42 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 181 log files, 42 edm output root files, 43 DQM output files
TriggerResults: no differences found

jfernan2 · 2022-01-05T08:20:45Z

Do we understand why now we get these CMS StaticAnalyzer warnings and in previous test/changes not?
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54a768/21531/llvm-analysis/legacy-mod-sa.txt

makortel · 2022-01-05T14:25:33Z

I guess those get reported because this PR touches those files. Those changes are not visible in the diff because one commit makes the change, and a later commit undoes those changes. It would be better to squash the commits into one, and maybe change the title of the PR to something along "Remove some unnecessary header includes".

bsunanda · 2022-01-07T15:02:25Z

@jfernan2 Can you sign this - we need to fix the other DQM codes later

jfernan2 · 2022-01-07T18:07:57Z

+1

cmsbuild · 2022-01-07T18:08:18Z

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-01-07T19:05:20Z

+1

I wouldn't ask to squash the commits, so that the reason about why the other touched files were not fixed remains in the historyir

Avoid compilation warnings in some of the codes of Validation/RecoTau

ac80bd8

cmsbuild added this to the CMSSW_12_3_X milestone Dec 28, 2021

cmsbuild added code-checks-pending dqm-pending orp-pending pending-signatures tests-pending labels Dec 28, 2021

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

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

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

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Dec 30, 2021

makortel reviewed Dec 30, 2021

View reviewed changes

Restore legacy code for DQMFileLoader.h

1ddead7

cmsbuild added code-checks-pending and removed dqm-approved fully-signed tests-approved code-checks-approved labels Jan 4, 2022

makortel reviewed Jan 4, 2022

View reviewed changes

Restore also the files utilizing DQMStore

8f13cf7

cmsbuild added code-checks-pending tests-pending and removed tests-started code-checks-approved labels Jan 4, 2022

cmsbuild added tests-started and removed tests-pending labels Jan 4, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 4, 2022

cmsbuild added tests-approved and removed tests-started labels Jan 4, 2022

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Jan 7, 2022

cmsbuild added orp-approved and removed orp-pending labels Jan 7, 2022

cmsbuild merged commit 5ade6b3 into cms-sw:master Jan 7, 2022

cmsbuild mentioned this pull request Jan 10, 2022

Update Geant4 to 10.7ref09 cms-sw/cmsdist#7436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run3-Sim101F Avoid compilation warnings in some of the codes of Validation/RecoTau #36608

Run3-Sim101F Avoid compilation warnings in some of the codes of Validation/RecoTau #36608

bsunanda commented Dec 28, 2021

cmsbuild commented Dec 28, 2021

cmsbuild commented Dec 28, 2021

bsunanda commented Dec 29, 2021

cmsbuild commented Dec 29, 2021

jfernan2 commented Dec 30, 2021

cmsbuild commented Dec 30, 2021

makortel Dec 30, 2021

bsunanda Dec 31, 2021

makortel Jan 4, 2022

bsunanda Jan 4, 2022

makortel Jan 4, 2022

makortel Jan 4, 2022

makortel Jan 4, 2022

bsunanda commented Jan 4, 2022

cmsbuild commented Jan 4, 2022

cmsbuild commented Jan 4, 2022

cmsbuild commented Jan 4, 2022

jfernan2 commented Jan 5, 2022

makortel commented Jan 5, 2022

bsunanda commented Jan 7, 2022

jfernan2 commented Jan 7, 2022

cmsbuild commented Jan 7, 2022

perrotta commented Jan 7, 2022

Run3-Sim101F Avoid compilation warnings in some of the codes of Validation/RecoTau #36608

Run3-Sim101F Avoid compilation warnings in some of the codes of Validation/RecoTau #36608

Conversation

bsunanda commented Dec 28, 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 28, 2021

cmsbuild commented Dec 28, 2021

bsunanda commented Dec 29, 2021

cmsbuild commented Dec 29, 2021

Comparison Summary

jfernan2 commented Dec 30, 2021

cmsbuild commented Dec 30, 2021

makortel Dec 30, 2021

Choose a reason for hiding this comment

bsunanda Dec 31, 2021

Choose a reason for hiding this comment

makortel Jan 4, 2022

Choose a reason for hiding this comment

bsunanda Jan 4, 2022

Choose a reason for hiding this comment

makortel Jan 4, 2022

Choose a reason for hiding this comment

makortel Jan 4, 2022

Choose a reason for hiding this comment

makortel Jan 4, 2022

Choose a reason for hiding this comment

bsunanda commented Jan 4, 2022

cmsbuild commented Jan 4, 2022

cmsbuild commented Jan 4, 2022

cmsbuild commented Jan 4, 2022

Comparison Summary

jfernan2 commented Jan 5, 2022

makortel commented Jan 5, 2022

bsunanda commented Jan 7, 2022

jfernan2 commented Jan 7, 2022

cmsbuild commented Jan 7, 2022

perrotta commented Jan 7, 2022