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

add full5x5 shower variables to reco::Photon #5906

Merged
merged 1 commit into from Oct 22, 2014

Conversation

bendavid
Copy link
Contributor

Correcting an inconsistency which entered in 71x and propagated to 72x.

For history purposes:
In CMSSW_7_0_X the full5x5 shower variables were added to pat::Electron and pat::Photon so they would be easily accessible in the MiniAOD (the need for these variables arose after the reco part of the release was closed and they were added during the miniaod development stage)

In 71X the full5x5 variables were added to the reco::GsfElectron, but due to some oversight/miscommunication never to the reco::Photon. The changes adding the full5x5 variables to the pat::Electron AND photon classes were nevertheless intentionally kept out of 71x, resulting in these variables being nowhere accessible for photons in 72x MiniAOD. (Well they could be recomputed from the rechits, but this is a nasty regression compared to 70x from an end user point of view.)

@lgray @arizzi @gpetruc

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @bendavid (Josh Bendavid) for CMSSW_7_2_X.

add full5x5 shower variables to reco::Photon

It involves the following packages:

DataFormats/EgammaCandidates
RecoEgamma/EgammaPhotonProducers

@cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@lgray 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.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@bendavid
Copy link
Contributor Author

One disclaimer: In order to keep the code change minimal, I've reused the ShowerShape struct (as was done for the electron)

Since the HoverE related variables are actually not depending on full5x5 or not, I left these extra data members as zero.

This can be further cleaned up by splitting the pure EM shower shapes into a separate struct (but then we should do the same for the GsfElectron) and I would suggest to leave that for 73x if we care.

@slava77
Copy link
Contributor

slava77 commented Oct 21, 2014

the code looks ok
I will run a short matrix to see the variables show up

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Oct 21, 2014

+1

for #5906 7aadc58

tested in CMSSW_7_2_0 (test area sign720a)

Here's one example comparing e5x5 default (blue) with the full5x5 e5x5 just as a proof of existence
made in 4.53 (100 events in Photon2012B )
wf4p53_photone5x5_dflt_vs_full5x5

Event size of the gedPhoton branch is up by 10% on 100 events (should be less, I guess on more events); the non-ged photon branch size is up by 1% (the cost of zeros).

@cmsbuild note that I've updated the validate.C script to make plots in my https://github.com/slava77/cms-reco-tools ... how do we get jenkins in sync, we may need to discuss this in the core meeting

On a 72X related subject, I see a random difference in my tests of this PR in a csv variable in wflow 4.37: running valgrind now, to be followed up in slava77#53

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine). @nclopezo can you please take care of it?

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

davidlange6 added a commit that referenced this pull request Oct 22, 2014
add full5x5 shower variables to reco::Photon
@davidlange6 davidlange6 merged commit 32e849f into cms-sw:CMSSW_7_2_X Oct 22, 2014
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.

None yet

4 participants