[RECONSTRUCTION] code-checks/format; clean code to remove global functions #34140

smuzaffar · 2021-06-15T14:26:42Z

code-checks/format applied on full cmssw with extra misc-definitions-in-headers clang-tidy check
clean up various headers to remove functions in global namespace by either moving the function inside the class or merging the .h and .cc files of plugins
delete dead/unused code RecoTracker/ConversionSeedGenerators/plugins/Conv4HitsReco.cc

cmsbuild · 2021-06-15T14:37:10Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34140/23336

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

cmsbuild · 2021-06-15T14:37:31Z

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master.

It involves the following packages:

CommonTools/RecoAlgos
DataFormats/DTRecHit
EventFilter/HcalRawToDigi
MagneticField/GeomBuilder
PhysicsTools/PatUtils
RecoBTag/FeatureTools
RecoBTag/ImpactParameter
RecoBTag/SecondaryVertex
RecoEgamma/EgammaTools
RecoMuon/MuonIdentification
RecoParticleFlow/PFClusterProducer
RecoPixelVertexing/PixelTriplets
RecoPixelVertexing/PixelVertexFinding
RecoTracker/ConversionSeedGenerators
RecoTracker/DebugTools
RecoVertex/AdaptiveVertexFinder
TrackingTools/AnalyticalJacobians
TrackingTools/TrackFitters

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@emilbols, @gouskos, @felicepantaleo, @jainshilpi, @hatakeyamak, @rappoccio, @Martin-Grunewald, @ahinzmann, @abbiendi, @varuns23, @seemasharmafnal, @mmarionncern, @JyothsnaKomaragiri, @makortel, @mtosi, @JanFSchulte, @jhgoh, @lgray, @jdolen, @HuguesBrun, @cericeci, @hqucms, @ferencek, @trocino, @rociovilar, @sscruz, @Sam-Harper, @cbernet, @GiacomoSguazzoni, @rovere, @VinInn, @jdamgov, @bellan, @nhanvtran, @gkasieczka, @schoef, @ebrondol, @abdoulline, @dgulhan, @clelange, @namapane, @battibass, @Fedespring, @calderona, @sobhatta, @lecriste, @afiqaize, @gpetruc, @mariadalfonso, @andrzejnovak, @ram1123 this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

perrotta · 2021-06-15T15:15:43Z

please test

cmsbuild · 2021-06-15T16:30:49Z

-1

Failed Tests: Build
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c54a17/15975/summary.html
COMMIT: e33940c
CMSSW: CMSSW_12_0_X_2021-06-15-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34140/15975/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_0_X_2021-06-15-1100/src/TrackingTools/AnalyticalJacobians/test/ErrorPropOri_t.cpp
>> Building binary ErrorPropOri_t
/cvmfs/cms-ib.cern.ch/nweek-02685/slc7_amd64_gcc900/external/gcc/9.3.0/bin/../lib/gcc/x86_64-unknown-linux-gnu/9.3.0/../../../../x86_64-unknown-linux-gnu/bin/ld: tmp/slc7_amd64_gcc900/src/TrackingTools/AnalyticalJacobians/test/ErrorPropOri_t/ErrorPropOri_t.cpp.o: in function `main':
ErrorPropOri_t.cpp:(.text.startup+0x7b6): undefined reference to `AnalyticalCurvilinearJacobian::computeFullJacobian(GlobalTrajectoryParameters const&, Point3DBase const&, Vector3DBase const&, Vector3DBase const&, double const&)'
/cvmfs/cms-ib.cern.ch/nweek-02685/slc7_amd64_gcc900/external/gcc/9.3.0/bin/../lib/gcc/x86_64-unknown-linux-gnu/9.3.0/../../../../x86_64-unknown-linux-gnu/bin/ld: ErrorPropOri_t.cpp:(.text.startup+0xba0): undefined reference to `AnalyticalCurvilinearJacobian::computeFullJacobian(GlobalTrajectoryParameters const&, Point3DBase const&, Vector3DBase const&, Vector3DBase const&, double const&)'
collect2: error: ld returned 1 exit status
>> Deleted: tmp/slc7_amd64_gcc900/src/TrackingTools/AnalyticalJacobians/test/ErrorPropOri_t/ErrorPropOri_t
gmake: *** [tmp/slc7_amd64_gcc900/src/TrackingTools/AnalyticalJacobians/test/ErrorPropOri_t/ErrorPropOri_t] Error 1
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_0_X_2021-06-15-1100/src/TrackingTools/AnalyticalJacobians/test/ErrorProp_t.cpp
>> Building binary ErrorProp_t
/cvmfs/cms-ib.cern.ch/nweek-02685/slc7_amd64_gcc900/external/gcc/9.3.0/bin/../lib/gcc/x86_64-unknown-linux-gnu/9.3.0/../../../../x86_64-unknown-linux-gnu/bin/ld: tmp/slc7_amd64_gcc900/src/TrackingTools/AnalyticalJacobians/test/ErrorProp_t/ErrorProp_t.cpp.o: in function `main':

