Review of reco::PFCandidate::vertex() function

[veelken]

Hi,

I came across a 'ProductNotFound' exception when rerunning some tau reconstruction code on a ROOT file that contains a collection of PFCandidates ('particleFlowTmp' for Phase-2 HLT trigger studies).

The issue is that the reco::PFCandidate::vertex() function returns not simply a vertex position, but dereferences different types of edm::Refs, depending on the type of PFCandidate (vertexType_):
https://github.com/cms-sw/cmssw/blob/master/DataFormats/ParticleFlowCandidate/src/PFCandidate.cc#L602-L634
This behaviour means that one must not drop the GSF electron collection and any of the muon collections from the ROOT file in case one wants to evaluate the position of the PFCandidate vertex later. I consider this to be quite a burden and wonder if this is really neccessary. Why not set the data-member (m_state.vertex_) for the vertex position in the reco::LeafCandidate base-class to the best estimate of the vertex position in the reco::PFCandidate producer code and simply return that data-member in the reco::PFCandidate::vertex() function ?

This is not an urgent issue for me personally, but I wanted to bring it to your attention, because the current behaviour of the code forces users to know all the different types of muon collections that were used as input to the PFCandidate producer and keep all those muon collections in their ROOT files, in order to analyze a collection of PFCandidates (even if they do not care about GSF electrons and muons at all).

[cmsbuild]

A new Issue was created by @veelken Christian Veelken.

@Dr15Jones, @dpiparo, @silviodonato, @smuzaffar, @makortel, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[makortel]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

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

[slava77]

@hatakeyamak @bendavid

[slava77]

@veelken

I came across a 'ProductNotFound' exception when rerunning some tau reconstruction code on a ROOT file that contains a collection of PFCandidates ('particleFlowTmp' for Phase-2 HLT trigger studies).

please provide a fully reproducible example for your case
Thank you

[veelken]

Hi,

I send you instructions for reproducing the exception attached below [*],
together with the output that I get when executing these commands on my machine [**].

With kind regards,

Christian

[*]

