Fix a discrepancy in Puppi weights when running on MiniAOD w/ useExistingWeights=False #28033

hqucms · 2019-09-20T07:52:27Z

PR description:

When running Puppi on MiniAOD w/ useExistingWeights=False, in some cases there is a discrepancy between the puppiWeight and the rescaled pT (i.e., rescaled pT != original pT * packCand->puppiWeight()). This is tracked to:

cmssw/CommonTools/PileupAlgos/plugins/PuppiProducer.cc

Lines 275 to 291 in fb7c976

    
           int iPuppiMatched = fUseExistingWeights ? val : fPuppiContainer->recoToPup()[val]; 
        
           if (iPuppiMatched >= 0) { 
        
             auto const& puppiMatched = lCandidates[iPuppiMatched]; 
        
             pVec.SetPxPyPzE(puppiMatched.px(), puppiMatched.py(), puppiMatched.pz(), puppiMatched.E()); 
        
             if (fClonePackedCands && (!fUseExistingWeights)) { 
        
               if (fPuppiForLeptons) 
        
                 pCand->setPuppiWeight(pCand->puppiWeight(), lWeights[iPuppiMatched]); 
        
               else 
        
                 pCand->setPuppiWeight(lWeights[iPuppiMatched], pCand->puppiWeightNoLep()); 
        
             } 
        
           } else { 
        
             pVec.SetPxPyPzE(0, 0, 0, 0); 
        
             if (fClonePackedCands && (!fUseExistingWeights)) { 
        
               pCand->setPuppiWeight(0, 0); 
        
             } 
        
           } 
        
           puppiP4s.push_back(pVec);

on L281 and L283, iPuppiMatched should not be used to access the puppi weight from lWeights, as lWeights is 1-to-1 with hPFProduct -- instead val is the correct index.

As this only affects the puppi weights set for the new packedCandidates, this discrepancy does not affect cases when useExistingWeights=True, or running on RECO/AOD inputs.

This PR fixes this discrepancy and also simplifies the logic to compute the Puppi-scaled p4.

@ahinzmann

cmsbuild · 2019-09-20T07:52:57Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-09-20T07:59:04Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28033/11959

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CommonTools/PileupAlgos/interface/PuppiContainer.h modified in PR(s): Simplify PUPPI Photon producer and change PUPPI isolation algo #27929
- File CommonTools/PileupAlgos/plugins/PuppiProducer.cc modified in PR(s): Simplify PUPPI Photon producer and change PUPPI isolation algo #27929, Updating Puppiproducer to include fillDescriptions v2 #28020
- File CommonTools/PileupAlgos/src/PuppiContainer.cc modified in PR(s): Simplify PUPPI Photon producer and change PUPPI isolation algo #27929

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28033/11959/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28033/11959/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

ahinzmann · 2019-09-20T07:59:22Z

@hqucms this would be a nice simplification.
I wonder if this also works for puppiNoLeptons and puppiForMET, where the particle list is shorter than the original list of candidates, because leptons are removed before running PUPPI?
(if it breaks puppiNoLeptons, we could consider to include a doNoLeptons into the PuppiProducer and get rid of the candidate filter in puppiForMET, simplifying also puppiForMET)

cmsbuild · 2019-09-20T08:08:26Z

The code-checks are being triggered in jenkins.

hqucms · 2019-09-20T08:13:14Z

@ahinzmann I think nothing should break as the weight list lWeights is always equal to the input candidate list, right? The output particle/p4 list (lCandidates) is not needed after the simplification.

cmsbuild · 2019-09-20T08:15:31Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28033/11961

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CommonTools/PileupAlgos/interface/PuppiContainer.h modified in PR(s): Simplify PUPPI Photon producer and change PUPPI isolation algo #27929
- File CommonTools/PileupAlgos/plugins/PuppiProducer.cc modified in PR(s): Simplify PUPPI Photon producer and change PUPPI isolation algo #27929, Updating Puppiproducer to include fillDescriptions v2 #28020
- File CommonTools/PileupAlgos/src/PuppiContainer.cc modified in PR(s): Simplify PUPPI Photon producer and change PUPPI isolation algo #27929

cmsbuild · 2019-09-20T08:16:03Z

A new Pull Request was created by @hqucms (Huilin Qu) for master.

It involves the following packages:

CommonTools/PileupAlgos

@perrotta, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @ahinzmann, @riga, @jdolen, @gkasieczka, @clelange this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

ahinzmann · 2019-09-20T08:25:23Z

Ok. Just since I got paranoid with this piece of code, could you make a comparison of puppiMET and the weights of the puppiForMET particles:

running makePuppies on AOD (just like in the official MiniAOD sequence)
running makePuppiesFromMiniAOD on MiniAOD with useExistingWeights=True
running makePuppiesFromMiniAOD on MiniAOD with useExistingWeights=False
I believe 1. and 2. should be part of some validation workflows, but 3. is not.

In fact it would be nice to have example config for those three cases in the /test folder as well. Currently there is only a test for 2.

hqucms · 2019-09-20T08:44:39Z

@ahinzmann Since 1 and 2 are in the PR validation workflow, maybe we can first trigger a PR test to check them.

perrotta · 2019-09-20T08:50:32Z

please test

cmsbuild · 2019-09-20T08:50:58Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2589/console Started: 2019/09/20 12:13

cmsbuild · 2019-09-20T11:47:11Z

+1
Tested at: fc452b1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dc8543/2589/summary.html

cmsbuild · 2019-09-20T14:48:11Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dc8543/2589/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 66 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2958092
DQMHistoTests: Total failures: 303
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2957448
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 145 log files, 15 edm output root files, 34 DQM output files

hqucms · 2019-09-26T01:02:28Z

Ok. Just since I got paranoid with this piece of code, could you make a comparison of puppiMET and the weights of the puppiForMET particles:

running makePuppies on AOD (just like in the official MiniAOD sequence)

running makePuppiesFromMiniAOD on MiniAOD with useExistingWeights=True

running makePuppiesFromMiniAOD on MiniAOD with useExistingWeights=False
I believe 1. and 2. should be part of some validation workflows, but 3. is not.

In fact it would be nice to have example config for those three cases in the /test folder as well. Currently there is only a test for 2.

@ahinzmann
I have run the tests on a JetHT 2017F data file for the 3 options and found no change in puppiMET with this PR [0]. I have not figured out how to access the weights for puppiForMET particles though -- it seems that they are not saved in the event content?

[0] https://cernbox.cern.ch/index.php/s/LUlVAKr3vSH47Cp

hqucms · 2019-09-26T01:15:57Z

Regarding the small changes in the b-tagging discriminants in the PR test: they are caused by changes in numerical precision in the calculation of the puppi scaled p4. Before the PR, the candidate p4 is computed with double -> float (when creating the RecoObj) -> double, now is double -> double without the intermediate conversion. Inserting an intermediate conversion to float can reproduce the old behavior exactly.

ahinzmann · 2019-09-26T06:57:11Z

@hqucms The puppiForMET particles are indeed no saved. But the puppiNoLepton weights (which is a subset of puppiForMET) are stored in the packedCandidates.
Regarding b-tagging, I'd say we should go for the solution without the intermediate conversion.

slava77 · 2019-09-30T08:25:15Z

@ahinzmann
please clarify if your comments earlier in the thread were already addressed.
Thank you.

ahinzmann · 2019-09-30T13:06:43Z

Looking at the three sumET plots in [0], useExistingWeights=True and False indeed look almost the same, however aod2mini looks very different from the other two. It doesn't seem caused by this PR, but certainly something that we need to follow up on.
@hqucms You plotted sumET, but in fact it's better to look at MET (simply called ".pt" in slimmedMETsPuppi)

[0] https://cernbox.cern.ch/index.php/s/LUlVAKr3vSH47Cp

I conclude that the PR is fine.

slava77 · 2019-10-02T15:53:42Z

+1

for #28033 fc452b1

code changes are in line with the PR description
jenkins tests pass and comparisons with the baseline show small differences in puppi-related variables in PAT jets and MET
- local tests show that slimmedJetsPuppi have changes in their kinematics at numerical precision level (the pt stays the same, but eta changes within delta of 4e-6).

fabiocos · 2019-10-04T20:08:45Z

+1

fabiocos · 2019-10-04T20:08:50Z

merge

ahinzmann · 2019-10-10T15:13:30Z

@hqucms @ALL
Could we backport this to 10_2 and 10_6? During PUPPI tuning activities we stumbled on the same issue and would like to make sure no JetMET analysis is affected by this. Even if production is not affected by this.

santocch · 2019-10-12T06:43:07Z

+1

cmsbuild · 2019-10-12T06:43:32Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will be automatically merged.

hqucms · 2019-10-14T15:50:01Z

@hqucms @ALL
Could we backport this to 10_2 and 10_6? During PUPPI tuning activities we stumbled on the same issue and would like to make sure no JetMET analysis is affected by this. Even if production is not affected by this.

@ahinzmann
Please see #28169 and #28170 for the backports. I made a minimal change to fix the weights, leaving out the code simplification part in this PR (which leads to some small changes in e.g., the b-tag discs at numerical precision level, due to the removal of some float--double conversion).

…-sw#28033)

cmsbuild added this to the CMSSW_11_0_X milestone Sep 20, 2019

cmsbuild added analysis-pending code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 20, 2019

cmsbuild added code-checks-rejected and removed code-checks-pending labels Sep 20, 2019

Fix discrepancy when useExistingWeights=False.

fc452b1

hqucms force-pushed the fix-puppi-master branch from d4d3006 to fc452b1 Compare September 20, 2019 08:07

cmsbuild added code-checks-pending and removed code-checks-rejected labels Sep 20, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 20, 2019

cmsbuild added tests-started and removed tests-pending labels Sep 20, 2019

cmsbuild added comparison-available and removed comparison-pending labels Sep 20, 2019