smuzaffar · 2021-06-15T16:40:36Z

TrackingTools/AnalyticalJacobians/src/AnalyticalCurvilinearJacobianSSE.icc

-                                                        const GlobalVector& p,
-                                                        const GlobalVector& h,
-                                                        const double& s) {
+inline void AnalyticalCurvilinearJacobian::computeFullJacobian(const GlobalTrajectoryParameters& globalParameters,


@makortel , looks like this change caused the build error #34140 (comment)

I woiuld suggest to add //NOLINT(misc-definitions-in-headers) for this

Hmm. In this case the AnalyticalCurvilinearJacobianSSE.icc (and AnalyticalCurvilinearJacobianEXT.icc) are more source-like than header-like files. An easy way would be to rename these files to use another file extension, but that was disfavored earlier (#33366 (comment)).

An alternative would be to rename those files to .cc, and move the #ifdefs there along

// in AnalyticalCurvilinearJacobian.cc #if !(defined(USE_SSEVECT) || defined(USE_EXTVECT)) || defined(TRPRFN_SCALAR) void AnalyticalCurvilinearJacobian::computeFullJacobian(...) { ... } #endif // in AnalyticalCurvilinearJacobianSSE.cc #if defined(USE_SSEVECT) && !defined(TRPRFN_SCALAR) void AnalyticalCurvilinearJacobian::computeFullJacobian(...) { ... } #endif // in AnalyticalCurvilinearJacobianEXT.cc #if defined(USE_EXTVECT) && !defined(TRPRFN_SCALAR) void AnalyticalCurvilinearJacobian::computeFullJacobian(...) { ... } #endif

but keeping the #else case manually up to date would be prone to errors.

So maybe the NOLINT would be least bad.

thanks @makortel , I have added // NOLINTNEXTLINE(misc-definitions-in-headers) for this

smuzaffar · 2021-06-15T21:00:31Z

RecoPixelVertexing/PixelTriplets/plugins/CAHitNtupletGeneratorKernelsAlloc.h

@@ -6,7 +6,7 @@ template <>
 #ifdef __CUDACC__
 void CAHitNtupletGeneratorKernelsGPU::allocateOnGPU(int32_t nHits, cudaStream_t stream) {
 #else
-void CAHitNtupletGeneratorKernelsCPU::allocateOnGPU(int32_t nHits, cudaStream_t stream) {
+inline void CAHitNtupletGeneratorKernelsCPU::allocateOnGPU(int32_t nHits, cudaStream_t stream) {
 #endif


also, should we add the following for both of these definitions?

// NOLINTNEXTLINE(misc-definitions-in-headers)

Here we could do the same trick as in #33366 (comment), i.e. rename CAHitNtupletGeneratorKernelsAlloc.h to CAHitNtupletGeneratorKernelsAlloc.cc and in CAHitNtupletGeneratorKernelsAlloc.cu have #include "CAHitNtupletGeneratorKernelsAlloc.cc".

ah right, updated as suggested

cmsbuild · 2021-06-16T16:12:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34140/23367

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

cmsbuild · 2021-06-16T16:12:28Z

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

smuzaffar · 2021-06-16T16:15:32Z

please test

VinInn · 2021-06-16T16:39:18Z

RecoVertex/AdaptiveVertexFinder/plugins/TemplatedVertexArbitrator.h

@@ -42,8 +42,10 @@

 //#define VTXDEBUG

-const unsigned int nTracks(const reco::Vertex &sv) { return sv.nTracks(); }
-const unsigned int nTracks(const reco::VertexCompositePtrCandidate &sv) { return sv.numberOfSourceCandidatePtrs(); }
+inline const unsigned int nTracks(const reco::Vertex &sv) { return sv.nTracks(); }


I would embed this in place using constexpr if (std::is_same ... etc
or at least add some sort of namespace....

Is this really a header file?
it is a producer, so it shall be final and most probably is missing just two macros...

still a function named nTracks shall not be allowed in the global namespace so inline even if it becomes a .src

I guess we can just merge this header in to its source file ( https://github.com/cms-sw/cmssw/blob/master/RecoVertex/AdaptiveVertexFinder/plugins/VertexArbitrators.cc )

@cms-sw/reconstruction-l2 , any objection on merging TemplatedVertexArbitrator.h in to VertexArbitrators.cc which is the only src file which includes it

I think that it's fine to merge this to RecoVertex/AdaptiveVertexFinder/plugins/VertexArbitrators.cc

IIUC, the nTracks should be moved to the anonymous namespace (the easiest; else it's is_same with sfinae)

TemplatedVertexArbitrator.h is now merged in to VertexArbitrators.cc

slava77 · 2021-06-22T21:24:32Z

RecoTracker/ConversionSeedGenerators/plugins/Conv4HitsReco.cc

+#include <TVector3.h>
+#include "FWCore/MessageLogger/interface/MessageLogger.h"
+
+class Conv4HitsReco {


this is not quite an appropriate change. This class is not a plugin. The declaration should be available in a header file.
It is kind of dead code though and is perhaps OK to be removed.

yes looks like dead code, nothing is using it. +1 to drop it.

slava77 · 2021-06-22T21:39:44Z

TrackingTools/TrackFitters/plugins/KFFittingSmoother.h

@@ -126,13 +126,13 @@ namespace {
 #define PRINT LogTrace("")
 #endif

-  Trajectory KFFittingSmoother::fitOne(const Trajectory& t, fitType type) const {


it looks like this file was not updated. From the way it is written, we either need a single include protection with e.g. a #define KFFittingSmootherESProducer_cc in KFFittingSmootherESProducer.cc and then an ifdef in this file; or move the contents to KFFittingSmootherESProducer.cc

cmsbuild · 2021-06-22T22:15:20Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34140/23465

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

cmsbuild · 2021-06-22T22:15:46Z

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

smuzaffar · 2021-06-23T05:50:50Z

please test

cmsbuild · 2021-06-23T10:33:00Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c54a17/16181/summary.html
COMMIT: 3eb8cb5
CMSSW: CMSSW_12_0_X_2021-06-22-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34140/16181/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 7 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c54a17/16181/llvm-analysis/esrget-sa.txt for details.
CMS Clang-Tidy warnings: There are 4 Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c54a17/16181/llvm-analysis/cmsclangtidy.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: 38
DQMHistoTests: Total histograms compared: 2785631
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2785602
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 37 files compared)
Checked 160 log files, 37 edm output root files, 38 DQM output files
TriggerResults: no differences found

slava77 · 2021-06-23T15:01:37Z

+reconstruction

for #34140 3eb8cb5

code changes are in line with the PR description and the follow up review; the changes are technical (or remove unused code)
jenkins tests pass and comparisons with the baseline show no (relevant) differences

cmsbuild · 2021-06-23T15:02:01Z

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

qliphy · 2021-06-23T15:55:32Z

+1

cmsbuild added this to the CMSSW_12_0_X milestone Jun 15, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jun 15, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 15, 2021

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

cmsbuild added tests-rejected and removed tests-started labels Jun 15, 2021

smuzaffar commented Jun 15, 2021

View reviewed changes

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-rejected labels Jun 16, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 16, 2021

cmsbuild added tests-started and removed tests-pending labels Jun 16, 2021

VinInn reviewed Jun 16, 2021

View reviewed changes

cmsbuild mentioned this pull request Jun 22, 2021

Update L1 tracking for HL-LHC (fixed) #34028

Closed

slava77 reviewed Jun 22, 2021

View reviewed changes

smuzaffar added 2 commits June 23, 2021 00:06

remove dead code in Conv4HitsReco.cc

6935f28

move KFFittingSmoother.h in to its source file

3eb8cb5

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-approved labels Jun 22, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 22, 2021

cmsbuild added tests-started and removed tests-pending labels Jun 23, 2021

cmsbuild added tests-approved and removed tests-started labels Jun 23, 2021

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jun 23, 2021

cmsbuild added orp-approved and removed orp-pending labels Jun 23, 2021

cmsbuild merged commit 97ed514 into cms-sw:master Jun 23, 2021

smuzaffar deleted the 12_0-codechecks-RECONSTRUCTION branch June 23, 2021 16:08

cmsbuild mentioned this pull request Jun 23, 2021

remove 2017 tests from dataProcessing runCfgTest #34229

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RECONSTRUCTION] code-checks/format; clean code to remove global functions #34140

[RECONSTRUCTION] code-checks/format; clean code to remove global functions #34140

smuzaffar commented Jun 15, 2021 •

edited

cmsbuild commented Jun 15, 2021

cmsbuild commented Jun 15, 2021

perrotta commented Jun 15, 2021

cmsbuild commented Jun 15, 2021

smuzaffar Jun 15, 2021

smuzaffar Jun 15, 2021

makortel Jun 15, 2021

smuzaffar Jun 16, 2021

smuzaffar Jun 15, 2021

makortel Jun 15, 2021

smuzaffar Jun 16, 2021

cmsbuild commented Jun 16, 2021

cmsbuild commented Jun 16, 2021

smuzaffar commented Jun 16, 2021

VinInn Jun 16, 2021

VinInn Jun 16, 2021 •

edited

VinInn Jun 16, 2021 •

edited

smuzaffar Jun 16, 2021

smuzaffar Jun 16, 2021

slava77 Jun 17, 2021

smuzaffar Jun 21, 2021

slava77 Jun 22, 2021

smuzaffar Jun 22, 2021

slava77 Jun 22, 2021

cmsbuild commented Jun 22, 2021

cmsbuild commented Jun 22, 2021

smuzaffar commented Jun 23, 2021

cmsbuild commented Jun 23, 2021

slava77 commented Jun 23, 2021

cmsbuild commented Jun 23, 2021

qliphy commented Jun 23, 2021

[RECONSTRUCTION] code-checks/format; clean code to remove global functions #34140

[RECONSTRUCTION] code-checks/format; clean code to remove global functions #34140

Conversation

smuzaffar commented Jun 15, 2021 • edited

cmsbuild commented Jun 15, 2021

cmsbuild commented Jun 15, 2021

perrotta commented Jun 15, 2021

cmsbuild commented Jun 15, 2021

Build

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Jun 16, 2021

cmsbuild commented Jun 16, 2021

smuzaffar commented Jun 16, 2021

Choose a reason for hiding this comment

VinInn Jun 16, 2021 • edited

Choose a reason for hiding this comment

VinInn Jun 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Jun 22, 2021

cmsbuild commented Jun 22, 2021

smuzaffar commented Jun 23, 2021

cmsbuild commented Jun 23, 2021

Comparison Summary

slava77 commented Jun 23, 2021

cmsbuild commented Jun 23, 2021

qliphy commented Jun 23, 2021

smuzaffar commented Jun 15, 2021 •

edited

VinInn Jun 16, 2021 •

edited

VinInn Jun 16, 2021 •

edited