Bugfix to merging loop in PFEGAlgo #12939

lgray · 2016-01-14T09:38:25Z

Fixes a bug in the merging loop of PFEGAlgo where it was possible that EB and EE clusters end up in the same e/gamma candidate.

The rate of this bug appears to be one in a few billion.

…kes sense to merge

lgray · 2016-01-14T09:38:35Z

@cmsbuild please test

cmsbuild · 2016-01-14T09:38:44Z

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

cmsbuild · 2016-01-14T09:38:46Z

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_8_0_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

slava77 · 2016-01-14T10:35:35Z

RecoParticleFlow/PFProducer/src/PFEGammaAlgo.cc

+         // -- check that the front is still mergeable!
+         if( !mergeTest(*roToMerge) ) {
+            // back up one step and repartition(since we will increment later)
+           nomerge = std::partition(roToMerge--,ROs.end(),mergeTest);


is this one safe against running out of bounds?

If this condition is really rare and not really expected, please add a LogWarning here

Safe against running out of bounds, yes since it starts from begin()+1.
I'll add the log warning.

cmsbuild · 2016-01-14T11:18:32Z

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

cmsbuild · 2016-01-14T12:32:33Z

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

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

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

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

@cmsbuild please test

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

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

cmsbuild · 2016-01-14T12:50:15Z

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

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

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

cmsbuild · 2016-01-14T15:44:10Z

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

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

@cmsbuild please test

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

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

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

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

cmsbuild · 2016-01-14T18:45:59Z

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

cmsbuild · 2016-01-14T19:42:25Z

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

slava77 · 2016-01-15T04:43:03Z

+1

for #12939 f6c612d

the same code change as in Bugfix to merging loop in PFEGAlgo (76X) #12940 (76X), the same topic branch
the bugfix is tested extensively in 76X; jenkins here shows no differences

cmsbuild · 2016-01-15T04:43:21Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_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-15T14:50:43Z

+1

Bugfix to merging loop in PFEGAlgo

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_8_0_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

lgray mentioned this pull request Jan 14, 2016

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

Merged

slava77 reviewed Jan 14, 2016
View reviewed changes

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 comparison-available and removed comparison-pending labels Jan 14, 2016

do a more targetted fix

f6c612d

cmsbuild added tests-pending comparison-pending and removed comparison-available tests-approved 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

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 #12939 from lgray/fix_merging_loop_pfeg

f84cb95

Bugfix to merging loop in PFEGAlgo

cmsbuild merged commit f84cb95 into cms-sw:CMSSW_8_0_X Jan 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix to merging loop in PFEGAlgo #12939

Bugfix to merging loop in PFEGAlgo #12939

lgray commented Jan 14, 2016

lgray commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

slava77 Jan 14, 2016

slava77 Jan 14, 2016

lgray 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

cmsbuild commented Jan 14, 2016

slava77 commented Jan 15, 2016

cmsbuild commented Jan 15, 2016

davidlange6 commented Jan 15, 2016

Bugfix to merging loop in PFEGAlgo #12939

Bugfix to merging loop in PFEGAlgo #12939

Conversation

lgray commented Jan 14, 2016

lgray commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

cmsbuild commented Jan 14, 2016

slava77 Jan 14, 2016

Choose a reason for hiding this comment

slava77 Jan 14, 2016

Choose a reason for hiding this comment

lgray Jan 14, 2016

Choose a reason for hiding this comment

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

cmsbuild commented Jan 14, 2016

slava77 commented Jan 15, 2016

cmsbuild commented Jan 15, 2016

davidlange6 commented Jan 15, 2016