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

Material budget update for Phase 2 Tilted Trackers plugged on Phase 2 Pixel #16373

Merged
merged 3 commits into from Oct 29, 2016

Conversation

ghugo83
Copy link
Contributor

@ghugo83 ghugo83 commented Oct 27, 2016

This is the twin sister PR of #15963 , but for Phase 2 Tilted Tracker plugged on Phase 2 Pixel.

@rovere @ebrondol @venturia @ianna @alkemyst @boudoul @VinInn

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

Geometry/TrackerCommonData
Geometry/TrackerRecoData
Geometry/TrackerSimData

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

@ebrondol
Copy link
Contributor

@kpedro88

@kpedro88
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 27, 2016

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@rovere
Copy link
Contributor

rovere commented Oct 27, 2016

Bah,
to me part of the Tracker is missing from the recoMaterial description.
I see TrackerRecMaterialTOBPixelBarrelLayer{1-6} barrel layers (which does not quite match the 4 pixels + 6 outer barrel layers) and only TrackerRecMaterialTIDDisk{3-5} for the forward part which, again, does not seem to match the real D4 geometry.
Just to give you a feeling of how it was for the D1 geometry: link
Puzzled.
Of course, they have been splitted, damn'it!
Geometry/TrackerRecoData/data/PhaseII/TiltedTracker4021/pixelRecoMaterial.xml
Can we, for once, just once, have a coherent approach to work?

@ghugo83
Copy link
Contributor Author

ghugo83 commented Oct 28, 2016

Hi Marco,
The naming should be reviewed, this was already discussed, and sometimes paths to several discs are gathered if they refer to the same values, as you of course know. But this was already like that for D1.

As purely regards the paths to the geometry itself, the 6 paths your mention are for the 6 layers of the Outer Tracker. The paths of the Outer Tracker discs appear for the 5 discs (but can be gathered in a same group if values are identical).
Then, the reco information of the Pixel, like all the rest which regards the Pixel, is in a different file. You also obviously have paths for the 4 layers and the 11 discs.

This is why I wanted to see you, I read your PR #16116 a few days ago before trying to have a look at the material budget validation. One of the points which I did not understand is that you do not have pixelRecoMaterial.xml mentioned.
Also, at some point there are some checks with Silicone I am not sure I fully understood, but let's just see directly, this will be much easier.

@rovere
Copy link
Contributor

rovere commented Oct 28, 2016

Ciao Gabrielle,
we will chat, in fact, in few minutes and clarify all points.
The fact that there was no trace of pixelRecoMaterial is due to the fact that this file was never ever used previously: apparently someone introduced it for the tilted version 4021. All material properties have always been included into 1 file, trackerRecoMaterial.xml, for both pixel and SiStrip, and this old behaviour has to be restored, IMHO.

@ebrondol
Copy link
Contributor

Hi all, I produced the comparison for D4 with including this PR for different samples: singleMu pt=1GeV, singleMu pt=10GeV, singleMu pt=100GeV and ttbar. The efficiency and the hits distributions as well as the fakes for low pT momentum tracks are much much better (for example for the ttbar sample, eff/fake and hits).

@VinInn
Copy link
Contributor

VinInn commented Oct 28, 2016

type urgent

@ianna
Copy link
Contributor

ianna commented Oct 28, 2016

+1

@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

Hi @rovere, all - just to confirm -this PR is ok for you in the end?

@rovere
Copy link
Contributor

rovere commented Oct 29, 2016

Ciao @davidlange6 ,
yes I am. There will be other technical changes, but not for this release which I desperately want to be out of my way/life, and I bet you share the feeling....

@davidlange6
Copy link
Contributor

+1
yep..

@cmsbuild cmsbuild merged commit 4a38244 into cms-sw:CMSSW_8_1_X Oct 29, 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

8 participants