-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
NAN in run 3 PF candidate dz leading to NAN in run 3 DeepMET #44976
Comments
cms-bot internal usage |
A new Issue was created by @steggema. @rappoccio, @makortel, @antoniovilela, @smuzaffar, @sextonkennedy, @Dr15Jones can you please review it and eventually sign/assign? Thanks. cms-bot commands are listed here |
FYI @cms-sw/jetmet-pog-l2 @cms-sw/pf-l2 @stahlleiton @cms-sw/reconstruction-l2 |
assign reconstruction |
New categories assigned: reconstruction @jfernan2,@mandrenguyen you have been requested to review this Pull request/Issue and eventually sign? Thanks |
type jetmet, pf |
what are the |
Are the full kinematics nan or just dz? We had some instances where problematic tracks led to this issue last year, but it's extremely difficult to debug to a general class of problems. I would start by excluding them from the deep met. |
right, thanks, it could be related or similar, here is the documentation of that issue which Kenneth had debugged #39110 |
To answer the questions in one go:
To reproduce: A few events with the issue can be obtained with |
I looked a bit more at the "bestTrack" (when it exists), and they look really bizarre to me (just looking at the handful of events i mentioned above, i can look at more if people are curious):
|
can you check in 2023 or 2024 MC if the issue is still present or already addressed by Kenneth's fix? |
The issue still appears to be there in 2023 and 2024 MC. The two samples I checked are |
Do I understand correctly that these tracks are present in the general-track collections already and being propagated to PF? |
To be very clear, I should say that I only looked at MiniAOD. There the "bestTrack" method returns tracks with the features that I mentioned. I would assume that this implies that they should be in the general-tracks collections and be high-purity since they make it into PF, but it's of course conceivable that something else happens. I think the AOD sample that corresponds to the MINIAOD one above is available so it should be possible to get the events and look into the collections there. I'll see if I can easily do it and will report back in the thread. |
Turns out that AODSIM isn't available... I actually can't easily find a run 3 sample that has both MiniAODSIM and AODSIM available - if someone can point me to such a sample, I'm happy to check. |
@cms-sw/pdmv-l2 can you help finding a AODSIM for run3 for checking the above mentioned issue for which we need to access general track collection? And @steggema just out of curiosity, we expect this issue to also show up in Run3 data, right? If any example of data event is found, kindly paste the event pointer here. |
Here are the AODSIM for the above two samples In any case, AODSIM should be available by default. |
Thanks, the DY->tau tau-> mu tauh sample indeed has AODSIM available on disk (the other doesn't), so I was able to run a quick check with it. What I did is:
My main finding is that these tracks have finite dz, vz, and ptError values (whereas they are nan, nan, and inf for the bestTracks). I print some more information below (*) if it helps. I would conclude that it means that something is going wrong in how these tracks are being used within the PF algorithm. Please let me know if there's anything else that I can do to help. (*) ######## Event 6 |
Sorry for the multiple posts, but I realised I could of course also simply look at the PFCandidates in AODSIM. Interestingly, they have finite dzError and finite dz of the bestTrack, but they already have all kinematic values at 0. I'm dumping some more information just below in case it helps for the first 20 or so candidates. ######## Event 0 |
thanks, interesting.. looks like something went wrong in AOD->miniAOD step |
Agreed. For what it's worth, it seems plausible to me that some of the packed candidate creation or methods rely on momenta not being zero, so maybe the right question to ask is what is the purpose of having a PF candidate with zero momentum in the first place... |
We had this discussion in the past. In principle zero momentum means that the kinematics aren't reliably estimated to be different than zero within uncertainties. But they can still play a role in candidate counting etc. I wouldn't say they are super useful, but it also wasn't obvious to me that we should kill them. Regardless we should avoid that zeros turn to nans, there's no world in which that is desirable. |
FWIW, at least these lines in cmssw/DataFormats/PatCandidates/src/PackedCandidate.cc Lines 42 to 43 in d376eb3
will result NaN with zero momentum track. |
If I recall correctly, I think the track properties are not stored in the packed PF candidate produced by the PAT packed candidate producer if the pt < 0.95 GeV |
This was lowered to |
Ah yes, but only if it strictly larger than 0 GeV (so candidates with pT = 0 would then have no track properties) |
thanks Matti this[1] change seems to help; Examples from Before change [1]
after change [1]
[1] --- a/DataFormats/PatCandidates/src/PackedCandidate.cc
+++ b/DataFormats/PatCandidates/src/PackedCandidate.cc
@@ -40,7 +40,9 @@ void pat::PackedCandidate::packVtx(bool unpackAfterwards) {
// float dl = dxPV * c + dyPV * s;
// float xRec = - dxy_ * s + dl * c, yRec = dxy_ * c + dl * s;
float pzpt = p4_.load()->Pz() / p4_.load()->Pt();
- dz_ = vertex_.load()->Z() - pv.Z() - (dxPV * c + dyPV * s) * pzpt;
+ if (!std::isnan(pzpt)) {
+ dz_ = vertex_.load()->Z() - pv.Z() - (dxPV * c + dyPV * s) * pzpt;
+ }
|
I'd suggest to check e.g. |
okay, how about this? - float pzpt = p4_.load()->Pz() / p4_.load()->Pt();
- dz_ = vertex_.load()->Z() - pv.Z() - (dxPV * c + dyPV * s) * pzpt;
+ dz_ = 0;
+ if (p4_.load()->Pt() != 0.f) {
+ float pzpt = p4_.load()->Pz() / p4_.load()->Pt();
+ dz_ = vertex_.load()->Z() - pv.Z() - (dxPV * c + dyPV * s) * pzpt;
+ } before the change
after the change
|
Looks reasonable to me, thanks. |
okay, I'm doing some more sanity checks, then I'll open a PR |
so its merged in 14_1 #45009 |
A backport to 14_0 sounds great to me. This should also only make it necessary to backport the DeepMET input check for possible ReNanoAOD campaigns. |
+1 |
This issue is fully signed and ready to be closed. |
Hi @swagata87 , all, something that is a little confusing in this comment #44976 (comment) is that you show |
It looks like there are multiple places where |
Checking the DeepMET performance in run 3, we found a significant fraction of events for which DeepMET evaluates to NAN, up to 1% in a four-top sample. We've just traced this down to events where we have a PF candidate where dz() returns NAN, which we don't catch https://github.com/cms-sw/cmssw/blob/master/RecoMET/METPUSubtraction/plugins/DeepMETProducer.cc#L85
The fix will be easy enough (just checking for NAN), but I was wondering if dz() returning NAN for a PF candidate is expected or if it could impact something else.
Tagging co-developers (@yongbinfeng @mseidel42) and whoever i could easily find on github from JME (@alkaloge) PF (@swagata87) Reco (@mandrenguyen)
The text was updated successfully, but these errors were encountered: