Bug fix to avoid duplicated clusters on the same track in upgraded FPIX #5889

venturia · 2014-10-19T14:54:03Z

This pull request includes a bug fix and a few changes in an analysis package

…f the DEBUG messages and the configurable root file name

cmsbuild · 2014-10-19T14:57:25Z

A new Pull Request was created by @venturia for CMSSW_6_2_X_SLHC.

Bug fix to avoid duplicated clusters on the same track in upgraded FPIX

It involves the following packages:

DPGAnalysis/SiStripTools
RecoTracker/TkDetLayers

@nclopezo, @monttj, @cmsbuild, @StoyanStoynev, @slava77, @vadler can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @GiacomoSguazzoni, @rovere, @VinInn, @gpetruc, @cerati, @threus 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.
@fratnikov, @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 · 2014-10-20T06:46:48Z

RecoTracker/TkDetLayers/src/PixelForwardLayer.cc

@@ -203,7 +203,7 @@ PixelForwardLayer::groupedCompatibleDetsV( const TrajectoryStateOnSurface& tsos,
  if(!closestResult_outer.empty()){
    DetGroupElement closestGel( closestResult_outer.front().front());
    float window = computeWindowSize( closestGel.det(), closestGel.trajectoryState(), est);
-    searchNeighbors( tsos, prop, est, crossings_inner, window, result_outer, false);
+    searchNeighbors( tsos, prop, est, crossings_outer, window, result_outer, false);


is this a long standing bug? should be propagated to 7_2_X and 7_3_X?
do we know the impact on Run1?

I think it affects only upgrade geometry, despite the file has the same name in SLHC and standard releases. In the near future the names will be different to make clear that they are about different geometries (and to allow different geometries in the same release).

On Oct 20, 2014, at 8:46 , Vincenzo Innocente <notifications@github.com mailto:notifications@github.com>
wrote:

In RecoTracker/TkDetLayers/src/PixelForwardLayer.cc:

@@ -203,7 +203,7 @@ PixelForwardLayer::groupedCompatibleDetsV( const TrajectoryStateOnSurface& tsos,
if(!closestResult_outer.empty()){
DetGroupElement closestGel( closestResult_outer.front().front());
float window = computeWindowSize( closestGel.det(), closestGel.trajectoryState(), est);

searchNeighbors( tsos, prop, est, crossings_inner, window, result_outer, false);

searchNeighbors( tsos, prop, est, crossings_outer, window, result_outer, false);

is this a long standing bug? should be propagated to 7_2_X and 7_3_X?
do we know the impact on Run1?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/5889/files#r19069736.

These have been introduced with 2ebe99f , which is only in 62XSLHC.

Can I ask to keep this pull request on hold until the Tracking POG has checked its impact on the usual efficiency, fake rate and duplicate plots both for phase 1 and phase 2? This is because if something is sizeably affected by that (I don't think so) some decisions have to be taken concerning the Technical Proposal results.

Andrea

On Oct 20, 2014, at 10:55 , Volker Adler <notifications@github.com mailto:notifications@github.com> wrote:

In RecoTracker/TkDetLayers/src/PixelForwardLayer.cc:

@@ -203,7 +203,7 @@ PixelForwardLayer::groupedCompatibleDetsV( const TrajectoryStateOnSurface& tsos,
if(!closestResult_outer.empty()){
DetGroupElement closestGel( closestResult_outer.front().front());
float window = computeWindowSize( closestGel.det(), closestGel.trajectoryState(), est);

searchNeighbors( tsos, prop, est, crossings_inner, window, result_outer, false);

searchNeighbors( tsos, prop, est, crossings_outer, window, result_outer, false);

These have been introduced with 2ebe99fhttps://github.com/cms-sw/cmssw/commit/2ebe99f5c10d2b0ba7cf601cca3918a8a7c3f350 , which is only in 62XSLHC.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/5889/files#r19073779.

mark-grimes · 2014-10-20T14:34:25Z

@venturia,
No problem, just keep us informed.

cerati · 2014-10-24T11:24:33Z

Hi, what are the plans for next releases and validation? It could be easier to integrate this and look at relvals rather than private checks. I think we want the bug fixed anyway, no?

cmsbuild · 2014-10-25T09:19:36Z

+1
Tested at: f6d7641
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5889/371/summary.html

mark-grimes · 2014-10-27T11:06:32Z

Next release is on hold until we get confirmation on some particle flow issues, probably at the upgrade reco meeting tomorrow (https://hypernews.cern.ch/HyperNews/CMS/get/eflow/934.html). It's already been delayed so could be tomorrow straight after the meeting if everything is okay.

venturia · 2014-10-27T13:15:03Z

Giuseppe,

your suggestion is fine, in principle, but:

I am not sure that the relvals, especially those for SLHC releases, are enough to look at the details which are needed in this case
it is far from obvious that if this bug fix has any impact, by default CMS wants to integrate it without any discussion because of the compatibility with the Technical Proposal results we have so far.

My take is the following: 1) this bug fix as "zero" impact on the number of reconstructed tracks and on their parameters, except for the number of hits since we remove the duplicated hits. This affect a few percent of the tracks with a hit in FPIX, 2) I would be interested to know if the definition of genuine/fake/duplicated track is instead affected in a visible way. I don't think so but it is worth checking since it would be silly struggling with the duplicate/fake rate plot results without having excluded trivial causes like known bugs.

Then once 1) and 2) are settled we can decide whether or not to integrate it in SLHC releases. For sure the bug will be fixed in 73x when the upgrade code is migrated there.

              Andrea

On Oct 24, 2014, at 13:24 , cerati <notifications@github.com mailto:notifications@github.com> wrote:

Hi, what are the plans for next releases and validation? It could be easier to integrate this and look at relvals rather than private checks. I think we want the bug fixed anyway, no?

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

cerati · 2014-10-27T21:16:42Z

Hi, it was checked by Kevin that it has negligible effect and thus this PR can be merged without any problem. Cheers, g.

mark-grimes · 2014-10-27T21:42:36Z

merge

Okay. I checked it over a few days ago, so if you're happy it can go in.

mark-grimes · 2014-10-28T10:13:53Z

merge

Hmm... try again. If this doesn't merge I'll ask the dev tools support team.

venturia · 2014-10-28T10:16:51Z

I am not sure I have understood: is it me who has to try again to make the pull request?

                                    Andrea

On Oct 28, 2014, at 11:13 , Mark Grimes <notifications@github.com mailto:notifications@github.com>
wrote:

merge

Hmm... try again. If this doesn't merge I'll ask the dev tools support team.

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

…c17p2 Bug fix to avoid duplicated clusters on the same track in upgraded FPIX

mark-grimes · 2014-10-28T10:20:10Z

No, sorry. To merge a pull request I put "merge" on the first line of a comment. I did it last night and it didn't work, so I was trying again.

Many changes, needs careful review.

venturia added 9 commits October 5, 2014 11:59

Update of cfg files to debug the phase 1 detids

30d8150

Added the custom cfg in the main cfg

f352668

Fix in the custom file name

2adc8b1

Number of events as parameter

8290cd0

Added the strip selections to the configuration, added the printout o…

a8b62af

…f the DEBUG messages and the configurable root file name

Added EDAnalyzer to look for tracks with duplicated clusters

6ea7965

Updates of the OccupancyPlotsTest cfg's

5d472a9

Removed ptyhon dump in cfg files

b399e3c

bug fix in the phase 1 pixel endcap navigation

f6d7641

cmsbuild added this to the Next CMSSW_6_2_X_SLHC milestone Oct 19, 2014

cmsbuild added analysis-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 19, 2014

VinInn reviewed Oct 20, 2014
View reviewed changes

cmsbuild added analysis-pending comparison-pending orp-pending and removed analysis-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 25, 2014

cmsbuild added pending-signatures reconstruction-pending tests-approved labels Oct 25, 2014

cmsbuild added a commit that referenced this pull request Oct 28, 2014

Merge pull request #5889 from venturia/duplicated_clusters_bugfix-slh…

e370b3d

…c17p2 Bug fix to avoid duplicated clusters on the same track in upgraded FPIX

cmsbuild merged commit e370b3d into cms-sw:CMSSW_6_2_X_SLHC Oct 28, 2014

venturia deleted the duplicated_clusters_bugfix-slhc17p2 branch November 8, 2014 15:35

venturia restored the duplicated_clusters_bugfix-slhc17p2 branch November 8, 2014 15:35

makortel referenced this pull request in makortel/cmssw Dec 11, 2014

RecoTracker: Changes to PixelForwardLayer from 6_2_0_SLHC21

9f0ec13

Many changes, needs careful review.

venturia deleted the duplicated_clusters_bugfix-slhc17p2 branch June 13, 2015 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fix to avoid duplicated clusters on the same track in upgraded FPIX #5889

Bug fix to avoid duplicated clusters on the same track in upgraded FPIX #5889

venturia commented Oct 19, 2014

cmsbuild commented Oct 19, 2014

VinInn Oct 20, 2014

venturia Oct 20, 2014

vadler Oct 20, 2014

venturia Oct 20, 2014

mark-grimes commented Oct 20, 2014

cerati commented Oct 24, 2014

cmsbuild commented Oct 25, 2014

mark-grimes commented Oct 27, 2014

venturia commented Oct 27, 2014

cerati commented Oct 27, 2014

mark-grimes commented Oct 27, 2014

mark-grimes commented Oct 28, 2014

venturia commented Oct 28, 2014

mark-grimes commented Oct 28, 2014

Bug fix to avoid duplicated clusters on the same track in upgraded FPIX #5889

Bug fix to avoid duplicated clusters on the same track in upgraded FPIX #5889

Conversation

venturia commented Oct 19, 2014

cmsbuild commented Oct 19, 2014

VinInn Oct 20, 2014

Choose a reason for hiding this comment

venturia Oct 20, 2014

Choose a reason for hiding this comment

vadler Oct 20, 2014

Choose a reason for hiding this comment

venturia Oct 20, 2014

Choose a reason for hiding this comment

mark-grimes commented Oct 20, 2014

cerati commented Oct 24, 2014

cmsbuild commented Oct 25, 2014

mark-grimes commented Oct 27, 2014

venturia commented Oct 27, 2014

cerati commented Oct 27, 2014

mark-grimes commented Oct 27, 2014

mark-grimes commented Oct 28, 2014

venturia commented Oct 28, 2014

mark-grimes commented Oct 28, 2014