Rewrite and Improvements for PFRecHit and PFCluster Producers #2730

lgray · 2014-03-05T14:19:38Z

Built on top of CMSSW_7_1_X_2014-03-05-0200.
For testing on top of pre3 use the branch: pfcluster_rebuild_onpre3.

This PR is a complete rewrite of PFRecHit and PFCluster Producers/Algos.
The rewrite focuses on making the clustering and rechit building modular and easy to adapt to using new detector information.

NOTE: PFRootEvent is removed from the release, it is likely to come back in some form later.

Expected changes:

Timing improvements to PFRecHit and PFCluster producers across the board
- Some large, esp. preshower rechits
Changes per-cluster in energy and position assignment in some small cases where we have corrected the navigation topology. ECAL and HCAL are affected here.
- Previously it was sometimes the case that a was 'a' neighbor of 'b' but 'b' was not of 'a' in ecal endcap.
- We no longer use the CaloTower det ID navigation, this causes some churn in the HCAL clusters.
Some very minor churn (one in few million clusters) for HO/HF/Preshower from fast_log / fast_expf
Churn in Jet and MET responses per-event due to cluster changes, but no change in the resulting distribution Mean/RMS/Tails.

Additional comments from Michalis (reproduced up here to pull everything together):

Also let me comment that the (almost transient) PF DataFormats on RecHits and Clusters have been changed to accomodate the new changes. Since they didnt allow to assign navigation for more complicated geometries [i.e 3D ]
A quality cut in the HO is removed since the photodetectors are being replaced with SiPMs. We expect an increase in HO clusters and rechits in rings 1/2. This is seen in antons' analysis.
- This quality cut will be added back now in this PR.

bachtis · 2014-03-05T14:23:01Z

Also let me comment that the (almost transient) PF DataFormats on RecHits and Clusters have been changed to accomodate the new changes. Since they didnt allow to assign navigation for more complicated geometries [i.e 3D ]

cmsbuild · 2014-03-05T14:23:29Z

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

Rewrite and Improvements for PFRecHit and PFCluster Producers

It involves the following packages:

DataFormats/ParticleFlowReco
HLTrigger/Configuration
RecoParticleFlow/Configuration
RecoParticleFlow/PFClusterProducer
RecoParticleFlow/PFRootEvent

@perrotta, @cmsbuild, @thspeer, @fwyzard, @Martin-Grunewald, @anton-a, @nclopezo, @slava77, @Degano 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, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

nclopezo · 2014-03-05T14:42:09Z

@lgray
Hi Lindsey,
This pull request is unmergeable, could you please rebase it?

lgray · 2014-03-05T14:42:57Z

@nclopezo what IB should I take?

Martin-Grunewald · 2014-03-05T14:47:15Z

-1
HLTrigger/Configuration changes have to go through ConfDB.

lgray · 2014-03-05T14:49:20Z

@Martin-Grunewald The HLT changes are patch for making tests run so that we could validate this change set. We can remove those changes and go through conf db in another step.

nclopezo · 2014-03-05T14:57:02Z

@lgray you should take CMSSW_7_1_X_2014-03-05-0200

cmsbuild · 2014-03-05T15:03:31Z

Pull request #2730 was updated. @perrotta, @cmsbuild, @thspeer, @fwyzard, @Martin-Grunewald, @anton-a, @nclopezo, @slava77, @Degano can you please check and sign again.

lgray · 2014-03-05T15:07:43Z

re-rebasing to CMSSW_7_1_X_2014-03-05-0200... jumped the gun and rebased to the afternoon IB first.

lgray · 2014-03-05T15:16:46Z

@nclopezo now on CMSSW_7_1_X_2014-03-05-0200

cmsbuild · 2014-03-05T15:18:27Z

Pull request #2730 was updated. @perrotta, @cmsbuild, @thspeer, @fwyzard, @Martin-Grunewald, @anton-a, @nclopezo, @slava77, @Degano can you please check and sign again.

