No need for GsfElectronCoreBaseProducer and merging .h and .cc files for EgammaElectronProducers #28746

guitargeek · 2020-01-15T22:03:35Z

PR description:

Now that #28429 is merged, it is the moment for a couple of more changes in RecoEgamma/ElectronProducers. One thing I'd like to address is the inheritance of plugins from "BaseProducers", which is something that I think sometimes awkwardly lies between plugin configuration and separate plugins. The GsfElectronCoreBaseProducer is one of these instances where it's probably not good to do it: the base producer just manages three products which are not even used by all inheriting producers and has a two-liner member function. Repeating this code in the inheriting producers is actually less verbose than having the base class, while making it clear which products are actually used and also making the transition to global modules easier.

Additionally, I apply several modernizations to the ElectronCore producers like the usage of edm::EDGetTokenT and also gave a similar treatment to the GEDGsfElectronFinalizer while at it.

PR validation:

CMSSW compiles and local matrix tests pass.

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

No backport intended.

cmsbuild · 2020-01-15T22:04:01Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-01-15T22:11:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28746/13379

This PR adds an extra 24KB to repository

cmsbuild · 2020-01-15T22:11:32Z

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

It involves the following packages:

RecoEgamma/EgammaElectronProducers

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @jainshilpi, @rovere, @lgray, @sobhatta, @afiqaize, @varuns23 this is something you requested to watch as well.
@davidlange6, @silviodonato, @fabiocos you are the release manager for this.

cms-bot commands are listed here

slava77 · 2020-01-15T22:24:58Z

@cmsbuild please test

cmsbuild · 2020-01-15T22:25:22Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/4241/console Started: 2020/01/15 23:27

makortel · 2020-01-15T22:48:09Z

RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronCoreProducer.cc

  }

-  event.put(std::move(electrons));
+  event.emplace(putToken_, electrons);


Just to note that this will call the copy constructor of reco::GsfElectronCoreCollection inside emplace(). Using move like

event.emplace(putToken_, std::move(electrons));

would use the move constructor (that would be much cheaper for e.g. std::vectors).

Thanks @makortel for the heads up and taking care! I'm changing that of course.

cmsbuild · 2020-01-15T23:37:13Z

+1
Tested at: 94eb010
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e814e7/4241/summary.html
CMSSW: CMSSW_11_1_X_2020-01-15-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-01-15T23:37:16Z

Comparison job queued.

cmsbuild · 2020-01-16T01:10:22Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2711715
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2711368
DQMHistoTests: Total skipped: 346
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

slava77 · 2020-01-16T13:43:37Z

@cmsbuild please test

cmsbuild · 2020-01-16T13:44:02Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/4247/console Started: 2020/01/16 14:46

cmsbuild · 2020-01-16T14:55:07Z

+1
Tested at: fdcbc69
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e814e7/4247/summary.html
CMSSW: CMSSW_11_1_X_2020-01-16-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-01-16T14:55:12Z

Comparison job queued.

cmsbuild · 2020-01-16T16:25:59Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2711715
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2711368
DQMHistoTests: Total skipped: 346
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

silviodonato · 2020-01-21T08:32:09Z

@perrotta @slava77 do you have further comments on this PR?

silviodonato · 2020-01-28T15:19:33Z

This is just a friendly reminder for @slava77 and @perrotta

perrotta · 2020-01-29T10:34:30Z

RecoEgamma/EgammaElectronProducers/plugins/GEDGsfElectronCoreProducer.cc

 #include "DataFormats/EgammaCandidates/interface/GsfElectronCore.h"
-#include "DataFormats/ParticleFlowReco/interface/GsfPFRecTrack.h"
-#include "DataFormats/GsfTrackReco/interface/GsfTrack.h"


Why are you removing these include's? Are you relying on them being included through GsfElectronCore.h?
I would personally further add GsfTrackFwd.h, for completeness.

Again, correct me if I'm wrong, but I think these *Fwd.h are not meant to be included in *.cc files, and that's why I removed them. Indeed, I'm relying here on the indirect include via GsfElectronCore.h which I think is fine because the *Track objects in this producer are only used as arguments for GsfElectronCore constructors or member functions, so we can rely on GsfElectronCore.h to have the good typedefs imported to use it's own functions. Is that OK for you?

perrotta · 2020-01-29T10:45:02Z

RecoEgamma/EgammaElectronProducers/plugins/GEDGsfElectronFinalizer.cc

+#include "CommonTools/CandAlgos/interface/ModifyObjectValueBase.h"
+#include "DataFormats/Common/interface/ValueMap.h"
+#include "DataFormats/EgammaCandidates/interface/GsfElectron.h"
+#include "DataFormats/ParticleFlowCandidate/interface/PFCandidate.h"


