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

Cleanup of Old MVA assignment / kill dead code / bugfixes #1644

Merged
merged 3 commits into from Dec 10, 2013

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Dec 2, 2013

Built ontop of 700pre8 + patches, can rebase if needed.

Fix assignment of electron cluster and improve agreement of "mva" variable, you can see the improvement in agreement between mva values for (tracker-Driven && ECAL-Driven) electrons. The old mva-id isn't cut on anywhere in the ged reconstruction, so it is purely there for comparison with the old electrons / cosmetics.

Removed a few hundred lines of dead code from PFEGammaAlgo.cc/.h, none of it was called so this is purely cosmetic.

Fixed yet another EB/EE mixing bug, fixed a bug where clusters were being double counted.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2013

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

Cleanup of Old MVA assignment + kill dead code

It involves the following packages:

RecoParticleFlow/PFProducer

@nclopezo, @cmsbuild, @thspeer, @slava77 can you please review it and eventually sign? Thanks.
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.
@ktf you are the release manager for this.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2013

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 3, 2013

@slava77
Copy link
Contributor

slava77 commented Dec 5, 2013

@slava77 working on it

@slava77
Copy link
Contributor

slava77 commented Dec 6, 2013

Test results for 761de18 done in CMSSW_7_0_X_2013-12-05-0200 as sign277

Normal mode: changes are only in gedGsfElectron mvaInput_earlyBrem and mvaOutput_mva.

mvaInput_earlyBrem changes seem a bit more than epsilon.
See, e.g. the change for gamma-35GeV sample
all_sign277vsorig_singlegammapt35wf19p0c_recogsfelectrons_gedgsfelectrons__reco_obj_mvainput_earlybrem

The downstream effect of the above change is visible in the mva output. e.g., same sample
all_sign277vsorig_singlegammapt35wf19p0c_recogsfelectrons_gedgsfelectrons__reco_obj_mvaoutput_mva

@slava77
Copy link
Contributor

slava77 commented Dec 6, 2013

GED mode:
the mva changes are visible at the same level, as expected.

In addition, there are changes in the particle content (more PF candidates assigned as electrons, more visible in background or photon samples)

e.g. gamma35 sample eta of pfCandidates assigned to electrons, increase of ~5%, mostly in EE
all_sign277-withgedvsorig-withged_singlegammapt35wf19p0c_recopfcandidates_particleflow__reco_obj_eta17

Shapes of the distributions (other than mva) didn't change appreciably.

@slava77
Copy link
Contributor

slava77 commented Dec 6, 2013

Lindsey,

are these increases in electron assignments to pfcandidates expected (the effect seems to be 5-10%, enhanced in events with real photons)?

@cmsbuild
Copy link
Contributor

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

@lgray
Copy link
Contributor Author

lgray commented Dec 10, 2013

@slava77 Fixed the bug for assigning the early brem flag. Should be fairly similar now. I also included a patch to prevent proto-EGCandidates from getting merged if they both have a GSF track and clusters. This may also solve the problem of the crash that you sent an email about.

@cmsbuild
Copy link
Contributor

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

fix the assignment of early/late brem.
fix local locking of ECAL clusters.
@cmsbuild
Copy link
Contributor

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

@slava77
Copy link
Contributor

slava77 commented Dec 10, 2013

+1

Comparisons, based on 4261d3f in CMSSW_7_0_X_2013-12-05-0200 (sign277a)

Regular mode first:

  • generally small changes
  • PFEGammaAlgo::mergeROsByAnyLink: PFEGammaProducer:particleFlowEGamma warning is gone (good)
  • earlyBrem variable now has values at -1
  • changes are only in particleFlowEGamma|logErrorHarvester|gedGsfElectrons|gedPhotons

Most visible changes are in gamma10 , which has the most sensitivity thanks for being near threshold.
Diffs are visible only for low R9 (<0.6)

Electron Pt 35: the only noticeable drop is in the overlap region (eta~1.5),
screen shot 2013-12-10 at 6 02 33 pm

GED Mode
In general, changes are as expected.

  • Changes in gamma pt=10 sample are the most visible:
    • ~10% drop in golden electrons, almost matching increase in showering
  • other samples, esp fake-dominated have reduced yields of photons, mostly at low R9

@slava77
Copy link
Contributor

slava77 commented Dec 10, 2013

forgot to mention: this PR fixes
"Assertion `bclus.hitsAndFractions().at(0).first.subdetId()==EcalBarrel' failed."
crash in
RECODreHLT jobs failed due to the EcalClusterLocal error.
http://cms-project-relval.web.cern.ch/cms-project-relval/failures/CMSSW_7_0_0_pre9/standard_2/RunJet2012C__RelVal_jet2012C_Job1.tar.gz

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next IBs unless changes or unless it breaks tests. @ktf can you please take care of it?

ktf added a commit that referenced this pull request Dec 10, 2013
Reco fixes -- Cleanup of Old MVA assignment / kill dead code / bugfixes
@ktf ktf merged commit 323efe4 into cms-sw:CMSSW_7_0_X Dec 10, 2013
@cmsbuild
Copy link
Contributor

ggovi pushed a commit to ggovi/cmssw that referenced this pull request Jan 11, 2017
Advance root to tip of branch 6-02-00.
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