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

Turn off GEM [un]packer in phase2 #34560

Merged
merged 2 commits into from Jul 24, 2021

Conversation

watson-ij
Copy link
Contributor

PR description:

The digi packer/unpacker format for GEM in Phase2 is not yet developed. So, we are turning off the packer for GEM in phase 2 simulations.

PR validation:

Checked that run3 and phase2 workflows still run and the GEM digi distributions are the same. Checked that the relevant modules are turned off in the process schedule.

if this PR is a backport please specify the original PR and why you need to backport that PR:

@jshlee

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34560/24092

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @watson-ij (Ian J. Watson) for master.

It involves the following packages:

  • Configuration/StandardSequences (operations)
  • RecoLocalMuon/GEMRecHit (upgrade, reconstruction)
  • Validation/MuonGEMDigis (dqm)

@perrotta, @andrius-k, @kmaeshima, @ErnestaP, @kpedro88, @cmsbuild, @silviodonato, @srimanob, @jfernan2, @ahmad3213, @slava77, @jpata, @qliphy, @davidlange6, @fabiocos, @rvenditti can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @fabiocos, @makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @jhgoh, @VinInn, @jshlee, @bellan, @rovere, @lecriste, @mtosi, @ebrondol, @mmusich, @dgulhan, @slomeo 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

@jfernan2
Copy link
Contributor

Hi @watson-ij
May I ask you to include yourself along with your github username in the following egroup?
https://e-groups.cern.ch/e-groups/Egroup.do?egroupName=cms-dqm-validation-developers-gem&tab=3
Thanks

@watson-ij
Copy link
Contributor Author

Hi @watson-ij
May I ask you to include yourself along with your github username in the following egroup?
https://e-groups.cern.ch/e-groups/Egroup.do?egroupName=cms-dqm-validation-developers-gem&tab=3
Thanks

Yep, just added myself there

@jfernan2
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d0bffc/17009/summary.html
COMMIT: d9730e8
CMSSW: CMSSW_12_0_X_2021-07-19-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34560/17009/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 2996268
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2996245
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor

+1

@@ -47,7 +47,8 @@
run2_GEM_2017.toReplaceWith(DigiToRawTask, _gem_DigiToRawTask)

from Configuration.Eras.Modifier_run3_GEM_cff import run3_GEM
run3_GEM.toReplaceWith(DigiToRawTask, _gem_DigiToRawTask)
from Configuration.Eras.Modifier_phase2_GEM_cff import phase2_GEM
(run3_GEM & ~phase2_GEM).toReplaceWith(DigiToRawTask, _gem_DigiToRawTask)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand the need to update this line. Will we have the situation that we will define Run-3 and Phase-2 in the same config? I think the current one in master applied only to Run-3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Phase2 [2] takes run3 [1] and adds phase2_GEM uses phase2_GEM and run3_GEM. For GEM reconstruction, the way its currently working is that run3_GEM turns everything on, and phase2_GEM applies some minor modifications when needed. This has been my understanding anyway.

[1] https://github.com/cms-sw/cmssw/blob/master/Configuration/Eras/python/Era_Run3_cff.py
[2] https://github.com/cms-sw/cmssw/blob/master/Configuration/Eras/python/Era_Phase2_cff.py

@@ -9,3 +9,6 @@
#maskFile = cms.FileInPath("RecoLocalMuon/GEMRecHit/data/maskedStrips.txt"),
#deadFile = cms.FileInPath("RecoLocalMuon/GEMRecHit/data/deadStrips.txt")
)

from Configuration.Eras.Modifier_phase2_GEM_cff import phase2_GEM
phase2_GEM.toModify(gemRecHits, gemDigiLabel = cms.InputTag("simMuonGEMDigis"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we expect to see the difference in the test when you use simMuonGEMDigis instead of muonGEMDigis? What is done currently w/o this PR?

Copy link
Contributor Author

@watson-ij watson-ij Jul 20, 2021

Choose a reason for hiding this comment

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

No, there shouldn't be a difference. The muonGEMDigis are created by sending the simMuonGEMDigis through the packer/unpacker chain, and this should essentially act as a test of the packer. Since for phase2, the RAW format isn't established yet and we are currently working on getting the unpacker setup with the MWGR data now coming from the GEMs, we want to skip the these steps for phase2 and just use the simMuonGEMDigis.

@srimanob
Copy link
Contributor

+Upgrade

This PR is to turn off packer/unpacker of Phase-2 GEM. simDigi is used instead of digi. No change is expected from this PR.

@cmsbuild
Copy link
Contributor

Pull request #34560 was updated. @perrotta, @andrius-k, @kmaeshima, @ErnestaP, @kpedro88, @cmsbuild, @silviodonato, @srimanob, @jfernan2, @ahmad3213, @slava77, @jpata, @qliphy, @davidlange6, @fabiocos, @rvenditti can you please check and sign again.

@jfernan2
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

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

Comparison Summary

Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 2998564
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2998541
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Jul 23, 2021

+reconstruction

for #34560 a64da1a

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show no differences in monitored products; the product comparison check shows that the phase-2 (FEVT) output does not have GEMDetIdGEMDigiMuonDigiCollection_muonGEMDigis__, which is apparently expected

@perrotta
Copy link
Contributor

+operations

@srimanob
Copy link
Contributor

+Upgrade

@qliphy
Copy link
Contributor

qliphy commented Jul 24, 2021

+operations

@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)

@qliphy
Copy link
Contributor

qliphy commented Jul 24, 2021

+1

@cmsbuild cmsbuild merged commit 74ffde3 into cms-sw:master Jul 24, 2021
@yeckang yeckang mentioned this pull request Aug 11, 2021
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