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

updated pixel cabling (2017 & 2018) and EGM regressions #17479

Conversation

franzoni
Copy link

In this PR we'll take care of

The first commit only takes are of pixel cabling. I'll put in the regressions after the first round of tests (full documentation will follow), updating this PR with a new commit

@franzoni
Copy link
Author

please test

@cmsbuild cmsbuild added this to the Next CMSSW_9_0_X milestone Feb 10, 2017
@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 10, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17726/console Started: 2017/02/10 07:03

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @franzoni (Giovanni Franzoni) for CMSSW_9_0_X.

It involves the following packages:

Configuration/AlCa

@ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @Martin-Grunewald, @ghellwig, @tocheng 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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@franzoni
Copy link
Author

franzoni commented Feb 10, 2017

hello @veszpv @tvami

can you please educate me on how the is used in the MC digi-reco sequence ?
I was expecting that the same payload would be used in packing the RAW as well as in unpacking them, which gave me the expectation identity would be observed between:
SiPixelFedCablingMap_phase1_v6
and
SiPixelFedCablingMap_phase1_v4
instead there are changes localize in a certain region of the pixel detector for the 2017 workflow:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_9_0_X_2017-02-09-2300+17479/18269/10021.0_TenMuE_0_200+TenMuE_0_200_pythia8_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/
=>
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_9_0_X_2017-02-09-2300+17479/18269/10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/PixelPhase1_FED_Readout.html

can you please comment on whether these changes are expected and desired, or not ?
The fact that tracking is not affected at all (in both the 2017 relvals).

@slava77 you may want to keep an eye on this too

@franzoni
Copy link
Author

given this explanations:
https://hypernews.cern.ch/HyperNews/CMS/get/calibrations/2815/1/1.html
and the fact that the only plots that changes are "occupancy_per_Channel_FED_XXX" type plots,

could it be that the same physical hits in the same geometric volumes in space
are simply assigned to a permutation of FED numbers ?

That would explain the fact that tracking is in no way affected.

@franzoni
Copy link
Author

Summary of changes in Global Tags

RunI simulation

RunII simulation

RunI data

RunII data

Upgrade

@cmsbuild
Copy link
Contributor

Pull request #17479 was updated. @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @mmusich, @davidlange6 can you please check and sign again.

@franzoni
Copy link
Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 12, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17751/console Started: 2017/02/12 02:09

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 12, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17753/console Started: 2017/02/12 09:53

@smuzaffar
Copy link
Contributor

@franzoni , test should run now. Last time they failed because the weekly IBs area was cleaned up and scram DB was missing. I will fix this in next days so that weekly cleanup does not break PR tests

@franzoni
Copy link
Author

thanks @smuzaffar !

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@franzoni
Copy link
Author

as expected the introduction of the egamma regressions
(not consumed right now in any CMSSW standard sequence)
has not changed the picture introduced by the picture already described here #17479 (comment) as a result of the pixel cabling map.

@veszpv @tvami,
if you can please give input on the FED numbering differences at earliest convenience,
we can integrate this PR and move on to add in other AlCa changes needed.
Thanks!

@veszpv
Copy link
Contributor

veszpv commented Feb 13, 2017

@franzoni Your explanation is correct: the new cabling map associates a different FED+channel number for the same DetId. Since this mapping is done consistently in both digitization (digi2raw) and reconstruction (raw2digi), it is transparent in terms of high level reco. However, the content of RAW, the intermediate step, will be different. So the differences in validation are expected, as those plots are meant to display information from the RAW content.

@franzoni
Copy link
Author

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

@franzoni
Copy link
Author

@davidlange6
Having this pr merged before tomorrow's orp, if possible,
would help the integration of more conditions in the pipeline

Thanks, @veszpv

@slava77
Copy link
Contributor

slava77 commented Feb 14, 2017

@davidlange6
we need this PR to integrate #17506
Please clarify if this can be merged today
Thank you.

@davidlange6
Copy link
Contributor

+1
yes, i missed it

@cmsbuild cmsbuild merged commit b6fd472 into cms-sw:CMSSW_9_0_X Feb 14, 2017
@franzoni
Copy link
Author

np!
I'll proceed with integrating another amount by tomorrow afternoon.

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

6 participants