lgray · 2014-03-05T15:19:08Z

Sorry for the noise, fixed obvious problem from rebasing.

cmsbuild · 2014-03-05T15:23:27Z

Pull request #2730 was updated. @perrotta, @cmsbuild, @thspeer, @fwyzard, @Martin-Grunewald, @anton-a, @nclopezo, @slava77, @Degano can you please check and sign again.

lgray · 2014-03-05T16:41:06Z

---- PLOT DUMP ----

ECAL Cluster Energies (from RelValTTBar_13):

HCAL Cluster Energies:

PS Cluster Energies:

MET Response:

Jet Response:

cmsbuild · 2014-03-05T16:55:15Z

-1
When I ran the RelVals I found an error in the following worklfows:
50101.0 step2

runTheMatrix-results/50101.0_SingleMuPt10+SingleMuPt10FSIdINPUT+SingleMuPt10FS_ID/step2_SingleMuPt10+SingleMuPt10FSIdINPUT+SingleMuPt10FS_ID.log
----- Begin Fatal Exception 05-Mar-2014 17:33:08 CET-----------------------
An exception of category 'FileReadError' occurred while
   [0] Processing run: 1 lumi: 5 event: 12001
   [1] Running path 'FEVTDEBUGHLToutput_step'
   [2] Calling event method for module PoolOutputModule/'FEVTDEBUGHLToutput'
   [3] Reading branch recoPFRecHits_particleFlowRecHitECAL__HLT.
   Additional Info:
      [a] Fatal Root Error: @SUB=TStreamerInfo::BuildOld
Cannot convert reco::PFRecHit::neighbours4_ from type:vector to type:edm::RefVector,reco::PFRecHit,edm::refhelper::FindUsingAdvance,reco::PFRecHit> >, skip element
----- End Fatal Exception -------------------------------------------------

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

bachtis · 2014-03-05T16:57:19Z

Yes this is because the dataformats have changed .... are old dataformats written on this test as input?

Dr15Jones · 2014-03-05T16:57:53Z

DataFormats/ParticleFlowReco/interface/PFCluster.h

+
+    /// transient layer
+    PFLayer::Layer layer_; 
+
 #if !defined(__CINT__) && !defined(__MAKECINT__) && !defined(__REFLEX__)
     /// \todo move to PFClusterTools
    static std::atomic<int>    depthCorMode_;


Any chance this could be moved as the comment says? Would help aleviate threading concerns.

Ah, you mean the std::atomic stuff. I am pretty sure none of those variables are used any more.

Martin-Grunewald · 2014-03-13T14:45:19Z

+1

cmsbuild · 2014-03-13T14:48:26Z

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

lgray · 2014-03-13T16:33:12Z

@ktf Possible to merge? @anton-a ran and produced results using the latest commit.

slava77 · 2014-03-13T17:13:11Z

Here's a bit cherry-picked summary for timing in pf-related modules
in CMSSW_7_1_X_2014-03-12-1400 using 200 events of matrix wflow 202.0

        particleFlowRecHitECAL   12.6115 ms/ev -> 19.8254 ms/ev
        particleFlowRecHitPS     17.1531 ms/ev -> 20.6083 ms/ev
        particleFlowClusterPS    20.7397 ms/ev -> 15.0606 ms/ev
        particleFlowClusterECAL          108.816 ms/ev -> 34.6245 ms/ev
        particleFlowClusterECALUncorrected       34.6637 ms/ev   (new module, need to add to above)
        particleFlowClusterHCAL          35.9695 ms/ev -> 19.7524 ms/ev
        particleFlowRecHitHO     0.218994 ms/ev -> 1.17935 ms/ev
        multi5x5SuperClustersWithPreshower       34.6906 ms/ev -> 22.3338 ms/ev  (not sure about this one)

The only somewhat surprising part is the particleFlowRecHitHO, which is increased by a factor of a few. I see the same difference in HO pfrechit in this final PR as well as with your initial submission
#2730 4616648

bachtis · 2014-03-13T17:49:11Z

@slava77 is this data or MC ? If it is data it could be that the cleaning is done at the end now and we should ignore since the cuts will change..If it is MC the only change is that I am now using in all modules the CaloNavigator class to find neighbours [check PFRecHitCaloNavigator] so this class could possibly be centrally optimized?

slava77 · 2014-03-13T17:50:40Z

wflow 202.0 is ttbar MC with 2012-like pileup

bachtis · 2014-03-13T17:52:35Z

Then it has to .be the navigator [I assume there are no rechits to be cleaned in this sample] .In that case I can not do something since the custom navigation that was done in PFhad small problems.....

slava77 · 2014-03-13T22:06:59Z

It looks like all the cost in the HO is in PFHcalRecHitCreator importRecHits (the navigation doesn't really show up), based on igprof.
Looking at PFHcalRecHitCreator.h, I think the main reason for more CPU is that you create a PFRecHit (which includes quite a bit of geometry manipulations) before the threshold and other quality checks. The old code does it only after the HO hit passes cuts.

bachtis · 2014-03-13T22:12:22Z

Thanks! OK but I dont want to change that .. When the quality tests are running you need to have the PFrecHit since the quality tests can change its energy etc. We need this functionality to migrate the HF clustering from hits [where short +long fibre is combined ] and to calibrate specific towers in boundaries. Since cumulatively we are doing much better I would prefer to stay like this .

slava77 · 2014-03-13T22:15:30Z

On 3/13/14, 5:12 PM, bachtis wrote:

Thanks! OK but I dont want to change that .. When the quality tests are
running you need to have the PFrecHit since the quality tests can change
its energy etc. We need this functionality to migrate the HF clustering
from hits [where short +long fibre is combined ] and to calibrate
specific towers in boundaries. Since cumulatively we are doing much
better I would prefer to stay like this .

OK.
In this case the cost is fairly low.
Also, it's specific to the Run-1 HO hit "quality" (only inner rings are
good for pf).
So, this increase in relative weight is going to go away

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

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

slava77 · 2014-03-13T22:23:24Z

RecoParticleFlow/PFClusterProducer/interface/PFHcalRecHitCreator.h

