Make ElectronMVA usable in FWlite again #25337

guitargeek · 2018-11-26T02:04:32Z

Hi,

in the discussion about issue #23676 it became evident that the current electron MVA ID does not work in FWlite/Python. Mainly the reason for this was that the MVA classes have to get additional variables not in the objects from edm::ValueMaps and the objects therefore need to be wrapped in something Ref-like (edm::Ptrs in the current implementation).

To get around this, we propose to add another helper class (MVAVariableHelper) to the Egamma MVA framework which calculates the extra variables and passes is to the MVA classes, which in turn don't need any other event or provenance information no more. The same function used by the new MVAVariableHelper to get the extra electron variables can also used in FWLite. An example will be included in the test code. There is also a new Python wrapper class which makes using the most recent Electron MVAs for 2016 and 2017 in Python very easy.

Furthermore, I cleaned up the MVANtuplizers a bit and more importantly changed the interface of the ConversionTools to not require edm::Handles anymore. Which makes their usage a bit more consistent in FWlite/Python with the other code I wrote.

Local matrix tests pass and I checked the output of the new fwlite test and the ElectronMVANtuplizer for numerical agreement of MVA values. Validation plots here [1]. If one wants to reproduce the comparison, the pt threshold in the ElectronMVANtuplizer has to be set to zero [2] and one could make the comparison plots with this script for example.

I hope everyone is fine with these changes and I don't have to touch the MVA code again any time soon.

Cheers,
Jonas

Shout-out to who discussed in the issue-thread @Dr15Jones, @makortel and of course to @cbernet for the nice discussion and help but in particular the FWlite example!

[1] https://rembserj.web.cern.ch/rembserj/plots/CMSSW_PRs/25337/
[2] https://github.com/cms-sw/cmssw/blob/master/RecoEgamma/ElectronIdentification/plugins/ElectronMVANtuplizer.cc#L424
[3] https://gitlab.cern.ch/snippets/636

cmsbuild · 2018-11-26T02:04:55Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-11-26T02:08:10Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25337/7360

cmsbuild · 2018-11-26T02:08:28Z

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

It involves the following packages:

DQMOffline/Lumi
DQMOffline/Trigger
DQMServices/Examples
EgammaAnalysis/ElectronTools
HLTriggerOffline/SUSYBSM
PhysicsTools/NanoAOD
PhysicsTools/PatAlgos
RecoBTag/SoftLepton
RecoEgamma/EgammaTools
RecoEgamma/ElectronIdentification
RecoEgamma/PhotonIdentification
Validation/RecoEgamma

@perrotta, @andrius-k, @kmaeshima, @schneiml, @monttj, @cmsbuild, @jfernan2, @fgolf, @slava77, @peruzzim can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @jainshilpi, @rappoccio, @HeinerTholen, @varuns23, @seemasharmafnal, @mmarionncern, @JyothsnaKomaragiri, @ahinzmann, @smoortga, @acaudron, @jhgoh, @lgray, @jdolen, @HuguesBrun, @drkovalskyi, @ferencek, @trocino, @rociovilar, @Sam-Harper, @barvic, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @mtosi, @clelange, @battibass, @calderona, @mverzett, @gpetruc, @imarches, @mariadalfonso, @pvmulder, @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-11-26T08:49:21Z

please test

cmsbuild · 2018-11-26T08:49:43Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31843/console Started: 2018/11/26 11:40

Sam-Harper · 2018-11-26T09:46:39Z

@guitargeek , thank you for this, really appreciate it!

cbernet · 2018-11-26T09:53:49Z