What about

#include "DataFormats/EgammaCandidates/interface/GsfElectronFwd.h" #include "DataFormats/ParticleFlowCandidate/interface/PFCandidateFwd.h"

GsfElectronFwd.h is already correctly imported in GsfElectron.h and PFCandidateFwd.h is already correctly imported in PFCandidate.h.

perrotta · 2020-01-29T10:56:54Z

RecoEgamma/EgammaElectronProducers/plugins/GsfElectronCoreEcalDrivenProducer.cc

 #include "DataFormats/EgammaCandidates/interface/GsfElectronCore.h"
 #include "DataFormats/ParticleFlowReco/interface/GsfPFRecTrack.h"
 #include "DataFormats/GsfTrackReco/interface/GsfTrack.h"
-#include "DataFormats/EgammaReco/interface/SuperClusterFwd.h"


Why are these two include's removed?

The include of ElectronSeedFwd.h is removed because this is already correctly included in ElectronSeed.h. The SuperCluster include is removed because again it's a Fwd and we only use the reco::SuperClusterRef as an argument for one of the member functions in GsfElectronCore [1]. So again I think it's fair to get this definition indirectly via GsfElectronCore.h.

[1] https://github.com/cms-sw/cmssw/blob/master/DataFormats/EgammaCandidates/interface/GsfElectronCore.h#L55

perrotta · 2020-01-29T11:14:19Z

RecoEgamma/EgammaElectronProducers/plugins/GsfElectronCoreEcalDrivenProducer.cc

