Update to fix late validation plots from PR 15562 #15669
Conversation
…en CandViewMerger is used, the referening of the packedcandidates to the orignal leptons is somehow broken. PuppiPhoton relies on these references to remove the give pfcandidates assigned to a good photon weight 1. If this is broken Puppi photon will add back the missing candidates even if they are there. The solution is to either run puppi no lepton or puppiphoton, but not both. The problem with this is that puppi vs puppi no lepton will reduce the MET response by a 2-3% whereas puppiphoton vs puppi will make puppiMET robust in photon+jet events. We prfer to have puppi photon. On another note, after miniAOD is run, the puppinolepton weights attached to the packed candidates give weight 1 for the leptons, so puppiphoton can be run on the miniAOD using the precomputed puppinolepton weights allowing for the best puppiMET peformance.
|
A new Pull Request was created by @violatingcp (Philip Harris) for CMSSW_8_1_X. It involves the following packages: PhysicsTools/PatAlgos @cmsbuild, @cvuosalo, @slava77, @montjj, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are list here #13028 |
| @@ -21,8 +21,8 @@ def makePuppies( process ): | |||
| process.puppiMerged = cms.EDProducer("CandViewMerger",src = cms.VInputTag( 'puppiNoLep','pfLeptonsPUPPET')) | |||
| process.load('CommonTools.PileupAlgos.PhotonPuppi_cff') | |||
| process.puppiForMET = process.puppiPhoton.clone() | |||
| process.puppiForMET.puppiCandName = 'puppiMerged' | |||
|
|
|||
| #process.puppiForMET.puppiCandName = 'puppiMerged' | |||
There was a problem hiding this comment.
is this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant
|
@violatingcp please propagate the fix to 80X, better by updating #15429 |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Updated the PR was not merged, so the update is easier in this case. On Tue, Aug 30, 2016 at 10:21 PM, Slava Krutelyov notifications@github.com
|
|
I'm not sure that this PR fixes the problems seen in #15562. I tested workflow 25200.0_ZEE_13 with 500 events against baseline CMSSW_8_1_0_pre11. The test results show that the changes in Puppi jet energy and Et are not reversed by this PR. The other changes seen in #15562 are not reversed either, as seen below. |
|
Thats correct. Only the effect of the electrons on the MET will change. On Tue, Sep 6, 2016 at 12:10 AM, Carl Vuosalo notifications@github.com
|
|
In addition, there is a large shuffling of Puppi MET phi values, which seems odd, as well as significant changes in electron and muon Et, as shown below. |
|
@violatingcp: Please elaborate. Could you please list the problems seen in testing of #15562, of those, which are intended to be addressed by this PR? |
|
I have no idea what black is and it seems the events you are comparing to
On Tue, Sep 6, 2016 at 12:26 AM, Carl Vuosalo notifications@github.com
|
|
@violatingcp |
|
Why are the events different and what is black in the second plot? The code On Tue, Sep 6, 2016 at 12:50 AM, Slava Krutelyov notifications@github.com
|
…nd rather do a delta R matching to the candidates. This allows for use on Reco and and Puppi NoLepton collections without directly relying on the references to fix things. This is a result of studies which show that using the collection puppiMerged or the reco Puppi candidates breaks the use of referene keys to link things making Delta R the only option. The reco code is thus updated with what should be the BEST puppi MET version. However the validated version consists of commenting out the puppiMerged line
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
@cvuosalo if you follow up with the review, please make sure to check the timing on events with PU (the same ZEE sample should be OK). |
…nd rather do a delta R matching to the candidates. This allows for use on Reco and and Puppi NoLepton collections without directly relying on the references to fix things. This is a result of studies which show that using the collection puppiMerged or the reco Puppi candidates breaks the use of referene keys to link things making Delta R the only option. The reco code is thus updated with what should be the BEST puppi MET version. However the validated version consists of commenting out the puppiMerged line. This pull request follows the equivalent 81X verision cms-sw#15669
|
@violatingcp
@slava77 do you agree ? |
|
On 9/8/16 8:20 AM, mariadalfonso wrote:
Sounds good. In interest of doing less harm, in case these two steps have other issues, I guess you can update #15429
|
|
@slava77 @cvuosalo let us know how your validation of this extra DR. #15772 (descoped) will not be modified |
|
After discussion with @mariadalfonso we have decided #15669 contains all the fixes we would like. The proposal of two additional steps would require a larger re-engineering and consequently we stay with the current #15429 for the 80X release and #15669 for the 81X provided the checks come back with the expected good performance. |
|
@cvuosalo we should keep 81X in sync with all new features/bugfixes that were added to 80X. |
|
+1 Puppi photon fixes for #15562. This PR is related to #15429, #15562, and #15772 . The code changes match the corresponding changes in #15429, which is already merged. Jenkins tests against baseline CMSSW_8_1_X_2016-09-08-0900 show numerous tiny DQM plot changes, but most are not significant. The slightly larger changes are as expected, with two examples shown below. See #15429, which represents this PR plus #15562 for 80X, for more test results and plots. From Jenkins workflow 25202.0_TTbar_13:
|
…nd rather do a delta R matching to the candidates. This allows for use on Reco and and Puppi NoLepton collections without directly relying on the references to fix things. This is a result of studies which show that using the collection puppiMerged or the reco Puppi candidates breaks the use of referene keys to link things making Delta R the only option. The reco code is thus updated with what should be the BEST puppi MET version. However the validated version consists of commenting out the puppiMerged line. This pull request follows the equivalent 81X verision cms-sw#15669
Following concerns in #15562 we have fix based on the validation plots. This results from the fact that when CandViewMerger is used, the referening of the packedcandidates to the orignal leptons is somehow broken. PuppiPhoton relies on these references to remove the give pfcandidates assigned to a good photon weight 1. If this is broken Puppi photon will add back the missing candidates even if they are there. The solution is to either run puppi no lepton or puppiphoton, but not both. The problem with this is that puppi vs puppi no lepton will reduce the MET response by a 2-3% whereas puppiphoton vs puppi will make puppiMET robust in photon+jet events. We prfer to have puppi photon. On another note, after miniAOD is run, the puppinolepton weights attached to the packed candidates give weight 1 for the leptons, so puppiphoton can be run on the miniAOD using the precomputed puppinolepton weights allowing for the best puppiMET peformance.