hqucms closed this Sep 26, 2019

hqucms reopened this Sep 26, 2019

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Oct 2, 2019

cmsbuild added orp-approved and removed orp-pending labels Oct 4, 2019

cmsbuild merged commit 7903ae0 into cms-sw:master Oct 4, 2019

cmsbuild added analysis-approved fully-signed and removed analysis-pending pending-signatures labels Oct 12, 2019

This was referenced Oct 14, 2019

[10_6_X] Fix a discrepancy in Puppi weights when running on MiniAOD w/ useExistingWeights=False #28169

Merged

[10_2_X] Fix a discrepancy in Puppi weights when running on MiniAOD w/ useExistingWeights=False #28170

Merged

kpedro88 added a commit to kpedro88/cmssw that referenced this pull request Apr 27, 2022

avoid use of lCandidates for useExistingWeights case (inspired by cms…

8bf214a

…-sw#28033)

kpedro88 mentioned this pull request Apr 27, 2022

Backport Puppi speedups [10_6_X] #37718

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a discrepancy in Puppi weights when running on MiniAOD w/ useExistingWeights=False #28033

Fix a discrepancy in Puppi weights when running on MiniAOD w/ useExistingWeights=False #28033

hqucms commented Sep 20, 2019

cmsbuild commented Sep 20, 2019

cmsbuild commented Sep 20, 2019

ahinzmann commented Sep 20, 2019 •

edited

cmsbuild commented Sep 20, 2019

hqucms commented Sep 20, 2019

cmsbuild commented Sep 20, 2019

cmsbuild commented Sep 20, 2019

ahinzmann commented Sep 20, 2019 •

edited

hqucms commented Sep 20, 2019

perrotta commented Sep 20, 2019

cmsbuild commented Sep 20, 2019 •

edited

cmsbuild commented Sep 20, 2019

cmsbuild commented Sep 20, 2019

hqucms commented Sep 26, 2019

hqucms commented Sep 26, 2019

ahinzmann commented Sep 26, 2019

slava77 commented Sep 30, 2019

ahinzmann commented Sep 30, 2019 •

edited

slava77 commented Oct 2, 2019

fabiocos commented Oct 4, 2019

fabiocos commented Oct 4, 2019

ahinzmann commented Oct 10, 2019

santocch commented Oct 12, 2019

cmsbuild commented Oct 12, 2019

hqucms commented Oct 14, 2019

	int iPuppiMatched = fUseExistingWeights ? val : fPuppiContainer->recoToPup()[val];
	if (iPuppiMatched >= 0) {
	auto const& puppiMatched = lCandidates[iPuppiMatched];
	pVec.SetPxPyPzE(puppiMatched.px(), puppiMatched.py(), puppiMatched.pz(), puppiMatched.E());
	if (fClonePackedCands && (!fUseExistingWeights)) {
	if (fPuppiForLeptons)
	pCand->setPuppiWeight(pCand->puppiWeight(), lWeights[iPuppiMatched]);
	else
	pCand->setPuppiWeight(lWeights[iPuppiMatched], pCand->puppiWeightNoLep());
	}
	} else {
	pVec.SetPxPyPzE(0, 0, 0, 0);
	if (fClonePackedCands && (!fUseExistingWeights)) {
	pCand->setPuppiWeight(0, 0);
	}
	}
	puppiP4s.push_back(pVec);

Fix a discrepancy in Puppi weights when running on MiniAOD w/ useExistingWeights=False #28033

Fix a discrepancy in Puppi weights when running on MiniAOD w/ useExistingWeights=False #28033

Conversation

hqucms commented Sep 20, 2019

PR description:

cmsbuild commented Sep 20, 2019

cmsbuild commented Sep 20, 2019

ahinzmann commented Sep 20, 2019 • edited

cmsbuild commented Sep 20, 2019

hqucms commented Sep 20, 2019

cmsbuild commented Sep 20, 2019

cmsbuild commented Sep 20, 2019

ahinzmann commented Sep 20, 2019 • edited

hqucms commented Sep 20, 2019

perrotta commented Sep 20, 2019

cmsbuild commented Sep 20, 2019 • edited

cmsbuild commented Sep 20, 2019

cmsbuild commented Sep 20, 2019

hqucms commented Sep 26, 2019

hqucms commented Sep 26, 2019

ahinzmann commented Sep 26, 2019

slava77 commented Sep 30, 2019

ahinzmann commented Sep 30, 2019 • edited

slava77 commented Oct 2, 2019

fabiocos commented Oct 4, 2019

fabiocos commented Oct 4, 2019

ahinzmann commented Oct 10, 2019

santocch commented Oct 12, 2019

cmsbuild commented Oct 12, 2019

hqucms commented Oct 14, 2019

ahinzmann commented Sep 20, 2019 •

edited

ahinzmann commented Sep 20, 2019 •

edited

cmsbuild commented Sep 20, 2019 •

edited

ahinzmann commented Sep 30, 2019 •

edited