CHS/Puppi vertex unification follow-up

[jpata]

At the PPD meeting on October 7, some unexpected changes were reported in the JP b-tagger [1] going from 12_1_0_pre2->pre3.
A possible culprit PR could be #33885, where the track-vertex association was modified.
@arizzi has left some comments on the original PR.

Additionally, the following points were brought up by Andrea (please clarify if edits are needed):

1. did you verify that this PR is not killing the "correct PV selection" in events with difficult PV selection?

2. we wanted to have this (track-PV) association in the miniaod candidate without external maps. Concerning the map, the problem is for example they are hardly readable in FWlite or even in simple selectors used to build nanoaod configuration ... and that MiniAOD is meant to be a more analysis friendly format. maps can still be accessed, not the biggest deal but you need to rekey them on PackedPFCands. consolidating the association in the PackedCandidates was the philosophy we used, if we change it we need to understand the consequences in all calling code (that is analysis code you do not have under control)

@ahinzmann, Anna Benecke (I couldn't find her github), could you please follow up on this?

[1] https://indico.cern.ch/event/1082441/#7-validation-report, slide 14

[jpata]

assign reconstruction

[cmsbuild]

New categories assigned: reconstruction

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

[cmsbuild]

A new Issue was created by @jpata Joosep Pata.

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

cms-bot commands are listed here

[arizzi]

one particular problem is with "protecting" ivtx=0 at this level, what is the reason to do it here rather than in the calling code?
this protection could be preventing the proper sorting of the vertices (that would define what ivtx=0 will be after sorting)

[jpata]

@cms-sw/jetmet-pog-l2 @cms-sw/btv-pog-l2 kind ping on this

[arizzi]

I think andreas is off till next week

[ahinzmann]

one particular problem is with "protecting" ivtx=0 at this level, what is the reason to do it here rather than in the calling code? this protection could be preventing the proper sorting of the vertices (that would define what ivtx=0 will be after sorting)

answering to this point (but not the whole issue yet). The protection must of course be disabled for sorting (and is), but only used for association (ideally after sorting).

[arizzi]

@ahinzmann but what is the reason for having this protection in the association? this whitelisting can be always done a posteriori.
Suppose you decide to reinterpret the event looking at #vtx=1 with the current schema you do not know anymore which tracks are associated to vtx0 because they were closest (and other criteria) vs those associated because vtx0 is protected at high pt.

I strongly advise against making such vtx0 protection persistent. It can in fact even added in the MiniAOD accessors (you can edit fromPV to have an additional whitelisting cut)

[jpata]

Note that the plan is to discuss this on Tuesday, Nov 2 at JMAR:
https://indico.cern.ch/event/1091000/#4-tbc-discussion-about-unified

At that meeting, it should be decided if the original PR should be reverted in time for the next release on ~Nov 9: https://twiki.cern.ch/twiki/bin/viewauth/CMS/CMSSW_12_2_0

[jpata]

From the meeting today, it seems like we are waiting for a confirmation from @cms-sw/btv-pog-l2 what is the issue with JP from this PR. We need to be prepared to revert the PR unless the issue is understood & fixed this week.

@ahinzmann @anna.benecke@uclouvain.be could you please prepare a draft PR which reverts the original #33885, so that we can have it tested and ready to be merged before the end of the week? Hopefully the issue will be understood, but just in case.

Thank you.

[ahinzmann]

yes. is there a recipe to create a revert-PR? (like the recipe available for backports)
However, before reverting, we would anyways need understanding if pre3 or pre2 is more reasonable for BTV.
from this plot, it seems like in pre3 AOD and MiniAOD JP ROC is in sync, while in pre2 AOD JP was better than JP at MiniAOD.

[jpata]

yes. is there a recipe to create a revert-PR? (like the recipe available for backports)

I'm not aware of a CMS recipe, but the usual git flow seems to work.

git checkout master
git pull
git checkout -b revert_33885
#revert all commits from 33885 one-by-one in reverse order, solve conflicts as appropriate
git revert d216e83ddb3f0bae19a77ad33479a84f72d162a5
... 
git revert e10f0bf39dc5f91381095a896922cd258718fd0f

However, before reverting, we would anyways need understanding if pre3 or pre2 is more reasonable for BTV.

@cms-sw/btv-pog-l2 should comment on this. In addition, BTV please confirm definitely that the culprit for JP differences is this PR (that the differences in JP appear after this PR and nothing else).

Thanks!

[jpata]

In case it's helpful, I checked 12_1_0_pre2 (ref) vs. 12_1_0_pre2+#33885 (new) on the Run3 workflow 11834.21 with 1000 events. All the plots can be found here: https://jpata.web.cern.ch/jpata/reco/chs_puppi_vtx_issue_35583/

I don't see a major difference in JP from the PR. The differences in RECO/PAT quantities look small, similar to what JMAR has reported and what was originally seen on the PR #33885.

RECO: recoJetedmRefToBaseProdTofloatsAssociationVector_pfJetProbabilityBJetTags

PAT: patJets_slimmedJets__PAT_obj___pairDiscriVector__1__second

@emilbols @cms-sw/btv-pog-l2 can you please check the BTV validation and the plots linked above?

[slava77]

In case it's helpful, I checked 12_1_0_pre2 (ref) vs. 12_1_0_pre2+#33885 (new) on the Run3 workflow 11834.21 with 1000 events. All the plots can be found here: https://jpata.web.cern.ch/jpata/reco/chs_puppi_vtx_issue_35583/

this looks like just the fwlite comparison plots.
Do you have the DQM comparisons?

[johnalison]

Heres an update from the BTV side:

• The miniAOD vs DQM differences are understood and the mini and DQM are indeed both consistent.
  (The original difference was b/c the miniAOD comparison was using the RECOOnly sample, the DQM was not)
• The changes in JP tagger are seen in standard validation samples but not in the RECOOnly sample

pre3:
https://cernbox.cern.ch/index.php/s/w463PAKSbOKA1kN
pre3 RECOonly:
https://cernbox.cern.ch/index.php/s/KX7spf0lAG9Hwt9
(slide 37 is the JP Tagger)

Based on this, I think that BTV we can sign off on pre3 from the performance side: "As changes expected"

[jpata]

Thanks for this, @johnalison!

So we conclude that the PR is fine from the BTV side. It looks like there is no need or justification to revert the PR based on this, at least.

There are two remaining questions:

• the technical / design-choice issues brought up by @arizzi (use of maps, protecting ivtx=0, possible use in downstream analysis, ...)
• we observe a bit of a slowdown in MINI/step4 which may be related to this PR (TBC): slowdown in PFCandidatePrimaryVertexSorter in CMSSW_12_1_0_pre2 -> CMSSW_12_1_0_pre3 #35986

which reco proposes to address in a follow-up PR (rather than a revert). Let us know your thoughts!

[jpata]

Just to conclude on the BTV issues from the reco side, I checked 11834.0 (Run3 ttbar+PU) in two cases:

• pre2 ("ref") vs. pre2+33885 ("this PR")
  
  

• pre2 ("ref") vs. pre3 ("this PR")
  
  

It looks like there is a small difference in the b-jet vs. light jet discrimination introduced by 33885, but a bigger one by something else that entered in pre3 (changes around JP~0). In any case, as far as I see, what was proposed above still applies.

[ahinzmann]

Thanks for this, @johnalison!

So we conclude that the PR is fine from the BTV side. It looks like there is no need or justification to revert the PR based on this, at least.

There are two remaining questions:

* the technical / design-choice issues brought up by @arizzi (use of maps, protecting ivtx=0, possible use in downstream analysis, ...)

* we observe a bit of a slowdown in MINI/step4 which may be related to this PR (TBC): [slowdown in PFCandidatePrimaryVertexSorter in CMSSW_12_1_0_pre2 -> CMSSW_12_1_0_pre3 #35986](https://github.com/cms-sw/cmssw/issues/35986)

which reco proposes to address in a follow-up PR (rather than a revert). Let us know your thoughts!

Agree, we can make a followup PR, thinking of a better place to compute the unified particle-vertex association criterion used for CHS+PUPPI. At MiniAOD-level Andrea suggested fromPV, but need to find a place at AOD-level as well, ideally using the same piece of code as fromPV. Before we touch fromPV, we need to conclude on the studies which criteria to use for CHS Run3.

[qliphy]

@jpata Could this be closed now? ( as JP b-tagger differences are now understood, and #36639 is merged to address follow up questions.)

[jpata]

+reconstruction

[cmsbuild]

This issue is fully signed and ready to be closed.

[perrotta]

+reconstruction

@jpata you are the original author of this issue.
Perhaps, beside signing it, you can also close it: or do you have some other goal in mind for it?

[jpata]

@jpata you are the original author of this issue. Perhaps, beside signing it, you can also close it: or do you have some other goal in mind for it?

I keep forgetting the procedure to close, thanks for the reminder