Removed asserts preventing fast re-recoes a la ECALELF #6556

shervin86 · 2014-11-22T11:33:52Z

The removal of asserts should not hurt, but prevents fast re-recoes
@lgray and cms-phys-conveners-EGM should probably double check ad approve

cmsbuild · 2014-11-22T11:36:18Z

A new Pull Request was created by @shervin86 for CMSSW_7_3_X.

Removed asserts preventing fast re-recoes a la ECALELF

It involves the following packages:

DataFormats/EgammaCandidates

@cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@lgray 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.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

slava77 · 2014-11-22T11:54:39Z

@Sam-Harper
Sam, you should also get yourself added to the list of watchers of the egamma-related packages
you can do it at https://github.com/cms-sw/cmssw/issues/new

cmsbuild · 2014-11-22T13:44:07Z

+1
Tested at: 8811538
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6556/1086/summary.html

cmsbuild · 2014-11-22T14:07:13Z

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

StoyanStoynev · 2014-11-23T09:59:56Z

DataFormats/EgammaCandidates/src/GsfElectron.cc

-  assert(ctfInfo.ctfTrack==(GsfElectron::core()->ctfTrack())) ;
-  assert(ctfInfo.shFracInnerHits==(GsfElectron::core()->ctfGsfOverlap())) ;
+  //  assert(ctfInfo.ctfTrack==(GsfElectron::core()->ctfTrack())) ;
+  //  assert(ctfInfo.shFracInnerHits==(GsfElectron::core()->ctfGsfOverlap())) ;


You should really "remove" them if this is the decision, not commenting them out.

If I receive the green light I can do another pull request removing the lines.

Shervin

On Sun, 23 Nov 2014, StoyanStoynev wrote:

Date: Sun, 23 Nov 2014 02:00:00 -0800
From: StoyanStoynev notifications@github.com
Reply-To: cms-sw/cmssw
<reply+0045c225c004a1e368e80fd5885cca6f92c6443a95aab3cc92cf000000011089722
092a170ce013cd2ca@reply.github.com>
To: cms-sw/cmssw cmssw@noreply.github.com
Cc: shervin86 shervin@cern.ch
Subject: Re: [cmssw] Removed asserts preventing fast re-recoes a la ECALELF
(#6556)

@@ -42,8 +42,8 @@ GsfElectron::GsfElectron
setVertex(math::XYZPoint(te.positionAtVtx.x(),te.positionAtVtx.y(),te.positionAtVtx.z())) ;
setPdgId(-11_charge) ;
/if (ecalDrivenSeed())*/ corrections.correctedEcalEnergy = superCluster()->energy() ;

assert(ctfInfo.ctfTrack==(GsfElectron::core()->ctfTrack())) ;

assert(ctfInfo.shFracInnerHits==(GsfElectron::core()->ctfGsfOverlap())) ;

// assert(ctfInfo.ctfTrack==(GsfElectron::core()->ctfTrack())) ;

// assert(ctfInfo.shFracInnerHits==(GsfElectron::core()->ctfGsfOverlap())) ;

You should really "remove" them if this is the decision, not commenting them out.

Reply to this email directly or view it on GitHub:
https://github.com/cms-sw/cmssw/pull/6556/files#r20763338

I don't get it - what is the point of another PR?

Ok, let me know if/what to do.

Shervin

On Mon, 24 Nov 2014, StoyanStoynev wrote:

Date: Mon, 24 Nov 2014 01:59:15 -0800
From: StoyanStoynev notifications@github.com
Reply-To: cms-sw/cmssw
<reply+0045c225d1ad8dbf71e46d0c9e820fecb03c52472fc680f692cf00000001108ac37
392a170ce013d16c9@reply.github.com>
To: cms-sw/cmssw cmssw@noreply.github.com
Cc: shervin86 shervin@cern.ch
Subject: Re: [cmssw] Removed asserts preventing fast re-recoes a la ECALELF
(#6556)

@@ -42,8 +42,8 @@ GsfElectron::GsfElectron
setVertex(math::XYZPoint(te.positionAtVtx.x(),te.positionAtVtx.y(),te.positionAtVtx.z())) ;
setPdgId(-11_charge) ;
/if (ecalDrivenSeed())*/ corrections.correctedEcalEnergy = superCluster()->energy() ;

assert(ctfInfo.ctfTrack==(GsfElectron::core()->ctfTrack())) ;

assert(ctfInfo.shFracInnerHits==(GsfElectron::core()->ctfGsfOverlap())) ;

// assert(ctfInfo.ctfTrack==(GsfElectron::core()->ctfTrack())) ;

// assert(ctfInfo.shFracInnerHits==(GsfElectron::core()->ctfGsfOverlap())) ;

I don't get it - what is the point of another PR?

Reply to this email directly or view it on GitHub:
https://github.com/cms-sw/cmssw/pull/6556/files#r20780745

Sam-Harper · 2014-11-24T10:33:18Z

I'm still a little confused about this. Surely this is just checking you have the right core and if you dont, well bad things can happen. Why is it causing problems? Is the fast re-reco not fully making things?

StoyanStoynev · 2014-11-25T13:18:05Z

I expect the question by Sam to be answered (I expected @lgray to comment earlier but he was likely busy).

lgray · 2014-11-25T13:20:10Z

@StoyanStoynev Yes, Monday was interesting. Anyway, this questions definitely needs to be answered since it is a basic sanity check in the normal reconstruction.

StoyanStoynev · 2014-12-01T15:05:47Z

Is there any progress in addressing the question(s) raised?

StoyanStoynev · 2014-12-05T17:01:57Z

ping

shervin86 · 2014-12-05T18:47:30Z

Sorry for the late answer.
I removed those asserts in my private code 2 years ago and always patched the GsfElectron module since then. I don't remember exactly why.
In any case this is not relevant. There is no reason to have asserts in the code, these are not protections but used only for debugging purpose. In any case they cause the crash in production...
I wonder why are we waiting for particular approval of this pull request.

                        Shervin

On Fri, 5 Dec 2014, StoyanStoynev wrote:

Date: Fri, 5 Dec 2014 09:02:00 -0800
From: StoyanStoynev notifications@github.com
Reply-To: cms-sw/cmssw
<reply+0045c225b24de175c5e77802ec9ea675f521a419a3059eaa92cf000000011099a70
892a169ce02f7a518@reply.github.com>
To: cms-sw/cmssw cmssw@noreply.github.com
Cc: shervin86 shervin@cern.ch
Subject: Re: [cmssw] Removed asserts preventing fast re-recoes a la ECALELF
(#6556)

ping

Reply to this email directly or view it on GitHub:
#6556 (comment)

StoyanStoynev · 2014-12-06T08:42:49Z

On the reco part we were waiting the POG convener and RECO coordinator questions to be answered. That is - why this is causing problems at all? Even if you resolve the crash you may be hiding another problem because the crash can not be explained by this part of the code itself (as Lindsay said these were sanity checks).

StoyanStoynev · 2014-12-09T14:12:26Z

@lgray @Sam-Harper Can this be discussed in one of the POG meetings and decide what to do?

davidlange6 · 2014-12-12T17:06:43Z

Closing this as it is definitely for 74x at this point

Removed asserts preventing fast re-recoes a la ECALELF

8811538

cmsbuild added this to the Next CMSSW_7_3_X milestone Nov 22, 2014

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

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

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

cmsbuild added the tests-approved label Nov 22, 2014

StoyanStoynev reviewed Nov 23, 2014
View reviewed changes

cmsbuild mentioned this pull request Nov 26, 2014

Removed asserts preventing fast re-recoes a la ECALELF #6625

Merged

davidlange6 closed this Dec 12, 2014

shervin86 deleted the GsfElectronFix branch February 25, 2015 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed asserts preventing fast re-recoes a la ECALELF #6556

Removed asserts preventing fast re-recoes a la ECALELF #6556

shervin86 commented Nov 22, 2014

cmsbuild commented Nov 22, 2014

slava77 commented Nov 22, 2014

cmsbuild commented Nov 22, 2014

cmsbuild commented Nov 22, 2014

StoyanStoynev Nov 23, 2014

shervin86 Nov 24, 2014

StoyanStoynev Nov 24, 2014

shervin86 Nov 24, 2014

Sam-Harper commented Nov 24, 2014

StoyanStoynev commented Nov 25, 2014

lgray commented Nov 25, 2014

StoyanStoynev commented Dec 1, 2014

StoyanStoynev commented Dec 5, 2014

shervin86 commented Dec 5, 2014

StoyanStoynev commented Dec 6, 2014

StoyanStoynev commented Dec 9, 2014

davidlange6 commented Dec 12, 2014

Removed asserts preventing fast re-recoes a la ECALELF #6556

Removed asserts preventing fast re-recoes a la ECALELF #6556

Conversation

shervin86 commented Nov 22, 2014

cmsbuild commented Nov 22, 2014

slava77 commented Nov 22, 2014

cmsbuild commented Nov 22, 2014

cmsbuild commented Nov 22, 2014

StoyanStoynev Nov 23, 2014

Choose a reason for hiding this comment

shervin86 Nov 24, 2014

Choose a reason for hiding this comment

StoyanStoynev Nov 24, 2014

Choose a reason for hiding this comment

shervin86 Nov 24, 2014

Choose a reason for hiding this comment

Sam-Harper commented Nov 24, 2014

StoyanStoynev commented Nov 25, 2014

lgray commented Nov 25, 2014

StoyanStoynev commented Dec 1, 2014

StoyanStoynev commented Dec 5, 2014

shervin86 commented Dec 5, 2014

StoyanStoynev commented Dec 6, 2014

StoyanStoynev commented Dec 9, 2014

davidlange6 commented Dec 12, 2014