Reco Updates -- Fix ParticleFlow ECAL Cluster timing. #837

lgray · 2013-09-15T16:58:53Z

Down to 0.05 sec/event from 0.12 sec/event in #706.
The timing was 0.03 sec/event originally.
Further improvements would require factorization of PositionCalc code to take advantage of the PFCluster's refs to its rechits. Most of the extra time is spent building the necessary structures for pos calc and then in poscalc it is spent searching for the rechits in the sorted collection.

Since the PFClusters have the refs to the rechits that compose them, searching for the associated rechit again isn't really necessary.

@argiro : Perhaps we could factor apart the rechit energy determination and position weighting parts of PositionCalc into two steps? If the sorted rechit collection wasn't required for the latter it would speed things up significantly in PFClusterAlgo.

…er by refactorizing PositionCalc

cmsbuild · 2013-09-15T17:02:35Z

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

Fix particleFlowClusterECAL timing.

It involves the following packages:

RecoParticleFlow/Configuration
RecoParticleFlow/PFClusterProducer

@thspeer, @slava77 can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@ktf you are the release manager for this.

argiro · 2013-09-16T07:19:46Z

On 15 Sep 2013, at 18:58, Lindsey Gray notifications@github.com wrote:

@bendavid @slava77

Down to 0.05 sec/event from 0.12 sec/event in #706.
The timing was 0.03 sec/event originally.
Further improvements would require factorization of PositionCalc code to take advantage of the PFCluster's refs to its rechits. Most of the extra time is spent building the necessary structures for pos calc and then in poscalc it is spent searching for the rechits in the sorted collection.

Since the PFClusters have the refs to the rechits that compose them, searching for the associated rechit again isn't really necessary.

@argiro : Perhaps we could factor apart the rechit energy determination and position weighting parts of PositionCalc into two steps?

sure, it's probably quicker if you do the change and I will review and cross check

Stefano

You can merge this Pull Request by running

git pull https://github.com/lgray/cmssw fix_ECALpfclustertiming
Or view, comment on, or merge it at:

#837

Commit Summary

• changes to RecoToDisplay
• fix to cluster timing, down to ~0.05 sec/event from 0.12, can do better by refactorizing PositionCalc
File Changes

• M RecoParticleFlow/Configuration/test/RecoToDisplay_batch_cfg.py (4)
• M RecoParticleFlow/Configuration/test/RecoToDisplay_cfg.py (6)
• M RecoParticleFlow/PFClusterProducer/interface/PFClusterAlgo.h (25)
• M RecoParticleFlow/PFClusterProducer/src/PFClusterAlgo.cc (69)
Patch Links:

• https://github.com/cms-sw/cmssw/pull/837.patch
• https://github.com/cms-sw/cmssw/pull/837.diff

nclopezo · 2013-09-16T09:07:44Z

+1
https://cmssdt.cern.ch/jenkins/job/Pull-Request-Integration/ARCHITECTURE=slc5_amd64_gcc481/512/console
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/512

lgray · 2013-09-16T12:02:26Z

@nclopezo http://t.qkme.me/3svyaf.jpg

nclopezo · 2013-09-16T12:09:03Z

Hi @lgray
I am not signing it. Giulio told me to write a +1 on the first line of my comment from now on, each time the pull requests passes all the usual tests. He will use it on cms-bot, as I understood.

lgray · 2013-09-16T12:13:56Z

@nclopezo Ahh ok, I was just uninformed. :-)

ktf · 2013-09-16T12:23:20Z

Indeed. The plan is to have a "tests-pending" / "tests-done" label so that we can quickly find out what as been tested and what not, eventually resetting the label if a pull request is updated. @nclopezo this does not prevent you from actually reading the comments. In this particular case a change was requested and in any case you'll have to run again.

cmsbuild · 2013-09-17T10:42:36Z

Pull request #837 was updated. @nclopezo, @thspeer, @slava77 can you please check and sign again.

lgray · 2013-09-17T10:43:05Z

@argiro Please have a look at the above commit and double check that I haven't caused some bug.
The timing of particleFlowClusterECAL is now down to 0.036 sec/Event.
@slava77 Is that acceptable?

slava77 · 2013-09-24T22:36:02Z

@slava77 working on it

slava77 · 2013-09-25T12:59:57Z

