Slight refactoring of PFEGammaAlgo #27045

guitargeek · 2019-05-31T20:34:27Z

PR description:

While other PRs are working on PFAlgo, I still have some suggestions for PFEGammaAlgo. In this PR I propose to:

Have a const reference to the employed GBRForests as a member so one does not have to pass them as a function argument with the rather obscure name hoc (for "HeavyObjectCache"). Having const members is ok after all, I only have it with the mutable members.
Have a new output type for the PFAlgo, so one only has to use the algo by the operator() instead of retrieving all the results that are saved as mutable members of the algo via getter functions
Merge PFEGammaProducer.h with .cc

PFEGammaAlgo has now only half as many mutable members as before, so we are on a good track towards a nice thread safe algo there I think!

PR validation:

CMSSW compiles and local matrix tests pass.

if this PR is a backport please specify the original PR:

No backport intended.

cmsbuild · 2019-05-31T20:35:00Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-05-31T20:42:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27045/10126

This PR adds an extra 44KB to repository

cmsbuild · 2019-05-31T20:42:34Z

A new Pull Request was created by @guitargeek (Jonas Rembser) for master.

It involves the following packages:

RecoParticleFlow/PFProducer

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @hatakeyamak, @lgray, @seemasharmafnal, @cbernet, @bachtis this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

perrotta · 2019-06-04T13:27:16Z

please test

cmsbuild · 2019-06-04T13:27:53Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/745/console Started: 2019/06/04 15:28

cmsbuild · 2019-06-04T14:59:43Z

+1
Tested at: 8c59c0a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5c9751/745/summary.html

cmsbuild · 2019-06-04T14:59:46Z

Comparison job queued.

cmsbuild · 2019-06-04T17:09:19Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5c9751/745/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 33
DQMHistoTests: Total histograms compared: 3210222
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3209887
DQMHistoTests: Total skipped: 334
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
Checked 137 log files, 14 edm output root files, 33 DQM output files

perrotta · 2019-06-10T10:34:22Z

RecoParticleFlow/PFProducer/src/PFEGammaAlgo.cc

  _splayedblock.clear();
-  _refinableObjects.clear();
+  refinableObjects.clear();


This clearing is not needed any more, being refinableObjects now a local variable which was just defined here

True! Thanks for reviewing

perrotta · 2019-06-10T10:53:31Z

RecoParticleFlow/PFProducer/plugins/PFEGammaProducer.cc

+
+ private:
+
+  void setPFEGParameters(PFEGammaAlgo::PFEGConfigInfo&);


You removed the impementation of this function, no need to define it

cmsbuild · 2019-06-10T11:33:42Z

The code-checks are being triggered in jenkins.

guitargeek · 2019-06-10T11:38:41Z

Hi, I followed your two suggestions and made a little additional change that makes sense to me: I took the primaryVertex out of the cfg_ member, because I think the point of these "configuration structs" should be that they contain immutable configuration data only.

cmsbuild · 2019-06-10T11:39:49Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27045/10299

This PR adds an extra 48KB to repository

cmsbuild · 2019-06-10T11:40:17Z

Pull request #27045 was updated. @perrotta, @cmsbuild, @slava77 can you please check and sign again.

perrotta · 2019-06-10T13:12:55Z

please test

cmsbuild · 2019-06-10T13:13:24Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/856/console Started: 2019/06/10 15:14

cmsbuild · 2019-06-10T14:41:15Z

+1
Tested at: baa7000
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5c9751/856/summary.html

cmsbuild · 2019-06-10T14:41:19Z

Comparison job queued.

cmsbuild · 2019-06-10T16:16:16Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5c9751/856/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 33
DQMHistoTests: Total histograms compared: 3215458
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3215122
DQMHistoTests: Total skipped: 334
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
Checked 137 log files, 14 edm output root files, 33 DQM output files

perrotta · 2019-06-10T19:25:13Z

+1

Code refactoring and simplification according to what described
Jenkins tests pass and show no differences

cmsbuild · 2019-06-10T19:25:40Z

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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2019-06-11T20:10:23Z

+1

