Merge pull request #35953 from guitargeek/ElectronMVAEstimatorRun2_fw…

…lite Restructure VID Python/FWLite code and avoid generating dictionaries for `Reco*/*Identification` packages
cms-sw · Jan 25, 2022 · f88f508 · f88f508
2 parents 5787b89 + d4a53bb
commit f88f508
Show file tree

Hide file tree

Showing 29 changed files with 156 additions and 236 deletions.
diff --git a/FWCore/Utilities/interface/thread_safety_macros.h b/FWCore/Utilities/interface/thread_safety_macros.h
@@ -1,6 +1,6 @@
 #ifndef FWCore_Utilites_thread_safe_macros_h
 #define FWCore_Utilites_thread_safe_macros_h
-#if !defined __ROOTCLING__ && !defined __INTEL_COMPILER && !defined __NVCC__
+#if !defined __CLING__ && !defined __INTEL_COMPILER && !defined __NVCC__
 #define CMS_THREAD_SAFE [[cms::thread_safe]]
 #define CMS_SA_ALLOW [[cms::sa_allow]]
 #define CMS_THREAD_GUARD(_var_) [[cms::thread_guard(#_var_)]]

diff --git a/PhysicsTools/SelectorUtils/interface/MakePtrFromCollection.h b/PhysicsTools/SelectorUtils/interface/MakePtrFromCollection.h
diff --git a/PhysicsTools/SelectorUtils/interface/MakePyVIDClassBuilder.h b/PhysicsTools/SelectorUtils/interface/MakePyVIDClassBuilder.h
diff --git a/PhysicsTools/SelectorUtils/interface/PrintVIDToString.h b/PhysicsTools/SelectorUtils/interface/PrintVIDToString.h
diff --git a/PhysicsTools/SelectorUtils/python/VIDSelectorBase.py b/PhysicsTools/SelectorUtils/python/VIDSelectorBase.py
@@ -13,6 +13,34 @@
 ROOT.gSystem.Load("libDataFormatsFWLite.so");
 ROOT.FWLiteEnabler.enable()
 
