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

Implementation of Fall17 cutbased Electron ID V2 #23477

Merged
merged 11 commits into from Jun 21, 2018

Conversation

tomcornelis
Copy link
Contributor

@tomcornelis tomcornelis commented Jun 5, 2018

This pull request contains:

  • Fall17 effective areas based on 94X samples
  • Introduction of scaled pfRelIso cut
  • Implementation of Fall17 cutbased ElectronID workingpoints V2

Link to slides of latest update (30th of May 2018):
https://indico.cern.ch/event/732971/contributions/3022843/attachments/1658685/2656462/eleIdTuning.pdf

Earlier updates preceding the final update:
https://indico.cern.ch/event/697079/contributions/2968124/attachments/1632990/2604194/eleIdTuning.pdf
https://indico.cern.ch/event/697576/contributions/2940576/attachments/1620927/2578913/eleIdTuning.pdf

@Sam-Harper
@lsoffi
@guitargeek

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2018

A new Pull Request was created by @UAEDF-tomc (Tom Cornelis) for master.

It involves the following packages:

RecoEgamma/ElectronIdentification

@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 Jun 5, 2018

@cmsbuild please test

@UAEDF-tomc
please add a link to slides presented in an EGM meeting that summarize performance expected from this PR
Thank you.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28478/console Started: 2018/06/05 14:06

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2018

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-23477/28478/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: 2902471
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2902280
  • 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

@@ -0,0 +1,8 @@
# |eta| min |eta| max effective area
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add some documentation comments similar to effAreaElectrons_cone03_pfNeuHadronsAndPhotons_92X

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

const float _barrelC0, _endcapC0, _barrelCpt, _endcapCpt, _barrelCutOff;
bool _isRelativeIso;
EffectiveAreas _effectiveAreas;
edm::Handle<double> _rhoHandle;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do not use leading underscore in variable names (trailing is good)
https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/CodingAndStyleRules.pdf

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(note that in my original commit I had the leading underscores because I based my implementation on the already existing GsfEleEffAreaPFIsoCut.cc in the same directory, which also had leading underscores)

#central_id_registry.register(cutBasedElectronID_Fall17_94X_V1_veto.idName, '43be9b381a8d9b0910b7f81a5ad8ff3a')
#central_id_registry.register(cutBasedElectronID_Fall17_94X_V1_loose.idName, '0b8456d622494441fe713a6858e0f7c1')
#central_id_registry.register(cutBasedElectronID_Fall17_94X_V1_medium.idName, 'a238ee70910de53d36866e89768500e9')
#central_id_registry.register(cutBasedElectronID_Fall17_94X_V1_tight.idName, '4acb2d2796efde7fba75380ce8823fc2')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the comments above imply that these lines with MD5 hash values should be uncommented

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@slava77
Copy link
Contributor

slava77 commented Jun 8, 2018

@UAEDF-tomc
I see that the new IDs are not enabled in the standard workflows (mini or nano). when (or why not now) ?

@slava77
Copy link
Contributor

slava77 commented Jun 8, 2018

#23477 (comment)

Link to slides of latest update (30th of May 2018):

Thank you for the links.
Please add them in the PR description

@tomcornelis
Copy link
Contributor Author

I think I understand now: these _electron_id_modules_WorkingPoints.WorkingPoints define the ones which are used to built some integer which contain both veto, loose, medium and tight. Of course this is limited to 4 workingpoints as they have to fit in the integer, and it does not make sense to have V1 and V2 both listed here, so I only keep V2. The V1 should still be saved in the NanoAOD as it is included in the userIntsFromBool below.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #23477 was updated. @perrotta, @monttj, @cmsbuild, @slava77, @gpetruc, @arizzi can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Jun 19, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 19, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28759/console Started: 2018/06/19 15:25

@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-23477/28759/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 420 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 2902013
  • DQMHistoTests: Total failures: 17
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2901806
  • 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

@slava77
Copy link
Contributor

slava77 commented Jun 20, 2018

+1

for #23477 0e7aa5e

  • V1 cut-based electron IDs were (re)added since the last reco signoff (instead of simply replacing V1 by V2).
  • jenkins tests pass and comparisons with the baseline show some small relatively small differences in the electron ID plots in the nanoAOD comparisons and also in variables matching this pattern slimmedElectrons__[^$]*_electronIDs_\|slimmedElectrons__[^$]*_userInts_ in miniAOD workflows, as expected.

@fabiocos
Copy link
Contributor

+1

@arizzi @gpetruc please notice the nanoAOD modification

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 29cf2ed into cms-sw:master Jun 21, 2018
Sam-Harper pushed a commit to Sam-Harper/cmssw that referenced this pull request Oct 12, 2018
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
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