Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove PFElectronAlgo and PFPhotonAlgo used back in Run 1 #26813

Merged
merged 6 commits into from May 21, 2019
Merged

Remove PFElectronAlgo and PFPhotonAlgo used back in Run 1 #26813

merged 6 commits into from May 21, 2019

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented May 16, 2019

PR description:

The old egamma particle flow code has not been used for 5 years but still is strongly entangled with the current particle flow code, which poses an unnecessary maintenance burden. As discussed with @bendavid, @Sam-Harper, @slava77 and @hatakeyamak, a clean-up of the legacy PF egamma code is on our todo list for the 11XY cycle.

I keep things minimal for now so it can be easily reviewed and keep the risk of differences minimal:

  • remove the PFElectronAlgo and PFPhotonAlgo classes
  • remove PFAlgo code that requires any of the two classes (could easily be spotted by usePFPhotons_ or usePFPElectrons_)
  • remove the configuration for these algos from PFProducer
  • in passing, merge the PFProducer.h file with the PFProducer.cc file

Of course what's still missing is cleaning up the configurations from now unused parameters, of better even implement a fillDescriptions method for the PFProducer. However, this would require editing various HLT configs which are huge and tedious to edit, so I think that action could be for another PF (or this one after initial tests pass and no differences are observed).

Fixes #26658.

PR validation:

CMSSW compiles and matrix tests pass.

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

No backport is intended.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26813/9828

  • This PR adds an extra 48KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

RecoEgamma/Configuration
RecoParticleFlow/PFProducer

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

@slava77
Copy link
Contributor

slava77 commented May 16, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 16, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/286/console Started: 2019/05/17 00:35

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

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

@Sam-Harper
Copy link
Contributor

happy days :D Thanks Jonas!

@perrotta
Copy link
Contributor

@guitargeek

What about cleaning since now from the un-needed parameters also the particleFlowTmp config in RecoParticleFlow/PFProducer/python/particleFlow_cfi.py?

This will get automatically propagated to the HLT configs when they will be parsed next (no need to touch them, therefore: since there is no fillDescritpion in this code, the unneeded parameters remaining in the HLT configs in the meanwhile will not hurt, and there will be no need to customize them, then).

I think you should further remove those old parameters from the configs which clones that particleFlowTmp. If the list in https://cmssdt.cern.ch/lxr/search?%21v=CMSSW_11_0_X_2019-05-19-2300&_filestring=&_string=particleFlowTmp is complete they should be just a few:

  • RecoHI/Configuration/python/Reconstruction_hiPF_cff.py
  • RecoHI/Configuration/python/customise_PF.py
  • RecoJets/JetProducers/python/hltParticleFlowForJets_cfi.py
  • RecoParticleFlow/Configuration/test/RecoToDisplay_cfg.py

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26813/9881

  • This PR adds an extra 68KB to repository

@cmsbuild
Copy link
Contributor

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

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 21, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/363/console Started: 2019/05/21 07:07

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

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

@perrotta
Copy link
Contributor

+1

  • Removal of unused parts of code and configs
  • Jenkins tests pass and show no differences

Once merged, this will affect #26825, as part of the code in PFAlgo.cc which is modified in that PR is removed here. Since one of the two PR's has to be rebased anyhow, I think it it is easier if one first do the removal, and then the modification will be avoided in the other PR

@cmsbuild
Copy link
Contributor

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)

@guitargeek
Copy link
Contributor Author

Thank you Andrea, that is indeed the easiest way and also what we discussed in jpata#44.

@fabiocos
Copy link
Contributor

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleaning up Run 1 EGamma ParticleFlow code
6 participants