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 unused variable use_preshower from PFECALSuperClusterProducer #37756

Merged
merged 2 commits into from
May 5, 2022

Conversation

swagata87
Copy link
Contributor

PR description:

The variable use_preshower in PFECALSuperClusterProducer.cc is not used anywhere, but just creates confusion. It seems like setting it to true/false would lead to using/not using preshower energy for superclustering; while that is not the case. This variable has no effect anywhere and should just be removed to avoid further confusion.

This PR is just meant for code cleanup. No change in physics is expected.

PR validation:

cmsDriver.py --filein /store/relval/CMSSW_12_4_0_pre3/RelValZpToEE_m6000_14TeV/GEN-SIM-DIGI-RAW/123X_mcRun3_2021_realistic_v14-v1/2580000/42d345d9-79a2-4006-8d6b-b4241a553f42.root --fileout file:AOD.root --mc --eventcontent AODSIM --runUnscheduled --customise Configuration/DataProcessing/Utils.addMonitoring --datatier AODSIM --conditions 123X_mcRun3_2021_realistic_v14 --step RAW2DIGI,RECO --geometry DB:Extended --era Run3 --python_filename aod_cfg.py --beamspot Run3RoundOptics25ns13TeVLowSigmaZ --no_exec -n 200
then
cmsRun aod_cfg.py
It ran fine.

This PR is not a backport.
Backport not necessary.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 1, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37756/29627

@cmsbuild
Copy link
Contributor

cmsbuild commented May 1, 2022

A new Pull Request was created by @swagata87 (Swagata Mukherjee) for master.

It involves the following packages:

  • HLTrigger/Configuration (hlt)
  • RecoEcal/EgammaClusterProducers (reconstruction)

@Martin-Grunewald, @clacaputo, @cmsbuild, @missirol, @slava77, @jpata can you please review it and eventually sign? Thanks.
@Sam-Harper, @silviodonato, @rchatter, @wrtabb, @jainshilpi, @valsdav, @rovere, @lgray, @Martin-Grunewald, @missirol, @sobhatta, @thomreis, @afiqaize, @simonepigazzini, @lecriste, @argiro, @varuns23, @ram1123 this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@missirol
Copy link
Contributor

missirol commented May 1, 2022

@swagata87, thanks for the cleanup.

We also need to add a customisation for the Phase-1 HLT menus, e.g.

diff --git a/HLTrigger/Configuration/python/customizeHLTforCMSSW.py b/HLTrigger/Configuration/python/customizeHLTforCMSSW.py
index 23820895fc2..72e70bc3651 100644
--- a/HLTrigger/Configuration/python/customizeHLTforCMSSW.py
+++ b/HLTrigger/Configuration/python/customizeHLTforCMSSW.py
@@ -172,6 +172,16 @@ def customiseFor37231(process):
 
     return process
 
+def customiseFor37756(process):
+    """https://github.com/cms-sw/cmssw/pull/37756
+    Removal of use_preshower parameter from PFECALSuperClusterProducer
+    """
+    for prod in producers_by_type(process, 'PFECALSuperClusterProducer'):
+        if hasattr(prod, 'use_preshower'):
+            delattr(prod, 'use_preshower')
+
+    return process
+
 
 def customiseForOffline(process):
 #   https://its.cern.ch/jira/browse/CMSHLT-2271
@@ -192,5 +202,6 @@ def customizeHLTforCMSSW(process, menuType="GRun"):
     # process = customiseFor12718(process)
 
     process = customiseFor37231(process)
+    process = customiseFor37756(process)
 
     return process

@swagata87
Copy link
Contributor Author

thanks Marino. I will include it soon.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37756/29649

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2022

Pull request #37756 was updated. @Martin-Grunewald, @clacaputo, @cmsbuild, @missirol, @slava77, @jpata can you please check and sign again.

@Martin-Grunewald
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 2, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-899626/24386/summary.html
COMMIT: a5d6d5a
CMSSW: CMSSW_12_4_X_2022-05-02-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37756/24386/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
  • Reco comparison results: 3 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3703092
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3703062
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 205 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@missirol
Copy link
Contributor

missirol commented May 3, 2022

+hlt

@missirol
Copy link
Contributor

missirol commented May 3, 2022

#37646 is already fully signed, and might get merged before this PR. Since both modify customizeHLTforCMSSW in a similar way, this PR will likely run into a merge-conflict if #37646 is merged first, and will thus need to be rebased.

@missirol
Copy link
Contributor

missirol commented May 4, 2022

This branch has conflicts that must be resolved

Unfortunately, we now have a merge-conflict, as expected after the integration of #37646.

@swagata87 , this PR should be rebased on CMSSW_12_4_X_2022-05-03-2300, or later IBs (you can just cherry-pick the commits and fix the conflicts manually, I think).

@swagata87
Copy link
Contributor Author

Thanks Marino, I rebased on CMSSW_12_4_X_2022-05-03-2300

@perrotta
Copy link
Contributor

perrotta commented May 4, 2022

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37756/29706

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2022

Pull request #37756 was updated. @jpata, @missirol, @Martin-Grunewald, @clacaputo, @slava77 can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-899626/24440/summary.html
COMMIT: ece096d
CMSSW: CMSSW_12_4_X_2022-05-03-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37756/24440/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
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3700548
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3700524
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 205 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@missirol
Copy link
Contributor

missirol commented May 4, 2022

+hlt

@clacaputo
Copy link
Contributor

+reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented May 5, 2022

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

perrotta commented May 5, 2022

+1

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.

6 participants