@lgray
Hi Lindsey,
there are many changes in the outputs.

Changes in PF photons in EE are quite significant for TTBarPU workflow (202.0)

The same distribution looks ~OK for signle gamma 35 sample.

Could you please investigate and see if there is a bug.
In general, it would be better to have technical/timing performance changes come without undeclared changes in physics.

I was testing with respect to CMSSW_7_0_X_2013-09-24-1400

Thanks

Slava

cmsbuild · 2013-09-26T12:02:33Z

Pull request #837 was updated. @nclopezo, @smuzaffar, @thspeer, @slava77 can you please check and sign again.

cmsbuild · 2013-09-26T12:22:33Z

Pull request #837 was updated. @nclopezo, @smuzaffar, @thspeer, @slava77 can you please check and sign again.

…iteration

cmsbuild · 2013-09-27T12:42:34Z

Pull request #837 was updated. @nclopezo, @smuzaffar, @thspeer, @slava77 can you please check and sign again.

slava77 · 2013-09-27T16:45:39Z

RecoEcal/EgammaCoreTools/interface/PositionCalc.h

+  if( 0 == iRecHits || 0 == iSubGeom ) {
+    throw cms::Exception("PositionCalc")
+      << "Calculate_Location() called uninitialized or wrong initialization.";
+  }

  if( 0 != iDetIds.size()   &&


(since we are still fishing for a problem in this PR, why not make a simple fix)
! iDetIds.empty()
(and similarly below) takes less cycles

slava77 · 2013-09-30T11:59:01Z

Hi Lindsey,

Redoing the tests from scratch still didn't help.
There are still large discrepancies.

It's not immediately obvious to me that the changes in the PFClusterAlgo are identical
(PositionCalc looks OK).

Slava

cmsbuild · 2013-09-30T12:27:34Z

Pull request #837 was updated. @nclopezo, @smuzaffar, @thspeer, @slava77 can you please check and sign again.

nclopezo · 2013-09-30T13:53:40Z

+1

https://cmssdt.cern.ch/jenkins/job/Pull-Request-Integration/ARCHITECTURE=slc5_amd64_gcc481/699/console
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/699

And you can see the comparison with CMSSW_7_0_X_2013-09-30-0200 here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_7_0_X_2013-09-30-0200+837/78/

ktf · 2013-09-30T13:56:49Z

Logs are suspiciously too equal. We should check they are actually correct.

slava77 · 2013-09-30T13:58:39Z

Which logs?

ktf · 2013-09-30T14:25:24Z

The comparison log @nclopezo post ed

Sent from my iPhone

On 30/set/2013, at 15:58, slava77 notifications@github.com wrote:

Which logs?

—
Reply to this email directly or view it on GitHub.

slava77 · 2013-09-30T14:35:01Z

Do you mean the baseLineComparisons ? (my inference powers are dwindling)

nclopezo · 2013-09-30T14:38:14Z

yes, that ones

slava77 · 2013-09-30T14:50:34Z

OK, that was the goal of this PR, to have no differences in physics performance
and the last few iterations were not successful to get there.

lgray · 2013-09-30T15:18:24Z

@slava77 I'll check again to make sure the timing improvements are there after moving the push_back().

slava77 · 2013-09-30T15:23:14Z

Lindsey,
I'm running my tests already.
So, you can just twiddle your thumbs in the meantime.

lgray · 2013-09-30T16:32:42Z

@slava77 0.045 sec/event from running on 200 events of ttbar. Let me know how the rest turns out.

slava77 · 2013-09-30T20:03:00Z

The EBCLT dicluster mass is the only kind of distribution that's somewhat different.
Since the diff is in bin zero, I'm guessing this is all numerical. The difference is similar in ele/gamma guns and in ttbarPU

slava77 · 2013-09-30T20:11:51Z

+1

Tested 8eba4e6 in CMSSW_7_0_X_2013-09-24-1400 as sign254.
There are no differences in histogram level comparisons of products.
Minor difference of numerical nature appears in a few DQM ecal monitoring plots. Maybe the most visible is displayed above.

Timing of particleFlowClusterECAL went down reasonably close to the performance before changes made in #706.
For a reference, 202.0 200 events total time in reco went from 6sec to 24 sec (#706) to 8sec (#837).

cmsbuild · 2013-09-30T20:12:36Z

This pull request is fully signed and it will be integrated in one of the next IBs unless changes or unless it breaks tests. @ktf can you please take care of it?

Reco Updates -- Fix ParticleFlow ECAL Cluster timing.

lgray added 2 commits September 11, 2013 11:13

changes to RecoToDisplay

a38c1a3

fix to cluster timing, down to ~0.05 sec/event from 0.12, can do bett…

6163569

…er by refactorizing PositionCalc

further timing improvements to PositionCalc

bc264b3

pull detId push_back out of if statement

951a4bb

0 -> 0.0

a49486d

lgray added 3 commits September 27, 2013 14:32

reinterpret_cast to safer static_cast

66260f9

better precision in evaluation of log weight, one fewer multiply per …

f0a1d79

…iteration

if statement to improve timing for linear weighting case

dffd558

slava77 reviewed Sep 27, 2013
View reviewed changes

move push_back infront of continues

8eba4e6

slava77 mentioned this pull request Sep 30, 2013

particleFlowClusterECAL timing degraded with #706 slava77/cmssw#5

Closed

ktf added a commit that referenced this pull request Oct 2, 2013

Merge pull request #837 from lgray/fix_ECALpfclustertiming

163a843

Reco Updates -- Fix ParticleFlow ECAL Cluster timing.

ktf merged commit 163a843 into cms-sw:CMSSW_7_0_X Oct 2, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reco Updates -- Fix ParticleFlow ECAL Cluster timing. #837

Reco Updates -- Fix ParticleFlow ECAL Cluster timing. #837

lgray commented Sep 15, 2013

cmsbuild commented Sep 15, 2013

argiro commented Sep 16, 2013

nclopezo commented Sep 16, 2013

lgray commented Sep 16, 2013

nclopezo commented Sep 16, 2013

lgray commented Sep 16, 2013

ktf commented Sep 16, 2013

cmsbuild commented Sep 17, 2013

lgray commented Sep 17, 2013

slava77 commented Sep 24, 2013

slava77 commented Sep 25, 2013

cmsbuild commented Sep 26, 2013

cmsbuild commented Sep 26, 2013

cmsbuild commented Sep 27, 2013

slava77 Sep 27, 2013

slava77 commented Sep 30, 2013

cmsbuild commented Sep 30, 2013

nclopezo commented Sep 30, 2013

ktf commented Sep 30, 2013

slava77 commented Sep 30, 2013

ktf commented Sep 30, 2013

slava77 commented Sep 30, 2013

nclopezo commented Sep 30, 2013

slava77 commented Sep 30, 2013

lgray commented Sep 30, 2013

slava77 commented Sep 30, 2013

lgray commented Sep 30, 2013

slava77 commented Sep 30, 2013

slava77 commented Sep 30, 2013

cmsbuild commented Sep 30, 2013

Reco Updates -- Fix ParticleFlow ECAL Cluster timing. #837

Reco Updates -- Fix ParticleFlow ECAL Cluster timing. #837

Conversation

lgray commented Sep 15, 2013

cmsbuild commented Sep 15, 2013

argiro commented Sep 16, 2013

nclopezo commented Sep 16, 2013

lgray commented Sep 16, 2013

nclopezo commented Sep 16, 2013

lgray commented Sep 16, 2013

ktf commented Sep 16, 2013

cmsbuild commented Sep 17, 2013

lgray commented Sep 17, 2013

slava77 commented Sep 24, 2013

slava77 commented Sep 25, 2013

cmsbuild commented Sep 26, 2013

cmsbuild commented Sep 26, 2013

cmsbuild commented Sep 27, 2013

slava77 Sep 27, 2013

Choose a reason for hiding this comment

slava77 commented Sep 30, 2013

cmsbuild commented Sep 30, 2013

nclopezo commented Sep 30, 2013

ktf commented Sep 30, 2013

slava77 commented Sep 30, 2013

ktf commented Sep 30, 2013

slava77 commented Sep 30, 2013

nclopezo commented Sep 30, 2013

slava77 commented Sep 30, 2013

lgray commented Sep 30, 2013

slava77 commented Sep 30, 2013

lgray commented Sep 30, 2013

slava77 commented Sep 30, 2013

slava77 commented Sep 30, 2013

cmsbuild commented Sep 30, 2013