Fix duplicate hits outer tracker

[rovere]

FYI,
@cerati, @venturia

[cmsbuild]

A new Pull Request was created by @rovere (Marco Rovere) for CMSSW_6_2_X_SLHC.

Fix duplicate hits outer tracker

It involves the following packages:

RecoTracker/TkDetLayers

@nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please review it and eventually sign? Thanks.
@ghellwig, @gpetruc, @GiacomoSguazzoni, @cerati, @venturia 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.
@andersonjacob, @mark-grimes you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

[VinInn]

ciao.
What was the evidence/effect of this bug?

[venturia]

Good question Vincenzo,

if I got it right the same hit can be used more than once in a track. I think that this fix is to mitigate this problem in the phase 2 tracking where the problem is due to a bad choice of the detid schema and a bad interplay of it with the Navigation code. On the other hand I am wondering if there is a way to check whether this happens in general, regardless it is a bug or a feature, and to clean the hit collection to remove duplicates.

                                                                            Andrea

On May 9, 2014, at 9:14 , Vincenzo Innocente <notifications@github.commailto:notifications@github.com>
wrote:

ciao.
What was the evidence/effect of this bug?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3715#issuecomment-42639461.

[rovere]

Ciao Vincenzo,
to illustrate the problem, I attach a plot that shows the fraction of clusters (w.r.t the total number of reconstructed cluster on that DetId) on every detId in which the very same cluster (same Key) is used more that once in the very same track. As you can see the problem is usually below the 10% level, except for a particularly nasty ring: as explained by Andrea, this is due to a bad interplay between the real geometry (rings spaced in r) and its representation in C++(blades spaced in phi). I have code to produce this plot which is tuned to PhaseII but I can produce the very same plot for our current Tracker to check if the same problems is there too.

Ciao,
Marco.

[VinInn]

"this is due to a bad interplay between the real geometry (rings spaced in r) and its representation in C++(blades spaced in phi)."

Should we not look into making the reco geometry for this ring more realistic?

[rovere]

Ciao Vincenzo,
indeed we should and we will: these fixes are meant for the TP whose time-scale does not fit w/ the redesign you are asking for.

Ciao,
Marco.

[venturia]

Yes, sure, there is also a proposal for a new detid schema. But everytime there is a proposal for a detid schema there is a certain relaxation time needed to recover from the "panic mode".
On the other hand I personally think we have to investigate the possibility to have the Navigation code less dependent on the detid methods (or, now, the TrackerTopology methods) by introducing an additional layer which helps the Navigation to understand how to interpret the detid.

                           Andrea

On May 9, 2014, at 10:02 , Vincenzo Innocente <notifications@github.commailto:notifications@github.com>
wrote:

"this is due to a bad interplay between the real geometry (rings spaced in r) and its representation in C++(blades spaced in phi)."

Should we not look into making the reco geometry for this ring more realistic?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3715#issuecomment-42642160.

[VinInn]

On 9 May, 2014, at 10:07 AM, venturia notifications@github.com wrote:

Yes, sure, there is also a proposal for a new detid schema. But everytime there is a proposal for a detid schema there is a certain relaxation time needed to recover from the "panic mode".
On the other hand I personally think we have to investigate the possibility to have the Navigation code less dependent on the detid methods (or, now, the TrackerTopology methods) by introducing an additional layer which helps the Navigation to understand how to interpret the detid.

+1
My preference goes to make the current detId just a sequential number and move the seqNum->detId to the geometry, while today is the inverse!

[venturia]

Well,

your approach for sure will make the need for the intermediate layer even more burning.

                             Andrea

On May 9, 2014, at 10:10 , Vincenzo Innocente <notifications@github.commailto:notifications@github.com>
wrote:

On 9 May, 2014, at 10:07 AM, venturia <notifications@github.commailto:notifications@github.com> wrote:

Yes, sure, there is also a proposal for a new detid schema. But everytime there is a proposal for a detid schema there is a certain relaxation time needed to recover from the "panic mode".
On the other hand I personally think we have to investigate the possibility to have the Navigation code less dependent on the detid methods (or, now, the TrackerTopology methods) by introducing an additional layer which helps the Navigation to understand how to interpret the detid.

+1
My preference goes to make the current detId just a sequential number and move the seqNum->detId to the geometry, while today is the inverse!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3715#issuecomment-42642669.

[rovere]

Sorry to jump into this topic w/o all the necessary knowledge to comment... Yet the problem here is not directly related to any DetId number/schema usage in a direct way: we simply pretend to use the same C++ representation that describes blades to describe also rings, which is plain wrong.

[andersonjacob]

Tested along with #3800

passes the regular tests and fails those that are known to fail

10000
10200
10400
11200
11400
12000
12800
13000
13600
14600
12200 Step0-PASSED 0 Step1-PASSED 0 Step2-FAILED 16640 Step3-NOTRUN 0
12400 Step0-PASSED 0 Step1-FAILED 34304 Step2-NOTRUN 0 Step3-NOTRUN 0
12600 Step0-PASSED 0 Step1-FAILED 34304 Step2-NOTRUN 0 Step3-NOTRUN 0
13800 Step0-PASSED 0 Step1-FAILED 34304 Step2-NOTRUN 0 Step3-NOTRUN 0
14000 Step0-PASSED 0 Step1-FAILED 34304 Step2-NOTRUN 0 Step3-NOTRUN 0
14200 Step0-PASSED 0 Step1-PASSED 0 Step2-FAILED 34304 Step3-NOTRUN 0
14400 Step0-PASSED 0 Step1-PASSED 0 Step2-FAILED 16640 Step3-NOTRUN 0

[andersonjacob]

merge