-  if (!eleCore->ecalDrivenSeed()) {
-    delete eleCore;
+  if (!eleCore.ecalDrivenSeed()) {
+    electrons.pop_back();


Why not checking first if gsfTrackRef.ecalDrivenSeed(), and then emplace_back only in that case?

Good idea, but it turns out the GsfTrack does not have this member function, so one would have to reproduce the logic from the GsfElectronCore producer I think [1]. I'm not really sure that we can find a quick and safe solution without code duplication in the scope of this PR, or do you have any idea?

[1] https://github.com/cms-sw/cmssw/blob/master/DataFormats/EgammaCandidates/src/GsfElectronCore.cc#L15

perrotta · 2020-01-29T11:17:04Z

RecoEgamma/EgammaElectronProducers/plugins/LowPtGsfElectronCoreProducer.cc

@@ -1,62 +1,79 @@
 #include "DataFormats/EgammaCandidates/interface/GsfElectronCore.h"
-#include "DataFormats/EgammaCandidates/interface/GsfElectronCoreFwd.h"


Still missing the point why these includes are removed, as well as SuperClusterFwd.h

Someone correct me if I'm wrong, but here is the line of thought I had:

In general, the point of forward declarations is to keep header files light. This LowPtGsfElectronCoreProducer.cc is not a header file and therefore the actual GsfElectronCore.h is needed instead of GsfElectronCoreFwd.h. This makes GsfElectronCoreFwd.h superfluous because it's already included in GsfElectronCore.h [1], like it should be to get the typedefs.

[1] https://github.com/cms-sw/cmssw/blob/master/DataFormats/EgammaCandidates/interface/GsfElectronCore.h

perrotta · 2020-01-29T15:19:44Z

Thank you @guitargeek for checking.

About the inclusion of header files: I think to remember that it was recommended to include in every package the headers of all classes needed internally, to avoid troubles in case the header nesting the needed ones got changed. (In our case this doesn't even apply, because as you correctly point out, the same definitions are repeated in the non-forward declared header).
Moreover, I don't find that rule coded anywhere for CMS [1]: therefore I won't insist on it

[1] http://cms-sw.github.io/cms_coding_rules.html

perrotta · 2020-01-29T15:22:04Z

+1

Electron producers in RecoEgamma/EgammaElectronProducers are simplified, by removing the intermediate layer of a base producer, as well as by applying other code modernization in them
Jenkins test pass and show no difference with the baseline

cmsbuild · 2020-01-29T15:22:28Z

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

silviodonato · 2020-01-30T09:17:37Z

+1

cmsbuild added this to the CMSSW_11_1_X milestone Jan 15, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jan 15, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 15, 2020

cmsbuild added tests-started and removed tests-pending labels Jan 15, 2020

makortel reviewed Jan 15, 2020

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Jan 15, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jan 16, 2020

guitargeek added 4 commits January 16, 2020 10:04

remove GsfElectronCoreBaseProducer

6703e1a

further modernization of electron core producers

0930308

some modernization of GEDGsfElectronFinalizer

4937160

std::move collections when emplacing them into events

fdcbc69

cmsbuild removed code-checks-approved comparison-available labels Jan 16, 2020

guitargeek changed the title ~~No need for GsfElectronCoreBaseProducer~~ No need for GsfElectronCoreBaseProducer and merging .h and .cc files for EgammaElectronProducers Jan 16, 2020

cmsbuild added tests-started and removed tests-pending labels Jan 16, 2020

cmsbuild added tests-approved and removed tests-started labels Jan 16, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jan 16, 2020

perrotta reviewed Jan 29, 2020

View reviewed changes

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jan 29, 2020

cmsbuild added orp-approved and removed orp-pending labels Jan 30, 2020

cmsbuild merged commit 47cd5a5 into cms-sw:master Jan 30, 2020

guitargeek deleted the ElectronAlgosCoreProducers_1 branch January 30, 2020 09:22

Dr15Jones mentioned this pull request Feb 1, 2020

GEDGsfElectronFinalizer is not thread safe #28849

Closed

perrotta mentioned this pull request Feb 2, 2020

Bring back GEDGsfElectronFinalizer to be a stream module, instead of a global one #28853

Merged

slava77 mentioned this pull request Dec 9, 2020

Add low-pT electrons to MINIAOD, update ID, improve end user experience (back port of 31220) #32372

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No need for GsfElectronCoreBaseProducer and merging .h and .cc files for EgammaElectronProducers #28746

No need for GsfElectronCoreBaseProducer and merging .h and .cc files for EgammaElectronProducers #28746

guitargeek commented Jan 15, 2020

cmsbuild commented Jan 15, 2020

cmsbuild commented Jan 15, 2020

cmsbuild commented Jan 15, 2020

slava77 commented Jan 15, 2020

cmsbuild commented Jan 15, 2020 •

edited

makortel Jan 15, 2020

guitargeek Jan 16, 2020

cmsbuild commented Jan 15, 2020

cmsbuild commented Jan 15, 2020

cmsbuild commented Jan 16, 2020

slava77 commented Jan 16, 2020

cmsbuild commented Jan 16, 2020 •

edited

cmsbuild commented Jan 16, 2020

cmsbuild commented Jan 16, 2020

cmsbuild commented Jan 16, 2020

silviodonato commented Jan 21, 2020

silviodonato commented Jan 28, 2020

perrotta Jan 29, 2020

guitargeek Jan 29, 2020

perrotta Jan 29, 2020

guitargeek Jan 29, 2020

perrotta Jan 29, 2020

guitargeek Jan 29, 2020

perrotta Jan 29, 2020

guitargeek Jan 29, 2020

perrotta Jan 29, 2020

guitargeek Jan 29, 2020 •

edited

perrotta commented Jan 29, 2020

perrotta commented Jan 29, 2020

cmsbuild commented Jan 29, 2020

silviodonato commented Jan 30, 2020

		@@ -1,62 +1,79 @@
		#include "DataFormats/EgammaCandidates/interface/GsfElectronCore.h"
		#include "DataFormats/EgammaCandidates/interface/GsfElectronCoreFwd.h"

No need for GsfElectronCoreBaseProducer and merging .h and .cc files for EgammaElectronProducers #28746

No need for GsfElectronCoreBaseProducer and merging .h and .cc files for EgammaElectronProducers #28746

Conversation

guitargeek commented Jan 15, 2020

PR description:

PR validation:

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

cmsbuild commented Jan 15, 2020

cmsbuild commented Jan 15, 2020

cmsbuild commented Jan 15, 2020

slava77 commented Jan 15, 2020

cmsbuild commented Jan 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Jan 15, 2020

cmsbuild commented Jan 15, 2020

cmsbuild commented Jan 16, 2020

slava77 commented Jan 16, 2020

cmsbuild commented Jan 16, 2020 • edited

cmsbuild commented Jan 16, 2020

cmsbuild commented Jan 16, 2020

cmsbuild commented Jan 16, 2020

silviodonato commented Jan 21, 2020

silviodonato commented Jan 28, 2020

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

guitargeek Jan 29, 2020 • edited

Choose a reason for hiding this comment

perrotta commented Jan 29, 2020

perrotta commented Jan 29, 2020

cmsbuild commented Jan 29, 2020

silviodonato commented Jan 30, 2020

cmsbuild commented Jan 15, 2020 •

edited

cmsbuild commented Jan 16, 2020 •

edited

guitargeek Jan 29, 2020 •

edited