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

Making Phase2 T2 (and 2023D2) more realistic [using Pixel phase2 rather than a la Phase1] - Renamed as T4 and 2023D10 Resp. #16971

Merged
merged 12 commits into from Dec 18, 2016

Conversation

boudoul
Copy link
Contributor

@boudoul boudoul commented Dec 11, 2016

Greetings, this is to finally get rid of the 'pixel a la phase1' which was only used in 2023D2 - Instead we are using the phase2 pixel together with the already existing Flat Outer Tracker (that we would like to keep)- Work done by @ghugo83

@smuzaffar This PR should be tested with :
cms-data/SLHCUpgradeSimulations-Geometry#4

Tested with the Workflow 20400
In case there is an interference with #16965 (or any further PRs..) , I will update accordingly

Adding usual watchers : @delaere , @atricomi , @rovere , @VinInn @makortel @ebrondol @venturia, @ghugo83

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @boudoul (boudoul) for CMSSW_9_0_X.

It involves the following packages:

Configuration/Geometry
Geometry/CMSCommonData
Geometry/TrackerCommonData
Geometry/TrackerRecoData
Geometry/TrackerSimData

@civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @VinInn, @Martin-Grunewald, @ghugo83, @venturia 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

@kpedro88
Copy link
Contributor

@boudoul luckily, the unit test added in #16965 should now prevent any inconsistencies

@iahmad-khan
Copy link
Contributor

iahmad-khan commented Dec 12, 2016

@cmsbuild, please test with cms-sw/cmsdist#2732

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 12, 2016

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@ianna
Copy link
Contributor

ianna commented Dec 12, 2016

+1

@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. @slava77, @davidlange6, @smuzaffar

@ebrondol
Copy link
Contributor

@slava77 should I produce the comparison for D2 before/after the PR?

@slava77
Copy link
Contributor

slava77 commented Dec 12, 2016 via email

@cmsbuild
Copy link
Contributor

@davidlange6
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 15, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17054/console Started: 2016/12/15 14:45

@alja
Copy link
Contributor

alja commented Dec 15, 2016

+1

@ianna
Copy link
Contributor

ianna commented Dec 15, 2016

+1

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@rekovic
Copy link
Contributor

rekovic commented Dec 15, 2016

+1

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Dec 16, 2016

+1

for #16971 ad038ff

  • changes are in line with the description; reco code is minimally affected (only fixups in a test config file RecoLocalTracker/SiPhase2Clusterizer/test/ClustersValidationTest_flat_cfg.py )
  • jenkins tests pass and comparisons with baseline show no relevant differences
    • unrelated to this PR: run 2 wf 136.731 MechanicalView tracker plots are again on.vs.off in baseline.vs.new
  • plots with D10 performance were posted earlier by @ebrondol and show expected behavior

@davidlange6 davidlange6 merged commit 18ec97e into cms-sw:CMSSW_9_0_X Dec 18, 2016
@ebrondol
Copy link
Contributor

ebrondol commented Dec 19, 2016

@atricomi
Copy link
Contributor

atricomi commented Dec 19, 2016 via email

@ebrondol
Copy link
Contributor

ebrondol commented Dec 20, 2016

@atricomi
Ok, so probably also this doc file needs to be updated:
https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/Geometry/TrackerNumberingBuilder/doc/GeometricDetBuilder.png
(it is still without the tilted geometry and the new Pixel ... )
@venturia Do you know if there are any others?

@ebrondol
Copy link
Contributor

@atricomi @venturia You can find my modifications on the two files discussed in this branch. I think the pixel part is still not up-to-date.

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