Merge .h and .cc files of plugins in some CommonTools packages #35216

guitargeek · 2021-09-09T14:34:26Z

PR description:

Part of the series of PRs that merges .h files with the .cc files of the plugins in some CMSSW subsystems.

This continues the work done in #34660.

It makes maintaining the plugins much easier and also excludes the possibility to wrongly include plugin header files.

The changes made in this PR are only copy-paste and sorting includes to make the review easy.

Note that the ShallowCloneProducer.h file was moved from CommonTools/CandAlgos/interface to CommonTools/RecoAlgos/plugins because it is the only place where it is used.

PR validation:

CMSSW compiles.

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

No backport intended.

cmsbuild · 2021-09-09T14:41:43Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35216/25162

This PR adds an extra 36KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CommonTools/ParticleFlow/plugins/PFPileUp.cc modified in PR(s): Unify vertex association between CHS and PUPPI #33885
- File CommonTools/ParticleFlow/plugins/PFPileUp.h modified in PR(s): Unify vertex association between CHS and PUPPI #33885
- File CommonTools/ParticleFlow/plugins/SealModule.cc modified in PR(s): Unify vertex association between CHS and PUPPI #33885
- File CommonTools/PileupAlgos/plugins/PuppiProducer.cc modified in PR(s): Unify vertex association between CHS and PUPPI #33885
- File CommonTools/PileupAlgos/plugins/PuppiProducer.h modified in PR(s): Unify vertex association between CHS and PUPPI #33885

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-35216/25162/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35216/25162/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

jpata · 2021-09-10T12:02:24Z

@cmsbuild code-checks

cmsbuild · 2021-09-11T09:16:41Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35216/25199

This PR adds an extra 12KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CommonTools/ParticleFlow/plugins/PFPileUp.cc modified in PR(s): Unify vertex association between CHS and PUPPI #33885
- File CommonTools/ParticleFlow/plugins/PFPileUp.h modified in PR(s): Unify vertex association between CHS and PUPPI #33885
- File CommonTools/ParticleFlow/plugins/SealModule.cc modified in PR(s): Unify vertex association between CHS and PUPPI #33885
- File CommonTools/PileupAlgos/plugins/PuppiProducer.cc modified in PR(s): Unify vertex association between CHS and PUPPI #33885
- File CommonTools/PileupAlgos/plugins/PuppiProducer.h modified in PR(s): Unify vertex association between CHS and PUPPI #33885

cmsbuild · 2021-09-11T09:17:04Z

A new Pull Request was created by @guitargeek (Jonas Rembser) for master.

It involves the following packages:

