HCAL reco speed up by David L. #6669

StoyanStoynev · 2014-11-27T16:51:22Z

This is a copy from https://github.com/davidlange6/cmssw/tree/hcal_speedups_141121 (davidlange6@477c4b2). For now the purpose of the PR is just to make the code available and ease the discussion. I am to comment more below but overall the code is a bit over 20% faster with minor regressions.

StoyanStoynev · 2014-11-27T16:52:21Z

RecoLocalCalo/HcalRecAlgos/src/HBHEPulseShapeFlag.cc

+      if(OffsetTemp + 25 >= (int)cipSize)
+	C1 = CumulativeIdealPulse[cipSize-1];
+      else
+	if( OffsetTemp  >= 25)


I asked David if this is intended or an error.

I think it's an error.
The old code had everything in terms of "OffsetTemp + 25". There were two ifs: one for at least cipSize, and one "between 0 and below cipSize". So, else if ( OffsetTemp >= 25) should be else if ( OffsetTemp >= - 25)
It's a bit surprising that there is no large effect from this, maybe it's numerically small

cmsbuild · 2014-11-27T16:56:27Z

A new Pull Request was created by @StoyanStoynev for CMSSW_7_4_X.

HCAL reco speed up by David L.

It involves the following packages:

RecoLocalCalo/HcalRecAlgos
RecoLocalCalo/HcalRecProducers

@cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@argiro this is something you requested to watch as well.
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.

StoyanStoynev · 2014-11-27T17:04:23Z

I ran on ttbar sample (wf 202, 200 events) including with igprof. From the time report I see
for the hbheprereco module : 0.0930 -> 0.0768 sec/evt . From igprof I see
HcalHitReconstructor::produce() : 22.7 -> 17.9 CPU ticks (or marks), HcalSimpleRecAlgo::reconstruct() : 20.1 -> 16.4 ticks , HBHEPulseShapeFlagSetter::SetPulseShapeFlags() : 1.48 -> 0.47 ticks . Without going into details, this is consistent with 20+% speeding up of the relevant code. I only saw very minor "random" fluctuations in when comparing to the base IB (it was in 73X) with DQM and FWLite scripts.

StoyanStoynev · 2014-11-27T17:08:37Z

I expect interested parties to comment/test/adopt (@lihux25 @violatingcp @abdoulline @bachtis ).

StoyanStoynev · 2014-11-27T17:10:01Z

RecoLocalCalo/HcalRecAlgos/src/HcalSimpleRecAlgo.cc

