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

bug fix to EG PF isolation map #7922

Merged
merged 3 commits into from Feb 28, 2015

Conversation

Sam-Harper
Copy link
Contributor

Dear All,

@matteosan1 @lgray

This change effects the product named particleBasedIsolation. It is used by E/gamma in the computation of electron and photon particle flow isolation at analysis level. It is a final product of the reconstruction chain, nothing in RECO depends on it.

This map keeps track of which particle flow candidates make up the GedGsfElectron/GedPhoton. In the case the GedEle/Pho pass PF selection, this should only be the identified particle flow ele/pho. In the case the ele/pho fails PF selection, the ele/pho is broken up into charged/neutral/photons as PFAlgo sees fit. The idea of the map is to flag which particles came from this break up and then veto them in the isolation sum. If you fail to do this, you will get a nice peak of PFIsol/et at 1 as all the ele/phos energy is dumped into the isolation region.

We have seen that the map is slightly bugged and is as such slightly greedy and sums in objects it shouldnt, particularly in the case of photon isolation. After extensive testing, we have decided that the map is to stay the same for neutral and charged hadrons but we will require the PFPhoton's leading ECAL element to be either the GedEle/Pho's supercluster or one of its subclusters in addition to previous requirements. This change fixes this, it is a small improvement but an improvement none the less.

https://indico.cern.ch/event/370494/contribution/3
https://indico.cern.ch/event/305124/contribution/4
https://sharper.web.cern.ch/sharper/cms/talks/debugingEGIsoMap_160215_update.pdf

Cheers,
Sam

… from the electron for it to be considered part of electron
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Sam-Harper for CMSSW_7_4_X.

bug fix to EG PF isolation map

It involves the following packages:

RecoEgamma/EgammaIsolationAlgos

@cmsbuild, @cvuosalo, @nclopezo, @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.
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.

@slava77
Copy link
Contributor

slava77 commented Feb 24, 2015

Hi Sam,

Please change the file name to PF*
It's a more common use case in CMSSW

@Sam-Harper
Copy link
Contributor Author

Done. Just re-running the tests now and will push it shortly.

@cmsbuild
Copy link
Contributor

Pull request #7922 was updated. @cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Feb 24, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

Starting matrix tests

reco::PFCandidateRef pfCandRef(reco::PFCandidateRef(pfCandidateHandle,lCand));

float dR = 0.0;
if( coneSize_ < 10.0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This 10.0 looks like a magic number, which CMS coding rules frown upon. Could you replace it with an appropriately named const float?

@cvuosalo
Copy link
Contributor

@Degano, @nclopezo: Yet another Jenkins failure -- "Comparison with the baseline" stuck in Queued state. I don't actually need the results because I ran my own matrix tests, but it is worrisome that in the last week there have been several Jenkins failures.

@cvuosalo
Copy link
Contributor

+1

For #7922 3e3cb6b

This PR is a bug fix to the E/gamma isolation map. It is not expected to change reconstruction because it involves isolation calculation that occurs after reconstruction.

The code change is satisfactory, as are the Jenkins test results. Matrix tests against baseline CMSSW_7_4_X_2015-02-24-0200 show no significant differences.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @ktf, @smuzaffar

@davidlange6
Copy link
Contributor

+1

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

5 participants