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

Backport of #14247: Small improvements in Pixel track reconstruction #15288

Merged
merged 12 commits into from Aug 26, 2016

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Jul 26, 2016

This PR backports PR #14247 from CMSSW_8_1_X to CMSSW_8_0_X. Original description by @VinInn

relevant almost exclusively for HLT.
No regression expected
(in reco notably as little of the modified code run offline)

The first commit (7cf0c3b) is actually from #13831, but is needed here because of dependence. The commit is already included in #15240.

Tested in 8_0_14 (rebased on top of 8_0_16), no changes observed in RECO quantities. Tiny changes (level of 1e-14) are observed in HLT pixel track "point of closest approach wrt. BeamSpot" as shown in #15151 (comment).

@fwyzard @gennai @slava77 @VinInn

@makortel
Copy link
Contributor Author

tracked at #15151

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

CommonTools/Utils
RecoPixelVertexing/PixelLowPtUtilities
RecoPixelVertexing/PixelTrackFitting
RecoTracker/CkfPattern
RecoTracker/TkHitPairs
RecoTracker/TkSeedGenerator
RecoTracker/TkSeedingLayers
TrackingTools/GsfTools

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @mschrode, @gpetruc, @dgulhan this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@slava77
Copy link
Contributor

slava77 commented Jul 26, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 26, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/14234/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Aug 3, 2016

@makortel
could you please rebase to remove the commit which is already in 80X (DynArray edits in 7cf0c3b)

This PR gives little gain for HLT timing, on the other hand it looks pretty safe, since there are no changes in RECO and only tiny numerical changes in HLT. So, even the ~0.4% gain may be worth the effort.

Conflicts:
	RecoPixelVertexing/PixelTrackFitting/src/PixelTrackCleanerBySharedHits.cc
Conflicts:
	RecoPixelVertexing/PixelTrackFitting/src/PixelTrackCleanerBySharedHits.cc
Conflicts:
	RecoPixelVertexing/PixelTrackFitting/src/PixelTrackCleanerBySharedHits.cc
Conflicts:
	RecoPixelVertexing/PixelTrackFitting/src/PixelTrackCleanerBySharedHits.cc
Conflicts:
	RecoPixelVertexing/PixelTrackFitting/src/PixelTrackCleanerBySharedHits.cc
Conflicts:
	RecoPixelVertexing/PixelTrackFitting/src/PixelTrackCleanerBySharedHits.cc
@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 3, 2016

Pull request #15288 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again.

@makortel
Copy link
Contributor Author

makortel commented Aug 3, 2016

@Slava rebased on top of 8_0_16 omitting the DynArray commit.

@slava77
Copy link
Contributor

slava77 commented Aug 3, 2016

@cmsbuild please test
... despite that I expect jenkins tests to fail in 140.53

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 3, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/14350/console

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 3, 2016

-1

Tested at: a9c2ea0

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

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
140.53 step1

DAS Error

@slava77
Copy link
Contributor

slava77 commented Aug 4, 2016

+1

for #15288 a9c2ea0

  • backport matches changes made in Small improvements in Pixel track reconstruction #14247, less the 81X-specific diffs (pixel quadruplets are only in 81X). Detailed validation in 81X showed only tiny changes in HLT {x,y}PointOfClosestApproachVsZ0wrtBS_GenTk and related plots
  • jenkins tests passed for 1371e8d and comparisons with baseline show no difference; a9c2ea0 is just a rebase and the jenkins tests passed up to the point of failure due to missing HI workflow files
  • local timing tests (using Run276935_LS80-220 /online/collisions/2016/25ns15e33/v3.0/HLT/V3) confirm small, about 3-4% individual improvements in HLT in hltIter*Pixel and in other Pixel modules. The net effect from the selected modules is about 0.6 ms out of ~290 ms counting running modules or out of 360 ms including all remaining framework overhead. This is about 0.2%. Event totals show ~0.3% improvement, but this estimate has a much larger uncertainty from job time reproducibility.
   -0.058033      -0.02%         0.83 ms/ev ->         0.78 ms/ev hltPixelTracksReg
   -0.044447      -0.01%         0.86 ms/ev ->         0.82 ms/ev hltPixelTracksForNoPU
   -0.043411      -0.01%         0.85 ms/ev ->         0.82 ms/ev hltFastPVPixelTracks
   -0.042202      -0.01%         0.69 ms/ev ->         0.66 ms/ev hltIter2PFlowPixelSeedsForBTag
   -0.041873      -0.06%         4.59 ms/ev ->         4.41 ms/ev hltIter2PFlowPixelSeeds
   -0.034901      -0.14%        12.22 ms/ev ->        11.80 ms/ev hltIter1PFlowPixelSeeds

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 4, 2016

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (but tests are reportedly failing). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar

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