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

Integration of the E\gamma Cluster Position Calculator into the PFClusterProducer #706

Merged
merged 2 commits into from Sep 13, 2013

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Sep 3, 2013

@jbendavid @ekfriis @matteosan1 @cms-jet @cms-met

This PR incorporates the result of a study done that demonstrated improved cluster position resolution when the Particle Flow default electron/photon position corrections were replaced with the default E\gamma corrections.

The main improvement is seen in the ECAL endcaps, but there is some slight improvement for the barrel.

You can see the study here:
https://indico.cern.ch/getFile.py/access?contribId=0&resId=0&materialId=slides&confId=270535

I've @'d the Tau POG and jet/met since this should impact their particle-based reconstruction.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 3, 2013

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

Integration of the E\gamma Cluster Position Calculator into the PFClusterProducer

It involves the following packages:

RecoEcal/EgammaCoreTools
RecoParticleFlow/Configuration
RecoParticleFlow/PFClusterProducer

@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.

@nclopezo
Copy link
Contributor

nclopezo commented Sep 4, 2013

Hi,

I ran the tests on top of CMSSW_7_0_X_2013-09-04-0200, all tests passed:

https://cmssdt.cern.ch/jenkins/job/Pull-Request-Integration/ARCHITECTURE=slc5_amd64_gcc472/381/console

you can see the artifacts here:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/381

@slava77
Copy link
Contributor

slava77 commented Sep 12, 2013

working on it @slava77

if( 0 == iRecHits ||
0 == iSubGeom )
{
throw(std::runtime_error("\n\nPositionCalc::Calculate_Location called uninitialized or wrong initialization.\n\n"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a cms::Exception

@slava77
Copy link
Contributor

slava77 commented Sep 13, 2013

@lgray
Lindsey, please follow up on timing performance of particleFlowClusterECAL
The new version is taking ~4-6 times more depending on the test workflow
Thanks

@slava77
Copy link
Contributor

slava77 commented Sep 13, 2013

+1

(If this is still making it to pre4, please merge, otherwise, we can take a bit of time to solve CPU/timing degradation)

tested a311925 in CMSSW_7_0_X_2013-09-10-0200

As expected, observe significant improvement (mostly in endcaps) of reco-true matching in phi (mainly) and a bit in eta.
This is visible in electron and photon gun samples, nothing is really visible in high-PU or QCD samples, as it should be (no real stuff to do matching here).

I observe no degradation in MET.
Supposedly, there should also be an improvement in tau reco (mostly a guess, nothing visible in the test samples, which don't really have clean taus)

Main regression is in CPU performance: increase of a factor of 4-6 in timing of particleFlowClusterECAL.
Physics improvements would benefit more from a full validation cycle now.

@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 Sep 13, 2013
Integration of the E\gamma Cluster Position Calculator into the PFClusterProducer
@ktf ktf merged commit f98e088 into cms-sw:CMSSW_7_0_X Sep 13, 2013
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