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

New Calibration for D11 Geometry #18658

Merged
merged 1 commit into from
May 18, 2017

Conversation

atricomi
Copy link
Contributor

New cluster shape calibrations have been produced to take into account of the axis orientation in the T5 Tracker Geometry (barrel pixel geometry)

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @atricomi (Alessia Tricomi) for CMSSW_9_1_X.

It involves the following packages:

RecoPixelVertexing/PixelLowPtUtilities

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

cms-bot commands are listed here

@VinInn
Copy link
Contributor

VinInn commented May 10, 2017

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 10, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19716/console Started: 2017/05/10 14:53

@slava77
Copy link
Contributor

slava77 commented May 10, 2017

assign upgrade

@cmsbuild
Copy link
Contributor

New categories assigned: upgrade

@kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks

@slava77
Copy link
Contributor

slava77 commented May 10, 2017

@atricomi
does this new shape calibration work OK with T3?

I think that we need both efficiency/fake plots and timing estimates on PU200 before this can be signed off.

@boudoul
Copy link
Contributor

boudoul commented May 10, 2017

hello @slava77 - T3 is going to be deprecated - We just discuss this at the UPSG meeting - Therefore we should focus our attention to T5 now (cleaning of the T3 to make it deprecated will come in a separated PR) - Thanks

@slava77
Copy link
Contributor

slava77 commented May 10, 2017 via email

@atricomi
Copy link
Contributor Author

atricomi commented May 10, 2017 via email

@VinInn
Copy link
Contributor

VinInn commented May 10, 2017

I hope that T5 RelVal are NOT signed off for pre3

@slava77
Copy link
Contributor

slava77 commented May 10, 2017

@atricomi
now I also notice that this is a 91X PR.
Do you and UPSG want to delay 91X release and have this PR included in the TDR release?

@slava77
Copy link
Contributor

slava77 commented May 10, 2017

in any case, please create a PR for 92X, since integration starts there.

@VinInn
Copy link
Contributor

VinInn commented May 10, 2017

Please submit it to master as well!

@boudoul
Copy link
Contributor

boudoul commented May 10, 2017

ok good point for 91X, I don't know but will inquire/ discuss with UPSG
In any case, please @atricomi , create a PR for master (I also missed that this one was for 91X)

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@ebrondol
Copy link
Contributor

ebrondol commented May 10, 2017

We put the full set of plots here and we will update the page when the pu sample are ready.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18658/19716/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2942 differences found in the comparisons
  • DQMHistoTests: Total files compared: 24
  • DQMHistoTests: Total histograms compared: 1831084
  • DQMHistoTests: Total failures: 26409
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1804495
  • DQMHistoTests: Total skipped: 180
  • DQMHistoTests: Total Missing objects: 0
  • Checked 98 log files, 14 edm output root files, 24 DQM output files

@kpedro88
Copy link
Contributor

@boudoul will deprecation of T3 imply deprecation of T4 also? Will there be a need for T6 = T4 w/ extra disk?

@boudoul
Copy link
Contributor

boudoul commented May 12, 2017

hi @kpedro88 , this is a good question - the main usage of T4 is related to the flat tracker for track trigger purpose (and therefore the config of the pixel is not relevant, since the track trigger occurs only in the outer part ) - so for sure no need for T6 as you are suggesting. Now the question of the removal of T4 should be adressed- I know that there will be discussion during the tracker week (next week) where presumably we will get answer to this question

@VinInn
Copy link
Contributor

VinInn commented May 13, 2017

Has this been submitted to master (sorry cannot find it)?
what the plan to merge in master?: clearly any development base on D11 requires this PR...

@atricomi
Copy link
Contributor Author

atricomi commented May 13, 2017 via email

@slava77
Copy link
Contributor

slava77 commented May 13, 2017

I was waiting for tests with PU.

This is in response to #18658 (comment)

@atricomi
Copy link
Contributor Author

atricomi commented May 13, 2017 via email

@slava77
Copy link
Contributor

slava77 commented May 15, 2017

@kpedro88 is this development still needed for 91X?
Please clarify.
Thank you.
I guess we'll know better on Tuesday in the release planning.

@kpedro88
Copy link
Contributor

I am also wondering that. I asked the UPS coordinators for their input.

@slava77
Copy link
Contributor

slava77 commented May 15, 2017

@kpedro88
one related question: was it intentional still to not have T5 in the jenkins PR tests?
Currently running D4, D8, D12, and D15 are all T3

@kpedro88
Copy link
Contributor

@slava77 at the time I updated the matrix tests, the T3 vs T5 decision hadn't been made yet. Now apparently it has (in favor of T5), so the set of tests could be updated again. But maybe this can wait until T3 is officially deprecated.

@ebrondol
Copy link
Contributor

ebrondol commented May 16, 2017

I have produced the comparison for 10 events with ttbar+PU200: link
The efficiency increase. Also does the fake, but in the HP collection: link the effect is almost negligible.
It can be double-checked later, the overall effect of this PR is very good and needed for any production with 910 release.

@slava77
Copy link
Contributor

slava77 commented May 17, 2017

The following is based on 910pre3:

  • Looking at muon gun events: increase in efficiency for T3 (D12 in plots below) appears to be more significant than for T5 (D14). In both cases the final eff is pretty close to 100%

T3/D12
wf24411d12tenmu_generalpt09_eff_eta

T5/D14
wf26211d14tenmu_generalpt09_eff_eta

For T5/D14 with PU200:

wf26434d14pu200_generalalltp_eff_eta

Fake rate is up somewhat moderately (well, it nearly doubles in eta ~0.8 region)
wf26434d14pu200_generalpt09_fake_eta

high purity is much more stable
wf26434d14pu200_hp_fake_eta

At higher level:

  • electronGsfTracks yield is down by 14% , based on MTV plots it is all from fakes (efficiency plot is almost unchanged)

wf26434d14pu200_gsf_fake_eta

There is somewhat noticeable improvement in b-tagging performance (MVA v2 change is less impressive; so a part of this may be a fluctuation)
wf26434d14pu200_csvv2_lvb_roc

CPU time in PU200 is up overall by about 3%;

  • most of it is in iterative tracking which is up by about 10% (building steps are slower by 40% for pixelPair and by 15-20% for triplet steps and low-pt and detached quadruplet steps). This looks OK given the efficiency increase.
  • electron tracking module times are down in total by about 10% (with no apparent loss in efficiency. Good.).

@slava77
Copy link
Contributor

slava77 commented May 17, 2017

+1

for #18658 7dafeac

  • update in the pixel cluster shape files for phase-2
  • jenkins tests pass and comparisons show differences only in phase-2 workflows
  • local tested confirm expected behavior without bad side-effects for high-level reco, including improvements even for the T3 tracker which was not the main target of this update.
    • CPU time is up by 3% in PU200

@kpedro88
Copy link
Contributor

+1
thanks to @slava77 for the careful review
todo: address deprecation of T3

@cmsbuild
Copy link
Contributor

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

@boudoul
Copy link
Contributor

boudoul commented May 17, 2017

thanks @kpedro88 and @slava77 for the review, the deprecation of T3 is going to happen within a few days (for master ) , I delayed my homework a bit due to the ongoing tracker week

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 9c1ca9e into cms-sw:CMSSW_9_1_X May 18, 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.

8 participants