Simplify inheritance hirachy of EcalClusterFunctions #28332

guitargeek · 2019-11-01T15:18:40Z

PR description:

On my egamma refactors, I frequently encounter these plugins inheriting from EcalClusterFunctionBaseClass and it's always a bit complicated to understand what is going on.

For every plugin (let's say the EcalClusterCrackCorrection, there is another intermediate class defined (EcalClusterCrackCorrectionBaseClass) which is private to the plugins directory and is only used such that the EcalClusterCrackCorrection can inherit from it. This pattern is repeated for all the plugins. I would suggest to simplify this by having the plugins all inherit from EcalClusterFunctionBaseClass directly. Like this, the RecoEcal/EgammaCoreTools/plugins directory looks radically simpler and it would really help for understanding the code and future refactors. For example, one can now just do git grep EcalClusterFunctionBaseClass to find all the plugins that inherit from it. This PR doesn't make any actual changes to the code, it just reshuffles some lines.

PR validation:

CMSSW complies and local matrix tests pass.

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

No backport intended.

cmsbuild · 2019-11-01T15:20:52Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-11-01T15:29:19Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28332/12593

This PR adds an extra 48KB to repository

cmsbuild · 2019-11-01T15:29:44Z

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

It involves the following packages:

RecoEcal/EgammaCoreTools
RecoEgamma/EgammaPhotonProducers

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

slava77 · 2019-11-01T15:40:55Z

@ferriff it looks like you are the original developer/designer of these tools in RecoEcal/EgammaCoreTools
please check that this update is still OK wrt the original design ideas or if those are not needed anymore.
@argiro please confirm that this is fine for the ECAL DPG.
Thank you.

guitargeek · 2019-11-01T16:02:31Z

I assumed the idea was to maybe have at some point separate factories for alternative functions that serve the same purpose, like crack correction or energy correction, is that correct? But since these don't exist, this inheritances were just complicated boilerplate at this point.

slava77 · 2019-11-01T17:19:26Z

@cmsbuild please test

cmsbuild · 2019-11-01T17:19:53Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3307/console Started: 2019/11/01 18:21

cmsbuild · 2019-11-01T18:47:11Z

+1
Tested at: 9b7d45d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ada09/3307/summary.html

cmsbuild · 2019-11-01T18:47:14Z

Comparison job queued.

cmsbuild · 2019-11-01T20:24:51Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ada09/3307/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2939026
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2938683
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

VinInn · 2019-11-03T12:19:47Z

is this inheritance needed at all?

perrotta · 2019-11-05T15:42:29Z

RecoEcal/EgammaCoreTools/plugins/EcalBasicClusterLocalContCorrection.cc

@@ -13,25 +21,75 @@
 #include "Geometry/Records/interface/CaloGeometryRecord.h"
 #include "DataFormats/EcalDetId/interface/EcalSubdetector.h"
 #include "Geometry/CaloGeometry/interface/TruncatedPyramid.h"
-
-//////From DummyHepMCAnalyzer.cc:
 #include "FWCore/Framework/interface/EDAnalyzer.h"


Is this include needed?

perrotta · 2019-11-05T15:44:29Z

RecoEcal/EgammaCoreTools/plugins/EcalBasicClusterLocalContCorrection.cc

+#include "FWCore/Framework/interface/EventSetup.h"
+#include "FWCore/Framework/interface/ESHandle.h"
+#include "RecoEcal/EgammaCoreTools/interface/EcalClusterFunctionBaseClass.h"
+#include "FWCore/Framework/interface/ESHandle.h"


perrotta · 2019-11-05T15:46:39Z

RecoEcal/EgammaCoreTools/plugins/EcalClusterCrackCorrection.cc

+#include "FWCore/Framework/interface/ESHandle.h"
+#include "RecoEcal/EgammaCoreTools/interface/EcalClusterFunctionBaseClass.h"
+#include "FWCore/ParameterSet/interface/ParameterSet.h"
+#include "FWCore/Framework/interface/ESHandle.h"


perrotta · 2019-11-05T15:47:14Z

RecoEcal/EgammaCoreTools/plugins/EcalClusterCrackCorrection.cc

@@ -13,32 +19,92 @@
 #include "Geometry/Records/interface/CaloGeometryRecord.h"
 #include "DataFormats/EcalDetId/interface/EcalSubdetector.h"
 #include "Geometry/CaloGeometry/interface/TruncatedPyramid.h"
-
-//////From DummyHepMCAnalyzer.cc:
 #include "FWCore/Framework/interface/EDAnalyzer.h"


Is this needed?

perrotta · 2019-11-05T15:47:42Z

RecoEcal/EgammaCoreTools/plugins/EcalClusterCrackCorrection.cc

+#include "FWCore/Framework/interface/EventSetup.h"
+#include "FWCore/Framework/interface/ESHandle.h"
+#include "RecoEcal/EgammaCoreTools/interface/EcalClusterFunctionBaseClass.h"
+#include "FWCore/ParameterSet/interface/ParameterSet.h"


perrotta · 2019-11-05T15:58:08Z

RecoEcal/EgammaCoreTools/plugins/EcalClusterEnergyUncertaintyObjectSpecific.cc

+#include "CondFormats/EcalObjects/interface/EcalClusterEnergyUncertaintyParameters.h"
+#include "DataFormats/EgammaReco/interface/BasicCluster.h"
+#include "FWCore/Framework/interface/EventSetup.h"
+#include "FWCore/Framework/interface/ESHandle.h"


perrotta · 2019-11-05T15:58:33Z

RecoEcal/EgammaCoreTools/plugins/EcalClusterLocalContCorrection.cc

@@ -13,19 +19,70 @@
 #include "Geometry/Records/interface/CaloGeometryRecord.h"
 #include "DataFormats/EcalDetId/interface/EcalSubdetector.h"
 #include "Geometry/CaloGeometry/interface/TruncatedPyramid.h"
-
-//////From DummyHepMCAnalyzer.cc:
 #include "FWCore/Framework/interface/EDAnalyzer.h"


Is this include needed?

perrotta · 2019-11-05T15:59:22Z

RecoEcal/EgammaCoreTools/plugins/EcalClusterLocalContCorrection.cc

+#include "CondFormats/EcalObjects/interface/EcalClusterLocalContCorrParameters.h"
+#include "FWCore/Framework/interface/EventSetup.h"
+#include "DataFormats/EgammaReco/interface/BasicCluster.h"
+#include "FWCore/Framework/interface/EventSetup.h"


perrotta · 2019-11-05T15:59:36Z

RecoEcal/EgammaCoreTools/plugins/EcalClusterLocalContCorrection.cc

+#include "FWCore/Framework/interface/EventSetup.h"
+#include "DataFormats/EgammaReco/interface/BasicCluster.h"
+#include "FWCore/Framework/interface/EventSetup.h"
+#include "FWCore/Framework/interface/ESHandle.h"


perrotta · 2019-11-05T15:59:49Z

RecoEcal/EgammaCoreTools/plugins/EcalClusterLocalContCorrection.cc

+#include "FWCore/Framework/interface/ESHandle.h"
+#include "CondFormats/EcalObjects/interface/EcalClusterLocalContCorrParameters.h"
+#include "FWCore/Framework/interface/EventSetup.h"
+#include "DataFormats/EgammaReco/interface/BasicCluster.h"


cmsbuild · 2019-11-06T09:49:54Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-11-06T09:57:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28332/12639

This PR adds an extra 28KB to repository

cmsbuild · 2019-11-06T09:57:52Z

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

guitargeek · 2019-11-06T09:58:05Z

is this inheritance needed at all?

@VinInn Right now it is technically used because many plugins in Egamma use the EcalClusterFunctionFactory to create these helper functions. Whether this is the good way to use these helper functions is another question. In this PR I only wanted to address the "internal" inheritance hierarchy in the plugins directory for now.

perrotta · 2019-11-06T13:05:42Z

please test

cmsbuild · 2019-11-06T13:06:09Z

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

cmsbuild · 2019-11-06T14:29:36Z

+1
Tested at: 7d9538e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ada09/3367/summary.html

cmsbuild · 2019-11-06T14:29:40Z

Comparison job queued.

cmsbuild · 2019-11-06T16:04:53Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ada09/3367/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2939026
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2938683
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

perrotta · 2019-11-08T11:51:35Z

+1

The simplification achieved by eliminating the intermediate layer of base classes is welcome, and blessed (in private communications) by the ECAL contacts
Additional simplification and cleanings also applied during the review
Plugins functionalities are not affected
Jenkins tests pass and show no differences

cmsbuild · 2019-11-08T11:52:07Z

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-11-09T09:18:03Z

+1

guitargeek added 2 commits October 31, 2019 00:54

simplified inheritance structure of EcalClusterFunctions

b73ba33

removed a few faulty includes

9b7d45d

cmsbuild added this to the CMSSW_11_0_X milestone Nov 1, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Nov 1, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 1, 2019

cmsbuild added tests-started and removed tests-pending labels Nov 1, 2019

cmsbuild added tests-approved and removed tests-started labels Nov 1, 2019

cmsbuild added comparison-available and removed comparison-pending labels Nov 1, 2019

perrotta reviewed Nov 5, 2019

View reviewed changes

cleaned up includes and TMath usage

7d9538e

cmsbuild added the tests-pending label Nov 6, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 6, 2019

cmsbuild added tests-started and removed tests-pending labels Nov 6, 2019

cmsbuild added tests-approved and removed tests-started labels Nov 6, 2019

cmsbuild added comparison-available and removed comparison-pending labels Nov 6, 2019

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Nov 8, 2019

cmsbuild added orp-approved and removed orp-pending labels Nov 9, 2019

cmsbuild merged commit 1c116f6 into cms-sw:master Nov 9, 2019

guitargeek deleted the EcalClusterFunctions_1 branch November 10, 2019 11:05

guitargeek mentioned this pull request Nov 20, 2019

No actual use for EcalClusterShapeFunction in HiEgammaAlgos #28428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify inheritance hirachy of EcalClusterFunctions #28332

Simplify inheritance hirachy of EcalClusterFunctions #28332

guitargeek commented Nov 1, 2019

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 1, 2019

slava77 commented Nov 1, 2019

guitargeek commented Nov 1, 2019

slava77 commented Nov 1, 2019

cmsbuild commented Nov 1, 2019 •

edited

Loading

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 1, 2019

VinInn commented Nov 3, 2019

perrotta Nov 5, 2019

perrotta Nov 5, 2019

perrotta Nov 5, 2019

perrotta Nov 5, 2019

perrotta Nov 5, 2019

perrotta Nov 5, 2019

perrotta Nov 5, 2019

perrotta Nov 5, 2019

perrotta Nov 5, 2019

perrotta Nov 5, 2019

cmsbuild commented Nov 6, 2019

cmsbuild commented Nov 6, 2019

cmsbuild commented Nov 6, 2019

guitargeek commented Nov 6, 2019

perrotta commented Nov 6, 2019

cmsbuild commented Nov 6, 2019 •

edited

Loading

cmsbuild commented Nov 6, 2019

cmsbuild commented Nov 6, 2019

cmsbuild commented Nov 6, 2019

perrotta commented Nov 8, 2019

cmsbuild commented Nov 8, 2019

fabiocos commented Nov 9, 2019

Simplify inheritance hirachy of EcalClusterFunctions #28332

Simplify inheritance hirachy of EcalClusterFunctions #28332

Conversation

guitargeek commented Nov 1, 2019

PR description:

PR validation:

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

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 1, 2019

slava77 commented Nov 1, 2019

guitargeek commented Nov 1, 2019

slava77 commented Nov 1, 2019

cmsbuild commented Nov 1, 2019 • edited Loading

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 1, 2019

cmsbuild commented Nov 1, 2019

VinInn commented Nov 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Nov 6, 2019

cmsbuild commented Nov 6, 2019

cmsbuild commented Nov 6, 2019

guitargeek commented Nov 6, 2019

perrotta commented Nov 6, 2019

cmsbuild commented Nov 6, 2019 • edited Loading

cmsbuild commented Nov 6, 2019

cmsbuild commented Nov 6, 2019

cmsbuild commented Nov 6, 2019

perrotta commented Nov 8, 2019

cmsbuild commented Nov 8, 2019

fabiocos commented Nov 9, 2019

cmsbuild commented Nov 1, 2019 •

edited

Loading

cmsbuild commented Nov 6, 2019 •

edited

Loading