simplify elementLoop flow, add comments #27901

jpata · 2019-08-30T01:25:54Z

PR description:

Further minor simplification of PFAlgo::elementLoop to identify core functionality.
Added a few comments.
Improved the logging output.
removed some obsolete code
No physics modifications

PR validation:

Checked that the output of runTheMatrix.py -l 38 did not change (7344 bytes in step3.root).

Other related PRs

#27505
#27153
#26894

cmsbuild · 2019-08-30T01:26:20Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-08-30T01:33:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27901/11699

This PR adds an extra 44KB to repository

cmsbuild · 2019-08-30T01:33:49Z

A new Pull Request was created by @jpata (Joosep Pata) for master.

It involves the following packages:

RecoParticleFlow/PFProducer

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @hatakeyamak, @lgray, @seemasharmafnal, @cbernet, @bachtis this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

slava77 · 2019-08-30T14:40:22Z

@cmsbuild please test

cmsbuild · 2019-08-30T14:40:49Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2279/console Started: 2019/08/30 16:41

cmsbuild · 2019-08-30T16:16:55Z

+1
Tested at: 69c32c6
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-300536/2279/summary.html

cmsbuild · 2019-08-30T16:16:59Z

Comparison job queued.

cmsbuild · 2019-08-30T18:18:21Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-300536/2279/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2955696
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2955354
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 145 log files, 15 edm output root files, 34 DQM output files

slava77 · 2019-09-03T16:33:48Z

RecoParticleFlow/PFProducer/src/PFAlgo.cc

-    unsigned iTrack = iEle;
-    reco::MuonRef muonRef = elements[iTrack].muonRef();
-    if (muonRef.isNonnull()) {
-      LogTrace("PFAlgo|elementLoop") << "track " << iTrack << " has a valid muon reference. " << endl


I do not see a replacement for this check on muonRef.
Is this just an unnecessary debugging piece?

Thanks for spotting that. Since isNonnull does not throw, in essence, this is just a debugging piece. I removed it since muonRef is not used anywhere else in function, so we might as well keep the code simpler.

slava77 · 2019-09-03T16:47:03Z

+1

for #27901 69c32c6

code changes are in line with the PR description
- one set of LogTrace printouts was omitted, which seems OK anyways
jenkins tests pass and comparisons with the baseline show no differences

cmsbuild · 2019-09-04T22:03:28Z

-1

Tested at: 69c32c6

Build:

I found compilation error when building:

STRXFRM_INTERCEPTOR_IMPL(wcsxfrm, dest, src, len);
^
[2361/4208] /build/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/slc7_amd64_gcc700/external/gcc/7.0.0-pafccj/bin/c++  -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Passes -I/build/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/slc7_amd64_gcc700/external/llvm/8.0.1-27600e/llvm-8.0.1-a0f653798ce2e441cabef0311f11f8fd2ce8b1f0/llvm/lib/Passes -Iinclude -I/build/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/slc7_amd64_gcc700/external/llvm/8.0.1-27600e/llvm-8.0.1-a0f653798ce2e441cabef0311f11f8fd2ce8b1f0/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -fPIC -MD -MT lib/Passes/CMakeFiles/LLVMPasses.dir/PassBuilder.cpp.o -MF lib/Passes/CMakeFiles/LLVMPasses.dir/PassBuilder.cpp.o.d -o lib/Passes/CMakeFiles/LLVMPasses.dir/PassBuilder.cpp.o -c /build/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/slc7_amd64_gcc700/external/llvm/8.0.1-27600e/llvm-8.0.1-a0f653798ce2e441cabef0311f11f8fd2ce8b1f0/llvm/lib/Passes/PassBuilder.cpp
[2362/4208] /build/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/slc7_amd64_gcc700/external/gcc/7.0.0-pafccj/bin/c++  -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/lld/lib/Driver -I/build/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/slc7_amd64_gcc700/external/llvm/8.0.1-27600e/llvm-8.0.1-a0f653798ce2e441cabef0311f11f8fd2ce8b1f0/lld/lib/Driver -I/build/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/slc7_amd64_gcc700/external/llvm/8.0.1-27600e/llvm-8.0.1-a0f653798ce2e441cabef0311f11f8fd2ce8b1f0/lld/include -Itools/lld/include -Iinclude -I/build/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/slc7_amd64_gcc700/external/llvm/8.0.1-27600e/llvm-8.0.1-a0f653798ce2e441cabef0311f11f8fd2ce8b1f0/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -fPIC -MD -MT tools/lld/lib/Driver/CMakeFiles/lldDriver.dir/DarwinLdDriver.cpp.o -MF tools/lld/lib/Driver/CMakeFiles/lldDriver.dir/DarwinLdDriver.cpp.o.d -o tools/lld/lib/Driver/CMakeFiles/lldDriver.dir/DarwinLdDriver.cpp.o -c /build/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/slc7_amd64_gcc700/external/llvm/8.0.1-27600e/llvm-8.0.1-a0f653798ce2e441cabef0311f11f8fd2ce8b1f0/lld/lib/Driver/DarwinLdDriver.cpp
ninja: build stopped: subcommand failed.
error: Bad exit status from /build/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/rpm-tmp.vJZyZ3 (%build)


RPM build errors:
Bad exit status from /build/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/rpm-tmp.vJZyZ3 (%build)

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-27600e/2371/summary.html

gartung · 2019-09-04T22:10:22Z

please test

cmsbuild · 2019-09-04T22:10:47Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2372/console Started: 2019/09/05 00:11

cmsbuild · 2019-09-04T23:04:17Z

The tests are being triggered in jenkins.
Tested with other pull request(s) cms-externals/llvm-project#2
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2374/console Started: 2019/09/05 01:09
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2375/console Started: 2019/09/05 01:18

cmsbuild · 2019-09-05T02:24:01Z

+1
Tested at: 69c32c6
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-27600e/2375/summary.html

cmsbuild · 2019-09-05T02:24:06Z

Comparison job queued.

cmsbuild · 2019-09-05T04:54:56Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-27600e/2375/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2955700
DQMHistoTests: Total failures: 4
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2955355
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 145 log files, 15 edm output root files, 34 DQM output files

fabiocos · 2019-09-06T07:58:26Z

@smuzaffar @slava77 it looks to me that the last issue discussed in this PR is unrelated to the PR itself but more a problem being debugged with the latest llvm settings, so this PR in itself could move forward

smuzaffar · 2019-09-06T08:07:01Z

@fabiocos , yes this is correct.
Issue was with newer llvm which is now fixed ( thanks to @gartung ). The static analysis false positive which @slava77 mentioned here #27901 (comment) has been fixed now.

fabiocos · 2019-09-06T08:38:40Z

+1

simplify elementLoop flow, add comments

69c32c6

cmsbuild added this to the CMSSW_11_0_X milestone Aug 30, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 30, 2019

jpata mentioned this pull request Aug 30, 2019

PFAlgo refactoring jpata/cmssw#56

Closed

6 tasks

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 30, 2019

cmsbuild added tests-started and removed tests-pending labels Aug 30, 2019

cmsbuild added tests-approved and removed tests-started labels Aug 30, 2019

cmsbuild mentioned this pull request Sep 2, 2019

Add energy regression and particle identification to RecoHGCal/TICL. #27634

Closed

slava77 reviewed Sep 3, 2019

View reviewed changes

cmsbuild added comparison-available and removed comparison-pending pending-signatures reconstruction-pending labels Sep 3, 2019

cmsbuild added tests-started and removed tests-approved labels Sep 4, 2019

cmsbuild added comparison-pending tests-rejected and removed comparison-available tests-started labels Sep 4, 2019

cmsbuild added tests-pending tests-started and removed tests-rejected requires-external tests-pending labels Sep 4, 2019

cmsbuild added the requires-external label Sep 4, 2019

cmsbuild added tests-approved and removed tests-started labels Sep 5, 2019

cmsbuild added comparison-available and removed comparison-pending labels Sep 5, 2019

cmsbuild added orp-approved and removed orp-pending labels Sep 6, 2019

cmsbuild merged commit 00da9f8 into cms-sw:master Sep 6, 2019

jpata deleted the pfalgo_elementloop_factorize branch October 6, 2021 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify elementLoop flow, add comments #27901

simplify elementLoop flow, add comments #27901

jpata commented Aug 30, 2019

cmsbuild commented Aug 30, 2019

cmsbuild commented Aug 30, 2019

cmsbuild commented Aug 30, 2019

slava77 commented Aug 30, 2019

cmsbuild commented Aug 30, 2019 •

edited

cmsbuild commented Aug 30, 2019

cmsbuild commented Aug 30, 2019

cmsbuild commented Aug 30, 2019

slava77 Sep 3, 2019

jpata Sep 3, 2019

slava77 commented Sep 3, 2019

cmsbuild commented Sep 4, 2019

gartung commented Sep 4, 2019

cmsbuild commented Sep 4, 2019 •

edited

cmsbuild commented Sep 4, 2019 •

edited

cmsbuild commented Sep 5, 2019

cmsbuild commented Sep 5, 2019

cmsbuild commented Sep 5, 2019

fabiocos commented Sep 6, 2019

smuzaffar commented Sep 6, 2019

fabiocos commented Sep 6, 2019

simplify elementLoop flow, add comments #27901

simplify elementLoop flow, add comments #27901

Conversation

jpata commented Aug 30, 2019

PR description:

PR validation:

Other related PRs

cmsbuild commented Aug 30, 2019

cmsbuild commented Aug 30, 2019

cmsbuild commented Aug 30, 2019

slava77 commented Aug 30, 2019

cmsbuild commented Aug 30, 2019 • edited

cmsbuild commented Aug 30, 2019

cmsbuild commented Aug 30, 2019

cmsbuild commented Aug 30, 2019

slava77 Sep 3, 2019

Choose a reason for hiding this comment

jpata Sep 3, 2019

Choose a reason for hiding this comment

slava77 commented Sep 3, 2019

cmsbuild commented Sep 4, 2019

gartung commented Sep 4, 2019

cmsbuild commented Sep 4, 2019 • edited

cmsbuild commented Sep 4, 2019 • edited

cmsbuild commented Sep 5, 2019

cmsbuild commented Sep 5, 2019

cmsbuild commented Sep 5, 2019

fabiocos commented Sep 6, 2019

smuzaffar commented Sep 6, 2019

fabiocos commented Sep 6, 2019

cmsbuild commented Aug 30, 2019 •

edited

cmsbuild commented Sep 4, 2019 •

edited

cmsbuild commented Sep 4, 2019 •

edited