VID Selectors don't work in FWLite anymore

[guitargeek]

Hello fellow cms devs,

some time ago a remark about electron IDs not working in FWLite appeared on the hypernews [1]. I investigated this problem, made some tests and found out that these VID Selectors don't work for a long time actually: since 8_2_0. I was testing it with this script [2].

Now, I basically found the problem, its this guard here [3] which is always guarding that line since I suspect this PR [4]. Since I have no clue what was the intended purpose of all these precompiler directives I refrained from trying to fix it myself and just wanted to make you aware of this issue.

Cheers,
Jonas

[1] https://hypernews.cern.ch/HyperNews/CMS/get/egamma/2091.html
[2] https://github.com/cms-sw/cmssw/blob/master/PhysicsTools/SelectorUtils/test/pyfwlite_test.py
[3] https://github.com/cms-sw/cmssw/blob/master/PhysicsTools/SelectorUtils/interface/VersionedSelector.h#L226
[4] 24556e3#diff-02450dcd86ab6ff6590c4439d9f6acd2

[cmsbuild]

A new Issue was created by @guitargeek Jonas Rembser.

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[Dr15Jones]

@smuzaffar looks like you made the original change?

[slava77]

assign reconstruction
to transfer the signature request from #23803

[cmsbuild]

New categories assigned: reconstruction

@slava77,@perrotta you have been requested to review this Pull request/Issue and eventually sign? Thanks

[slava77]

@guitargeek
is this issue still unresolved?
There was some activity related to it in #24131, but it's unclear if this fixed the problem

[guitargeek]

Hi @slava77, I have not tried it recently but I suppose the VID in FWLite is still broken. It does not seem that people care too much however.

The discussion in #24131 was not related to the VID selectors, but to using the Egamma MVAEstimator classes in FWLite.

Sorry I can't say much on these FWLite issues, since I don't use it. But recently I got in contact with someone who wants to use the new Egamma MVAs in FWLite [1], so maybe things will move forward from there.

[1] https://hypernews.cern.ch/HyperNews/CMS/get/egamma-elecid/132.html

[cbernet]

Hello, I would just like to point out that the fact that electron ID V2 does not work in FWLite is a verious serious blocker for our analyses. Indeed, we are reading mini AODs and re-running POG recipes in FWLite when necessary before making our ntuples. Having to run the full framework at this stage just for electron ID would force us to add a step to our computing workflow and to store the resulting modified mini AODs, hence a waste of computing time and space.

I think the main issue is here:

https://github.com/guitargeek/cmssw/blob/master/RecoEgamma/ElectronIdentification/src/ElectronMVAEstimatorRun2.cc#L80

Starting from there, the code is designed to run on an edm::Ptr<reco::GsfElectron>.
As far as I know it is not possible to get a such an edm::Ptr in FWlite. @Dr15Jones could you please comment? Therefore, for the code to run in FWLite, it would be necessary to adapt it to deal with reco::GsfElectrons directly.

In mvaValue the ptr is used in the following ways:

• in findCategory: this method can be adapted easily I think because the gsfPtr is dereferenced here and here

• by instances of MVAVariableManager like here. This looks more problematic to us. Indeed, its getValue is getting something from the event (not sure this will work in FWLite either...), e.g. here. It seems to be a value associated to the pointer itself and stored in the event. I don't really know how we could retrieve it in FWLite.

@GaelTouquet @lucastorterotot are certainly willing to help on this, but we do not see how we could do it alone at this stage. @guitargeek @Dr15Jones could we maybe set up a discussion on skype so that we could try and find a solution?

[Dr15Jones]

@cbernet if you have an edm::Ptr<> that you've read from the input file in FWLite, that object will function correctly. You can not create a new edm::Ptr<> from scratch (at least not trivially).

[Dr15Jones]

@cbernet both fwlite::Event and edm::Event inherit from edm::EventBase. So any function that takes a edm::EventBase will work in FWLite.

[cbernet]

Thanks for your comments Chris, this is helping a lot. We’ll give it a try and will comment back here in a few days.

…

Le 20 nov. 2018 à 16:30, Chris Jones ***@***.***> a écrit : @cbernet <https://github.com/cbernet> both fwlite::Event and edm::Event inherit from edm::EventBase. So any function that takes a edm::EventBase will work in FWLite. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23676 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD8ku4m5KGePCghOtGVoOATBSzo4XmFlks5uxCAQgaJpZM4U2dZ4>.

[guitargeek]

Hi @cbernet, that's very interesting that the new implementation might not work in a real use case!

I would have taken much care about fwlite compatibility if I I would have had any code to test it with, maybe you can point me to some code where you were running the old 2016 Egamma MVA recipe with fwlite and it worked?

[cbernet]

Hi @Dr15Jones, could you please tell me how to read an edm::Ptr from the input file in FWLite with python?

I'm doing the following which gives me a ROOT.pat::Electron object. This object can be fed to C++ functions taking a PAT::Electron (or GsfElectron) native pointer.

import ROOT

from DataFormats.FWLite import Events, Handle

events = Events('root://cms-xrd-global.cern.ch//store/mc/RunIIFall17MiniAODv2/VBFHToTauTau_M125_13TeV_powheg_pythia8/MINIAODSIM/PU2017_12Apr2018_94X_mc2017_realistic_v14-v1/90000/549B2DC8-C443-E811-9DAF-A0369FE2C17C.root')
# events = Events('test.root')

handle  = Handle ('std::vector<pat::Electron>')

for i,event in enumerate(events): 
    event.getByLabel(('slimmedElectrons'),handle)
    electrons = handle.product()
    if not len(electrons):
        continue
    print electrons[0]
    # gives : <ROOT.pat::Electron object at 0x10270900>
    import pdb; pdb.set_trace()
    if i==100: 
        break

[Dr15Jones]

Hi @Dr15Jones, could you please tell me how to read an edm::Ptr from the input file in FWLite with python?

I just meant that if a edm::Ptr<> is stored as an object in one of the TBranches of the file, then all abilities of the edm::Ptr<> will work.

[cbernet]

Ok, I see. But my issue here is that when I read electrons using FWLite in python, I get a ROOT.pat::Electron object. But I need to give this electron to this method which takes an edm::Ptr to the electron, and I don't find a way to do it.

@guitargeek could you please tell me where the mvaValue method is used in the full framework when re-running on miniaod? I would need to understand how you get the edm::Ptr to the electron before calling the method.

[makortel]

Does

cmssw/RecoEgamma/ElectronIdentification/src/ElectronMVAEstimatorRun2.cc

Line 80 in e33a0d8

