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

Second tuning of Pixel cluster shape cut for Phase 2 #17110

Merged
merged 1 commit into from Jan 14, 2017

Conversation

ebrondol
Copy link
Contributor

@ebrondol ebrondol commented Jan 4, 2017

After the PR #16943 where the parameters of SiPixel local reco have been changed to match the specs of the phase2 read-out chip, a second tuning of the pixel cluster shape cut for Phase 2 has been done.
Few changes in performance - but nothing drastic - are expected.

informing @delaere @atricomi @boudoul

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 4, 2017

A new Pull Request was created by @ebrondol for CMSSW_9_0_X.

It involves the following packages:

RecoPixelVertexing/PixelLowPtUtilities

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

cms-bot commands are listed here #13028

@ebrondol
Copy link
Contributor Author

ebrondol commented Jan 4, 2017

@slava77 since this PR is going to change some performance, while the PR I submit yesterday (#17106) is just updating the documentation, I would prefer to keep them separate. Let me know if you have any problem with that..

@slava77
Copy link
Contributor

slava77 commented Jan 4, 2017

@cmsbuild please test

@ebrondol
no problem with having two separate PRs, they are covering different features

Have you run some tests with PU200 to see that timing stays under control?

Do you have any visual representation of the changes in the cluster shape at the lower level (e.g. cut value vs cluster size or something natural wrt definition of the cut)?

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 4, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17176/console Started: 2017/01/04 15:26

@ebrondol
Copy link
Contributor Author

ebrondol commented Jan 4, 2017

Hi @slava77 , for the first question, I did not run at high PU but I think it should be pretty stable the timing performance respect to before.
I do not have any visual representation at the cluster level. Maybe @emiglior can comment on that?

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 4, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 4, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 4, 2017

@slava77
Copy link
Contributor

slava77 commented Jan 6, 2017

@ebrondol
please clarify from which workflow are your plots in the PR description

@slava77
Copy link
Contributor

slava77 commented Jan 6, 2017

I observe virtually no differences after running in CMSSW_9_0_0_pre2

  • 500 events 10 mu E 0 to 200 without PU (20011.0,22811.0,23611.0 or D7, D4Timing and D9)
  • 100 events ttbar with PU200 D4Timing (wf 23034)

The efficiency plot from 22811 "/DQMData/Run 1/Tracking/Run summary/Track/generalPt09_trackingParticleRecoAsssociation/effic" with this PR shows a plot similar to what was posted in the PR description in red
general_eff_vs_eta_min075

Is something missing in this PR (is the .par file change all that's needed) ?

@ebrondol
Copy link
Contributor Author

ebrondol commented Jan 9, 2017

Hi @slava77 , yes, the file .par is the only change needed in principle. I have used 1k events coming from RelValSingleMuPt10Extended with 0PU (wf 21218).

@slava77
Copy link
Contributor

slava77 commented Jan 9, 2017 via email

@ebrondol
Copy link
Contributor Author

Hi @slava77 , I am also double checking in CMSSW_9_0_X_2017-01-08-2300 with the WF #21218.0. I will post the results before the end of the day.

@ebrondol
Copy link
Contributor Author

With the same statistic, I see what you also reported: http://ebrondol.web.cern.ch/ebrondol/Phase2Tracking/PR17110/SingleMuPt10Extended/plots_ootb/effandfake1.pdf
no difference between with or without the PR. This is very strange..

@slava77
Copy link
Contributor

slava77 commented Jan 12, 2017

@ebrondol
did you happen to gain some better understanding of the issue in this PR of the lack of changes in more recent tests compared to the tests done before the PR submission?

Since you were able to reproduce the lack of changes, maybe it is still possible to go back to the setup that had desired changes and compare/debug.

@ebrondol
Copy link
Contributor Author

@slava77 After some test, I think that the lost of efficiency is due to another problem and cannot be solved with this re-tuning. Said so, probably would be good to put this PR in release to keep consistency between the geometry/digitization and the tuning (with no changes on the performance).

@slava77
Copy link
Contributor

slava77 commented Jan 13, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Jan 13, 2017

+1

for #17110 5c1751c

  • pixelShape_Phase2Tk.par update for consistency with the new tracker response parameters
  • jenkins tests pass and comparisons show only small changes
  • local tests on larger samples confirm that changes are small as noted earlier in the thread

@cmsbuild
Copy link
Contributor

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

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 3d3b67e into cms-sw:CMSSW_9_0_X Jan 14, 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

4 participants