mkdir debugCMSSW
cd debugCMSSW
cmsrel CMSSW_11_1_0
cd CMSSW_11_1_0/src/
cmsenv
mkdir -p HLTrigger/TallinnHLTPFTauAnalyzer
cd HLTrigger/TallinnHLTPFTauAnalyzer
cp -r /afs/cern.ch/user/v/veelken/public/debugCMSSW/CMSSW_11_1_0/src/HLTrigger/TallinnHLTPFTauAnalyzer/* .
scram b -j 4
cd test
cmsRun debugPFCandidateVertex_cfg.py

[**]

...
PFCandidate #415 (type = PFChargedHadron): pT = 2.44837, eta = 0.801213, phi = 1.26002
vertex: x = -0.00217696, y = 0.000701925, z = 2.26277
bestTrack: pT = 2.44837, eta = 0.801213, phi = 1.26002
vertex: x = -0.00217696, y = 0.000701925, z = 2.26277
calorimeter energy: ECAL = 1.86464, HCAL = 1.5928
PFCandidate #418 (type = PFMuon): pT = 2.05756, eta = 1.24327, phi = 0.454681
vertex: x = ----- Begin Fatal Exception 24-Jul-2020 18:13:36 EEST-----------------------
An exception of category 'ProductNotFound' occurred while
[0] Processing Event run: 1 lumi: 2073 event: 294229 stream: 0
[1] Running path 'p'
[2] Calling method for module DumpRecoPFCandidates/'dumpPFCandidates'
Exception Message:
RefCore: A request to resolve a reference to a product of type 'std::vectorreco::Muon' with ProductID '3:700'
can not be satisfied because the product cannot be found.
Probably the branch containing the product is not stored in the input file.
Additional Info:
[a] If you wish to continue processing events after a ProductNotFound exception,
add "SkipEvent = cms.untracked.vstring('ProductNotFound')" to the "options" PSet in the configuration.

----- End Fatal Exception -------------------------------------------------
24-Jul-2020 18:13:36 EEST Closed file file:debug_RAW2DIGI_RECO.root

[veelken]

My initial instructions contained an inaccessible file path (/home/veelken). I have copied the files to /afs/cern.ch/user/v/veelken/public/debugCMSSW/CMSSW_11_1_0/src/HLTrigger/TallinnHLTPFTauAnalyzer and updated the instructions above accordingly. The new instructions should work by simply copy & pasting them into a terminal.

[hatakeyamak]

@veelken Thank you. I have to think what is the good thing to do. Is it sufficient to add some validity check of muonRef() and gsfTrackRef(), or do you have a better suggestion?

[veelken]

@hatakeyamak Adding a validity check for muonRef().isAvailable() and gsfTrackRef().isAvailable() is what I have implemented as a temporary workaround in my CMSSW area for now. The corresponding code is attached below [*].

While adding a validity check solves my use-case, I see one potential issue: in case a user drops a muon or electron collection and the isAvailable() check fails, he or she may get a different vertex position, i.e. the physics behaviour may change.

I think a better solution is to move the code from the PFCandidate::vertex() function to the PFProducer and set the LeafCandidate::m_state.vertex_ data-member to the return value of the PFCandidate::vertex() function directly in the PFProducer.
(There is a math::XYZVector available to store the vertex position in the LeafCandidate, so why not use it ? ;)

[*]

const math::XYZPoint& PFCandidate::vertex() const {
switch (vertexType_) {
case kCandVertex:
return LeafCandidate::vertex();
break;
case kTrkVertex:
if ( trackRef().isAvailable() ) return trackRef()->vertex();
break;
case kComMuonVertex:
if ( muonRef().isAvailable() && muonRef()->combinedMuon().isAvailable() ) return muonRef()->combinedMuon()->vertex();
break;
case kSAMuonVertex:
if ( muonRef().isAvailable() && muonRef()->standAloneMuon().isAvailable() ) return muonRef()->standAloneMuon()->vertex();
break;
case kTrkMuonVertex:
if ( muonRef().isAvailable() && muonRef()->track().isAvailable() ) return muonRef()->track()->vertex();
break;
case kTPFMSMuonVertex:
if ( muonRef().isAvailable() && muonRef()->tpfmsTrack().isAvailable() ) return muonRef()->tpfmsTrack()->vertex();
break;
case kPickyMuonVertex:
if ( muonRef().isAvailable() && muonRef()->pickyTrack().isAvailable() ) return muonRef()->pickyTrack()->vertex();
break;
case kDYTMuonVertex:
if ( muonRef().isAvailable() && muonRef()->dytTrack().isAvailable() ) return muonRef()->dytTrack()->vertex();
break;
case kGSFVertex:
if ( gsfTrackRef().isAvailable() ) return gsfTrackRef()->vertex();
break;
}
return LeafCandidate::vertex();
}

[hatakeyamak]

I see. Your suggestion will add some to event size, right? If it is a minimal addition, this sounds good to me. If we don't hear objections, is it something you can help by making a PR?

[veelken]

Hi Ken, I would prefer if someone else can do this change. I expect that changing the code will take only 1-2 hours of time, but unfortunately I don't have enough time for validation at the moment.

[hatakeyamak]

Hi Christian. I understand.

Just as a possibility, If you are available and can do some rough implementation, we can probably take it over. It's just that. we are operating with a very limited personpower, and trying to get help whenever possible. If not possible, I totally understand.

[veelken]

Hi Ken,

that will work. I will create a pull-request and get back to you.

[veelken]

Dear Ken,

here is the pull-request: #31298

I believe the code is fully implemented. I have verified that the code compiles, but I haven't tested it on a RelVal sample yet. Would be great if you could take care of that. Note that the vertex position for PFCandidates of type electron is taken from the gsfTrackRef->vertex(), which I believe is the intended behaviour, while the previous code took the vertex position of PFCandidates of type electron from the trackRef->vertex(), because the enum PFCandidate::kGSFVertex was never set (an LXR search for kGSFVertex returns nothing). I expect that the difference between gsfTrackRef->vertex() and trackRef->vertex() is small for most PFCandidates, but I just wanted to point this out, in case you do see some differences in the vertex position of PFCandidates of type electron in the validation.

[hatakeyamak]

@veelken Thank you. I will take a look when I get a chance. It may have to be a couple of days later.

[jpata]

Christian's branch to take over by someone:
https://github.com/veelken/cmssw/tree/CMSSW_11_2_X

[jpata]

Fixed by #31456, can be closed.