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

Fix MultiTrackValidator sim dxy/dz wrt. PV histograms #17788

Merged
merged 4 commits into from Mar 7, 2017

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Mar 6, 2017

There was a bug in how the dxy and dz wrt. PV were calculated for TrackingParticles in MultiTrackValidator. It did not take into account the fact that the position of point of closest approach to beamline (PCA) returned by ParemtersDefinerTP shifted by the beamspot position, leading to biases in dxy and dz wrt. PV (most noticeably ~1 cm shift in dz). This PR suggests to fix this problem by making the calculation of dxy and dz more uniform between Track and TrackingParticle:

  • ParametersDefinerTP leaves the PCA position to be wrt. (0,0,0) (as is Track reference point)
  • for dxy/dz wrt. a different point than (0,0,0), the point coordinates need to be given explicitly (as for Track)
    • The copy-pasted formulas are now abstracted to templated functions (there is some copy-paste between those and reco::TrackBase, dealing with that is left to a later exercise)

Tested in 9_0_0_pre4, expecting changes in MTV sim dxy and dz wrt. PV histograms (should be better centered around 0 now).

@rovere @VinInn

…rackingParticle

Earlier the dxy/dz were implicitly calculated wrt. BeamSpot as the
TrackingParticle PCA vertex was shifted by the BS position in
(Cosmic)ParametersDefinerForTP. When I added dxy/dz vs. PV, I didn't
realize this, and thefore the current MTV dxy/dz are actuall wrt
(BS-PV). Here the reference position is made explicit also for
TrackingParticles, as is already the case for Tracks. Helper functions
are added to reduce copy-paste.
Does not affect results (noticeably, at least). It would be easy to
remove sin/cos from dxy as well, but those will change results a bit,
so I leave it for later.
@makortel
Copy link
Contributor Author

makortel commented Mar 6, 2017

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18155/console Started: 2017/03/06 11:46

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2017

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

It involves the following packages:

RecoMuon/MuonIdentification
SimTracker/TrackAssociation
Validation/RecoMuon
Validation/RecoTrack

@perrotta, @civanch, @mdhildreth, @dmitrijus, @cmsbuild, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@trocino, @battibass, @felicepantaleo, @abbiendi, @GiacomoSguazzoni, @jhgoh, @VinInn, @echapon, @calderona, @HuguesBrun, @rovere, @threus, @wmtford, @sdevissc, @bellan, @mtosi, @dgulhan, @bachtis, @rociovilar this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2017

@dmitrijus
Copy link
Contributor

+1

@perrotta
Copy link
Contributor

perrotta commented Mar 7, 2017

+1
Changes in the reco code only consist in the removal of some unused code in test areas.
Validation histograms confirm that the only (expected) change is in Tracking histos, where the MTV TrackingParticleRecoAssociation is used: now the sim dxy and dz wrt. PV are both more nicely peaked at 0.

@davidlange6 davidlange6 merged commit 74a5a60 into cms-sw:master Mar 7, 2017
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

5 participants