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

Egamma IDs back to global cache #25101

Merged
merged 6 commits into from Nov 14, 2018
Merged

Egamma IDs back to global cache #25101

merged 6 commits into from Nov 14, 2018

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Nov 2, 2018

This basically a follow up to #23767, where I fixed a thread-safety issue in the new Egamma MVA ID implementation by just moving the IDs out of the global cache of the producer.

I did not consider this as a permanent solution, because the MVA IDs with the large GBRForests should indeed not be replicated for each thread. Here I propose:

  • To simply promote the MVAValueMapProducer to a global EDProducer to not duplicate the GBRForests
  • To wrap the employed StringCutObjectSelector<> and StringCutObjectFunction<> into a little class together with a std::mutex to make their operator() thread safe
  • Make the MultiToken thread safe so it can be used in global producers

The matrix tests pass and the changes were also a little bit battle tested with the multi-threaded ntuplizer in my MultileptonAnalysis.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2018

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

It involves the following packages:

RecoEgamma/EgammaTools
RecoEgamma/ElectronIdentification
RecoEgamma/PhotonIdentification

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@jainshilpi, @Sam-Harper, @varuns23, @lgray 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 Nov 2, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31438/console Started: 2018/11/02 12:35

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2018

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25101/31438/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: 2993155
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2992956
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 134 log files, 14 edm output root files, 32 DQM output files

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 6, 2018

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

@perrotta
Copy link
Contributor

perrotta commented Nov 6, 2018

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 6, 2018

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 6, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 6, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 6, 2018

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

Comparison Summary:

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

@perrotta
Copy link
Contributor

perrotta commented Nov 9, 2018

+1

  • It makes MVAValueMapProducer a global EDProducer
  • LIVE_MEMORY footprint does not scale any more with the number of threads for those MVAValueMapProducer's (with apparently a larger improvement for electrons than for photons)
  • Peak memory does not seem significantly affected overall, as well as timing
  • Jenkins tests pass and show no differences

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2018

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
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 8ff77ae into cms-sw:master Nov 14, 2018
@guitargeek guitargeek deleted the 10_4_x_dev branch November 14, 2018 10:16
@guitargeek guitargeek restored the 10_4_x_dev branch November 21, 2018 14:38
lucastorterotot pushed a commit to lucastorterotot/cmssw that referenced this pull request Nov 27, 2018
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
@guitargeek guitargeek deleted the 10_4_x_dev branch December 9, 2018 20:39
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.

None yet

5 participants