Some RecoParticleFlow/PFProducer cleanup #26190

guitargeek · 2019-03-15T08:20:25Z

PR description:

Going over the RecoParticleFlow/PFProducer and making some changes that should make the particle flow code easier to grasp:

Remove RecoParticleFlow/PFProducer/interface/Utils.h and implementation. The single function that was used from these utilities was also implemented in EgammaElectronAlgos. The implementation there has been moved to EgammaCoreTools such that it can also be used by the particle flow code without introducing a further package dependence. Maybe another location would be more adequate?
Removed simple comparator classes in favour of lambda functions, so one does not have to worry about reading an extra file just for the comparison functions
remove RecoParticleFlow/PFProducer/plugins/EFilter.h(.cc), which has not been used
all plugins that are template instantiations are now declared in plugins.cc as recommended in the new CMSSW code guidelines.

PR validation:

CMSSW compiles and local matrix tests pass.

cmsbuild · 2019-03-15T08:20:55Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-03-15T08:44:49Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26190/8785

This PR adds an extra 96KB to repository

cmsbuild · 2019-03-15T08:45:19Z

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

It involves the following packages:

EgammaAnalysis/ElectronTools
RecoBTag/SoftLepton
RecoEcal/EgammaCoreTools
RecoEgamma/EgammaElectronAlgos
RecoEgamma/EgammaElectronProducers
RecoParticleFlow/Configuration
RecoParticleFlow/PFProducer
RecoParticleFlow/PFTracking

@cmsbuild, @perrotta, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @Sam-Harper, @imarches, @makortel, @smoortga, @jainshilpi, @cbernet, @varuns23, @rovere, @lgray, @HeinerTholen, @JyothsnaKomaragiri, @mverzett, @bachtis, @hatakeyamak, @ferencek, @argiro, @pvmulder, @acaudron, @seemasharmafnal 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-03-15T08:52:29Z

please test

cmsbuild · 2019-03-15T08:52:58Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33608/console Started: 2019/03/15 09:53

cmsbuild · 2019-03-15T11:49:59Z

+1
Tested at: 0d9c3fc
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26190/33608/summary.html

cmsbuild · 2019-03-15T11:50:05Z

Comparison job queued.

makortel · 2019-03-15T14:02:19Z

RecoEcal/EgammaCoreTools/interface/Utils.h

+    while (phi>pi) phi -= pi2;
+    while (phi<-pi) phi += pi2;
+    return phi;
+}


This is redundant, see

cmssw/DataFormats/Math/interface/normalizedPhi.h

Lines 6 to 9 in c24591c

// return a value of phi into interval [-pi,+pi]

template<typename T>

constexpr

T normalizedPhi(T phi) { return reco::reduceRange(phi);}

cmssw/DataFormats/Math/interface/deltaPhi.h

Lines 13 to 20 in c24591c

// reduce to [-pi,pi]

template<typename T>

constexpr T reduceRange(T x) {

constexpr T o2pi = 1./(2.*M_PI);

if (std::abs(x) <= T(M_PI)) return x;

T n = std::round(x*o2pi);

return x - n*T(2.*M_PI);

}

Thank you! I was sure that it must have existed somewhere, just didn't know where.

cmsbuild · 2019-03-15T14:28:35Z

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

Comparison Summary:

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

cmsbuild · 2019-03-23T01:14:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26190/8880

This PR adds an extra 108KB to repository

cmsbuild · 2019-03-23T01:15:04Z

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

slava77 · 2019-03-23T01:29:11Z

@cmsbuild please test

cmsbuild · 2019-03-23T01:29:37Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33730/console Started: 2019/03/23 02:31

cmsbuild · 2019-03-23T04:36:55Z

+1
Tested at: 2529ef9
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26190/33730/summary.html

cmsbuild · 2019-03-23T04:37:01Z

Comparison job queued.

cmsbuild · 2019-03-23T11:22:39Z

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

Comparison Summary:

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

slava77 · 2019-03-25T18:12:52Z

RecoParticleFlow/PFProducer/plugins/kdtrees/KDTreeLinkerTrackHcal.cc

-  setCristalPhiEtaMaxSize(0.2);
-  setPhiOffset(0.32);
+  cristalPhiEtaMaxSize_ = 0.2;
+  phiOffset_ = 0.32;


these should be moved to the member initializer list before the constructor body

That did not work: I think it's because these are members of the base class and not this one. These values could be maybe passed to the constructor of the base class, but then the base constructor would have to be changed, as well as the signature of the corresponding factory and other code. I don't think all of that should be changed in a cleanup PR.

ok, thanks for checking.

slava77 · 2019-03-26T00:07:58Z

+1

for #26190 2529ef9