+
+      iEvent.getByToken(recHitToken_,recHitHandle);
+      for (unsigned int i=0;i<recHitHandle->size();++i) {
+	const Digi& erh = (*recHitHandle)[i];


Dereferencing a handle has a non-zero cost (just because the implementation is not inlined).
You are doing it here for every single hit.
This looks common for all of the code in this PR (just inherited from the old PF code).

bachtis · 2014-03-13T22:26:09Z

OK will fix that . Indeed it was copy paste from old module . I will have a technical commit next week towards the HCAl migration to hits [dactivated ] so I will add this.

cmsbuild · 2014-03-14T09:02:53Z

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2730/484/summary.html

cmsbuild · 2014-03-14T09:03:27Z

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

Reco -- Rewrite and Improvements for PFRecHit and PFCluster Producers

Reco -- Further code review from the end of #2730

lgray added hlt-pending labels Mar 5, 2014

lgray added hlt-rejected labels Mar 5, 2014

lgray added hlt-pending labels Mar 5, 2014

Dr15Jones reviewed Mar 5, 2014
View reviewed changes

lgray added the hlt-pending label Mar 5, 2014

lgray added fully-signed labels Mar 13, 2014

bachtis mentioned this pull request Mar 13, 2014

Consumes Migration for Particle Flow #2858

Merged

slava77 reviewed Mar 13, 2014
View reviewed changes

lgray added fully-signed labels Mar 14, 2014

ktf added a commit that referenced this pull request Mar 14, 2014

Merge pull request #2730 from lgray/pfcluster_rebuild

371428d

Reco -- Rewrite and Improvements for PFRecHit and PFCluster Producers

ktf merged commit 371428d into cms-sw:CMSSW_7_1_X Mar 14, 2014

lgray added a commit to lgray/cmssw that referenced this pull request Mar 14, 2014

further code review comments from cms-sw#2730

3bb044e

lgray added a commit to lgray/cmssw that referenced this pull request Mar 14, 2014

Further code review from the end of cms-sw#2730

b98d75b

lgray mentioned this pull request Mar 14, 2014

Further code review from the end of #2730 #2866

Merged

ktf added a commit that referenced this pull request Mar 17, 2014

Merge pull request #2866 from lgray/pfcluster_rebuild_morecodereview

a649877

Reco -- Further code review from the end of #2730

slava77 mentioned this pull request Jun 27, 2015

PFHCALDualTimeRecHitProducer merge with the main producer slava77/cmssw#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite and Improvements for PFRecHit and PFCluster Producers #2730

Rewrite and Improvements for PFRecHit and PFCluster Producers #2730

lgray commented Mar 5, 2014

bachtis commented Mar 5, 2014

cmsbuild commented Mar 5, 2014

nclopezo commented Mar 5, 2014

lgray commented Mar 5, 2014

Martin-Grunewald commented Mar 5, 2014

lgray commented Mar 5, 2014

nclopezo commented Mar 5, 2014

cmsbuild commented Mar 5, 2014

lgray commented Mar 5, 2014

lgray commented Mar 5, 2014

cmsbuild commented Mar 5, 2014

lgray commented Mar 5, 2014

cmsbuild commented Mar 5, 2014

lgray commented Mar 5, 2014

cmsbuild commented Mar 5, 2014

bachtis commented Mar 5, 2014

Dr15Jones Mar 5, 2014

lgray Mar 5, 2014

Martin-Grunewald commented Mar 13, 2014

cmsbuild commented Mar 13, 2014

lgray commented Mar 13, 2014

slava77 commented Mar 13, 2014

bachtis commented Mar 13, 2014

slava77 commented Mar 13, 2014

bachtis commented Mar 13, 2014

slava77 commented Mar 13, 2014

bachtis commented Mar 13, 2014

slava77 commented Mar 13, 2014

slava77 Mar 13, 2014

bachtis commented Mar 13, 2014

cmsbuild commented Mar 14, 2014

cmsbuild commented Mar 14, 2014

Rewrite and Improvements for PFRecHit and PFCluster Producers #2730

Rewrite and Improvements for PFRecHit and PFCluster Producers #2730

Conversation

lgray commented Mar 5, 2014

bachtis commented Mar 5, 2014

cmsbuild commented Mar 5, 2014

nclopezo commented Mar 5, 2014

lgray commented Mar 5, 2014

Martin-Grunewald commented Mar 5, 2014

lgray commented Mar 5, 2014

nclopezo commented Mar 5, 2014

cmsbuild commented Mar 5, 2014

lgray commented Mar 5, 2014

lgray commented Mar 5, 2014

cmsbuild commented Mar 5, 2014

lgray commented Mar 5, 2014

cmsbuild commented Mar 5, 2014

lgray commented Mar 5, 2014

cmsbuild commented Mar 5, 2014

bachtis commented Mar 5, 2014

Dr15Jones Mar 5, 2014

Choose a reason for hiding this comment

lgray Mar 5, 2014

Choose a reason for hiding this comment

Martin-Grunewald commented Mar 13, 2014

cmsbuild commented Mar 13, 2014

lgray commented Mar 13, 2014

slava77 commented Mar 13, 2014

bachtis commented Mar 13, 2014

slava77 commented Mar 13, 2014

bachtis commented Mar 13, 2014

slava77 commented Mar 13, 2014

bachtis commented Mar 13, 2014

slava77 commented Mar 13, 2014

slava77 Mar 13, 2014

Choose a reason for hiding this comment

bachtis commented Mar 13, 2014

cmsbuild commented Mar 14, 2014

cmsbuild commented Mar 14, 2014