Cache pixel cluster shape data #3211

makortel · 2014-04-04T11:05:25Z

This PR adds caching of pixel cluster shape data. As a prerequisite, in order to allow ClusterShapeTrajectoryFilter to read data product from Event, the BaseCkfTrajectoryBuilder and TrajectoryFilter derived classes are transformed from ESProducts to helper plugins.

The cache can be filled also on demand (onDemand parameter of SiPixelClusterShapeProducer). This mode might be interesting for HLT, at least in my tests there was improvement in timing. The downside is the need of some sort of synchronization, now done with edm::SerialTaskQueue (not sure if this is the best approach). I think any solution used for on-demand edmNew::DetSetVector<> should also work here.

The configuration migration relies on the newly added refToPSet_ feature (#3072, #3146). The configuration files for the removed ESProducers are still kept for ConfDB migration.

This PR removes BaseCkfTrajectoryBuilder and ClusterShapeTrajectoryFilter from the list of thread-unsafe ESProducts. BaseCkfTrajectoryBuilder::createStartingTrajectory() const does still modify mutable members, but as the BaseCkfTrajectoryBuilder objects are local to each EDModule, the class should in practice be thread-friendly at Event level.

This PR includes also various cleanups (mainly removal of edm::ParameterSet members) on

CkfTrackCandidateMakerBase and TransientInitialStateEstimator
ConversionTrackFinder
SeedGeneratorFromRegionHitsEDProducer
PhotonConversionTrajectorySeedProducerFromQuadruplets(Algo)
PhotonConversionTrajectorySeedProducerFromSingleLeg(Algo)

Future work to be done after this PR gets merged

Clean the obsolete ESProducer configuration files
Transform also MuonRoadTrajectoryBuilder from ESProduct to helper plugin (?)
- To be consistent with other TrajectoryBuilders (it would be the only one constructed with ESProducer)
- It doesn't seem to be used anywhere currently?
Remove mutable members from BaseCkfTrajectoryBuilder (done in 69fddd4 in this PR)

References

TRK POG meeting January 27 https://indico.cern.ch/event/292571/contribution/3/material/slides/0.pdf
RECO meeting March 6 https://indico.cern.ch/event/304170/session/4/contribution/15/material/slides/0.pdf
TRK POG meeting March 10 https://indico.cern.ch/event/306931/contribution/4/material/slides/0.pdf
TRK POG meeting March 24 https://indico.cern.ch/event/308199/contribution/8/2/material/slides/0.pdf
https://hypernews.cern.ch/HyperNews/CMS/get/recoTracking/1399.html
https://hypernews.cern.ch/HyperNews/CMS/get/code-perf/728.html
https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3244.html

Rebased and tested on CMSSW_7_1_X_2014-04-02-1400, should not change physics results.

cmsbuild · 2014-04-04T11:08:26Z

A new Pull Request was created by @makortel (Matti Kortelainen) for CMSSW_7_1_X.

Cache pixel cluster shape data

It involves the following packages:

Configuration/StandardSequences
DataFormats/SiPixelCluster
FWCore/Utilities
FastSimulation/Muons
RecoEgamma/EgammaElectronProducers
RecoEgamma/EgammaPhotonAlgos
RecoEgamma/EgammaPhotonProducers
RecoHI/HiMuonAlgos
RecoHI/HiTracking
RecoMuon/L3MuonIsolationProducer
RecoMuon/L3TrackFinder
RecoParticleFlow/PFTracking
RecoPixelVertexing/PixelLowPtUtilities
RecoPixelVertexing/PixelTriplets
RecoTracker/CkfPattern
RecoTracker/Configuration
RecoTracker/ConversionSeedGenerators
RecoTracker/DebugTools
RecoTracker/IterativeTracking
RecoTracker/TkSeedGenerator
RecoTracker/TkSeedingLayers
RecoVertex/NuclearInteractionProducer
SLHCUpgradeSimulations/Geometry
TrackingTools/GsfTracking
TrackingTools/Producers
TrackingTools/TrajectoryFiltering

@civanch, @Dr15Jones, @lveldere, @vlimant, @davidlange6, @ianna, @mdhildreth, @cmsbuild, @anton-a, @thspeer, @giamman, @slava77, @franzoni, @Degano, @ktf, @nclopezo can you please review it and eventually sign? Thanks.
@ghellwig, @wmtan, @GiacomoSguazzoni, @MiheeJo, @rovere, @lgray, @yenjie, @jazzitup, @kurtejung, @gpetruc, @cerati, @yetkinyilmaz, @mandrenguyen, @richard-cms, @bachtis, @venturia 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, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

makortel · 2014-04-04T11:09:14Z

@Martin-Grunewald Please find below a recipe for the ConfDB migration (tested privately). Let me know if any additional steps are needed. Once migrated, shall I cherry-pick the relevant commit(s) from you, or do you want to take over the PR? (I'd prefer the former)

For TrajectoryFilter and BaseCkfTrajectoryBuilder, each case is independent of others. Below I assume no change in labels in ESP->PSet for simplicity.

process.name = cms.ESProducer("TrajectoryFilterESProducer",
   ComponentName = cms.string("name"),
   filterPSet = cms.PSet(
       <content>
   )
)
===>
process.name = cms.PSet(
   <content> # contains already the correct ComponentType
)

process.name = cms.ESProducer("CkfTrajectoryBuilderESProducer",
   ComponentName = cms.string("name"),
   trajectoryFilterName = cms.string("trajectoryFilterName"),
   <content>
)
===>
process.name = cms.PSet(
   ComponentType = cms.string("CkfTrajectoryBuilder"),
   trajectoryFilter = cms.PSet(refToPSet_ = cms.string("trajectoryFilterName")),
   <content>
)

process.name = cms.ESProducer("MuonCkfTrajectoryBuilderESProducer",
    ComponentName = cms.string("name"),
    trajectoryFilterName = cms.string("trajectoryFilterName"),
    <content>
)
==>
process.name = cms.PSet(
    ComponentType = cms.string("MuonCkfTrajectoryBuilder"),
    trajectoryFilter = cms.PSet(refToPSet_ = cms.string("trajectoryFilterName")),
    <content>
)

process.name = cms.EDProducer("CkfTrackCandidateMaker",
   TrajectoryBuilder = cms.string("trajectoryBuilderName"),
   <content>
)
===>
process.name = cms.EDProducer("CkfTrackCandidateMaker",
   TrajectoryBuilder = cms.PSet(refToPSet_ = cms.string("trajectoryBuilderName")),
   <content>
)

process.name = cms.EDProducer("CkfTrajectoryMaker",
   TrajectoryBuilder = cms.string("trajectoryBuilderName"),
   <content>
)
===>
process.name = cms.EDProducer("CkfTrajectoryMaker",
   TrajectoryBuilder = cms.PSet(refToPSet_ = cms.string("trajectoryBuilderName")),
   <content>
)

Adding the cache producer and InputTag

We need one instance of SiPixelClusterShapeCacheProduce for each SiPixelClusterProducer. Currently, only some instances of PixelTrackProducer use the pixel cluster shape data. Below is a recipe on how to find the proper SiPixelClusterProducer given a PixelTrackProducer configuration.

process.pixelRecHitName = cms.EDProducer("SiPixelRecHitConverter",
    src = cms.InputTag("pixelClusterName")
    ...
)
process.seedingLayersName = cms.EDProducer("SeedingLayersEDProducer",
    FPix = cms.PSet(
        HitProducer = cms.string("pixelRecHitName")
        ...
    ),
    BPix = cms.PSet(
        HitProducer = cms.string("pixelRecHitName")
        ...
    ),
    ...
)
process.pixelTrackName = cms.EDProducer("PixelTrackProducer",
    ...
    OrderedHitsFactoryPSet = cms.PSet(
        ...
        SeedingLayers = cms.InputTag("seedingLayersName")
        GeneratorPSet = cms.PSet(
            ...
            SeedComparitorPSet = cms.PSet(
                ComponentName = cms.string("LowPtClusterShapeSeedComparitor"),
            )
        ),
    )
)

===>

process.pixelClusterShapeCacheName = cms.EDProducer("SiPixelClusterShapeCacheProducer", # new producer
    src = cms.InputTag("pixelClusterName")
)
process.pixelRecHitName = ... # unchanged
process.seedingLayersName = ... # unchanged
process.pixelTrackName = cms.EDProducer("PixelTrackProducer",
    ...
    OrderedHitsFactoryPSet = cms.PSet(
        ...
        SeedingLayers = cms.InputTag("seedingLayersName")
        GeneratorPSet = cms.PSet(
            ...
            SeedComparitorPSet = cms.PSet(
                ComponentName = cms.string("LowPtClusterShapeSeedComparitor"),
                clusterShapeCacheSrc = cms.InputTag("pixelClusterShapeCacheName") # new parameter
            )
        ),
    )
)
# insert pixelClusterShapeCacheName to the path where pixelTrackName
# is used, somewhere between pixelClusterName and pixelTrackName

makortel · 2014-04-04T11:09:53Z

Note that before the ConfDB has been migrated and the dumps updated, all workflows running HLT will fail. Therefore it is probably not worthwhile to run the tests yet.

makortel · 2014-04-04T11:11:22Z

FWCore/Utilities/interface/VecArray.h

@@ -0,0 +1,114 @@
+#ifndef FWCore_Utilities_VecArray_h


I also considered DataFormats/Common package for this template, but ended up putting it here.

Martin-Grunewald · 2014-04-04T11:21:58Z

Hi Matti,

Ah, now it is materialised. I'll make a branch from which you can add / cherry pick the HLT stuff.

makortel · 2014-04-04T11:44:43Z

Hi Martin,

Ok, thanks!

lveldere · 2014-04-04T11:49:53Z

FastSimulation/Muons/python/L3Muons_cff.py

@@ -1,6 +1,6 @@
 import FWCore.ParameterSet.Config as cms

 from RecoTracker.TransientTrackingRecHit.TransientTrackingRecHitBuilderWithoutRefit_cfi import *
-from RecoMuon.L3TrackFinder.MuonCkfTrajectoryBuilderESProducer_cff import *
+from RecoMuon.L3TrackFinder.MuonCkfTrajectoryBuilder_cff import *


FastSim seems to be the only thing that is importing from RecoMuon.L3TrackFinder.MuonCkfTrajectoryBuilder_cff
Can you confirm that the changes in that cfg file have no impact on physics?

Thanks

Lukas

I didn't test FastSim (shame on me), but based on grepping muonCkfTrajectoryBuilder appears only in https://github.com/cms-sw/cmssw/blob/CMSSW_7_1_X/FastSimulation/Muons/python/L3Muons_cfi.py#L19, where a FastL3MuonProducer EDProducer is configured. However,

the producer is configured only if a flag old==True, which is False by default in the configuration file, and

FastL3MuonProducer doesn't seem to exist anymore (nothing to migrate from ESProduct to helper plugin in here).

At the time I concluded that this piece of L3Muons_cfi.py would be obsolete, but didn't see the larger picture until now (i.e. if the "old" piece can indeed be removed, so could the import you noted).

sorry for my late reaction, I never received your reply in my inbox.

ok, I'll add this to my list of dead FastSim code.
I'll +1, under the condition that no FastSim regression is observed in the comparison test.

makortel · 2014-04-08T06:26:14Z

A small update based on private discussion with @VinInn, @cerati, and @rovere:

Replace mutable Propagator members of BaseCkfTrajectoryBuilder with always checking the seed direction
Deliver TkCloner to TransientInitialStateEstimator (requested by @VinInn)
Some further cleanup of ConversionTrackCandidateProducer

cmsbuild · 2014-04-08T06:28:20Z

Pull request #3211 was updated. @civanch, @Dr15Jones, @lveldere, @vlimant, @davidlange6, @ianna, @mdhildreth, @cmsbuild, @anton-a, @thspeer, @giamman, @slava77, @franzoni, @Degano, @ktf, @nclopezo can you please check and sign again.

lveldere · 2014-04-08T11:57:30Z

+1

under condition that no regression is observed in comparison test

ianna · 2014-04-08T13:06:01Z

+1

Martin-Grunewald · 2014-04-08T13:43:58Z

Hi Matti,

I have a problem with this part - the updates of the EDProducers:

process.name = cms.EDProducer("CkfTrackCandidateMaker",
TrajectoryBuilder = cms.string("trajectoryBuilderName"),

)
===>
process.name = cms.EDProducer("CkfTrackCandidateMaker",
TrajectoryBuilder = cms.PSet(refToPSet_ = cms.string("trajectoryBuilderName")),

)

process.name = cms.EDProducer("CkfTrajectoryMaker",
TrajectoryBuilder = cms.string("trajectoryBuilderName"),

)
===>
process.name = cms.EDProducer("CkfTrajectoryMaker",
TrajectoryBuilder = cms.PSet(refToPSet_ = cms.string("trajectoryBuilderName")),

)

Could you keep in the above the old TrajectoryBuilder as (then unused) string parameter
(ie, in the cfi file), while using TrajectoryBuilderPSet as the (new and different) name for the
new PSet parameter?
Eventually later we can remove the obsolete string parameter, but the PSet parameter should
keep the name TrajectoryBuilderPSet.

Thanks a lot

Martin

slava77 · 2014-04-15T05:01:43Z

+1

for #3211 1955e3d
tested in CMSSW_7_1_X_2014-04-14-0200 (test area sign340)

no regressions

No substantive change in CPU observed (using wf 202, ttbar PU as in 2012):
lowPtTripletStepSeeds go down by 10%, which is half-way offset by introduction of siPixelClusterShapeCache module

ktf · 2014-04-15T06:23:25Z

Deeming operations signature not needed. Reason why it is affected is the fact that Configuration/StandardSequences/python/Reconstruction_cff.py now has siPixelClusterShapeCache, which reco is ok with.

Reco -- Cache pixel cluster shape data

makortel · 2014-04-15T06:43:31Z

Now that this got merged, I'll follow up on the cleanup of the now-obsolete ESProducer configurations, and on the MuonRoadTrajectoryBuilder.

makortel · 2014-04-15T07:04:30Z

Oh, and I almost forgot, could Slava (or somebody else) comment if something should be done on the configuration files listed in #3211 (comment)? In principle I think they should be either fixed or removed as obsolete (but in practice leaving them as is could be an option too).

VinInn · 2014-04-15T07:07:43Z

again,
mail direclty POG/PAG involved…

are
RecoTracker/ConversionSeedGenerators/python/PhotonConversionTrajectorySeedProducerFromSingleLeg_cff.py
RecoTracker/ConversionSeedGenerators/python/PhotonConversionTrajectorySeedProducerFromSingleLeg_gsf_cff.py
really never used???

v.

On 15 Apr, 2014, at 9:04 AM, Matti Kortelainen notifications@github.com wrote:

Oh, and I almost forgot, could Slava (or somebody else) comment if something should be done on the configuration files listed in #3211 (comment)? In principle I think they should be either fixed or removed as obsolete (but in practice leaving them as is could be an option too).

—
Reply to this email directly or view it on GitHub.

cerati · 2014-04-15T08:17:11Z

looks like they are not used... I would rm and see what happens...

VinInn · 2014-04-15T08:29:24Z

Including the corresponding Seed Producer?

cerati · 2014-04-15T08:53:31Z

no the seed producer is run via this:
https://cmssdt.cern.ch/SDT/lxr/source/RecoTracker/ConversionSeedGenerators/python/ConversionStep_cff.py?v=CMSSW_7_1_0_pre6#003
included in:
https://cmssdt.cern.ch/SDT/lxr/source/RecoTracker/IterativeTracking/python/iterativeTk_cff.py?v=CMSSW_7_1_0_pre6#019

sorry I though the problem was with the cff's and not with the producer itself

makortel · 2014-04-15T08:55:12Z

As far as I'm concerned, the problem is in the cff's (mainly it is annoying to migrate potentially obsolete files).

Reco -- Post-#3211 cleanup (MuonRoadTrajectoryBuilder, configuration files)

makortel added comparison-pending labels Apr 4, 2014

makortel reviewed Apr 4, 2014
View reviewed changes

lveldere reviewed Apr 4, 2014
View reviewed changes

makortel added comparison-pending labels Apr 8, 2014

makortel added the comparison-pending label Apr 8, 2014

makortel added fastsim-approved labels Apr 14, 2014

makortel added comparison-available labels Apr 15, 2014

ktf added a commit that referenced this pull request Apr 15, 2014

Merge pull request #3211 from makortel/clustershape_product_v6

9f05177

Reco -- Cache pixel cluster shape data

ktf merged commit 9f05177 into cms-sw:CMSSW_7_1_X Apr 15, 2014

This was referenced Apr 23, 2014

bestHitOnly Bug Fix and maxCand Tuning #3333

Merged

Post-#3211 cleanup (MuonRoadTrajectoryBuilder, configuration files) #3446

Merged

nclopezo added a commit that referenced this pull request Apr 24, 2014

Merge pull request #3446 from makortel/cleanup3211

ef36529

Reco -- Post-#3211 cleanup (MuonRoadTrajectoryBuilder, configuration files)

makortel mentioned this pull request Feb 9, 2015

Phase1 (2015 + Pixel Phase1) Workflow up to Reco #7625

Closed

makortel deleted the clustershape_product_v6 branch October 20, 2016 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache pixel cluster shape data #3211

Cache pixel cluster shape data #3211

makortel commented Apr 4, 2014

cmsbuild commented Apr 4, 2014

makortel commented Apr 4, 2014

makortel commented Apr 4, 2014

makortel Apr 4, 2014

Martin-Grunewald commented Apr 4, 2014

makortel commented Apr 4, 2014

lveldere Apr 4, 2014

makortel Apr 4, 2014

lveldere Apr 8, 2014

makortel commented Apr 8, 2014

cmsbuild commented Apr 8, 2014

lveldere commented Apr 8, 2014

ianna commented Apr 8, 2014

Martin-Grunewald commented Apr 8, 2014

slava77 commented Apr 15, 2014

ktf commented Apr 15, 2014

makortel commented Apr 15, 2014

makortel commented Apr 15, 2014

VinInn commented Apr 15, 2014

cerati commented Apr 15, 2014

VinInn commented Apr 15, 2014

cerati commented Apr 15, 2014

makortel commented Apr 15, 2014

Cache pixel cluster shape data #3211

Cache pixel cluster shape data #3211

Conversation

makortel commented Apr 4, 2014

cmsbuild commented Apr 4, 2014

makortel commented Apr 4, 2014

makortel commented Apr 4, 2014

makortel Apr 4, 2014

Choose a reason for hiding this comment

Martin-Grunewald commented Apr 4, 2014

makortel commented Apr 4, 2014

lveldere Apr 4, 2014

Choose a reason for hiding this comment

makortel Apr 4, 2014

Choose a reason for hiding this comment

lveldere Apr 8, 2014

Choose a reason for hiding this comment

makortel commented Apr 8, 2014

cmsbuild commented Apr 8, 2014

lveldere commented Apr 8, 2014

ianna commented Apr 8, 2014

Martin-Grunewald commented Apr 8, 2014

slava77 commented Apr 15, 2014

ktf commented Apr 15, 2014

makortel commented Apr 15, 2014

makortel commented Apr 15, 2014

VinInn commented Apr 15, 2014

cerati commented Apr 15, 2014

VinInn commented Apr 15, 2014

cerati commented Apr 15, 2014

makortel commented Apr 15, 2014