Technical optimisation of e/gamma (local)covariances function (11_3_X) #33270

swagata87 · 2021-03-25T14:03:53Z

PR description:

This PR is a follow-up of my previous PR #33148, which was merged recently. In particular, I'm addressing this comment which was raised in the last RECO meeting. So, as requested by @VinInn, this PR changes the return type of localCovariances, scLocalCovariances, and covariances functions from std::vector<float> to std::array<float,3>. As only 3 floats are returned, array is a better and more memory-efficient choice.

This is a purely technical PR, and no change in physics is expected.

PR validation:

runTheMatrix.py successfully ran with 23234.0, 140.53, and 11634.0

This PR is not a backport.
No backport is needed.

cmsbuild · 2021-03-25T14:09:05Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33270/21764

This PR adds an extra 120KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33270/21764/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33270/21764/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

Martin-Grunewald · 2021-03-25T14:13:01Z

please test

VinInn · 2021-03-25T14:13:23Z

RecoEcal/EgammaCoreTools/interface/EcalClusterTools.h

-  v.push_back(covEtaEta);
-  v.push_back(covEtaPhi);
-  v.push_back(covPhiPhi);
+  std::array<float,3> v;


brace initialization?

VinInn · 2021-03-25T14:13:50Z

RecoEcal/EgammaCoreTools/interface/EcalClusterTools.h

-  v.push_back(covEtaEta);
-  v.push_back(covEtaPhi);
-  v.push_back(covPhiPhi);
+  std::array<float,3> v;


VinInn · 2021-03-25T14:19:03Z

RecoEcal/EgammaCoreTools/test/testEcalClusterLazyTools.cc