+# define some C++ helpers that are only used in this VID selector class and deriving classes
+ROOT.gInterpreter.Declare("""
+#include "FWCore/ParameterSetReader/interface/ParameterSetReader.h"
+
+template <class PhysObj>
+struct MakeVersionedSelector {
+  MakeVersionedSelector() {}
+
+  VersionedSelector<edm::Ptr<PhysObj> > operator()(const std::string& pset, const std::string& which_config) {
+    const edm::ParameterSet& temp = edm::readPSetsFrom(pset)->getParameter<edm::ParameterSet>(which_config);
+    return VersionedSelector<edm::Ptr<PhysObj> >(temp);
+  }
+
+  VersionedSelector<edm::Ptr<PhysObj> > operator()() { return VersionedSelector<edm::Ptr<PhysObj> >(); }
+};
+
+template <class Collection,
+          class InPhysObj = typename Collection::value_type,
+          class OutPhysObj = typename Collection::value_type>
+struct MakePtrFromCollection {
+  edm::Ptr<OutPhysObj> operator()(const Collection& coll, unsigned idx) {
+    edm::Ptr<InPhysObj> temp(&coll, idx);
+    return edm::Ptr<OutPhysObj>(temp);
+  }
+};
+""")
+
+
 config_template = """
 import FWCore.ParameterSet.Config as cms
 
@@ -34,11 +62,10 @@ def id_generator(size=6, chars=string.ascii_uppercase + string.digits):
     return ''.join(random.choice(chars) for _ in range(size))
 
 class VIDSelectorBase:
-    def __init__(self, vidSelectorBuilder, ptrMaker, printer, pythonpset = None):        
+    def __init__(self, vidSelectorBuilder, ptrMaker, pythonpset = None):
         self.__initialized = False
         self.__suffix = id_generator(12)
-        self.__printer = printer()
-        self.__ptrMaker = ptrMaker()
+        self.__ptrMaker = ptrMaker
         self.__selectorBuilder = vidSelectorBuilder()
         self.__instance = None
         if pythonpset is not None:
@@ -130,4 +157,6 @@ def md55Raw(self):
         return self.__instance.md55Raw()
 
     def __repr__(self):
-        return self.__printer(self.__instance)
+        out = ROOT.std.stringstream()
+        self.__instance.print(out)
+        return out.str();
diff --git a/PhysicsTools/SelectorUtils/src/classes.h b/PhysicsTools/SelectorUtils/src/classes.h
@@ -10,14 +10,12 @@
 #include "PhysicsTools/SelectorUtils/interface/PFJetIDSelectionFunctor.h"
 #include "PhysicsTools/SelectorUtils/interface/PVSelector.h"
 #include "PhysicsTools/SelectorUtils/interface/RunLumiSelector.h"
-#include "PhysicsTools/SelectorUtils/interface/MakePyVIDClassBuilder.h"
 
-namespace PhysicsTools_SelectorUtils {
-  struct dictionary {
-    pat::strbitset strbitset;
-    edm::Wrapper<pat::strbitset> wstrbitset;
-    std::vector<pat::strbitset> vstrbitset;
-    edm::Wrapper<std::vector<pat::strbitset> > wvstrbitset;
-  };
-
-}  // namespace PhysicsTools_SelectorUtils
+#include "PhysicsTools/SelectorUtils/interface/VersionedSelector.h"
+#include "DataFormats/PatCandidates/interface/Photon.h"
+#include "DataFormats/EgammaCandidates/interface/PhotonFwd.h"
+#include "DataFormats/MuonReco/interface/MuonFwd.h"
+#include "DataFormats/PatCandidates/interface/Muon.h"
+#include "DataFormats/EgammaCandidates/interface/GsfElectronFwd.h"
+#include "DataFormats/EgammaCandidates/interface/GsfElectron.h"
+#include "DataFormats/PatCandidates/interface/Electron.h"
diff --git a/PhysicsTools/SelectorUtils/src/classes_def.xml b/PhysicsTools/SelectorUtils/src/classes_def.xml
@@ -9,4 +9,9 @@
   <class name="MuonVPlusJetsIDSelectionFunctor"/>
   <class name="PVSelector"/>
   <class name="RunLumiSelector"/>
+  <class name="VersionedSelector<edm::Ptr<reco::Photon>>"/>
+  <class name="VersionedSelector<edm::Ptr<pat::Photon>>"/>
+  <class name="VersionedSelector<edm::Ptr<reco::Muon>>"/>
+  <class name="VersionedSelector<edm::Ptr<reco::GsfElectron>>"/>
+  <class name="VersionedSelector<edm::Ptr<pat::Electron>>"/>
 </lcgdict>
diff --git a/PhysicsTools/SelectorUtils/test/BuildFile.xml b/PhysicsTools/SelectorUtils/test/BuildFile.xml
@@ -0,0 +1,4 @@
+<bin file="testPhysicsToolsSelectorUtilsPythonTestsDriver.cpp">
+  <flags TEST_RUNNER_ARGS="/bin/bash PhysicsTools/SelectorUtils/test runPythonTests.sh"/>
+  <use name="FWCore/Utilities"/>
+</bin>
diff --git a/PhysicsTools/SelectorUtils/test/runPythonTests.sh b/PhysicsTools/SelectorUtils/test/runPythonTests.sh
@@ -0,0 +1,6 @@
+#!/bin/sh
+
+# Pass in name and status
+function die { echo $1: status $2 ;  exit $2; }
+
+python3 ${CMSSW_BASE}/src/PhysicsTools/SelectorUtils/test/test_vid_selectors.py
diff --git a/PhysicsTools/SelectorUtils/test/testPhysicsToolsSelectorUtilsPythonTestsDriver.cpp b/PhysicsTools/SelectorUtils/test/testPhysicsToolsSelectorUtilsPythonTestsDriver.cpp
@@ -0,0 +1,3 @@
+#include "FWCore/Utilities/interface/TestHelper.h"
+
+RUNTEST()
diff --git a/PhysicsTools/SelectorUtils/test/test_vid_selectors.py b/PhysicsTools/SelectorUtils/test/test_vid_selectors.py
@@ -0,0 +1,19 @@
+import unittest
+
+class TestVIDSelectros(unittest.TestCase):
+
+    def test_vid_selectors(self):
+        # Test that all VID selectors for Python/FWLite can be instantiated
+
+        from RecoMuon.MuonIdentification.VIDMuonSelector import VIDMuonSelector
+        from RecoEgamma.ElectronIdentification.VIDElectronSelector import VIDElectronSelector
+        from RecoEgamma.PhotonIdentification.VIDPhotonSelector import VIDPhotonSelector
+
+        VIDMuonSelector()
+        VIDElectronSelector()
+        VIDPhotonSelector()
+
+
+if __name__ == "__main__":
+
+    unittest.main(verbosity=2)
diff --git a/RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Base.h b/RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Base.h
@@ -1,20 +1,26 @@
 #ifndef RecoEgamma_EgammaTools_AnyMVAEstimatorRun2Base_H
 #define RecoEgamma_EgammaTools_AnyMVAEstimatorRun2Base_H
 
-#include "FWCore/ParameterSet/interface/ParameterSet.h"
+// Note on Python/FWLite support: please forward declare as much as possible in
+// this header, because there are usecases of generating the dictionaries for
+// this class on the fly (see notes in ElectronMVAEstimatorRun2.h for more
+// details).
 
-#include "FWCore/Framework/interface/Event.h"
-#include "FWCore/Framework/interface/ConsumesCollector.h"
+#include <string>
+#include <vector>
 
-#include "DataFormats/Candidate/interface/Candidate.h"
+namespace edm {
+  class ParameterSet;
+}  // namespace edm
+
+namespace reco {
+  class Candidate;
+}  // namespace reco
 
 class AnyMVAEstimatorRun2Base {
 public:
   // Constructor, destructor
-  AnyMVAEstimatorRun2Base(const edm::ParameterSet& conf)
-      : tag_(conf.getParameter<std::string>("mvaTag")),
-        nCategories_(conf.getParameter<int>("nCategories")),
-        debug_(conf.getUntrackedParameter<bool>("debug", false)) {}
+  AnyMVAEstimatorRun2Base(const edm::ParameterSet& conf);
 
   AnyMVAEstimatorRun2Base(const ::std::string& mvaName, const ::std::string& mvaTag, int nCategories, bool debug)
       : name_(mvaName), tag_(mvaTag), nCategories_(nCategories), debug_(debug) {}
@@ -68,8 +74,4 @@ class AnyMVAEstimatorRun2Base {
   const bool debug_;
 };
 
-// define the factory for this base class
-#include "FWCore/PluginManager/interface/PluginFactory.h"
-typedef edmplugin::PluginFactory<AnyMVAEstimatorRun2Base*(const edm::ParameterSet&)> AnyMVAEstimatorRun2Factory;
-
 #endif
diff --git a/RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Factory.h b/RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Factory.h
@@ -0,0 +1,16 @@
+#ifndef RecoEgamma_EgammaTools_AnyMVAEstimatorRun2Factory_H
+#define RecoEgamma_EgammaTools_AnyMVAEstimatorRun2Factory_H
+
+#include "FWCore/PluginManager/interface/PluginFactory.h"
+#include "RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Base.h"
+
+// This plugin factory typedef is not defined in the main header file
+// "AnyMVAEstimatorRun2Base.h", because there are usecases of generating the
+// dictionaries for AnyMVAEstimatorRun2Base on the fly (see notes in
+// ElectronMVAEstimatorRun2.h for more details). This doesn't work if
+// PluginFactory.h is included in the header file because of conflicting C++
+// modules.
+
+typedef edmplugin::PluginFactory<AnyMVAEstimatorRun2Base*(const edm::ParameterSet&)> AnyMVAEstimatorRun2Factory;
+
+#endif
diff --git a/RecoEgamma/EgammaTools/interface/MVAValueMapProducer.h b/RecoEgamma/EgammaTools/interface/MVAValueMapProducer.h
@@ -9,6 +9,7 @@
 #include "FWCore/Framework/interface/MakerMacros.h"
 #include "FWCore/ParameterSet/interface/ParameterSet.h"
 #include "RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Base.h"
+#include "RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Factory.h"
 #include "FWCore/Utilities/interface/EDGetToken.h"
 #include "RecoEgamma/EgammaTools/interface/MVAVariableHelper.h"
 #include "DataFormats/Common/interface/Handle.h"

diff --git a/RecoEgamma/EgammaTools/src/AnyMVAEstimatorRun2Base.cc b/RecoEgamma/EgammaTools/src/AnyMVAEstimatorRun2Base.cc
@@ -1,3 +1,10 @@
 #include "RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Base.h"
+#include "RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Factory.h"
+#include "FWCore/ParameterSet/interface/ParameterSet.h"
+
+AnyMVAEstimatorRun2Base::AnyMVAEstimatorRun2Base(const edm::ParameterSet& conf)
+    : tag_(conf.getParameter<std::string>("mvaTag")),
+      nCategories_(conf.getParameter<int>("nCategories")),
+      debug_(conf.getUntrackedParameter<bool>("debug", false)) {}
 
 EDM_REGISTER_PLUGINFACTORY(AnyMVAEstimatorRun2Factory, "AnyMVAEstimatorRun2Factory");
diff --git a/RecoEgamma/ElectronIdentification/interface/ElectronMVAEstimatorRun2.h b/RecoEgamma/ElectronIdentification/interface/ElectronMVAEstimatorRun2.h
@@ -1,22 +1,38 @@
 #ifndef RecoEgamma_ElectronIdentification_ElectronMVAEstimatorRun2_H
 #define RecoEgamma_ElectronIdentification_ElectronMVAEstimatorRun2_H
 
-#include "DataFormats/PatCandidates/interface/Electron.h"
-#include "RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Base.h"
-#include "CommonTools/MVAUtils/interface/GBRForestTools.h"
-#include "RecoEgamma/EgammaTools/interface/MVAVariableManager.h"
 #include "CommonTools/Utils/interface/StringCutObjectSelector.h"
 #include "CommonTools/Utils/interface/ThreadSafeFunctor.h"
+#include "CondFormats/GBRForest/interface/GBRForest.h"
+#include "RecoEgamma/EgammaTools/interface/AnyMVAEstimatorRun2Base.h"
+#include "RecoEgamma/EgammaTools/interface/MVAVariableManager.h"
+
+// Note on Python/FWLite support:
+//
+// The ElectronMVAEstimatorRun2 is not only used in the full CMSSW framework
+// via the AnyMVAEstimatorRun2Factory, but it is intended to also be used
+// standalone in Python/FWLite. However, we want to avoid building dictionaries
+// for the ElectronMVAEstimatorRun2 in this ElectronIdentification package,
+// becase algorithms and data formats should not mix in CMSSW.
+// That's why it has to be possible to create the dictionaries on the fly, for
+// example by running this line in a Python script:
+//
+// ```Python
+// ROOT.gInterpreter.Declare('#include "RecoEgamma/ElectronIdentification/interface/ElectronMVAEstimatorRun2.h"')
+// ```
+//
+// To speed up the dictionary generation and avoid errors caused by conflicting
+// C++ modules, we try to forwar declare as much as possible in
+// ElectronMVAEstimatorRun2.h and AnyMVAEstimatorRun2Base.h.
 
-#include "DataFormats/EgammaCandidates/interface/Conversion.h"
-#include "CommonTools/Egamma/interface/ConversionTools.h"
-#include "DataFormats/VertexReco/interface/Vertex.h"
+namespace reco {
+  class GsfElectron;
+}
 
 class ElectronMVAEstimatorRun2 : public AnyMVAEstimatorRun2Base {
 public:
-  // Constructor and destructor
+  // Constructor
   ElectronMVAEstimatorRun2(const edm::ParameterSet& conf);
-  ~ElectronMVAEstimatorRun2() override{};
   // For use with FWLite/Python
   ElectronMVAEstimatorRun2(const std::string& mvaTag,
                            const std::string& mvaName,

diff --git a/RecoEgamma/ElectronIdentification/python/FWLite.py b/RecoEgamma/ElectronIdentification/python/FWLite.py
@@ -40,6 +40,7 @@ def __call__(self, ele, rho, debug=False):
         '''
         if not self._init:
             print('Initializing ' + self.name + self.tag)
+            ROOT.gInterpreter.Declare('#include "RecoEgamma/ElectronIdentification/interface/ElectronMVAEstimatorRun2.h"')
             ROOT.gSystem.Load("libRecoEgammaElectronIdentification")
             categoryCutStrings =  ROOT.vector(ROOT.string)()
             for x in self.categoryCuts : 
@@ -65,9 +66,9 @@ def __init__(self, name, tag, working_points, logistic_transform=False):
 
     def _reformat_cut_definitions(self, working_points):
         new_definitions = dict()
-        for wpname, definitions in working_points.iteritems():
+        for wpname, definitions in working_points.items():
             new_definitions[wpname] = dict()
-            for name, cut in definitions.cuts.iteritems():
+            for name, cut in definitions.cuts.items():
                 categ_id = int(name.lstrip('cutCategory'))
                 cut = cut.replace('pt','x')
                 formula = ROOT.TFormula('_'.join([self.name, wpname, name]), cut)

diff --git a/RecoEgamma/ElectronIdentification/python/VIDElectronSelector.py b/RecoEgamma/ElectronIdentification/python/VIDElectronSelector.py
@@ -7,6 +7,4 @@ def __init__(self,pythonpset = None):
         ptrmaker = ROOT.MakePtrFromCollection(ROOT.vector(ROOT.pat.Electron),
                                               ROOT.pat.Electron,
                                               ROOT.reco.GsfElectron)
-        printer  = ROOT.PrintVIDToString(ROOT.reco.GsfElectron)
-        VIDSelectorBase.__init__(self,builder,ptrmaker,printer,pythonpset)
-
+        VIDSelectorBase.__init__(self,builder,ptrmaker,pythonpset)
diff --git a/RecoEgamma/ElectronIdentification/src/ElectronMVAEstimatorRun2.cc b/RecoEgamma/ElectronIdentification/src/ElectronMVAEstimatorRun2.cc
@@ -1,10 +1,14 @@
 #include "RecoEgamma/ElectronIdentification/interface/ElectronMVAEstimatorRun2.h"
-#include "RecoEgamma/EgammaTools/interface/MVAVariableHelper.h"
+
+#include "CommonTools/MVAUtils/interface/GBRForestTools.h"
+#include "DataFormats/PatCandidates/interface/Electron.h"
 #include "FWCore/MessageLogger/interface/MessageLogger.h"
+#include "FWCore/ParameterSet/interface/ParameterSet.h"
+#include "RecoEgamma/EgammaTools/interface/MVAVariableHelper.h"
 
 ElectronMVAEstimatorRun2::ElectronMVAEstimatorRun2(const edm::ParameterSet& conf)
     : AnyMVAEstimatorRun2Base(conf),
-      mvaVarMngr_(conf.getParameter<std::string>("variableDefinition"), MVAVariableHelper::indexMap()) {
+      mvaVarMngr_{conf.getParameter<std::string>("variableDefinition"), MVAVariableHelper::indexMap()} {
   const auto weightFileNames = conf.getParameter<std::vector<std::string> >("weightFileNames");
   const auto categoryCutStrings = conf.getParameter<std::vector<std::string> >("categoryCuts");
 
@@ -28,7 +32,7 @@ ElectronMVAEstimatorRun2::ElectronMVAEstimatorRun2(const std::string& mvaTag,
                                                    const std::vector<std::string>& weightFileNames,
                                                    bool debug)
     : AnyMVAEstimatorRun2Base(mvaName, mvaTag, nCategories, debug),
-      mvaVarMngr_(variableDefinition, MVAVariableHelper::indexMap()) {
+      mvaVarMngr_{variableDefinition, MVAVariableHelper::indexMap()} {
   if ((int)(categoryCutStrings.size()) != getNCategories())
     throw cms::Exception("MVA config failure: ")
         << "wrong number of category cuts in " << getName() << getTag() << std::endl;
@@ -48,7 +52,6 @@ void ElectronMVAEstimatorRun2::init(const std::vector<std::string>& weightFileNa
     throw cms::Exception("MVA config failure: ")
         << "wrong number of weightfiles in ElectronMVAEstimatorRun2" << getTag() << std::endl;
 
-  gbrForests_.clear();
   // Create a TMVA reader object for each category
   for (int i = 0; i < getNCategories(); i++) {
     std::vector<int> variablesInCategory;