-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ECAL esConsumes migration of CalibCalorimetry packages #35077
ECAL esConsumes migration of CalibCalorimetry packages #35077
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35077/24958
|
A new Pull Request was created by @thomreis (Thomas Reis) for master. It involves the following packages:
@malbouis, @yuanchao, @cmsbuild, @rekovic, @tlampen, @pohsun, @francescobrivio, @cecilecaillol, @tvami can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild , please test |
CalibCalorimetry/CaloMiscalibTools/interface/EcalRecHitRecalib.h
Outdated
Show resolved
Hide resolved
CalibCalorimetry/CaloMiscalibTools/plugins/HcalRecHitRecalib.cc
Outdated
Show resolved
Hide resolved
_qualpercent(iConfig.getUntrackedParameter<double>("qualPercent", 0.2)), | ||
_debug(iConfig.getUntrackedParameter<int>("debug", 0)), | ||
resdir_(iConfig.getUntrackedParameter<std::string>("resDir")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an internal naming inconsistency here as well, right? what was here earlier all started with an underscore, but the new code doesn't and in case of the resdir_
the underscore is in the end. Could this be made uniform in the code? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this looks rather ugly. The inconsistency in member variable naming scheme has been there before. The initializer list has just made it more apparent. Fixing all these variables will take significant time that I think is better spent migrating more packages to esConsumes.
Hi @thomreis I added some comments regarding the coding style rules, please have a look when you can! |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-35412c/18145/summary.html Comparison SummarySummary:
|
@cmsbuild , please test |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-35412c/18509/summary.html Comparison SummarySummary:
|
To my understanding, both of them are on vacation (at least that's what I've heard when we collected info for the Offline GT) |
@rekovic @cecilecaillol @lathomas would you please sign this? |
+1 |
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) |
+1 |
PR description:
The PR migrates ECAL related code in the CalibCalorimetry packages to esConsumes (#31061). In addition
getByLabel()
calls were replaced bygetByToken()
and legacy module types were migrated to multithreaded module types.Modified packages:
PR validation:
Code compiles. Where test configurations were present those were run as well.