CommonTools/CandAlgos (reconstruction)
CommonTools/ParticleFlow (reconstruction)
CommonTools/PileupAlgos (reconstruction)
CommonTools/RecoAlgos (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @ahinzmann, @abbiendi, @jhgoh, @jdolen, @gkasieczka, @hatakeyamak, @clelange, @cbernet 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

jpata · 2021-09-13T12:47:45Z

@cmsbuild please test

cmsbuild · 2021-09-13T15:55:07Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fc44f1/18548/summary.html
COMMIT: ccfea4b
CMSSW: CMSSW_12_1_X_2021-09-13-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35216/18548/install.sh to create a dev area with all the needed externals and cmssw changes.

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

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3001001
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000973
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

cmsbuild · 2021-09-14T15:03:41Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35216/25255

ERROR: Build errors found during clang-tidy run.

    tokenVertexAssociation_ = consumes<CandToVertex>(iConfig.getParameter<edm::InputTag>("vertexAssociation"));
                                       ^
CommonTools/PileupAlgos/plugins/PuppiProducer.cc:113:5: error: use of undeclared identifier 'tokenVertexAssociationQuality_' [clang-diagnostic-error]
    tokenVertexAssociationQuality_ =
    ^
CommonTools/PileupAlgos/plugins/PuppiProducer.cc:157:8: error: use of undeclared identifier 'fUseVertexAssociation' [clang-diagnostic-error]
  if ((fUseVertexAssociation) && (!fUseExistingWeights)) {
       ^
CommonTools/PileupAlgos/plugins/PuppiProducer.cc:158:31: error: use of undeclared identifier 'tokenVertexAssociation_' [clang-diagnostic-error]
    associatedPV = iEvent.get(tokenVertexAssociation_);
                              ^
CommonTools/PileupAlgos/plugins/PuppiProducer.cc:159:37: error: use of undeclared identifier 'tokenVertexAssociationQuality_' [clang-diagnostic-error]
    associationQuality = iEvent.get(tokenVertexAssociationQuality_);
                                    ^
CommonTools/PileupAlgos/plugins/PuppiProducer.cc:194:11: error: use of undeclared identifier 'fUseVertexAssociation' [clang-diagnostic-error]
      if (fUseVertexAssociation) {
          ^
CommonTools/PileupAlgos/plugins/PuppiProducer.cc:197:47: error: use of undeclared identifier 'vertexAssociationQuality_' [clang-diagnostic-error]
        if (PVOrig.isNonnull() && (quality >= vertexAssociationQuality_)) {
                                              ^
Suppressed 1746 warnings (1742 in non-user code, 3 NOLINT, 1 with check filters).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

cmsbuild · 2021-09-14T15:16:55Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35216/25258

This PR adds an extra 20KB to repository

cmsbuild · 2021-09-14T15:17:15Z

Pull request #35216 was updated. @jpata, @cmsbuild, @slava77 can you please check and sign again.

slava77 · 2021-09-15T12:40:10Z

@cmsbuild please test

cmsbuild · 2021-09-15T16:16:43Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fc44f1/18624/summary.html
COMMIT: f076690
CMSSW: CMSSW_12_1_X_2021-09-14-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35216/18624/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fc44f1/18624/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fc44f1/18624/git-merge-result

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

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 12 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000833
DQMHistoTests: Total failures: 11
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3000799
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 38 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

jpata

I checked that all the code changes look reasonable, nothing unexpected.

The reco differences in 1325.81, 136.731 seem to be related to the DNN non-reproducibility: #32628
Dumping the pairDiscri names:

73 pfParticleNetJetTags:probWcq
100 pfParticleNetDiscriminatorsJetTags:HccvsQCD
103 pfParticleNetDiscriminatorsJetTags:ZbbvsQCD

PFMet still looks like a legacy class, pinged PF contacts. It is not a blocker for this PR as such, can be a follow-up.

jpata · 2021-09-16T09:53:26Z

CommonTools/ParticleFlow/plugins/PFMET.cc

+#include <memory>
+#include <string>
+
+class PFMET : public edm::EDProducer {


Is this class used in production? If so, I believe it should be ported to either edm::one or preferrably edm::stream.
@laurenhay @marksan87

jpata · 2021-09-16T10:26:48Z

+reconstruction

technical, no unexpected reco changes (ONNX inference shows non-reproducibility here)
follow-up may be required to migrate PFMet to edm::one/edm::stream: Merge .h and .cc files of plugins in some CommonTools packages #35216 (comment) cc @laurenhay @marksan87

cmsbuild · 2021-09-16T10:27:12Z

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-09-16T14:14:25Z

+1

cmsbuild added this to the CMSSW_12_1_X milestone Sep 9, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 9, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Sep 9, 2021

cmsbuild added code-checks-pending and removed code-checks-rejected labels Sep 11, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 11, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 13, 2021

cmsbuild added tests-approved code-checks-pending tests-pending and removed tests-started tests-approved code-checks-approved labels Sep 13, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Sep 14, 2021

Merge .cc and .h for CommonTools/ParticleFlow and PileupAlgos plugins

9ad852c

guitargeek added 2 commits September 14, 2021 17:09

Move ShallowCloneProducer to RecoAlgos and define typenames in one file

d62d750

Make CandMatcher and NewCandMatcher aliases and not derived classes

f076690

cmsbuild added code-checks-pending and removed code-checks-rejected labels Sep 14, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 14, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 15, 2021

cmsbuild added tests-approved and removed tests-started labels Sep 15, 2021

jpata reviewed Sep 16, 2021

View reviewed changes

jpata mentioned this pull request Sep 16, 2021

Non-reproducibility in DeepTau in 1325.81 #32628

Open

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Sep 16, 2021

cmsbuild added orp-approved and removed orp-pending labels Sep 16, 2021

cmsbuild merged commit f77b403 into cms-sw:master Sep 16, 2021

guitargeek deleted the CommonTools_merging_1 branch September 16, 2021 14:19

cmsbuild mentioned this pull request Sep 20, 2021

Update Geant4 to 10.7.2 cms-sw/cmsdist#7316

Merged

guitargeek mentioned this pull request Aug 24, 2022

Merge .h and .cc files of plugins in RecoLocalCalo/EcalRecProducers #39171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge .h and .cc files of plugins in some CommonTools packages #35216

Merge .h and .cc files of plugins in some CommonTools packages #35216

guitargeek commented Sep 9, 2021

cmsbuild commented Sep 9, 2021

jpata commented Sep 10, 2021

cmsbuild commented Sep 11, 2021

cmsbuild commented Sep 11, 2021

jpata commented Sep 13, 2021

cmsbuild commented Sep 13, 2021

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

slava77 commented Sep 15, 2021

cmsbuild commented Sep 15, 2021

jpata left a comment

jpata Sep 16, 2021 •

edited

jpata commented Sep 16, 2021

cmsbuild commented Sep 16, 2021

qliphy commented Sep 16, 2021

Merge .h and .cc files of plugins in some CommonTools packages #35216

Merge .h and .cc files of plugins in some CommonTools packages #35216

Conversation

guitargeek commented Sep 9, 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 Sep 9, 2021

jpata commented Sep 10, 2021

cmsbuild commented Sep 11, 2021

cmsbuild commented Sep 11, 2021

jpata commented Sep 13, 2021

cmsbuild commented Sep 13, 2021

Comparison Summary

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

cmsbuild commented Sep 14, 2021

slava77 commented Sep 15, 2021

cmsbuild commented Sep 15, 2021

Comparison Summary

jpata left a comment

Choose a reason for hiding this comment

jpata Sep 16, 2021 • edited

Choose a reason for hiding this comment

jpata commented Sep 16, 2021

cmsbuild commented Sep 16, 2021

qliphy commented Sep 16, 2021

jpata Sep 16, 2021 •

edited