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

Delete commented out imports of deprecated JetCorrector cffs #40348

Merged
merged 2 commits into from Dec 21, 2022

Conversation

wddgit
Copy link
Contributor

@wddgit wddgit commented Dec 16, 2022

PR description:

We just removed central cff files related to deprecated JetCorrector's (#39953). This PR removes lines of Python configuration code where those files were imported and those particular lines were already commented out.

@vlimant suggested we do this centrally in Issue #40304. Easy to do, so I just did it.

PR validation:

Only deletes comment lines.

We just removed central cff files related to deprecated
JetCorrector's. This PR removes lines where those files
were imported and those lines already commented out.
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40348/33437

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @wddgit (W. David Dagenhart) for master.

It involves the following packages:

  • L1Trigger/L1TNtuples (l1)
  • PhysicsTools/PatAlgos (xpog, reconstruction)
  • PhysicsTools/TagAndProbe (analysis)
  • Validation/RecoJets (dqm)

@swertz, @vlimant, @epalencia, @micsucmed, @emanueleusai, @ahmad3213, @cmsbuild, @rekovic, @jfernan2, @clacaputo, @syuvivida, @pmandrik, @mandrenguyen, @cecilecaillol, @rvenditti can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @hatakeyamak, @emilbols, @Martin-Grunewald, @thomreis, @mbluj, @demuller, @seemasharmafnal, @mmarionncern, @kreczko, @jdolen, @ahinzmann, @missirol, @azotz, @eyigitba, @jdamgov, @gpetruc, @nhanvtran, @gkasieczka, @schoef, @andrzejnovak, @AlexDeMoor, @AnnikaStein, @JyothsnaKomaragiri, @dinyar, @mariadalfonso this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@wddgit
Copy link
Contributor Author

wddgit commented Dec 16, 2022

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5de819/29661/summary.html
COMMIT: 64b4a41
CMSSW: CMSSW_13_0_X_2022-12-16-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40348/29661/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: 27 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3557521
  • DQMHistoTests: Total failures: 1188
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3556311
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@vlimant
Copy link
Contributor

vlimant commented Dec 16, 2022

+1

@@ -389,7 +389,6 @@
## |_____/_/\_\\__\___|_| |_| |_|\__,_|_| \_/ \__,_|_| |___/
##
## Here we show how to use a module to compute an external variable
## process.load("JetMETCorrections.Configuration.DefaultJEC_cff")
## ak5PFResidual.useCondDB = False
Copy link
Contributor

Choose a reason for hiding this comment

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

This modifies a module from the line removed, can also get removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

Comment on lines 44 to 45

# get corrected jets
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# get corrected jets

Also this comment can now get removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

@@ -18,7 +18,6 @@
newAk4CaloL2L3Corrector
)

#from JetMETCorrections.Configuration.JetCorrectionServicesAllAlgos_cff import ak7CaloL2L3,ak7CaloL2Relative,ak7CaloL3Absolute
#newAk7CaloL2L3 = ak7CaloL2L3.clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#newAk7CaloL2L3 = ak7CaloL2L3.clone()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

@@ -29,7 +28,6 @@
newAk4PFL1FastL2L3Corrector
)

#from JetMETCorrections.Configuration.JetCorrectionServices_cff import ak4JPTL1FastL2L3,ak4JPTL1Fastjet,ak4JPTL2Relative,ak4JPTL3Absolute
#newAk4JPTL1FastL2L3 = ak4JPTL1FastL2L3.clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#newAk4JPTL1FastL2L3 = ak4JPTL1FastL2L3.clone()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40348/33460

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

Pull request #40348 was updated. @swertz, @vlimant, @epalencia, @micsucmed, @emanueleusai, @ahmad3213, @cmsbuild, @rekovic, @jfernan2, @clacaputo, @syuvivida, @pmandrik, @mandrenguyen, @cecilecaillol, @rvenditti can you please check and sign again.

@wddgit
Copy link
Contributor Author

wddgit commented Dec 19, 2022

please test

Made all changes requested in comments, deleted more unneeded comment lines.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5de819/29693/summary.html
COMMIT: 9d93f8f
CMSSW: CMSSW_13_0_X_2022-12-19-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40348/29693/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: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555748
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3555723
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

+1

  • This PR just removed some obsolete and already commented out lines: let merge it as such, to clean up the queue
  • I hope that @cms-sw/dqm-l2 @cms-sw/l1-l2 @cms-sw/reconstruction-l2 @cms-sw/xpog-l2 have nothing in contrary: otherwise just speak up

@perrotta
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit a8b2c3a into cms-sw:master Dec 21, 2022
@mandrenguyen
Copy link
Contributor

+1

@emanueleusai
Copy link
Member

+1

@wddgit wddgit deleted the deleteDeprecatedComments branch January 13, 2023 17:34
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