Hi all, I really appreciate this as well, and I would like to thank again @guitargeek very much for all the work. Just one thing though, there are a couple very small developments missing before we can apply the mva ID in FWLite. I did a PR to @guitargeek for that (https://github.com/guitargeek/cmssw/pull/1) and we will need to add these developments here (or in a later PR) when we're done.

@lucastorterotot @GaelTouquet

cmsbuild · 2018-11-26T13:55:32Z

+1
Tested at: a850fe3
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25337/31843/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
be75d46
824da84
4d72114
cb9fe52
a6d0e0c
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25337/31843/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25337/31843/git-merge-result

cmsbuild · 2018-11-26T13:55:36Z

Comparison job queued.

cmsbuild · 2018-11-26T15:43:10Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 1 differences found in the comparisons
DQMHistoTests: Total files compared: 33
DQMHistoTests: Total histograms compared: 3131839
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3131633
DQMHistoTests: Total skipped: 204
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

cmsbuild · 2018-12-04T18:03:10Z

+1
Tested at: 4e1c155
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25337/31986/summary.html

cmsbuild · 2018-12-04T18:03:15Z

Comparison job queued.

cmsbuild · 2018-12-04T20:18:18Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25337/31986/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: 3131939
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3131733
DQMHistoTests: Total skipped: 204
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 · 2018-12-04T21:55:24Z

I quickly glanced at the timing performance of the new code. With the TTbar PU workflow 25202 I could verify that there is indeed also evidence of some little optimization with this code, although the modules involved were not large CPU consumers even before:

  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
     removed      -0.00%         0.04 ms/ev ->         0.00 ms/ev electronMVAVariableHelper
   -0.514217      -0.01%         1.15 ms/ev ->         0.68 ms/ev gedGsfElectrons
   -0.261770      -0.00%         1.44 ms/ev ->         1.11 ms/ev electronMVAValueMapProducer
   -0.218485      -0.00%         1.01 ms/ev ->         0.81 ms/ev gedPhotons
  ---------- ------------     --------                  ----       ------------
Job total:  8.7089 s/ev ==> 8.56415 s/ev

perrotta · 2018-12-04T21:59:52Z

+1

The updates to the code clean it a bit and fulfil the purpose reported in the PR description of making ElectronMVA usable in FWlite, as demonstrated by the validation plots provided
This goes together with an overall optimization of the code itself
Jenkins tests pass, and show no differences in the reco output, as it was in the purpose of this PR

andrius-k · 2018-12-05T15:06:54Z

+1

fabiocos · 2018-12-06T10:04:25Z

@peruzzim @fgolf this PR is touching also the nanoAOD configuration, please check that it is not interfering with your developments, and sign it in case

fabiocos · 2018-12-09T10:33:55Z

@peruzzim @fgolf do I understand correctly that this is ok for you?

fabiocos · 2018-12-11T09:51:34Z

+1

the fwlite test provided runs smoothly

cmsbuild · 2018-12-11T09:51:57Z

Pull request #25337 was updated. @fgolf, @peruzzim can you please check and sign again.

fabiocos · 2018-12-11T09:52:00Z

merge

@peruzzim @fgolf please note this merge affects you

fabiocos · 2018-12-11T10:03:15Z

@peruzzim @fgolf now that the xpog category has been assigned, please sign it for reference

peruzzim · 2018-12-11T10:08:30Z

+xpog

for the modification to nano_cff.py, as NANOAOD workflows run and show no difference

cmsbuild added this to the CMSSW_10_4_X milestone Nov 26, 2018

cmsbuild added analysis-pending code-checks-pending comparison-pending dqm-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Nov 26, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 26, 2018

cmsbuild added tests-started and removed tests-pending labels Nov 26, 2018

cmsbuild added tests-approved and removed tests-started labels Nov 26, 2018

cmsbuild added comparison-available and removed comparison-pending code-checks-approved comparison-available tests-approved labels Nov 26, 2018

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

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

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Dec 4, 2018

cmsbuild added dqm-approved and removed dqm-pending labels Dec 5, 2018

cmsbuild added orp-approved xpog-pending and removed orp-pending labels Dec 11, 2018

cmsbuild merged commit 77915b6 into cms-sw:master Dec 11, 2018

cmsbuild added xpog-approved and removed xpog-pending labels Dec 11, 2018

guitargeek mentioned this pull request Dec 13, 2018

Features needed for NanoAOD processing in 10_2_X (December 2018) #25425

Closed

4 tasks

guitargeek deleted the FWLite_MVA_master branch January 11, 2019 13:35

GaelTouquet mentioned this pull request Apr 12, 2019

2017 Electron ID v2 CERN-PH-CMG/cmg-cmssw#749

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ElectronMVA usable in FWlite again #25337

Make ElectronMVA usable in FWlite again #25337

guitargeek commented Nov 26, 2018 •

edited

cmsbuild commented Nov 26, 2018

cmsbuild commented Nov 26, 2018

cmsbuild commented Nov 26, 2018

perrotta commented Nov 26, 2018

cmsbuild commented Nov 26, 2018 •

edited

Sam-Harper commented Nov 26, 2018

cbernet commented Nov 26, 2018

cmsbuild commented Nov 26, 2018

cmsbuild commented Nov 26, 2018

cmsbuild commented Nov 26, 2018

cmsbuild commented Dec 4, 2018

cmsbuild commented Dec 4, 2018

cmsbuild commented Dec 4, 2018

perrotta commented Dec 4, 2018

perrotta commented Dec 4, 2018

andrius-k commented Dec 5, 2018

fabiocos commented Dec 6, 2018

fabiocos commented Dec 9, 2018

fabiocos commented Dec 11, 2018

cmsbuild commented Dec 11, 2018

fabiocos commented Dec 11, 2018

fabiocos commented Dec 11, 2018

peruzzim commented Dec 11, 2018

Make ElectronMVA usable in FWlite again #25337

Make ElectronMVA usable in FWlite again #25337

Conversation

guitargeek commented Nov 26, 2018 • edited

cmsbuild commented Nov 26, 2018

cmsbuild commented Nov 26, 2018

cmsbuild commented Nov 26, 2018

perrotta commented Nov 26, 2018

cmsbuild commented Nov 26, 2018 • edited

Sam-Harper commented Nov 26, 2018

cbernet commented Nov 26, 2018

cmsbuild commented Nov 26, 2018

cmsbuild commented Nov 26, 2018

cmsbuild commented Nov 26, 2018

cmsbuild commented Dec 4, 2018

cmsbuild commented Dec 4, 2018

cmsbuild commented Dec 4, 2018

perrotta commented Dec 4, 2018

perrotta commented Dec 4, 2018

andrius-k commented Dec 5, 2018

fabiocos commented Dec 6, 2018

fabiocos commented Dec 9, 2018

fabiocos commented Dec 11, 2018

cmsbuild commented Dec 11, 2018

fabiocos commented Dec 11, 2018

fabiocos commented Dec 11, 2018

peruzzim commented Dec 11, 2018

guitargeek commented Nov 26, 2018 •

edited

cmsbuild commented Nov 26, 2018 •

edited