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

Remove 50ns-labeled dynamic inefficiency payload usage from SiPixelDigitizer #34502

Merged
merged 3 commits into from
Jul 22, 2021

Conversation

carolinecollard
Copy link
Contributor

@carolinecollard carolinecollard commented Jul 15, 2021

PR description:

First part of the PR :

Removal of the code lines related to the 50ns labeled dynamic inefficiency for Pixels, in view of removing the payload
SiPixelDynamicInefficiencyRcd | 50ns | SiPixelDynamicInefficiency_13TeV_50ns_v2_mc
from the Global Tag, as discussed during the Pixel Offline Meeting of July 14 (https://indico.cern.ch/event/1000813/#11-removal-of-50ns-labeled-dyn).

I have made some checks in the SiPixelDigitizerAlgorithm::PixelEfficiencies::init_from_db function,
and verified that the code before and after my modifications delivers exactly the same SiPixelDynamicInefficiency information.

Second part of the PR (Global Tag changes):

In case when the simulations require 50 ns bunch spacing the previously labeled (50 ns specific) payload is used. Code changes and validation can be found in the PR34502 [2]. The differences of the new and old GTs can be found in [3].

This PR contains the removal of the unused key run1_mc_pa as well.

[1] https://hypernews.cern.ch/HyperNews/CMS/get/calibrations/4431.html
[2] #34502
[3] https://docs.google.com/spreadsheets/d/1FlABJmzWOrxKaNSUD_N980GQeO9ecYj4C8xXcl4IMNE/edit?usp=sharing

PR validation:
runTheMatrix.py -l limited,140.0,200.0,202.0,203.0,204.0,205.0,4001.0,4006.0,4007.0,4008.0,11634.0,12434.0,12834.0,1325.516,50200.0,7.22,145.0,281.0,10424.0,7.21,11224.0,250200.182,11024.2,7.4,12034.0,7.23,159.0,12834.0,7.24 --ibeos -j8

Private validation under
https://indico.cern.ch/event/1000813/contributions/4447059/attachments/2281939/3880637/20210714_PixelMeeting_DynEffCheck.pdf

plus wf25.0 and wf9.0 were tested
https://tvami.web.cern.ch/tvami/projects/PixelOffline/DynEffValidation/wf25/?match=ffic

https://tvami.web.cern.ch/tvami/projects/PixelOffline/DynEffValidation/wf9/?match=ffic

and show no significant changes.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34502/23982

  • This PR adds an extra 48KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @carolinecollard for master.

It involves the following packages:

  • SimTracker/SiPixelDigitizer (simulation)

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@makortel, @mtosi, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @OzAmram, @dkotlins, @ferencek, @mmusich, @threus, @dgulhan, @tvami this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy, @perrotta you are the release manager for this.

cms-bot commands are listed here

@tvami
Copy link
Contributor

tvami commented Jul 15, 2021

@cmsbuild , please test

@mmusich
Copy link
Contributor

mmusich commented Jul 15, 2021

@carolinecollard can the title be a little bit more explicative?
E.g.: "Remove 50ns-labeled dynamic ineffiiceny payload usage from SiPixelDigitizer" ?

@carolinecollard carolinecollard changed the title Remove50ns dyn ineff Remove 50ns-labeled dynamic ineffiiceny payload usage from SiPixelDigitizer Jul 15, 2021
@carolinecollard carolinecollard changed the title Remove 50ns-labeled dynamic ineffiiceny payload usage from SiPixelDigitizer Remove 50ns-labeled dynamic inefficiency payload usage from SiPixelDigitizer Jul 15, 2021
@makortel
Copy link
Contributor

This PR also resolves #33616.

Does this mean that the empty-labeled dynamic inefficiency payload in the GT corresponding the run2_mc_50ns autoCond key will be replaced by the payload that has the "50ns" label (assuming they are different)?

@tvami
Copy link
Contributor

tvami commented Jul 15, 2021

This PR also resolves #33616.

Does this mean that the empty-labeled dynamic inefficiency payload in the GT corresponding the run2_mc_50ns autoCond key will be replaced by the payload that has the "50ns" label (assuming they are different)?

Yes, they are different and yes we should indeed do that.

@mmusich
Copy link
Contributor

mmusich commented Jul 15, 2021

assign trk-dpg

@cmsbuild
Copy link
Contributor

New categories assigned: trk-dpg

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

@mmusich
Copy link
Contributor

mmusich commented Jul 15, 2021

hold

the change in the GT requested at #34502 (comment) should come first in order not to change the simulation behavior.
Also what about run1? IIRC it's 450ns but I am not sure if that's the case for all workflows

@cmsbuild
Copy link
Contributor

Pull request has been put on hold by @mmusich
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@cmsbuild cmsbuild added the hold label Jul 15, 2021
@makortel
Copy link
Contributor

Also what about run1? IIRC it's 450ns but I am not sure if that's the case for all workflows

I came across with this comment in matrix workflow definitions

# 50 ns at 8 TeV
workflows[200]=['',['ZEE','DIGIPU1','RECOPU1','HARVEST']]
#workflows[201]=['',['ZmumuJets_Pt_20_300','DIGIPU2','RECOPU2','HARVEST']]
workflows[202]=['',['TTbar','DIGIPU1','RECOPU1','HARVEST']]
workflows[203]=['',['H130GGgluonfusion','DIGIPU1','RECOPU1','HARVEST']]
workflows[204]=['',['QQH1352T','DIGIPU1','RECOPU1','HARVEST']]
workflows[205]=['',['ZTT','DIGIPU1','RECOPU1','HARVEST']]

@makortel
Copy link
Contributor

Also what about run1? IIRC it's 450ns but I am not sure if that's the case for all workflows

I came across with this comment in matrix workflow definitions

# 50 ns at 8 TeV
workflows[200]=['',['ZEE','DIGIPU1','RECOPU1','HARVEST']]

I looked into workflow 200, and indeed the process.mix.bunchspace is set to 50. Do we have any MC workflows with pileup for 7 TeV? (did that have the 450 ns bunch spacing? I have no recollection anymore)

@mmusich
Copy link
Contributor

mmusich commented Jul 15, 2021

I checked 25.0 and gave me 450ns

@makortel
Copy link
Contributor

I checked 25.0 and gave me 450ns

So presumably there is potential danger towards supporting 7 TeV MC with 450 ns bunchspacing and 8 TeV with 50 ns. Maybe this approach originates from those times, trying to support both with one GT?

@tvami
Copy link
Contributor

tvami commented Jul 15, 2021

Just to add that we didnt simulate the inefficiencies in the 7 TeV MC with 450 ns bunchspacing. That tag SiPixelDynamicInefficiency_8TeV_v1_mc is a trivial payload, all factors equal to 0.999.

@cmsbuild
Copy link
Contributor

Pull request #34502 was updated. @malbouis, @civanch, @yuanchao, @mdhildreth, @cmsbuild, @tsusa, @tlampen, @mmusich, @francescobrivio, @pohsun, @tvami can you please check and sign again.

@carolinecollard
Copy link
Contributor Author

@mmusich Hi Marco, I did the rebase with the help of Tamas and following the web page with instructions you pointed to me --> Now I am in CMSSW_12_0_X_2021-07-20-2300. During the rebase step, I still got conflicts for 3 keys : phase1_2021_realistic, phase1_2023_realistic, phase1_2024_realistic. Tamas was using the V4 while the original code uses the V3 for autoCond.py. Tamas recommended that I uses the V4. This what I did to resolve the conflicts. So I hope this will not introduce new conflicts here...

@tvami
Copy link
Contributor

tvami commented Jul 21, 2021

test parameters:

  • workflow = 140.0,200.0,202.0,203.0,204.0,205.0,4001.0,4006.0,4007.0,4008.0,11634.0,12434.0,12834.0,1325.516,50200.0,7.22,145.0,281.0,10424.0,7.21,11224.0,250200.182,11024.2,7.4,12034.0,7.23,159.0,12834.0,7.24

@tvami
Copy link
Contributor

tvami commented Jul 21, 2021

@cmsbuild , please test

@@ -2,29 +2,27 @@

### NEW KEYS ###
# GlobalTag for MC production with perfectly aligned and calibrated detector for Run1
'run1_design' : '113X_mcRun1_design_v3',
'run1_design' : '120X_mcRun1_design_v1',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes in autoCOnd GTs must be blessed by @cms-sw/alca-l2
I suspect that going back to older versions is outside the scope of this PR, and should be reverted

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AlCa is giving the blessing :) 120X_mcRun1_design_v1 is a newer version than 113X_mcRun1_design_v3, please note the CMSSW version numbering in the beginning of the string. As explained in the PR description, the differences between the GTs can be found in the attached google sheet. Specifically this one is row 5, https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/113X_mcRun1_design_v3/120X_mcRun1_design_v1
as you can see the difference is only between the desired tags.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-215eb5/17067/summary.html
COMMIT: 72ff08e
CMSSW: CMSSW_12_0_X_2021-07-20-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34502/17067/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

The workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/10424.0_TTbar_13+2017Design+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+RecoFakeHLT+HARVESTFakeHLT
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/11024.2_TTbar_13UP18HEfailINPUT+TTbar_13UP18HEfailINPUT+DigiFullHEfail+RecoFullHEfail+HARVESTFullHEfail+NanoFullHEfail
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/11224.0_TTbar_13+2018Design+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+RecoFakeHLT+HARVESTFakeHLT
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/12034.0_TTbar_14TeV+2021Design+TTbar_14TeV_TuneCP5_GenSim+Digi+Reco+HARVEST
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/140.0_HydjetQ_B12_5020GeV_2011+HydjetQ_B12_5020GeV_2011+DIGIHI2011+RECOHI2011+HARVESTHI2011
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/145.0_HydjetQ_B12_5020GeV_2015+HydjetQ_B12_5020GeV_2015+DIGIHI2015+RECOHI2015+HARVESTHI2015
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/203.0_H130GGgluonfusion+H130GGgluonfusion+DIGIPU1+RECOPU1+HARVEST
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/204.0_QQH1352T+QQH1352T+DIGIPU1+RECOPU1+HARVEST
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/205.0_ZTT+ZTT+DIGIPU1+RECOPU1+HARVEST
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/250200.182_ZEE_13UP18_RD+ZEE_13UP18_RD+DIGIPRMXUP18_PU25_RD+RECOPRMXUP18_PU25_L1TEgDQM_RD+HARVESTUP18_PU25_L1TEgDQM_RD
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/4001.0_TTbar+TTbar+DIGIPU1+RECOPUDBG
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/4006.0_SingleElectronFlatPt1To100+SingleElectronFlatPt1To100+DIGIPU1+RECOPU1
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/4007.0_QCD_Pt_30_80_BCtoE_8TeV+QCD_Pt_30_80_BCtoE_8TeV+DIGIPU1+RECOPUDBG
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/4008.0_QCD_Pt_80_170_BCtoE_8TeV+QCD_Pt_80_170_BCtoE_8TeV+DIGIPU1+RECOPUDBG
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/50200.0_ZEE_13+ZEE_13+DIGIUP15_PU50+RECOUP15_PU50_L1TEgDQM+HARVESTUP15_PU50_L1TEgDQM
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/7.21_Cosmics_UP17+Cosmics_UP17+DIGICOS_UP17+RECOCOS_UP17+ALCACOS_UP17+HARVESTCOS_UP17
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/7.22_Cosmics_UP16+Cosmics_UP16+DIGICOS_UP16+RECOCOS_UP16+ALCACOS_UP16+HARVESTCOS_UP16
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/7.23_Cosmics_UP21+Cosmics_UP21+DIGICOS_UP21+RECOCOS_UP21+ALCACOS_UP21+HARVESTCOS_UP21
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-215eb5/7.24_Cosmics_UP21_0T+Cosmics_UP21_0T+DIGICOS_UP21_0T+RECOCOS_UP21_0T+ALCACOS_UP21_0T+HARVESTCOS_UP21_0T

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3472 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 2996268
  • DQMHistoTests: Total failures: 4896
  • DQMHistoTests: Total nulls: 19
  • DQMHistoTests: Total successes: 2991331
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 45.703 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 140.53 ): 44.531 KiB Hcal/DigiRunHarvesting
  • DQMHistoSizes: changed ( 140.53 ): 1.172 KiB RPC/DCSInfo
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Jul 21, 2021

+alca

  • the effect for the failing comparisons for workflows 9.0 and 25.0 are proven to be negligible
  • failing comparisons for wf 140.53 is not connected to the PR, that's a data wf, and this PR touches simulation code and MC GTs

@mmusich
Copy link
Contributor

mmusich commented Jul 22, 2021

unassign trk-dpg

@civanch
Copy link
Contributor

civanch commented Jul 22, 2021

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy, @perrotta (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

+1
GT changes approved by AlCa

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