Preparing RecoEgamma_PhotonIdentification for Fall17 V2 MVA ID #24131
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24131/5819 |
|
A new Pull Request was created by @guitargeek (Jonas Rembser) for master. It involves the following packages: RecoEgamma/EgammaTools @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test with cms-sw/cmsdist#4230 |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| @@ -128,6 +128,8 @@ void MVAValueMapProducer<ParticleType>::produce(edm::Event& iEvent, const edm::E | |||
| << " failed to find a standard AOD or miniAOD particle collection " << std::endl; | |||
| } | |||
|
|
|||
| // Passed by reference to the mvaValue function to store the category | |||
| int cat = -1; | |||
There was a problem hiding this comment.
why is this defined here?
The variable is needed only in the loop below.
| @@ -0,0 +1,97 @@ | |||
| from PhysicsTools.SelectorUtils.centralIDRegistry import central_id_registry | |||
There was a problem hiding this comment.
this file was recovered with a commit "Brought back 2014 Electron MVA for backwards compatibility".
Please clarify in a bit more detail which backwards compatibility is implied here.
Can this file be even used with existing producers in this release?
There was a problem hiding this comment.
It's fully backwards compatible. It can be added to the value map produced https://github.com/cms-sw/cmssw/blob/master/RecoEgamma/ElectronIdentification/python/ElectronMVAValueMapProducer_cfi.py and produces the same values as the previous config file which was deleted.
There was a problem hiding this comment.
I think it's interesting to keep for comparisons. It's the only Run2 MVA ID which doesn't include conversion rejection yet. But if you think there is no place for it here I'll just keep it privately.
There was a problem hiding this comment.
if it's useful, it should be enabled in the unit tests
There was a problem hiding this comment.
IIUC, this was removed in 10_1_X.
Did EGM group find some new use cases for this ID?
There was a problem hiding this comment.
No, but for me it was useful to compare with HGCal since there is no conversion rejection there. But I see that I should better remove it here and keep it privately.
| @@ -0,0 +1,13 @@ | |||
| # This file contains Effective Area constants for | |||
There was a problem hiding this comment.
similar to mvaElectronID_PHYS14_PU20bx25_nonTrig_V1_cff.py, is this file needed or can it be used in this release?
There was a problem hiding this comment.
I don't know, since I don't maintain the effective areas. But it was in the externals before anyway, so it's not like it's added back (as is the case for the 2014 electron MVA).
| @@ -0,0 +1,13 @@ | |||
| # This file contains Effective Area constants for | |||
There was a problem hiding this comment.
You mean removing the effective are files completely?
There was a problem hiding this comment.
keep only the ones that are used.
I didn't find any use case
| @@ -0,0 +1,13 @@ | |||
| # This file contains Effective Area constants for | |||
| @@ -0,0 +1,13 @@ | |||
| # This file contains Effective Area constants for | |||
| @@ -0,0 +1,12 @@ | |||
| # This file contains Effective Area constants for | |||
| @@ -0,0 +1,13 @@ | |||
| # This file contains Effective Area constants for | |||
| desc.add<edm::InputTag>("particleBasedIsolation"); | ||
| desc.add<edm::InputTag>("pfCandidates"); | ||
| desc.add<edm::InputTag>("pfCandidatesMiniAOD"); | ||
| descriptions.addDefault(desc); |
There was a problem hiding this comment.
is it practical to write a valid default config file here?
From git grep, I see only one instance of photonIDValueMapProducer.
It looks like PhotonIDValueMapProducer_cfi can be removed and replaced here by an auto-generated file based on the fillDescriptions
There was a problem hiding this comment.
What about these phase2 specific inputs? Wouldn't we have to keep at least part of the config file for that? https://github.com/cms-sw/cmssw/blob/master/RecoEgamma/PhotonIdentification/python/PhotonIDValueMapProducer_cfi.py
Sorry I'm not so familiar with the python configurations...
There was a problem hiding this comment.
era modifiers can be added in a cff file which will include an auto-generated photonIDValueMapProducer_cfi
There was a problem hiding this comment.
So when I search, on GitHub, I find a few occurrences of PhotonIDValueMapProducer_cfi. What should I do? Add the era modifier in every file where PhotonIDValueMapProducer_cfi would be imported? Thanks for your help!
There was a problem hiding this comment.
descriptions.add("photonIDValueMapProducer")- mv PhotonIDValueMapProducer_cfi photonIDValueMapProducer_cff, start it with import of photonIDValueMapProducer_cfi
- replace all use cases of PhotonIDValueMapProducer_cfi by photonIDValueMapProducer_cff
| : AnyMVAEstimatorRun2Base(conf) | ||
| , mvaVarMngr_ (conf.getParameter<std::string>("variableDefinition")) | ||
| // So far only used in Run2Spring16NonTrigV1 | ||
| , effectiveAreas_ (conf.exists("effAreasConfigFile") ? |
There was a problem hiding this comment.
::exists method is rather an exception left for cases for rather old and complex pre-7X software.
Define a ::fillDescriptions method https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp
There was a problem hiding this comment.
But this is just a class and not a plugin. In this case, I can't implement a ::fillDescriptions method, right? And the plugin which uses this class should not need to know which specific parameters this MVA expects I think.
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 for #24131 317687b
@guitargeek please check that the PR description is still describing the final changes (the PHYS14 seems out of order now) @fabiocos please note that this PR has to go in together with cms-sw/cmsdist#4230 |
|
Ok thanks, I removed the PHYS14 related bullet points from the description. |
|
+1 |
|
merge |
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
Updated weight file names to fit the new naming scheme of the now gzipped weight files (see Update RecoEgamma-PhotonIdentification to 01-00-06 cmsdist#4230).
Implemented MVA category finding with cut parser for photons as well, now that the multithreading-related bugs got ironed out on the electron side:
https://github.com/guitargeek/cmssw/blob/PhotonMVA_10_3_X_without_new_ID/RecoEgamma/PhotonIdentification/plugins/PhotonMVAEstimator.cc#L127.
Started to improve
PhotonIDValueMapProducer.ccsince it became slow and difficult to maintain. I spotted for example the bug that https://github.com/cms-sw/cmssw/blob/master/RecoEgamma/PhotonIdentification/plugins/PhotonIDValueMapProducer.cc#L496 and https://github.com/cms-sw/cmssw/blob/master/RecoEgamma/PhotonIdentification/plugins/PhotonIDValueMapProducer.cc#L507 calculate exactly the same thing.Config file for actual Fall17 V2 PhotonMVA will follow later after validation within Egamma.
Some technical improvements.
Has to be tested with cms-sw/cmsdist#4230.