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

Tracking Fast cleaner during pattern recognition #14299

Merged
merged 5 commits into from May 2, 2016

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Apr 29, 2016

This PR introduces a new implementation of the TrajectoryCleaner during pattern reco.
It is much faster of the previous one as it relies on the fact that all trajectories originate from the same seed.
More important it now counts degrees of freedom instead of hits to account for cases a mix of 1D,2D and 4D hits are used during pattern recognition.
It also allows not to count hits that do not contribute to the chi2 (such as seed-hits during in-out)

A small regression may be observed as the lostHitPenalty of the trajectory builder have been lowered to 20 (from 30) to make it consistent with the value used elsewhere.

For the time being it affects only reconstruction using the "GroupedCkfTrajectoryBuilder"
(so, no HLT, Muon )

In future we need to make a massive cleanup of all these "Cleaners"

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_8_1_X.

It involves the following packages:

RecoTracker/CkfPattern
TrackingTools/TrajectoryCleaning

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

cms-bot commands are list here #13028

@VinInn
Copy link
Contributor Author

VinInn commented Apr 29, 2016

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 29, 2016

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

@cmsbuild
Copy link
Contributor

-1

Tested at: 0a13ab8

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

I found follow errors while testing this PR

Failed tests: AddOn ClangBuild

  • AddOn:

I found errors in the following addon tests:

cmsDriver.py RelVal -s HLT:HIon,RAW2DIGI,L1Reco,RECO --data --scenario=HeavyIons -n 10 --conditions auto:run2_data_HIon --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --era Run2_2016,Run2_HI --magField 38T_PostLS1 --processName=HLTRECO --filein file:RelVal_Raw_HIon_DATA.root --fileout file:RelVal_Raw_HIon_DATA_HLT_RECO.root : FAILED - time: date Fri Apr 29 14:59:32 2016-date Fri Apr 29 14:51:42 2016 s - exit: 34304

  • Clang:

I found a compilation error while trying to compile with clang:
I used this command:
scram b vclean && scram build -k -j 48 USER_CXXFLAGS='-fsyntax-only' COMPILER='llvm compile'

>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-04-28-2300/src/TrackingTools/TrajectoryCleaning/src/TrajectoryCleanerBySharedSeeds.cc 
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-04-28-2300/src/TrackingTools/TrajectoryCleaning/src/TrajectoryCleanerFactory.cc 
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-04-28-2300/src/TrackingTools/TrajectoryCleaning/src/FastTrajectoryCleaner.cc 
>> Compiling  /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-04-28-2300/src/TrackingTools/TrajectoryCleaning/src/TrajectoryCleaner.cc 
In file included from /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-04-28-2300/src/TrackingTools/TrajectoryCleaning/src/FastTrajectoryCleaner.cc:1:
/build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-04-28-2300/src/TrackingTools/TrajectoryCleaning/interface/FastTrajectoryCleaner.h:41:22: error: ISO C++11 does not allow access declarations; use using declarations instead
  TrajectoryCleaner::clean;
                     ^
  using 
1 error generated.
In file included from /build/cmsbuild/jenkins-workarea/workspace/ib-any-integration/CMSSW_8_1_X_2016-04-28-2300/src/TrackingTools/TrajectoryCleaning/src/TrajectoryCleanerBySharedSeeds.cc:4:


@cmsbuild
Copy link
Contributor


~FastTrajectoryCleaner(){}

TrajectoryCleaner::clean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this line? Is it intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting... gcc is happy about that??

@VinInn
Copy link
Contributor Author

VinInn commented Apr 29, 2016

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 29, 2016

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

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

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

@VinInn
Copy link
Contributor Author

VinInn commented Apr 30, 2016

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 30, 2016

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@makortel
Copy link
Contributor

@VinInn

  1. how could be that GroupedCKf is called at HLT (by heavyIon?)

Maybe GroupedCkf is coming from RECO (cmsDriver has -s HLT:HIon,RAW2DIGI,L1Reco,RECO)?

@slava77
Copy link
Contributor

slava77 commented May 2, 2016

@VinInn @makortel
Please provide MTV plots as a reference for expected changes from this PR.
Thank you.

@VinInn
Copy link
Contributor Author

VinInn commented May 2, 2016

I have some from a month ago
http://innocent.home.cern.ch/innocent/RelVal/pu35_81_FastClean/

Matti can provide some for PhaseI

@slava77
Copy link
Contributor

slava77 commented May 2, 2016

+1

for #14299 5c25610

  • code changes are OK
  • jenkins tests pass and comparisons with baseline show small differences starting from tracking variables, propagating to downstream variables without any significant impact on physics performance.
  • higher stat tests on ttbar PU35 provided in http://innocent.home.cern.ch/innocent/RelVal/pu35_81_FastClean/ show no significant change
  • local tests with additional workflows confirm the expected negligible impact on physics performance
  • timing check on 70 events on 25202 (skipping the first event for initialization) shows a somewhat consistent reduction in CPU in track building by about 3% with a total of about 0.5% of the total RECO time reduction.
   -0.069945      -0.01%        91.33 ms/ev ->        85.16 ms/ev jetCoreRegionalStepTrackCandidates
   -0.039307      -0.03%       629.79 ms/ev ->       605.51 ms/ev initialStepTrackCandidates
   -0.036057      -0.03%       634.78 ms/ev ->       612.29 ms/ev initialStepTrackCandidatesPreSplitting
   -0.033658      -0.02%       579.64 ms/ev ->       560.46 ms/ev detachedTripletStepTrackCandidates
   -0.032069      -0.02%       460.39 ms/ev ->       445.86 ms/ev lowPtTripletStepTrackCandidates
   -0.023825      -0.02%       875.91 ms/ev ->       855.29 ms/ev pixelPairStepTrackCandidates
   -0.015689      -0.01%       411.71 ms/ev ->       405.30 ms/ev pixelLessStepTrackCandidates
   -0.011872      -0.00%       211.06 ms/ev ->       208.57 ms/ev tobTecStepTrackCandidates
   +0.010836      +0.00%       191.83 ms/ev ->       193.92 ms/ev convTrackCandidates
Total in printed:        4086.44 ==> 3972.36 d: -114.08(-0.0279167)

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2016

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

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 836ba5d into cms-sw:CMSSW_8_1_X May 2, 2016
@makortel
Copy link
Contributor

makortel commented May 2, 2016

@VinInn @slava77 For the record, here are the plots for phase1 (baseline being the tracking introduced in #14327)
https://mkortela.web.cern.ch/mkortela/tracking/validation/CMSSW_8_1_0_pre2_pr14299/

@davidlange6
Copy link
Contributor

this PR is apparently the cause of low PT changes (20% of tracks). Could you confirm that is indeed agreed (it was not mentioned in the discussion above) @VinInn

@slava77
Copy link
Contributor

slava77 commented May 19, 2016

@VinInn
Copy link
Contributor Author

VinInn commented May 19, 2016

No is not this. Is the bug fix between pre2 and pre3
It is detailed in Kevin and Matti reports

@davidlange6
Copy link
Contributor

maybe its worth correcting the PPD slides then..

On May 19, 2016, at 2:47 PM, Vincenzo Innocente notifications@github.com wrote:

No is not this. Is the bug fix between pre2 and pre3
It is detailed in Kevin and Matti reports


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

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