Skip to content
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

Modifier to apply energy corrections to photons and electrons (miniAOD) for 76X #10689

Merged
merged 9 commits into from
Aug 24, 2015

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Aug 11, 2015

Like #10690 but for 76X. Implements most of the comments from Slava on the 75X version.
Payloads for GT have been queued for integration.

Supersedes #10650

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_7_6_X.

Modifier to apply energy corrections to photons and electrons (miniAOD) for 76X

It involves the following packages:

PhysicsTools/PatAlgos
RecoEgamma/EgammaPhotonProducers
RecoEgamma/EgammaTools
RecoEgamma/ElectronIdentification
RecoEgamma/PhotonIdentification

@cmsbuild, @cvuosalo, @vadler, @monttj, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @Sam-Harper, @imarches, @ahinzmann, @acaudron, @mmarionncern, @jdolen, @nhanvtran, @schoef, @ferencek, @gpetruc, @mariadalfonso, @pvmulder, @TaiSakuma this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@lgray
Copy link
Contributor Author

lgray commented Aug 11, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@lgray
Copy link
Contributor Author

lgray commented Aug 11, 2015

@slava77 Apologies, these tests will fail. Some commits weren't pushed.

@cmsbuild
Copy link
Contributor

-1
Tested at: 96fcf41
When I ran the RelVals I found an error in the following worklfows:
5.1 step1

runTheMatrix-results/5.1_TTbar+TTbarFS+HARVESTFS/step1_TTbar+TTbarFS+HARVESTFS.log

8.0 step1

runTheMatrix-results/8.0_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS/step1_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS.log

9.0 step1

runTheMatrix-results/9.0_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST/step1_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST.log

25.0 step1

runTheMatrix-results/25.0_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT/step1_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT.log

135.4 step1

runTheMatrix-results/135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step1_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log

1306.0 step1

runTheMatrix-results/1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15/step1_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15.log

1330.0 step1

runTheMatrix-results/1330.0_ZMM_13+ZMM_13+DIGIUP15+RECOUP15+HARVESTUP15/step1_ZMM_13+ZMM_13+DIGIUP15+RECOUP15+HARVESTUP15.log

101.0 step1

runTheMatrix-results/101.0_SingleElectronE120EHCAL+SingleElectronE120EHCAL/step1_SingleElectronE120EHCAL+SingleElectronE120EHCAL.log

25202.0 step1

runTheMatrix-results/25202.0_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25/step1_TTbar_13+TTbar_13+DIGIUP15_PU25+RECOUP15_PU25+HARVESTUP15_PU25.log

50202.0 step1

runTheMatrix-results/50202.0_TTbar_13+TTbar_13+DIGIUP15_PU50+RECOUP15_PU50+HARVESTUP15_PU50/step1_TTbar_13+TTbar_13+DIGIUP15_PU50+RECOUP15_PU50+HARVESTUP15_PU50.log

4.22 step1

DAS Error

4.53 step1

DAS Error

140.53 step1

DAS Error

1000.0 step1

DAS Error

1001.0 step1

DAS Error

1003.0 step1

DAS Error

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10689/7147/summary.html

@cmsbuild
Copy link
Contributor

Pull request #10689 was updated. @cmsbuild, @cvuosalo, @vadler, @monttj, @slava77 can you please check and sign again.

@lgray
Copy link
Contributor Author

lgray commented Aug 12, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

writeValueMap(iEvent, src, eleIEta ,eleIEta_);
writeValueMap(iEvent, src, eleCryPhi ,eleCryPhi_);
writeValueMap(iEvent, src, eleCryEta ,eleCryEta_);
lazyTools.reset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this really do anything useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frees memory between events.

@cmsbuild
Copy link
Contributor

-1

Tested at: e9ad0ce
I found errors in the following unit tests:

---> test runtestPhysicsToolsPatAlgos had ERRORS

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10689/7465/summary.html

