Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate HitPattern to use TrackerTopology #9959

Merged
merged 5 commits into from Jul 2, 2015

Conversation

makortel
Copy link
Contributor

This PR migrates HitPattern to use TrackerTopology instead of the tracker DetId classes. There were two kind of use-cases for filling HitPattern from DetIds:

  1. DetId originates from a hit
  2. DetId is constructed on the fly from another packed HitPattern-like format
    1. Old HitPattern (pre-TrackBase & HitPattern redesign discussed on the RECO meeting of 08/05/2014 #4455)
    2. PackedCandidate

Case 1 was straightforward to solve by percolating TrackerTopology to all necessary places to be passed to HitPattern.

Case 2 was more tricky, as it was not possible/practical to percolate TrackerTopology. Instead, I decided to skip the creation of a "dummy" DetId altogether, and added a new function to deliver the required information (subdet, layer, stereo bit) to HitPattern (appendTrackerHit() for tracker, corresponding appendMuonHit() still employs DetId to encode the information as that seemed to be the simplest solution for now).

Tested for 1 and 2.ii in CMSSW_7_5_X_2015-06-14-2300 + #9630 with wf 11325 (#9630/#9801 needed to verify 2.ii, i.e. CMSSW_7_6_X_2015-06-29-1100 or later). Case 2.i was tested separately with a file from /RelValTTbar_13/CMSSW_7_1_0-POSTLS171_V15-v1/GEN-SIM-RECO (predating #4455 so that the iorule is activated) by printing out all properties of HitPattern first in the IB, then in the IB+this branch, and finally making a diff of the two printouts (showing no differences).

@rovere @VinInn

I'm not perfectly happy on the solution (exposing some of the
internals in HitPattern/TrackBase interface), but I think this is the
simplest way to avoid DetIds in PackedCandidate.

In principle I could have used TrackerTopology to encode the DetId,
but percolating TrackerTopology here would lead to bad interface from
the client perspective. Anyway the hit pattern information is already
(packed) in the PackedCandidate, so it is only a matter of
transforming the existing information to the form of HitPattern.
Here it is not possible to have TrackerTopology object, so some
mechanism to deliver only the layer information to HitPattern in a way
or another is needed. Fortunately the mechanism used in
PackedCandidates works in here too, we just need a separate hit
appending function for muons (where the information can still be
delivered via DetIds).
@makortel
Copy link
Contributor Author

FYI @boudoul @venturia too

@cmsbuild
Copy link
Contributor

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

Migrate HitPattern to use TrackerTopology

It involves the following packages:

DataFormats/PatCandidates
DataFormats/TrackReco
FastSimulation/Tracking
RecoEgamma/EgammaPhotonProducers
RecoMuon/CosmicMuonProducer
RecoMuon/GlobalMuonProducer
RecoMuon/L2MuonProducer
RecoMuon/L3MuonProducer
RecoMuon/StandAloneMuonProducer
RecoMuon/TrackingTools
RecoPixelVertexing/PixelTrackFitting
RecoTracker/TrackProducer
SimMuon/MCTruth
Validation/RecoTrack

@civanch, @lveldere, @cvuosalo, @ssekmen, @mdhildreth, @monttj, @cmsbuild, @deguio, @slava77, @vadler, @danduggan can you please review it and eventually sign? Thanks.
@ghellwig, @wmtford, @Martin-Grunewald, @abbiendi, @venturia, @battibass, @jhgoh, @lgray, @cerati, @trocino, @rociovilar, @Sam-Harper, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @matt-komm, @mschrode, @dgulhan, @appeltel, @gpetruc, @istaslis, @bachtis 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.

@makortel
Copy link
Contributor Author

This PR "conflicts" with #9917 such that whichever gets merged first, the other one needs to migrate TrackFromSeedProducer (added in #9917) to use the new version of TrackBase::appendHits() introduced here.

@slava77
Copy link
Contributor

slava77 commented Jun 30, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@deguio
Copy link
Contributor

deguio commented Jul 2, 2015

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants