Forward-port three simple tracking PRs from 620_SLHC

[makortel]

This PR is a forward-port of the following PRs in 620_SLHC branch that are not yet in 74X

• Fix PixelTrackCleanerWrapper for quadruplet SeedingHitSet #4372: Fix PixelTrackCleanerWrapper for quadruplet SeedingHitSet
• Fpix navigation allfixed newdetid #3921: Fpix navigation allfixed newdetid
  • Everything else of it is already included except small enhancement in LogError message that looks possibly useful
• Remove vector reserve in SeedGeneratorFromRegionHits #4180: Remove vector reserve in SeedGeneratorFromRegionHits

No regression expected, no regression observed (tested with RelValTTbar_13 in 740pre6).

@rovere @VinInn @boudoul @venturia @davidlange6, please review

[cmsbuild]

A new Pull Request was created by @makortel (Matti Kortelainen) for CMSSW_7_4_X.

Forward-port three simple tracking PRs from 620_SLHC

It involves the following packages:

RecoPixelVertexing/PixelTrackFitting
RecoTracker/TkDetLayers
RecoTracker/TkSeedGenerator

@cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks.
@ghellwig, @GiacomoSguazzoni, @rovere, @VinInn, @appeltel, @gpetruc, @cerati, @dgulhan, @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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

[cvuosalo]

please test

[cmsbuild]

The tests are being triggered in jenkins.

[cvuosalo]

@makortel: I ran basic matrix tests and found an error in workflow 140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI (step 2):

cmsRun: /build/cvuosalo/reco/pr7627/CMSSW_7_4_0_pre6-testPR7627/src/RecoTracker/TkSeedGenerator/plugins/SeedGeneratorFromProtoTracksEDProducer.cc:126: virtual void SeedGeneratorFromProtoTracksEDProducer::produce(edm::Event&, const edm::EventSetup&): Assertion `hits.size()<4' failed.

Could a necessary file have been left out of this PR? Does SeedGeneratorFromProtoTracksEDProducer.cc have any relationship to this PR?

The Jenkins tests haven't completed yet, but when they do, I'll check if they show the same error.

[cmsbuild]

-1
Tested at: c99a7dd
When I ran the RelVals I found an error in the following worklfows:
140.53 step2

runTheMatrix-results/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/step2_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI.log

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7627/2471/summary.html

[makortel]

@cvuosalo Jenkins seems to agree with you, but for me runTheMatrix.py -l 140.53 succeeds, i.e. I'm unable to reproduce it. The problem can be related to this PR, as d9602cf allows to have 4-hit pixel tracks. Investigating...

[makortel]

Ok, it was a matter of git cms-addpkg RecoTracker/TkSeedGenerator && scram b, now I can reproduce the issue.

[makortel]

I have a bit more information. The "bug" (how we get 4-hit pixel track from 3-layer pixel detector) occurs in TrackCleaner (used by PixelTrackProducer labelled as "hiPixel3PrimTracks"). What happens is that it gets two 3-hit tracks as an input that share the hits on layers 2 and 3. The code then merges these two tracks such that the resulting track has 4 hits: 2 from layer 1, and 1 from both layers 2 and 3.

I have no idea if this is the intended behaviour of TrackCleaner (I'll dig in the code next), but what happens currently (i.e. before d9602cf) is that this 4-hit track gets truncated to 3-hit track with 2 hits from layer 1 and 1 from layer 2 (which, to me, sounds like a bug).

[makortel]

Based on the code in TrackCleaner::cleanTracks() it looks to me that it behaves as intended. I'm not sure how to proceed.

I would actually ask next why the assert in SeedGeneratorFromProtoTracksEDProducer is needed (not obvious from the code)?

[slava77]

Would the problem happen in pp tracking if the same kind of hit pattern is
present?
On Feb 10, 2015 4:17 AM, "Matti Kortelainen" notifications@github.com
wrote:

Based on the code in TrackCleaner::cleanTracks() it looks to me that it
behaves as intended. I'm not sure how to proceed.

I would actually ask next why the assert in
SeedGeneratorFromProtoTracksEDProducer is needed (not obvious from the
code)?

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

[makortel]

@slava77 AFAICT neither SeedGeneratorFromProtoTracksEDProducer or TrackCleaner are used in pp tracking. The (not-anymore-run-in-offline) pixelTracks uses PixelTrackCleanerBySharedHits as a cleaner, and it doesn't merge tracks (but rejects track with smaller pT from pair that shared >= 2 hits). In seeding of iterative tracking there is no "cleaning" step, i.e. the constructed 3-hit seeds are passed "as they are" to track finding.

[VinInn]

SeedGeneratorFromProtoTracksEDProducer is used at HLT, isn't it @mtosi ?

[makortel]

Right, I missed that one. But it seems to use the tracks from PixelTrackProducer+PixelTrackCleanerBySharedHits as an input, so there should be no 4-hit tracks and the assert should not fire.

[mtosi]

yes, SeedGeneratorFromProtoTracksEDProducer modules use pixelTracks

[slava77]

I'm trying to understand if HI is using the tracking configuration
correctly and intentionally
or it just happens to be some copy-paste of features working in 53X
which have changed since then
and which shouldn't really be used anymore?
If it's the latter, we should propagate updates to the HI reco.

On 2/10/15 5:33 AM, Matti Kortelainen wrote:

Right, I missed that one. But it seems to use the tracks from
PixelTrackProducer+PixelTrackCleanerBySharedHits as an input, so there
should be no 4-hit tracks and the assert should not fire.

—
Reply to this email directly or view it on GitHub
#7627 (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)

---

[slava77]

@yetkinyilmaz @makortel @VinInn @rovere
Could you please comment on #7627 (comment)

[yetkinyilmaz]

Thanks for the attention, I think I need to check with
@istaslis @mandrenguyen

[mandrenguyen]

@slava77
Pixel track merging is known feature in the HI tracking, which is presumably there to reduce the combinatorics. We haven't tested the actual benefit of this in quite some time though. Pixel tracks can (rarely) have up to 8 hits, I believe.

[makortel]

@mandrenguyen @yetkinyilmaz Given that the merging of pixel tracks is indeed a feature in the HI tracking, is the current behaviour of keeping only first 3 (with d9602cf 4) hits in a pixel track desirable?

[mandrenguyen]

@makortel
I'm not sure. We'll need a bit more time to understand what's going on here.

@dgulhan @appeltel Can you take a look?

[slava77]

Matt,
Please clarify if there was progress understanding the impact on HI.
Thank you.

[davidlange6]

closing in 74x - there is the same PR open in 75x (now the development release)

[mandrenguyen]

Hi Slava,
This is under study, but will take another ~week to sort out.

On 2/18/15 4:02 AM, Slava Krutelyov wrote:

Matt,
Please clarify if there was progress understanding the impact on HI.
Thank you.

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

---

Matthew Nguyen
Chargé de Recherche
LLR-Ecole Polytechnique
91128 Palaiseau FRANCE

+33 1 69 33 55 65

[appeltel]

After some preliminary study of 100 jet-embedded PbPb events in 740pre6, I see no substantial difference on reconstructed or matched tracks between the current cleaning and d9602cf . You can see the attached pT distribution, and other quality distributions look nearly identical.

I am concerned that the behavior of the cleaner as @makortel describes is clearly not what we want and I would like to look further into the effect of this on pixel tracking in PbPb in general. I would suppose that for the use case of offline PbPb track reconstruction, the trajectory of the pixel track truncated to hits from the first two layers is still reasonable, so after it gets converted to a seed the CKF probably just picks back up the hit on layer 3.

For the purpose of the PR, I don't see any problem, so long as we also remove the assert statement: RecoTracker/TkSeedGenerator/plugins/SeedGeneratorFromProtoTracksEDProducer.cc:126

I am not sure why the assert statement was just a sanity check or why it was ever there.

[makortel]

Thanks @appeltel.

Are there any objections to removing the assert in SeedGeneratorFromProtoTracksEDProducer.cc? @slava77 @cvuosalo @VinInn

[VinInn]

no clue what the assert is doing there.
maybe to protect against quadruplets?

[makortel]

I can only tell that it was added in commit 3917321 (CVS times).

[VinInn]

Ok, was a safety check.
remove it, given requirements changed.

[makortel]

Removed the assert, continuing the discussion in #7798