mvaValue( const edm::Ptr<reco::Candidate>& candPtr, const edm::EventBase & iEvent, int &iCategory) const {

really need an edm::Ptr<T>? (by very quick look it seems to be that the answer could be "no")

[guitargeek]

@makortel, since I have to get some information from precomputed value maps [1] I thought the edm::Ptr is necessary. Or is there another way? For sure I remember that it did not work with the bare reco::Candidate...

[1] https://github.com/cms-sw/cmssw/blob/master/RecoEgamma/EgammaTools/interface/MVAVariableManager.h#L88

[guitargeek]

@cbernet The mvaValue method is used here:

https://github.com/cms-sw/cmssw/blob/master/RecoEgamma/EgammaTools/interface/MVAValueMapProducer.h#L116

[makortel]

@guitargeek Thanks for pointing it out. The ValueMap::operator[] indeed needs something Ref-like (that contains ProductID and and an index)

cmssw/DataFormats/Common/interface/ValueMap.h

Lines 127 to 130 in e33a0d8

	template<typename RefKey>
	const_reference_type operator[](const RefKey & r) const {
	return get(r.id(), r.key());
	}

In principle @cbernet's example #23676 (comment) has all the needed ingredients. Maybe one could provide variants of the interface? Can one create an edm::Ref in FWLite?

[makortel]

Or would

handle  = Handle ('edm::View<pat::Electron>')

work?

[cbernet]

guys, really thanks for the suggestions.
@makortel I tried to read as a view as you wrote but it does not work.
@guitargeek I see that the code that accesses the electrons before running the mvaValue method actually does this to get a ptr.

But in python FWLite, the Handle object does not seem to have a ptrAt method:

dir(handle)
['__doc__', '__init__', '__module__', '__str__', '_addressOf', '_exception', '_nodel', '_resetWrapper', '_setStatus', '_type', '_typeInfo', '_typeInfoGetter', '_wrapper', 'isValid', 'product']

@Dr15Jones sorry to disturb again, and sorry if it's a silly question. Is there a way to build an edm::Ptr in python FWLite from a FWLite Handle?

If not, maybe I could read the input valuemaps mentioned by @guitargeek instead of the electron collection, and get the electron ptr from there, what do you think?

[Dr15Jones]

@Dr15Jones sorry to disturb again, and sorry if it's a silly question. Is there a way to build an edm::Ptr in python FWLite from a FWLite Handle?

I do not believe so.

[guitargeek]

I think that's not the simplest solution @cbernet. I think the easiest way would be to reorganize the MVA code a little bit such that the missing ID variables are not calculated by a separate producer and therefore have to be stored in value maps, but it's done within the MVAValueMapProducer starting from the GsfElectron and the other products without the need of edm::Ptr.

Actually I already started to write code for that some time age, but then I realized it somehow contradicts the idea of a modular framework if one producer gets to do too much and abandoned that idea. Maybe I should come back to that.

[Dr15Jones]

The fwlite::Handle does not provide access to the edm::Provenance object (which holds the edm::ProductID) since providing such access severely reduced the readback rate.

We could presumably add a new method to fwlite::Event which gives a type safe access to the ProductID for a data product in the event.

[cbernet]

Thanks @Dr15Jones, Jonas and I are now in touch, we will try and get the V2 e ID to run in python fwlite without modification of fwlite::Event for now (though what you propose sounds very useful to me)

[guitargeek]

Hi, I made a PR to address the issue raised by @cbernet as GitHub was automatically referencing here. We should continue the discussion there, in particular because actually this issue has actually nothing to do with it. I opened the issue because the VID selectors don't work, which is separate from the calculation of the MVA ID variables (that can be later used by VID).

Just wanted to point that out so nobody thinks the original issue was solved...

[cbernet]

Hi @guitargeek, again thank you! Could you please explain what the VID selectors are and point me to some place where they are used in the full framework?
We can get in touch on skype to see how to have them or something similar working in python+fwlite.

[guitargeek]

Hi @cbernet, the VID selectors allow you to specify cutflows and hash them, such that nobody can accidentally mess with the official cuts without triggering a warning in the log. They are also used in production to calculate the cut decisions stored in MiniAOD and NanoAOD. I don't know much about them in FWlite, only that there is this test script here which does not work for a long time:

https://github.com/cms-sw/cmssw/blob/master/PhysicsTools/SelectorUtils/test/pyfwlite_test.py

For the MVA, the VID cutflow is configured here: https://github.com/cms-sw/cmssw/blob/master/RecoEgamma/ElectronIdentification/python/Identification/mvaElectronID_tools.py#L97

You probably observe that the VID cutflow is not very interesting for MVAs since there is only one single cut on the MVA value. In fact you basically implemented the same functionality in FWlite in your recent PR [1] :)

In any case I don't have time to support the Electron VID selectors and this time it's also not my fault they don't work.

[1] https://github.com/guitargeek/cmssw/pull/1

[slava77]

@guitargeek @Sam-Harper
should we just close this with "will not fix"?

[guitargeek]

I agree. Looking at CERN-PH-CMG/cmg-cmssw#749 it seems the FWLite community has written new code to deal with the IDs anyway.

Should I just close it like that or you have to do something to mark it was "will not fix"?

[slava77]

Should I just close it like that

yes, please close.