@@ -102,9 +102,9 @@ void testEcalClusterLazyTools::analyze(const edm::Event& ev, const edm::EventSet
    std::cout << std::endl;
    std::vector<float> vLat = lazyTools.lat(clus);
    std::cout << "lat...................... " << vLat[0] << " " << vLat[1] << " " << vLat[2] << std::endl;
-    std::vector<float> vCov = lazyTools.covariances(clus);
+    std::array<float,3> vCov = lazyTools.covariances(clus);


here and everywhere else below one could use auto instead of the explicit type (btw if auto was used in first place no change was required!)
This is somehow stylistic (I prefer auto) so I leave to reco-conv. too say the final word

const auto& vCov = lazyTools.covariances(clus);

seems like a good choice?

cmsbuild · 2021-03-25T14:24:07Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33270/21765

This PR adds an extra 224KB to repository

cmsbuild · 2021-03-25T14:24:34Z

A new Pull Request was created by @swagata87 (Swagata Mukherjee) for master.

It involves the following packages:

DQMOffline/Ecal
DQMOffline/Trigger
EgammaAnalysis/ElectronTools
PhysicsTools/PatAlgos
RecoEcal/EgammaClusterAlgos
RecoEcal/EgammaCoreTools
RecoEgamma/EgammaElectronAlgos
RecoEgamma/EgammaHLTProducers
RecoEgamma/EgammaPhotonProducers
RecoEgamma/EgammaTools
RecoEgamma/PhotonIdentification

@perrotta, @cmsbuild, @andrius-k, @kmaeshima, @fwyzard, @ErnestaP, @ahmad3213, @Martin-Grunewald, @jfernan2, @slava77, @jpata, @santocch, @rvenditti can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @jainshilpi, @hatakeyamak, @emilbols, @argiro, @thomreis, @mbluj, @varuns23, @seemasharmafnal, @mmarionncern, @JyothsnaKomaragiri, @ahinzmann, @jhgoh, @lgray, @jdolen, @sscruz, @cericeci, @simonepigazzini, @ferencek, @trocino, @rociovilar, @Sam-Harper, @rovere, @jdamgov, @missirol, @nhanvtran, @gkasieczka, @schoef, @mtosi, @andrzejnovak, @clelange, @HuguesBrun, @rchatter, @swozniewski, @Fedespring, @calderona, @sobhatta, @lecriste, @afiqaize, @gpetruc, @mariadalfonso, @ram1123 this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

Martin-Grunewald · 2021-03-25T16:57:21Z

please test

cmsbuild · 2021-03-26T13:48:42Z

Pull request #33270 was updated. @perrotta, @cmsbuild, @andrius-k, @kmaeshima, @fwyzard, @ErnestaP, @ahmad3213, @Martin-Grunewald, @jfernan2, @slava77, @jpata, @santocch, @rvenditti can you please check and sign again.

jpata · 2021-03-26T13:58:10Z

@cmsbuild please test

cmsbuild · 2021-03-26T16:55:31Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-62800e/13793/summary.html
COMMIT: 9a90148
CMSSW: CMSSW_11_3_X_2021-03-26-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33270/13793/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: 7 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2639935
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2639900
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

jpata · 2021-03-29T08:33:55Z

+reconstruction

technical, EcalClusterLazyTools::covariances and localCovariances return array instead of a vector
no changes expected or observed

Thank you for this follow-up, @swagata87, and for @VinInn for pointing this out!

jfernan2 · 2021-03-29T10:51:25Z

+1

Martin-Grunewald · 2021-03-29T15:38:05Z

+1

santocch · 2021-03-30T19:42:21Z

+1

cmsbuild · 2021-03-30T19:42:43Z

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

qliphy · 2021-03-31T01:23:48Z

+1

move from vector to array for (local)covariances

3d9bd2c

cmsbuild added this to the CMSSW_11_3_X milestone Mar 25, 2021

cmsbuild added analysis-pending code-checks-pending dqm-pending hlt-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Mar 25, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Mar 25, 2021

VinInn reviewed Mar 25, 2021

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Mar 25, 2021

VinInn reviewed Mar 25, 2021

View reviewed changes

code check

100dd11

cmsbuild added code-checks-pending tests-pending and removed code-checks-rejected tests-started labels Mar 25, 2021

VinInn reviewed Mar 25, 2021

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 25, 2021

cmsbuild removed the tests-pending label Mar 25, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 26, 2021

cmsbuild added tests-started and removed tests-pending labels Mar 26, 2021

cmsbuild added tests-approved and removed tests-started labels Mar 26, 2021

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Mar 29, 2021

cmsbuild added dqm-approved and removed dqm-pending labels Mar 29, 2021

cmsbuild added hlt-approved and removed hlt-pending labels Mar 29, 2021

cmsbuild added analysis-approved fully-signed and removed analysis-pending pending-signatures labels Mar 30, 2021

cmsbuild added orp-approved and removed orp-pending labels Mar 31, 2021

cmsbuild merged commit bf9409a into cms-sw:master Mar 31, 2021

This was referenced Mar 31, 2021

Refactoring RootFile #33315

Merged

removing data files from interface #33317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Technical optimisation of e/gamma (local)covariances function (11_3_X) #33270

Technical optimisation of e/gamma (local)covariances function (11_3_X) #33270

swagata87 commented Mar 25, 2021

cmsbuild commented Mar 25, 2021

Martin-Grunewald commented Mar 25, 2021

VinInn Mar 25, 2021

VinInn Mar 25, 2021

VinInn Mar 25, 2021

jpata Mar 25, 2021

cmsbuild commented Mar 25, 2021

cmsbuild commented Mar 25, 2021

Martin-Grunewald commented Mar 25, 2021

cmsbuild commented Mar 26, 2021

jpata commented Mar 26, 2021

cmsbuild commented Mar 26, 2021

jpata commented Mar 29, 2021

jfernan2 commented Mar 29, 2021

Martin-Grunewald commented Mar 29, 2021

santocch commented Mar 30, 2021

cmsbuild commented Mar 30, 2021

qliphy commented Mar 31, 2021

Technical optimisation of e/gamma (local)covariances function (11_3_X) #33270

Technical optimisation of e/gamma (local)covariances function (11_3_X) #33270

Conversation

swagata87 commented Mar 25, 2021

PR description:

PR validation:

cmsbuild commented Mar 25, 2021

Martin-Grunewald commented Mar 25, 2021

VinInn Mar 25, 2021

Choose a reason for hiding this comment

VinInn Mar 25, 2021

Choose a reason for hiding this comment

VinInn Mar 25, 2021

Choose a reason for hiding this comment

jpata Mar 25, 2021

Choose a reason for hiding this comment

cmsbuild commented Mar 25, 2021

cmsbuild commented Mar 25, 2021

Martin-Grunewald commented Mar 25, 2021

cmsbuild commented Mar 26, 2021

jpata commented Mar 26, 2021

cmsbuild commented Mar 26, 2021

Comparison Summary

jpata commented Mar 29, 2021

jfernan2 commented Mar 29, 2021

Martin-Grunewald commented Mar 29, 2021

santocch commented Mar 30, 2021

cmsbuild commented Mar 30, 2021

qliphy commented Mar 31, 2021