@@ -62,6 +62,7 @@ void HcalSimpleRecAlgo::setpuCorrParams(bool   iPedestalConstraint, bool iTimeCo
 					double its3Chi2,double its4Chi2,double its345Chi2,double iChargeThreshold, int iFitTimes) { 
  if( iPedestalConstraint ) assert ( iPedSig );
  if( iTimeConstraint ) assert( iTimeSig );
+  std::cout << "ciao\n";


to be removed

abdoulline · 2014-11-27T17:59:13Z

I take the liberty to notify Igor.

While I appreciate David's effort to make it 20% faster,
and why not profit from it, if the results are very much
the same as before,
but it will not make it HLT-compliant, for the latter we need to
change the approach to some parameterized non-fitting form...

Salavat

On Thu, 27 Nov 2014, StoyanStoynev wrote:

I expect interested parties to comment/test/adopt (@lihux25 @violatingcp @abdoulline @bachtis ).

—
Reply to this email directly or view it on
GitHub.[AEx02r5oXCvXSIJhUOzAgOUVgLnPuzG4ks5nR1IYgaJpZM4DBgOW.gif]

slava77 · 2014-11-27T18:15:27Z

Hi Salavat,

My understanding of the plans for HLT was that it will most likely be
HLT-specific, while the current RECO version will stay at least for some
period of time (e.g. end-of-the-2015 reprocessing ).
Is this correct, or do you plan some imminent changes to the RECO
version of the method2?

Please clarify.
Thank you.

On 11/27/14, 11:59 AM, abdoulline wrote:

I take the liberty to notify Igor.

While I appreciate David's effort to make it 20% faster,
and why not profit from it, if the results are very much
the same as before,
but it will not make it HLT-compliant, for the latter we need to
change the approach to some parameterized non-fitting form...

Salavat

On Thu, 27 Nov 2014, StoyanStoynev wrote:

I expect interested parties to comment/test/adopt (@lihux25 @violatingcp @abdoulline @bachtis ).

—
Reply to this email directly or view it on
GitHub.[AEx02r5oXCvXSIJhUOzAgOUVgLnPuzG4ks5nR1IYgaJpZM4DBgOW.gif]

—
Reply to this email directly or view it on GitHub
#6669 (comment).

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

abdoulline · 2014-11-27T18:39:13Z

You're right, Slava. This is most plausible scenario.
HLT will stay apart...

On Thu, 27 Nov 2014, Slava Krutelyov wrote:

Hi Salavat,

My understanding of the plans for HLT was that it will most likely be
HLT-specific, while the current RECO version will stay at least for some
period of time (e.g. end-of-the-2015 reprocessing ).
Is this correct, or do you plan some imminent changes to the RECO
version of the method2?

Please clarify.
Thank you.

On 11/27/14, 11:59 AM, abdoulline wrote:

I take the liberty to notify Igor.

While I appreciate David's effort to make it 20% faster,
and why not profit from it, if the results are very much
the same as before,
but it will not make it HLT-compliant, for the latter we need to
change the approach to some parameterized non-fitting form...

Salavat

On Thu, 27 Nov 2014, StoyanStoynev wrote:

I expect interested parties to comment/test/adopt (@lihux25 @violatingcp @abdoulline
@bachtis ).

—
Reply to this email directly or view it on
GitHub.[AEx02r5oXCvXSIJhUOzAgOUVgLnPuzG4ks5nR1IYgaJpZM4DBgOW.gif]

—
Reply to this email directly or view it on GitHub
#6669 (comment).

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

—
Reply to this email directly or view it on
GitHub.[AEx02sokG9rh46OwrZwDyEcgDcz-Tve-ks5nR2HCgaJpZM4DBgOW.gif]

cmsbuild · 2014-11-28T08:45:58Z

+1
Tested at: 8b8b15d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6669/1219/summary.html

cmsbuild · 2014-11-28T09:28:21Z

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

cmsbuild · 2014-11-29T13:08:41Z

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

StoyanStoynev · 2014-12-01T15:13:36Z

@abdoulline Are you going to test or anyway make use of the PR? Our tests are with some limits so if you want us to sign off this PR better let us know you also approve it explicitly. In any case you have the code, you can just use it in your forthcoming (?) PR/s/.

abdoulline · 2014-12-01T15:28:55Z

as far as I see, all the relevant PR test distributions come out green,
so my suggestion is: it's worth having it in the (74X) release to proceed
with regular RelVals.

Salavat

On Mon, 1 Dec 2014, StoyanStoynev wrote:

@abdoulline Are you going to test or anyway make use of the PR? Our tests are with some
limits so if you want us to sign off this PR better let us know you also approve it
explicitly. In any case you have the code, you can just use it in your forthcoming (?)
PR/s/.

—
Reply to this email directly or view it on
GitHub.[AEx02nFVrYHfBbsM-3hcar0nklLnL_XFks5nTH0jgaJpZM4DBgOW.gif]

StoyanStoynev · 2014-12-01T16:54:22Z

+1
For 373ec3d
My tests were on David's tag (quoted in the description). The difference with the current one is a couple of couts and a bug fix (apparently the bug introduced by David didn't affect the performance much). My tests were on top of CMSSW_7_3_X_2014-11-21-1400. I said the speed up for the relevant code was ~20+% with minor differences in monitored histograms (DQM, FWLite scripts), I am giving an example with calo tower energy:

In addition jenkins also show no dramatic performance effects though they are statistically limited.
So, as desired by HCAL people I am putting this PR forward as is.

