Tackled majority of RecoEgamma code quality issues #23743

guitargeek · 2018-07-04T14:36:14Z

As advertised by @slava77 and @perrotta in the Reconstruction and Ananlysis Tools meetings, I took my "favourite" CMSSW packages (RecoEgamma/*) and addressed the complaints of the static analyser [1] which originate from RecoEgamma code.

The issues were mainly:

Inherits from edm::EDProducer,edm::EDFilter,edm::EDAnalyzer, or edm::OutputModule
Related to code which was actually outside RecoEgamma (most if not all const_cast issues)
A few dead assignments [2,3] and a branch condition evaluations to a garbage value [4]

Here, I propose fixes for the legacy modules and the small things in 3. The strategy was to use global::EDProducer if possible, stream::EDProducer where altering the class members in produce() could not trivially be avoided (two cases) and one::EDAnalyzer for the analysers where I was afraid that the parallelization would maybe break them (but they are not so important anyway).

In total, this fixes about 50 static analyzer issues. I tested locally, and the workflows without DAS errors succeeded.

[1] https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-static-analysis/CMSSW_10_2_X_2018-07-03-1100/slc6_amd64_gcc630/llvm-analysis/index.html
[2] https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-static-analysis/CMSSW_10_2_X_2018-07-03-1100/slc6_amd64_gcc630/llvm-analysis/report-b91ab3.html#EndPath
[3] https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-static-analysis/CMSSW_10_2_X_2018-07-03-1100/slc6_amd64_gcc630/llvm-analysis/report-d77667.html#EndPath
[4] https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-static-analysis/CMSSW_10_2_X_2018-07-03-1100/slc6_amd64_gcc630/llvm-analysis/report-2a785d.html#EndPath

cmsbuild · 2018-07-04T14:36:35Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-07-04T14:40:38Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23743/5414

cmsbuild · 2018-07-04T14:41:01Z

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

It involves the following packages:

RecoEgamma/EgammaElectronProducers
RecoEgamma/EgammaHFProducers
RecoEgamma/EgammaHLTProducers
RecoEgamma/EgammaIsolationAlgos
RecoEgamma/EgammaTools
RecoEgamma/Examples
RecoEgamma/PhotonIdentification

@perrotta, @cmsbuild, @silviodonato, @slava77, @Martin-Grunewald, @fwyzard can you please review it and eventually sign? Thanks.
@Sam-Harper, @battibass, @jainshilpi, @rovere, @lgray, @calderona, @HuguesBrun, @varuns23, @folguera 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 · 2018-07-04T14:53:15Z

please test

cmsbuild · 2018-07-04T14:53:34Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/29004/console Started: 2018/07/04 17:11

cmsbuild · 2018-07-04T18:18:49Z

+1
Tested at: 4cf1c6f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-23743/29004/summary.html

cmsbuild · 2018-07-04T18:18:51Z

Comparison job queued.

cmsbuild · 2018-07-04T19:28:28Z

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

Comparison Summary:

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

perrotta · 2018-07-13T08:51:33Z

RecoEgamma/EgammaHLTProducers/src/EgammaHLTElectronDetaDphiProducer.cc

    }
-  } else if (useTrackProjectionToEcal_ or recoveryForFailingPropagation) { 


(Just checking)
While the original formulation was clearly bugged and your fix preserves in a cleaner way the previous behaviour, are you sure that the original intention of the author here was not having insted (e.g.):

if (useTrackProjectionToEcal_ and recoveryForFailingPropagation) {

?

I'm actually happy with deleting that file. Its out of date and using it is an error. It'll be deleted as part of the big clean up anyways.

Thank you Sam.
Then why not to profit of this PR to clean up from all those useless and outdated classes? Or at least the ones included here (you can send the list to Jonas).
Or do you have a big clean-up ahead, and it will be integrated independently?

will see if I have time to look at which ones should be deleted. Sadly a bit busy right now with other things, such as HEM15/16.

So that means we just leave it like that and Sam does his cleaning later? Anyway, I took the opportunity to further clean RecoEgamma/ElectronIdentification a little bit.

cmsbuild · 2018-07-24T16:40:41Z

+1
Tested at: 8755bb3
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-23743/29397/summary.html

cmsbuild · 2018-07-24T16:40:44Z

Comparison job queued.

cmsbuild · 2018-07-24T17:54:03Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 31
DQMHistoTests: Total histograms compared: 2889291
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2889099
DQMHistoTests: Total skipped: 190
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
Checked 128 log files, 14 edm output root files, 31 DQM output files

perrotta · 2018-07-24T19:28:52Z

+1

Lot of issues found by the static analyzer are fixed: thank you Jonas!
Some cleaning in the code
Modules are made multithread aware and compliant
- 800 events run with a 2018 workflow, both single threaded and with 8 threads: nothing crashed in the multithreaded run, and the reco output was identically the same as for the single threaded run.
- At least for the modules run in production there seem to be no thread issues after this PR

Martin-Grunewald · 2018-07-26T08:19:53Z

+1

cmsbuild · 2018-07-26T08:20:16Z

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)

kpedro88 · 2018-07-27T15:31:10Z

+1

Electron MVA V2 (as in cms-sw#23700 cms-sw#23746 cms-sw#23767) All event based variables are now handeled by the new helper class Hardcoding of variables and clips removed for Fall17 Generalized Fall17Iso and Fall17NoIso Configurable categories and start to move Spring16 MVA Dedicated variable files for Spring16 and Fall17 Removed Spring16 C++ code All year specific C++ code removed Small improvements Cleared some inconsistencies in variable definitions. All IDs now validated. Restructured VID python code a bit New MVAVariableManager helper class Forgot to actually add the class go git The raw BDT score is now stored as well Spring15 PhotonMVA uses now MVAVariableManager Almost there for photon MVA All Run2 Photon MVA IDs use now the new PhotonMVAEstimator class Add variable files Rewrote some python, ElectronID cuts on raw MWA values and autodetection of gzipped weight files Implemented V2 MV Ele ID Cut values are now found with parser as well Include ntupizer for training cleaned up a bit Training Ntuplizer finished Use auto generatetd config files Finalized ntuplizer Auto updated Training ntuplizer Updated training ntuplizer Fix in training ntuplizer fixed wp90 iso V1 parameters Fix Added MVA output Validation ntuplizer for photons Bugfix Improved exception handling in GBRForestToors Changed egammaObjectModificationsInMiniAOD_cff.py to comply with new config files Add two lines in miniAOD_tools.py to cope with ElectronMVAVariableHelper Fixed Fall17V1 variables Addressed comments following visual code inspection Simplified code Included Fall17 MVAs in MiniAOD and NanoAOD Added electronMVAVariableHelper to relevant processes Revisited casting of candidates to electrons/photons Fixup - prefer dynamic_cast if possible Corrected the second C style cast in the photons Avoid undefined category for photons Changed category not defined error to warning Changed mvaValue if category not found to -999 Fixed possible memory leak Drop MVAObjectCache in MVAValueMapProducer Commit after some code review Comment out something so branch compiles Added weightfiles Some code cleaning (as in ID part of cms-sw#23743) Adapt GBRForestTools exception handling to 94X Fall17 cutbased Electron ID V2 (as in cms-sw#23477) Added Photon MVA weight files Technical Egamma MVA improvements (as in cms-sw#24131) Fall17_94X_v2 MVA Photon ID Fixing mvaClassName in egammaObjectModificationsInMiniAOD_cff.py Inclusion of V2 IDs in MiniAOD fix worst charged iso variable adding photon MVA V2 to the pat::Photon Fixed copy-paste typo in Spring15 weight files New MultiTokens (as in cms-sw#24312 cms-sw#24423) cache AOD/miniAOD values to speed up repeated computations Egamma IDs back to global cache (as in cms-sw#25101) Cutbased photonID Fall17 V2 update phoID v2 cutbased Renamed cut based ID and put in MiniAOD Modified PhotonNtuplizer to have genMatch information Added variables from Fall17 ID to PhotonMVANtuplizer New MVAVariableHelper for extra MVA variables Moved Photon MVA to new interface adapted constructors to run MVA estimator in python fwlite ElectronMVA can be used in FWlite Egamma Python wrapper class for Electron MVAs Got rid of ElectronMVAVariableHelper Adapted PhotonMVANtuplizer Combined Electron Ntuplizer config files Fixing problems detected while testing exposing category in FWLite to be able to apply working point later on infrastructure to take working points in python+fwlite. implemented for noiso v2 only so far working points for the other mvas. somthing fishy with GP v1 Spring16 WPs with logistic transform

cmsbuild added this to the CMSSW_10_2_X milestone Jul 4, 2018

cmsbuild added code-checks-pending comparison-pending hlt-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jul 4, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 4, 2018

cmsbuild added tests-started and removed tests-pending labels Jul 4, 2018

cmsbuild added tests-approved and removed tests-started labels Jul 4, 2018

cmsbuild added comparison-available and removed comparison-pending labels Jul 4, 2018

perrotta reviewed Jul 13, 2018

View reviewed changes

guitargeek added 5 commits July 13, 2018 11:55

Tackled majority of RecoEgamma code quality issues

82fe330

Removed obsolete NeuralNet class and moved estimators to GBRForest Tools

5778e46

Removed non-functional unused function

f8d2900

Cleaned remnants of ElectronNeuralNet and cleaned MVANtuplizer

86ce909

Removing MVAObjectCache

11892b2

cmsbuild added tests-started and removed tests-pending labels Jul 24, 2018

cmsbuild added tests-approved and removed tests-started labels Jul 24, 2018

cmsbuild added comparison-available and removed comparison-pending labels Jul 24, 2018

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jul 24, 2018

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Jul 26, 2018

cmsbuild added orp-approved and removed orp-pending labels Jul 27, 2018

cmsbuild merged commit 5bfa1f7 into cms-sw:master Jul 27, 2018

guitargeek deleted the RecoEgamma_CodeQuality branch July 27, 2018 16:08

Sam-Harper pushed a commit to Sam-Harper/cmssw that referenced this pull request Oct 12, 2018

Some code cleaning (as in ID part of cms-sw#23743)

9f69708

guitargeek mentioned this pull request Aug 31, 2019

Cleanup of EgammaHLTAlgos/Producers #27909

Merged

qliphy mentioned this pull request Jul 1, 2021

L1T Phase-1: Adding L1boosted objects #34235

Merged

This was referenced Jul 28, 2021

Introduce L1TMuonBarrelKalmanParams and Record #34494

Merged

Migrate ECAL Validation codes to ESConsumes #34637

Merged

jfernan2 mentioned this pull request Aug 5, 2021

Update DT DQM, calibration scripts, and local reco code to allow using "new" DB format #34612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tackled majority of RecoEgamma code quality issues #23743

Tackled majority of RecoEgamma code quality issues #23743

guitargeek commented Jul 4, 2018 •

edited

cmsbuild commented Jul 4, 2018

cmsbuild commented Jul 4, 2018

cmsbuild commented Jul 4, 2018

perrotta commented Jul 4, 2018

cmsbuild commented Jul 4, 2018 •

edited

cmsbuild commented Jul 4, 2018

cmsbuild commented Jul 4, 2018

cmsbuild commented Jul 4, 2018

perrotta Jul 13, 2018 •

edited

Sam-Harper Jul 13, 2018

perrotta Jul 13, 2018

Sam-Harper Jul 13, 2018

guitargeek Jul 13, 2018

cmsbuild commented Jul 24, 2018

cmsbuild commented Jul 24, 2018

cmsbuild commented Jul 24, 2018

perrotta commented Jul 24, 2018

Martin-Grunewald commented Jul 26, 2018

cmsbuild commented Jul 26, 2018

kpedro88 commented Jul 27, 2018

		}
		} else if (useTrackProjectionToEcal_ or recoveryForFailingPropagation) {

Tackled majority of RecoEgamma code quality issues #23743

Tackled majority of RecoEgamma code quality issues #23743

Conversation

guitargeek commented Jul 4, 2018 • edited

cmsbuild commented Jul 4, 2018

cmsbuild commented Jul 4, 2018

cmsbuild commented Jul 4, 2018

perrotta commented Jul 4, 2018

cmsbuild commented Jul 4, 2018 • edited

cmsbuild commented Jul 4, 2018

cmsbuild commented Jul 4, 2018

cmsbuild commented Jul 4, 2018

perrotta Jul 13, 2018 • edited

Choose a reason for hiding this comment

Sam-Harper Jul 13, 2018

Choose a reason for hiding this comment

perrotta Jul 13, 2018

Choose a reason for hiding this comment

Sam-Harper Jul 13, 2018

Choose a reason for hiding this comment

guitargeek Jul 13, 2018

Choose a reason for hiding this comment

cmsbuild commented Jul 24, 2018

cmsbuild commented Jul 24, 2018

cmsbuild commented Jul 24, 2018

perrotta commented Jul 24, 2018

Martin-Grunewald commented Jul 26, 2018

cmsbuild commented Jul 26, 2018

kpedro88 commented Jul 27, 2018

guitargeek commented Jul 4, 2018 •

edited

cmsbuild commented Jul 4, 2018 •

edited

perrotta Jul 13, 2018 •

edited