Bugfix to merging loop in PFEGAlgo (76X) #12940

lgray · 2016-01-14T09:39:50Z

Backport of #12939.

…kes sense to merge

lgray · 2016-01-14T09:39:59Z

@cmsbuild please test

cmsbuild · 2016-01-14T09:40:11Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/10492/console

cmsbuild · 2016-01-14T09:40:12Z

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_7_6_X.

It involves the following packages:

RecoParticleFlow/PFProducer

@cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @bachtis this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

+1|approve[d]|sign[ed]: L1/L2's to approve it
-1|reject[ed]: L1/L2's to reject it
assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
hold: L1/all L2's/release manager to mark it as on hold
unhold: L1/user who put this PR on hold
merge: L1/release managers to merge this request
[@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
[@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

cmsbuild · 2016-01-14T11:09:57Z

+1
Tested at: f496727
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12940/10492/summary.html

cmsbuild · 2016-01-14T12:09:28Z

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

cmsbuild · 2016-01-14T12:39:37Z

Pull request #12940 was updated. @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please check and sign again.

lgray · 2016-01-14T12:40:09Z

@cmsbuild please test

cmsbuild · 2016-01-14T12:40:31Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/10499/console

cmsbuild · 2016-01-14T12:51:00Z

-1
Tested at: 20ea22a
I found an error when building:

>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2016-01-10-1100/src/RecoParticleFlow/PFProducer/src/PFCandConnector.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2016-01-10-1100/src/RecoParticleFlow/PFProducer/src/PFEGammaAlgo.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2016-01-10-1100/src/RecoParticleFlow/PFProducer/src/PFEGammaFilters.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2016-01-10-1100/src/RecoParticleFlow/PFProducer/src/PFEGammaHeavyObjectCache.cc 
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2016-01-10-1100/src/RecoParticleFlow/PFProducer/src/PFEGammaAlgo.cc: In member function 'void PFEGammaAlgo::mergeROsByAnyLink(std::listPFEGammaAlgo::ProtoEGObject&)':
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2016-01-10-1100/src/RecoParticleFlow/PFProducer/src/PFEGammaAlgo.cc:1452:56: error: 'LOGWARNING' was not declared in this scope
            LOGWARNING("PFEGammaAlgo::mergeROsByAnyLink") 
                                                        ^
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2016-01-10-1100/src/RecoParticleFlow/PFProducer/src/PFElectronAlgo.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2016-01-10-1100/src/RecoParticleFlow/PFProducer/src/PFMuonAlgo.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2016-01-10-1100/src/RecoParticleFlow/PFProducer/src/PFPhotonAlgo.cc

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12940/10499/summary.html

cmsbuild · 2016-01-14T17:12:25Z

Pull request #12940 was updated. @cmsbuild, @cvuosalo, @davidlange6, @slava77 can you please check and sign again.

lgray · 2016-01-14T17:12:27Z

@cmsbuild please test

cmsbuild · 2016-01-14T17:12:45Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/10519/console

cmsbuild · 2016-01-14T18:47:30Z

+1
Tested at: f6c612d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12940/10519/summary.html

cmsbuild · 2016-01-14T19:58:17Z

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

slava77 · 2016-01-15T04:36:16Z

+1

for #12940 f6c612d

code changes look fairly localized/targeted as expected
jenkins tests pass and comparisons with baseline show no differences
previously crashing Run 256676, Event 947703079, LumiSection 673 /Run2015D/JetHT/RAW doesn't crash anymore and generates just one warning
additional ~2K events from several workflows tested to gain stats or add some more sanity checks: the warning doesn't show up

cmsbuild · 2016-01-15T04:36:34Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

davidlange6 · 2016-01-15T04:56:46Z

RecoParticleFlow/PFProducer/src/PFEGammaAlgo.cc

@@ -1445,6 +1445,18 @@ initializeProtoCands(std::list<PFEGammaAlgo::ProtoEGObject>& egobjs) {
 	 << "Found objects " << std::distance(mergestart,nomerge)
 	 << " to merge by links to the front!" << std::endl;
       for( auto roToMerge = mergestart; roToMerge != nomerge; ++roToMerge) {
+         //bugfix! L.Gray 14 Jan 2016 
+         // -- check that the front is still mergeable!
+         if( thefront.ecalclusters.size() && roToMerge->ecalclusters.size() ) {


size could better be !empty() [or less size()!=0]. Consider for a follow pr.

"empty()" (begin==end) by itself is cheaper than size.

davidlange6 · 2016-01-15T04:57:07Z

+1

Bugfix to merging loop in PFEGAlgo (76X)

bugfix to merging loop in PFEGAlgo, check that front of list still ma…

f496727

…kes sense to merge

cmsbuild added this to the Next CMSSW_7_6_X milestone Jan 14, 2016

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending comparison-pending labels Jan 14, 2016

cmsbuild added tests-started and removed tests-pending labels Jan 14, 2016

cmsbuild added tests-approved and removed tests-started labels Jan 14, 2016

cmsbuild added comparison-available tests-pending comparison-pending and removed comparison-pending comparison-available tests-approved labels Jan 14, 2016

cmsbuild added tests-started and removed tests-pending labels Jan 14, 2016

cmsbuild added tests-rejected and removed tests-started labels Jan 14, 2016

cmsbuild added tests-pending comparison-pending labels Jan 14, 2016

cmsbuild added tests-started and removed tests-pending labels Jan 14, 2016

cmsbuild added tests-approved and removed tests-started labels Jan 14, 2016

cmsbuild added comparison-available and removed comparison-pending labels Jan 14, 2016

cmsbuild added reconstruction-approved fully-signed and removed pending-signatures reconstruction-pending labels Jan 15, 2016

slava77 mentioned this pull request Jan 15, 2016

Bugfix to merging loop in PFEGAlgo #12939

Merged

davidlange6 reviewed Jan 15, 2016
View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Jan 15, 2016

cmsbuild added a commit that referenced this pull request Jan 15, 2016

Merge pull request #12940 from lgray/fix_merging_loop_pfeg

4278d6e

Bugfix to merging loop in PFEGAlgo (76X)

cmsbuild merged commit 4278d6e into cms-sw:CMSSW_7_6_X Jan 15, 2016

cmsbuild added a commit that referenced this pull request Jan 15, 2016

Merge pull request #12940 from lgray/fix_merging_loop_pfeg

e71d9d5

cmsbuild added a commit that referenced this pull request Jan 15, 2016

Merge pull request #12940 from lgray/fix_merging_loop_pfeg

113c338

cmsbuild added a commit that referenced this pull request Jan 15, 2016

Merge pull request #12940 from lgray/fix_merging_loop_pfeg

bbf2c75

arizzi mentioned this pull request Mar 7, 2016

CMG Heppy updates, 76X cbernet/cmssw#69

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix to merging loop in PFEGAlgo (76X) #12940

Bugfix to merging loop in PFEGAlgo (76X) #12940

lgray commented Jan 14, 2016

lgray commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

lgray commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

lgray commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

slava77 commented Jan 15, 2016

cmsbuild commented Jan 15, 2016

davidlange6 Jan 15, 2016

slava77 Jan 15, 2016

davidlange6 commented Jan 15, 2016

Bugfix to merging loop in PFEGAlgo (76X) #12940

Bugfix to merging loop in PFEGAlgo (76X) #12940

Conversation

lgray commented Jan 14, 2016

lgray commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

lgray commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

lgray commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

slava77 commented Jan 15, 2016

cmsbuild commented Jan 15, 2016

davidlange6 Jan 15, 2016

Choose a reason for hiding this comment

slava77 Jan 15, 2016

Choose a reason for hiding this comment

davidlange6 commented Jan 15, 2016