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

Customise function to increase cluster number check and seeding limits (80X) #16214

Merged

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Oct 14, 2016

This PR adds a customise function to increase cluster number check limits by

  • 2x for the "box"
  • adjust coefficient for "pixel < N + k*strip" from k=1/10 to k=1/7

and maxElement for PixelTripletLargeTipGenerator and MultiHitGeneratorFromChi2 by 5x. These should be sufficient to silence "TooManyClusters" and "TooManyTriplets" errors for the high pileup data.

Tested in 8_0_20 with

cmsDriver.py step3 --conditions 80X_dataRun2_Prompt_v14 -s RAW2DIGI,L1Reco,RECO \
  --runUnscheduled --process reRECO --data --era Run2_2016 --eventcontent RECO \
  --scenario pp --datatier RECO \
  --customise Configuration/DataProcessing/RecoTLR.customisePostEra_Run2_2016 \
  -n 5 --nThreads=5--fileout file:step3.root --filein <your-preferred-file> \
  --customise RecoTracker/Configuration/customiseClusterCheckForHighPileup.customiseClusterCheckForHighPileup

with a random file from ZeroBiasIsolatedBunch0 RAW.

@rovere @VinInn @mtosi

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

RecoTracker/Configuration

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @GiacomoSguazzoni, @rovere, @VinInn, @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 listed here #13028

@VinInn
Copy link
Contributor

VinInn commented Oct 14, 2016

@cmsbuild ,
please test

@makortel
Copy link
Contributor Author

@VinInn You have a line break in your message, maybe they need to be on a single line? (since the bot hasn't acted yet)

@VinInn
Copy link
Contributor

VinInn commented Oct 14, 2016

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 14, 2016

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

@slava77
Copy link
Contributor

slava77 commented Oct 14, 2016

@makortel we should have a PR in 81X
Please submit one.
Thanks.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@makortel makortel changed the title Customise function to increase cluster number check and seeding limits Customise function to increase cluster number check and seeding limits (80X) Oct 14, 2016
@makortel
Copy link
Contributor Author

@slava77 There you go #16219

@makortel
Copy link
Contributor Author

backport of #16219

@slava77
Copy link
Contributor

slava77 commented Oct 16, 2016

@makortel @VinInn
Are there good justifications for the increased values?
It looks more like selective increase of all fixed values by a factor of 10.

As discussed in https://hypernews.cern.ch/HyperNews/CMS/get/recoTracking/1653/1/1/1/1.html
it looks like we are primarily dealing with Npixel.vs.Nstrip relationship and (maybe also as a result of VFP change) this cut needs an update.
If that's accepted as a more appropriate change, I think the updated relationship for "pixel < a + b*strip" cut should become a default in tracking selections for 2017 and later.
For 80X we will then have this enabled via configuration just for this high PU run.

Changes wrt. default
- 2x for the box
- lower diagonal coefficient from 1/10 to 1/7
@makortel
Copy link
Contributor Author

@slava77 With maxElement=500k only detachedTripletStep was giving TooManyTriplets errors, setting maxElement=1M there are no TooMany* errors (with 100 events for both).

@slava77
Copy link
Contributor

slava77 commented Oct 19, 2016

@makortel
thank you
which file(s) where you using for the highest PU test? (IsolatedBunch PD or something else)
I wanted to see on ~fewK events where is the memory peak (possibly try 4, 8, 16 thread jobs).

@makortel
Copy link
Contributor Author

I took a random file from ZeroBiasIsolatedBunch0. Are you going to run further tests or you'd like me to run?

@slava77
Copy link
Contributor

slava77 commented Oct 19, 2016

@makortel
I may not get to the memory tests until tomorrow.
So, if you have time today, please check.
Thank you.

@makortel
Copy link
Contributor Author

@slava77 Ok, running (but few kevents will take a while).

@makortel
Copy link
Contributor Author

@slava77 I was able to run ~1300 events with 8 threads until the machine I was running on got booted. Maximum RSS (using SimpleMemoryCheck service) was 9.7 GB. The average time per event per thread was 4-5 min (on a 2.27 GHz Westmere, likely interfered with other activity).

In addition, within these ~1300 events, there were no TooMany* errors.

@slava77
Copy link
Contributor

slava77 commented Oct 20, 2016

@makortel thank you.
This should be useful for the Friday ops meeting discussion.

@slava77
Copy link
Contributor

slava77 commented Oct 20, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 20, 2016

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@venturia
Copy link
Contributor

We should make these limits configurable with a condition object and a database tag, eventually

@slava77
Copy link
Contributor

slava77 commented Oct 21, 2016

+1

for #16214 4987532

@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Oct 21, 2016

I guess some of these were seen already, but still

(red is 8_0_20_patch1 out of the box) LS60 in HLTPhysicsIsolatedBunch
wfhltphysicsisolatedbunch283171_glbeffs_pt1

wfhltphysicsisolatedbunch283171_goodpv

wfhltphysicsisolatedbunch283171_lambda

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 87a741f into cms-sw:CMSSW_8_0_X Oct 27, 2016
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