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

Fix ordering in Phase2OTEndcapRing #15608

Merged
merged 1 commit into from Aug 29, 2016

Conversation

ebrondol
Copy link
Contributor

Back-porting from SLHC release a fix in the order of the sub-modules in the endcap rings for phase2.
It has been developed on top of the CMSSW_8_1_X_2016-08-23-2300 release.
This change fixes partly the distribution of the number of hits in the tracks (comparison) and keep the eff/fakerate almost unchanged (comparison). For the comparison, 100 events of 14TeV ttbar production have been reconstructed in the tilted geometry.

No changes expected for phase0/1.

@VinInn @rovere @boudoul @delaere @kpedro88 for your information.

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

RecoTracker/TkDetLayers

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

@slava77
Copy link
Contributor

slava77 commented Aug 25, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 25, 2016

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

@ebrondol
Copy link
Contributor Author

ebrondol commented Aug 25, 2016

@slava77 @cvuosalo I have found the bug just today, so unfortunately I did not have the opportunity to present it yesterday at the meeting. If the tests are fine, it would be nice to have it in pre11 - it is important for the btagging in phase2.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

A test of workflow 10424.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2023D1 with 100 events against baseline CMSSW_8_1_X_2016-08-23-2300 shows this PR produces many differences, most of them small, but a few fairly significant. Here are some examples that may be of concern. Red is the PR; black is the baseline. @ebrondol please comment on whether these changes are expected.

striplayersvseta
striplayer2dvseta
lofrac

Some DQM plots show what might seem to be improvements:
sumpt
trkhits
pudiscrim

@cvuosalo
Copy link
Contributor

urgent

@ebrondol
Copy link
Contributor Author

Hi @cvuosalo , a part the first graph, the rest seem to me ok to me. Maybe @VinInn and @rovere want to also have a look and give their +1 about the physics performance.
About the first, I am not sure how that is filled for phase2.. what is exactly represented? @makortel do you know what is (and if there is) the respective plots in the MTV? I would be curious to see how does it look like in SLHC release..

@rovere
Copy link
Contributor

rovere commented Aug 29, 2016

Ciao @cvuosalo,
the changes are inline with expectations, at least from the hit-per-track point of view, as you can appreciate even more on this (extremely small) sample on mu-particle gun [link].

@cvuosalo
Copy link
Contributor

+1

For #15608 d19b2ce

For the Phase 2 outer tracker, add sorting of the sub-modules in the endcap rings.

The code changes are satisfactory. Jenkins tests against baseline CMSSW_8_1_X_2016-08-25-1100 show many differences, most of them small. An extended test allowed more careful assessment of differences, and a discussion of those results can be found above. The conclusion is that the differences are the intended result of this PR.

@cmsbuild
Copy link
Contributor

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, @smuzaffar

@davidlange6
Copy link
Contributor

+1

@kpedro88
Copy link
Contributor

@davidlange6 do we have to remove the "urgent" label for the bot to merge this?

@cmsbuild cmsbuild merged commit 9571b5f into cms-sw:CMSSW_8_1_X Aug 29, 2016
@kpedro88
Copy link
Contributor

@davidlange6 I guess not

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

7 participants