clusterDPhiToSeed2,
clusterDEtaToSeed0,
clusterDEtaToSeed1,
clusterDEtaToSeed2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, the unyielding desire to copy-paste.
To change this list, there are 8 places to edit.
😱

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Aug 19, 2015

+1

for #10689 e9ad0ce

  • code changes are OK (some excessive copy-pasting is expected to be refactored later)
  • jenkins tests pass (ignoring the unit test failing in IB as well); comparisons
  • conclusions on performance from the previous iteration of review Modifier to apply energy corrections to photons and electrons (miniAOD) for 76X #10689 (comment) are essentially unchanged
    • the last set of updates (after 0c46c3e) changed the electron parameter computations and this shows up as a smaller change in slimmed electron pt [this is from 251721 DoubleMu events]
      wfdoublemuon251721-0c46c3e-e9ad0ce-slimele-pt

the wishlist for the future:

  • reduce the size of GBRForestD [or check that it's not generating excessively large trees]
  • refactor the map I/O code so that modification to the variable set doesn't trigger changes in ~(10) places.

@slava77
Copy link
Contributor

slava77 commented Aug 20, 2015

Here are some plots from 200 ZEE PU35@25ns events (there are only 5; dEta is boring and dEt/Et is just as good as dEt)
[left or black is baseline]
wf25200_det_vs_et
wf25200_slimele_pt

20-30 GeV shifts are interesting, but I'm not sure if the DQM is using the same electrons in these cases.

@lgray
Copy link
Contributor Author

lgray commented Aug 20, 2015

Z mass?

@lgray
Copy link
Contributor Author

lgray commented Aug 20, 2015

Big shifts can happen in this case since tails aren't getting pulled up by regressing to the mean.

@slava77
Copy link
Contributor

slava77 commented Aug 20, 2015

On 8/20/15 5:09 PM, Lindsey Gray wrote:

Z mass?

Sure, once somebody puts it in the miniADO DQM sequence


Reply to this email directly or view it on GitHub
#10689 (comment).

@lgray
Copy link
Contributor Author

lgray commented Aug 20, 2015

ok

On Fri, Aug 21, 2015 at 12:20 AM, Slava Krutelyov [email protected]
wrote:

On 8/20/15 5:09 PM, Lindsey Gray wrote:

Z mass?

Sure, once somebody puts it in the miniADO DQM sequence


Reply to this email directly or view it on GitHub
#10689 (comment).


Reply to this email directly or view it on GitHub
#10689 (comment).

@lgray
Copy link
Contributor Author

lgray commented Aug 21, 2015

@slava77 double checked the z-peak in 200 ZEE PU35@25ns, using veto working point and taking two highest energy electrons. The improvement in the momentum resolution is obvious even with (the same) 66 events.
This PR is in red.

@matteosan1 @bendavid

screen shot 2015-08-21 at 09 37 16

The new RMS is 5.65 GeV.

@slava77
Copy link
Contributor

slava77 commented Aug 21, 2015

@monttj
please take a look, your signature is needed here
Thank you.

@monttj
Copy link
Contributor

monttj commented Aug 21, 2015

+1
So just to confirm myself,
we are applying this regression energy correction to Electrons and Photons by default?
even for analyses which are not sensitive to the resolution?

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (but tests are reportedly failing). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@lgray
Copy link
Contributor Author

lgray commented Aug 21, 2015

Hi Tae Jeong,

Yes, the next iteration of these corrections will be applied at RECO level.
This was for the MiniAOD reprocessing.

-L

On Fri, Aug 21, 2015 at 6:47 PM, Tae Jeong Kim [email protected]
wrote:

+1
So just to confirm myself,
we are applying this regression energy correction to Electrons and Photons
by default?
even for analyses which are not sensitive to the resolution?


Reply to this email directly or view it on GitHub
#10689 (comment).

davidlange6 added a commit that referenced this pull request Aug 24, 2015
Modifier to apply energy corrections to photons and electrons (miniAOD) for 76X
@davidlange6 davidlange6 merged commit 1b63fff into cms-sw:CMSSW_7_6_X Aug 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants