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

JetCorrector migration in DiJetAnalyzer, GammaJetAnalysis #40139

Conversation

wddgit
Copy link
Contributor

@wddgit wddgit commented Nov 23, 2022

PR description:

Upgrade modules named DiJetAnalyzer and GammaJetAnalysis in the package Calibration/ HcalCalibAlgos from using the deprecated JetCorrector class to use the reco::JetCorrector class.

(This is my second try at this. See PR #39804. Originally I proposed deleting these modules).

PR validation:

Note that there are no unit tests or RelVal tests of these modules and nothing in CMSSW uses them that I am aware of besides the standalone test configurations also modified by this PR. Those test configurations did not work before the changes in this PR and will need some work on issues unrelated to JetCorrectors to execute successfully. There are some comments added in the test configurations indicating at least some of the problems that would need to be fixed. I did not address issues unrelated to JetCorrectors in this PR.

I hacked around with this code and made temporary changes (commenting out parts) as a test to be able to see that the JetCorrector parts of it are parseable by Python and the JetCorrector producers execute (verified in Tracer output).

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40139/33131

  • This PR adds an extra 48KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • Calibration/HcalCalibAlgos (alca)

@malbouis, @yuanchao, @cmsbuild, @saumyaphor4252, @francescobrivio, @ChrisMisan, @tvami can you please review it and eventually sign? Thanks.
@bsunanda, @mmusich, @tocheng 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 Nov 23, 2022

please test

FYI @makortel

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-831f3b/29224/summary.html
COMMIT: 1c1ec6a
CMSSW: CMSSW_12_6_X_2022-11-23-1200/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40139/29224/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: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3417239
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3417214
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 206 log files, 48 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Nov 26, 2022

Hi @bsunanda @mariadalfonso , do you have any ways to test these changes locally?

@wddgit
Copy link
Contributor Author

wddgit commented Nov 28, 2022

If you give me new input files, then I will modify the the test configurations to use them and try executing the existing test configurations again. There are probably more problems than that to fix, but maybe that would be enough to get one or two of the configurations to run successfully (or maybe not). The more recent ones (but still old) are trying to read a CMSSW_7_3_X files or a local file that I do not have.

Note, that after this PR is merged we can proceed to merge the other PR that deletes the old JetCorrector code. After that, you can still upgrade DiJetAnalyzer and GammaJetAnalysis again at your convenience or when it is needed. You won't be able to go back to the old JetCorrectors, but you will be able to make whatever other changes you want or need.

@wddgit
Copy link
Contributor Author

wddgit commented Dec 2, 2022

This is now the last PR removing usage of the old JetCorrector class in CMSSW. The other PR that was still under review was just merged.

One point I want to emphasize. You will be able to make further modifications to this code after this is merged. If there is something you do not like, you will be able to fix it. If there are additional improvements desired, you will be able to make them. And since nothing currently uses these modules, this PR will not break anything. It is an incremental change that fixes one problem in these modules and moves them one step closer to being usable. With it, they will use the modern JetCorrectors instead of the deprecated JetCorrectors that don't work anymore.

@tvami
Copy link
Contributor

tvami commented Dec 2, 2022

+alca

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@wddgit
Copy link
Contributor Author

wddgit commented Dec 2, 2022

@tvami Thanks

@perrotta
Copy link
Contributor

perrotta commented Dec 2, 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.

None yet

4 participants