guitargeek added 3 commits May 23, 2019 15:50

Improved GBRForest handling in PFEGammaAlgo

87b70e8

Output struct for PFEGammaAlgo

31197b7

merged PFEGammaProducer.h with .cc

8c59c0a

cmsbuild added this to the CMSSW_11_0_X milestone May 31, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels May 31, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels May 31, 2019

cmsbuild added tests-started and removed tests-pending labels Jun 4, 2019

cmsbuild added tests-approved and removed tests-started labels Jun 4, 2019

cmsbuild added comparison-available and removed comparison-pending labels Jun 4, 2019

perrotta reviewed Jun 10, 2019

View reviewed changes

Code review and own member for primaryVertex

baa7000

cmsbuild removed code-checks-approved comparison-available labels Jun 10, 2019

cmsbuild added comparison-pending tests-pending labels Jun 10, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 10, 2019

cmsbuild added tests-started and removed tests-pending labels Jun 10, 2019

cmsbuild added tests-approved and removed tests-started labels Jun 10, 2019

cmsbuild added comparison-available and removed comparison-pending labels Jun 10, 2019

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jun 10, 2019

cmsbuild added orp-approved and removed orp-pending labels Jun 11, 2019

cmsbuild merged commit dce6495 into cms-sw:master Jun 11, 2019

guitargeek deleted the PFEGammaAlgo_1 branch June 11, 2019 22:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slight refactoring of PFEGammaAlgo #27045

Slight refactoring of PFEGammaAlgo #27045

guitargeek commented May 31, 2019

cmsbuild commented May 31, 2019

cmsbuild commented May 31, 2019

cmsbuild commented May 31, 2019

perrotta commented Jun 4, 2019

cmsbuild commented Jun 4, 2019 •

edited

cmsbuild commented Jun 4, 2019

cmsbuild commented Jun 4, 2019

cmsbuild commented Jun 4, 2019

perrotta Jun 10, 2019

guitargeek Jun 10, 2019

perrotta Jun 10, 2019

cmsbuild commented Jun 10, 2019

guitargeek commented Jun 10, 2019

cmsbuild commented Jun 10, 2019

cmsbuild commented Jun 10, 2019

perrotta commented Jun 10, 2019

cmsbuild commented Jun 10, 2019 •

edited

cmsbuild commented Jun 10, 2019

cmsbuild commented Jun 10, 2019

cmsbuild commented Jun 10, 2019

perrotta commented Jun 10, 2019

cmsbuild commented Jun 10, 2019

fabiocos commented Jun 11, 2019


		private:

		void setPFEGParameters(PFEGammaAlgo::PFEGConfigInfo&);

Slight refactoring of PFEGammaAlgo #27045

Slight refactoring of PFEGammaAlgo #27045

Conversation

guitargeek commented May 31, 2019

PR description:

PR validation:

if this PR is a backport please specify the original PR:

cmsbuild commented May 31, 2019

cmsbuild commented May 31, 2019

cmsbuild commented May 31, 2019

perrotta commented Jun 4, 2019

cmsbuild commented Jun 4, 2019 • edited

cmsbuild commented Jun 4, 2019

cmsbuild commented Jun 4, 2019

cmsbuild commented Jun 4, 2019

perrotta Jun 10, 2019

Choose a reason for hiding this comment

guitargeek Jun 10, 2019

Choose a reason for hiding this comment

perrotta Jun 10, 2019

Choose a reason for hiding this comment

cmsbuild commented Jun 10, 2019

guitargeek commented Jun 10, 2019

cmsbuild commented Jun 10, 2019

cmsbuild commented Jun 10, 2019

perrotta commented Jun 10, 2019

cmsbuild commented Jun 10, 2019 • edited

cmsbuild commented Jun 10, 2019

cmsbuild commented Jun 10, 2019

cmsbuild commented Jun 10, 2019

perrotta commented Jun 10, 2019

cmsbuild commented Jun 10, 2019

fabiocos commented Jun 11, 2019

cmsbuild commented Jun 4, 2019 •

edited

cmsbuild commented Jun 10, 2019 •

edited