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.

Sign up for GitHub

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

Jump to bottom

esConsumes for PhotonProducer/GsfPhotonProducer #35151

Merged
merged 3 commits into from
Sep 9, 2021
Merged

esConsumes for PhotonProducer/GsfPhotonProducer #35151

merged 3 commits into from
Sep 9, 2021

Conversation

Dr15Jones
Copy link
Contributor

PR description:

  • added esConsumes directly to modules
  • refactored helper classes to allow esConsumes

PR validation:

Code compiles.

@cmsbuild cmsbuild added this to the CMSSW_12_1_X milestone Sep 3, 2021
@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 3, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35151/25073

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 3, 2021

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

  • RecoEgamma/EgammaPhotonAlgos (reconstruction)
  • RecoEgamma/EgammaPhotonProducers (reconstruction)
  • RecoEgamma/EgammaTools (reconstruction)
  • RecoEgamma/PhotonIdentification (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @jainshilpi, @rovere, @lgray, @sobhatta, @lecriste, @afiqaize, @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

@Dr15Jones
Copy link
Contributor Author

Please test

@makortel makortel mentioned this pull request Sep 4, 2021
Closed
2 tasks
@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 4, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b4c99d/18308/summary.html
COMMIT: 67fec66
CMSSW: CMSSW_12_1_X_2021-09-03-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35151/18308/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: 39
  • DQMHistoTests: Total histograms compared: 3000404
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3000376
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

jpata
jpata reviewed Sep 7, 2021
View reviewed changes
Copy link
Contributor

@jpata jpata left a comment

Choose a reason for hiding this comment

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

To be confirmed if RecoEgamma/EgammaTools/plugins/EGEnergyAnalyzer.cc is actually still used.
cc @afiqaize @SohamBhattacharya and perhaps @bendavid

RecoEgamma/EgammaTools/plugins/EGEnergyAnalyzer.cc
Comment on lines +67 to +68
: corddbFactory_(consumesCollector(), "wgbrph"),
corfile(egEnergyCorrectorFactoryFromRootFile("/afs/cern.ch/user/b/bendavid/cmspublic/gbrv3ph.root")),
Copy link
Contributor

Choose a reason for hiding this comment

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

by the look of this default parameter, does this file rather belong in /test/? Or maybe it's not needed any more at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I am not the maintainer of the code, I'd prefer to have any such work done in a later PR.

@jpata
Copy link
Contributor

jpata commented Sep 8, 2021

+reconstruction

  • for 67fec66
  • technical, no reco changes
  • RecoEgamma/EgammaTools/plugins/EGEnergyAnalyzer.cc that is updated here looks like private test code which may better be removed or live under /test/, to be confirmed and done later (cc @afiqaize @SohamBhattacharya)

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 8, 2021

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
perrotta reviewed Sep 8, 2021
View reviewed changes
Copy link
Contributor

@perrotta perrotta left a comment

Choose a reason for hiding this comment

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

ONLY if this file needs to be modified anyhow, you could also remove one of the two duplicate inclusions of "Geometry/CaloTopology/interface/CaloTopology.h"

RecoEgamma/EgammaPhotonAlgos/src/PhotonEnergyCorrector.cc
if (!regressionCorrector_->IsInitialized())
regressionCorrector_->Initialize(theEventSetup, w_file_, weightsfromDB_);
if (not regressionCorrector_) {
regressionCorrector_ = std::make_unique<EGEnergyCorrector>(regressionCorrectorFactory_->build(theEventSetup));
Copy link
Contributor

Choose a reason for hiding this comment

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

@Dr15Jones how can regressionCorrector_ be non existent? It is always initialized in the constructor, see
https://github.com/cms-sw/cmssw/pull/35151/files#diff-57977e080ee8dc7a3782ed263ff9e13425dca7b2341cc5b20555b376442fe014R59

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If weightsfromDB_ is true than regressionCorrector_ is not set in the constructor and will be set here (since it has to wait until the EventSetup is ready).

Copy link
Contributor

Choose a reason for hiding this comment

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

If weightsfromDB_ is true than regressionCorrector_ is not set in the constructor and will be set here (since it has to wait until the EventSetup is ready).

Ah, ok, thank you! My fault, I misread regressionCorrectorFactory_ as regressionCorrector_

@perrotta
Copy link
Contributor

perrotta commented Sep 9, 2021

+1

@cmsbuild cmsbuild merged commit c66b98f into cms-sw:master Sep 9, 2021
@Dr15Jones Dr15Jones deleted the egammaphoton_esconsumes branch September 9, 2021 17:48
This was referenced Sep 9, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpata jpata jpata left review comments

@perrotta perrotta perrotta left review comments

Assignees
No one assigned
Labels
code-checks-approved fully-signed orp-approved reconstruction-approved tests-approved
Projects
None yet
Milestone
CMSSW_12_1_X
Development

Successfully merging this pull request may close these issues.
4 participants
@Dr15Jones @cmsbuild @jpata @perrotta