code changes are in line with the PR description and the follow up review
jenkins tests pass and comparisons with the baseline show no (relevant) differences

santocch · 2019-03-26T08:45:49Z

+1

cmsbuild · 2019-03-26T08:46:14Z

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-03-26T21:29:46Z

+1

cmsbuild added this to the CMSSW_10_6_X milestone Mar 15, 2019

cmsbuild added analysis-pending code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Mar 15, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 15, 2019

cmsbuild added tests-started and removed tests-pending labels Mar 15, 2019

cmsbuild added tests-approved and removed tests-started labels Mar 15, 2019

makortel reviewed Mar 15, 2019

View reviewed changes

cmsbuild added comparison-available code-checks-pending comparison-pending and removed comparison-pending code-checks-approved comparison-available tests-approved labels Mar 15, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 23, 2019

cmsbuild added tests-started and removed tests-pending labels Mar 23, 2019

cmsbuild added tests-approved and removed tests-started labels Mar 23, 2019

cmsbuild added comparison-available and removed comparison-pending labels Mar 23, 2019

slava77 reviewed Mar 25, 2019

View reviewed changes

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Mar 26, 2019

cmsbuild added analysis-approved fully-signed and removed analysis-pending pending-signatures labels Mar 26, 2019

cmsbuild added orp-approved and removed orp-pending labels Mar 26, 2019

cmsbuild merged commit a99c7c9 into cms-sw:master Mar 26, 2019

guitargeek deleted the PFProducer_cleanup branch April 24, 2019 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some RecoParticleFlow/PFProducer cleanup #26190

Some RecoParticleFlow/PFProducer cleanup #26190

guitargeek commented Mar 15, 2019

cmsbuild commented Mar 15, 2019

cmsbuild commented Mar 15, 2019

cmsbuild commented Mar 15, 2019

perrotta commented Mar 15, 2019

cmsbuild commented Mar 15, 2019 •

edited

cmsbuild commented Mar 15, 2019

cmsbuild commented Mar 15, 2019

makortel Mar 15, 2019

guitargeek Mar 15, 2019

cmsbuild commented Mar 15, 2019

cmsbuild commented Mar 23, 2019

cmsbuild commented Mar 23, 2019

slava77 commented Mar 23, 2019

cmsbuild commented Mar 23, 2019 •

edited

cmsbuild commented Mar 23, 2019

cmsbuild commented Mar 23, 2019

cmsbuild commented Mar 23, 2019

slava77 Mar 25, 2019

guitargeek Mar 25, 2019

slava77 Mar 26, 2019

slava77 commented Mar 26, 2019

santocch commented Mar 26, 2019

cmsbuild commented Mar 26, 2019

fabiocos commented Mar 26, 2019

	// return a value of phi into interval [-pi,+pi]
	template<typename T>
	constexpr
	T normalizedPhi(T phi) { return reco::reduceRange(phi);}

	// reduce to [-pi,pi]
	template<typename T>
	constexpr T reduceRange(T x) {
	constexpr T o2pi = 1./(2.*M_PI);
	if (std::abs(x) <= T(M_PI)) return x;
	T n = std::round(x*o2pi);
	return x - nT(2.M_PI);
	}

Some RecoParticleFlow/PFProducer cleanup #26190

Some RecoParticleFlow/PFProducer cleanup #26190

Conversation

guitargeek commented Mar 15, 2019

PR description:

PR validation:

cmsbuild commented Mar 15, 2019

cmsbuild commented Mar 15, 2019

cmsbuild commented Mar 15, 2019

perrotta commented Mar 15, 2019

cmsbuild commented Mar 15, 2019 • edited

cmsbuild commented Mar 15, 2019

cmsbuild commented Mar 15, 2019

makortel Mar 15, 2019

Choose a reason for hiding this comment

guitargeek Mar 15, 2019

Choose a reason for hiding this comment

cmsbuild commented Mar 15, 2019

cmsbuild commented Mar 23, 2019

cmsbuild commented Mar 23, 2019

slava77 commented Mar 23, 2019

cmsbuild commented Mar 23, 2019 • edited

cmsbuild commented Mar 23, 2019

cmsbuild commented Mar 23, 2019

cmsbuild commented Mar 23, 2019

slava77 Mar 25, 2019

Choose a reason for hiding this comment

guitargeek Mar 25, 2019

Choose a reason for hiding this comment

slava77 Mar 26, 2019

Choose a reason for hiding this comment

slava77 commented Mar 26, 2019

santocch commented Mar 26, 2019

cmsbuild commented Mar 26, 2019

fabiocos commented Mar 26, 2019

cmsbuild commented Mar 15, 2019 •

edited

cmsbuild commented Mar 23, 2019 •

edited