StoyanStoynev · 2014-12-01T16:54:46Z

The plot above is for wf 202.

cmsbuild · 2014-12-01T16:57:34Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes (tests are also fine). This pull request will be automatically merged.

…00/HCAL_David HCAL reco speed up by David L.

davidlange6 and others added 3 commits November 20, 2014 11:50

Merged refs/pull/6468/head from repository cms-sw

1df1332

speed up 25ns code - but getting down to minuit limitations

477c4b2

Merged hcal_speedups_141121 from repository davidlange6

8b8b15d

StoyanStoynev reviewed Nov 27, 2014
View reviewed changes

cmsbuild added this to the Next CMSSW_7_4_X milestone Nov 27, 2014

cmsbuild added comparison-pending pending-signatures reconstruction-pending tests-pending labels Nov 27, 2014

StoyanStoynev reviewed Nov 27, 2014
View reviewed changes

cmsbuild added comparison-pending pending-signatures reconstruction-pending tests-approved and removed comparison-pending pending-signatures reconstruction-pending tests-pending labels Nov 28, 2014

cmsbuild removed the comparison-pending label Nov 28, 2014

cmsbuild added comparison-pending pending-signatures reconstruction-pending tests-approved and removed reconstruction-pending tests-pending labels Nov 29, 2014

cmsbuild added comparison-available pending-signatures reconstruction-pending tests-approved and removed comparison-pending pending-signatures reconstruction-pending tests-approved labels Nov 29, 2014

cmsbuild removed comparison-available pending-signatures reconstruction-pending tests-approved labels Dec 1, 2014

cmsbuild added comparison-available fully-signed reconstruction-approved tests-approved labels Dec 1, 2014

cmsbuild added a commit that referenced this pull request Dec 1, 2014

Merge pull request #6669 from StoyanStoynev/CMSSW_7_4_X_2014-11-27-02…

14323cc

…00/HCAL_David HCAL reco speed up by David L.

cmsbuild merged commit 14323cc into cms-sw:CMSSW_7_4_X Dec 1, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HCAL reco speed up by David L. #6669

HCAL reco speed up by David L. #6669

StoyanStoynev commented Nov 27, 2014

StoyanStoynev Nov 27, 2014

slava77 Nov 27, 2014

cmsbuild commented Nov 27, 2014

StoyanStoynev commented Nov 27, 2014

StoyanStoynev commented Nov 27, 2014

StoyanStoynev Nov 27, 2014

abdoulline commented Nov 27, 2014

slava77 commented Nov 27, 2014

abdoulline commented Nov 27, 2014

cmsbuild commented Nov 28, 2014

cmsbuild commented Nov 28, 2014

cmsbuild commented Nov 29, 2014

StoyanStoynev commented Dec 1, 2014

abdoulline commented Dec 1, 2014

StoyanStoynev commented Dec 1, 2014

StoyanStoynev commented Dec 1, 2014

cmsbuild commented Dec 1, 2014

HCAL reco speed up by David L. #6669

HCAL reco speed up by David L. #6669

Conversation

StoyanStoynev commented Nov 27, 2014

StoyanStoynev Nov 27, 2014

Choose a reason for hiding this comment

slava77 Nov 27, 2014

Choose a reason for hiding this comment

cmsbuild commented Nov 27, 2014

StoyanStoynev commented Nov 27, 2014

StoyanStoynev commented Nov 27, 2014

StoyanStoynev Nov 27, 2014

Choose a reason for hiding this comment

abdoulline commented Nov 27, 2014

slava77 commented Nov 27, 2014

abdoulline commented Nov 27, 2014

cmsbuild commented Nov 28, 2014

cmsbuild commented Nov 28, 2014

cmsbuild commented Nov 29, 2014

StoyanStoynev commented Dec 1, 2014

abdoulline commented Dec 1, 2014

StoyanStoynev commented Dec 1, 2014

StoyanStoynev commented Dec 1, 2014

cmsbuild commented Dec 1, 2014