Check TrackingParticle ProductID in cluster->TrackingParticle association #13242

makortel · 2016-02-10T14:41:01Z

The current use of ClusterTPAssociationList in QuickTrackAssociatorByHits is a bit dangerous. Although edm::Refs to TrackingParticles are used in ClusterTPAssociationList, there are no checks done at any point if the Refs point to the same TrackingParticleCollection as those being associated in QuickTrackAssociatorByHits. Possible bugs would have the following symptoms

SimToReco association returns nothing for a given TrackingParticle
RecoToSim association returns associations of Track to the TrackingParticles used in ClusterTPAssociationList, that are not necessarily the same as were give as arguments

In order to catch such bugs, this PR adds a check for the edm::ProductID of the TrackingParticleCollection to ClusterTPAssociation(List) and QuickTrackAssociatorByHits by encapsulating the "raw" typedef vector<pair<...>> ClusterTPAssociationList as ClusterTPAssociation class that contains also the ProductID of the TrackingParticleCollection. On the same go I did some technical cleanup in ClusterTPAssociationProducer, including turning it to global module and using fillDescriptions().

In addition, QuickTrackAssociatorByHitsImpl::associate...(..., TrackingParticleRefVector) overloads are fixed for the case where the argument TrackingParticlRefVector is a subset of the TrackingParticles used in ClusterTPAssociation. Without the fix, the argument is ignored and the associations are done using the TrackingParticles in ClusterTPAssociation.

Running the short matrix revealed one such bug, in TkConvValidator. The existing plots from DQM GUI (https://goo.gl/zor134) look indeed to be poisoned by the bug (obviously one can judge only based on the empty efficiency plots). Minimal fix was to change TkConvValidator to read TrackingParticleRefVector.

Tested in 8_0_0_pre6, expecting changes only in EgammaV/ConversionValidator DQM folder.

@rovere @VinInn

…AssociationList

This way we can get rid of a nasty bug: - QuickTrackAssociatorByHits, by default, uses cluster->TrackingParticle mapping, that uses edm::Refs to the original TrackingParticle collection ("mix:MergedTrackTruth") - tpSelecForEfficiency and tpSelecForFakeRate create copies of the TrackingParticles Therefore - for efficiency, because of the copying, the edm::Refs to tpSelecForEfficiency collection differ from refs to "mix:MergedTrackTruth", and therefore no TrackingParticle of tpSelecForEfficiency get matched to any tracks - for fake rate, the tracks are matched to the TrackingParticles of the cluster->TrackingParticle mapping, i.e. "mix:MergedTrackTruth" resulting in empty efficiency plots, and fake rates computed using "mix:MergedTrackTruth" instead of tpSelecForFakeRate.

cmsbuild · 2016-02-10T14:41:22Z

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

It involves the following packages:

FastSimulation/Validation
SimTracker/TrackAssociatorProducers
SimTracker/TrackerHitAssociation
Validation/RecoEgamma
Validation/RecoTrack

@civanch, @lveldere, @ssekmen, @mdhildreth, @cmsbuild, @deguio, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@istaslis, @GiacomoSguazzoni, @rovere, @richard-cms, @Martin-Grunewald, @matt-komm, @wmtford, @cerati, @VinInn, @threus, @dgulhan this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

VinInn · 2016-02-10T14:42:22Z

cmsbuild, please test

cmsbuild · 2016-02-10T14:42:39Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11132/console

makortel · 2016-02-10T14:42:55Z

SimTracker/TrackerHitAssociation/interface/ClusterTPAssociation.h

@@ -0,0 +1,68 @@
+#ifndef SimTracker_TrackerHitAssociation_ClusterTPAssociation_h
+#define SimTracker_TrackerHitAssociation_ClusterTPAssociation_h


I'm not sure if this is the best package for this event product, but the dictionary for the preceding ClusterTPAssociationList was defined here.

makortel · 2016-02-10T14:44:12Z

Validation/RecoEgamma/plugins/TkConvValidator.cc

@@ -142,9 +142,9 @@ TkConvValidator::TkConvValidator( const edm::ParameterSet& pset )
    g4_simTk_Token_  = consumes<edm::SimTrackContainer>(pset.getParameter<edm::InputTag>("simTracks"));
    g4_simVtx_Token_ = consumes<edm::SimVertexContainer>(pset.getParameter<edm::InputTag>("simTracks"));

-    tpSelForEff_Token_ = consumes<TrackingParticleCollection> (
+    tpSelForEff_Token_ = consumes<TrackingParticleRefVector> (


It would be good if somebody from EGM could review this TkConvValidator part (but I don't know who to ask).

slava77 · 2016-02-10T15:09:33Z

@makortel
Is the execution time relatively unchanged?

makortel · 2016-02-10T15:26:32Z

@slava77 I didn't check, but I'd expect it to be. The ClusterTPAssociation itself is inlined except the actual ProductID checking which may throw. In the filling side, i.e. ClusterTPAssociationProducer, the check is done for each cluster-TP pair so there is some penalty. In the reading side, i.e. QuickTrackAssociatorByHits, the check is done only once per associateRecoToSim() and associateSimToReco() calls that do the associations for the entire track and TP collections.

In TkConvValidator side things should be only faster because of reduced copying, but I have no idea (without profiling) if it really shows up in the numbers or not.

cmsbuild · 2016-02-10T17:27:22Z

+1
Tested at: 495da25
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13242/11132/summary.html

cmsbuild · 2016-02-10T18:06:42Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13242/11132/summary.html

lveldere · 2016-02-11T07:51:01Z

+1

cmsbuild · 2016-02-15T09:09:56Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11210/console

cmsbuild · 2016-02-15T11:51:32Z

+1
Tested at: 00041e0
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13242/11210/summary.html

cmsbuild · 2016-02-15T13:11:04Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13242/11210/summary.html

lveldere · 2016-02-16T12:58:47Z

+1

deguio · 2016-02-16T14:33:30Z

+1

civanch · 2016-02-16T15:16:03Z

+1

cmsbuild · 2016-02-16T15:16:22Z

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

davidlange6 · 2016-02-16T17:22:04Z

@deguio - I take your +1 to mean that the e/gamma validation plot changes are ok..

davidlange6 · 2016-02-16T17:22:13Z

+1

Check TrackingParticle ProductID in cluster->TrackingParticle association

makortel added 6 commits February 10, 2016 09:26

Move ClusterTPAssociationProducer declaration to the source file

9bdaa36

Turn ClusterTPAssociationProducer to global::EDProducer

b812d26

Use fillDescriptions() in ClusterTPAssociationProducer

596a723

Replace ClusterTPAssociationList with properly encapsulated ClusterTP…

76804cf

…AssociationList

Check for TrackingParticle ProductID in QuickTrackAssociatorByHits

b82e398

cmsbuild added this to the Next CMSSW_8_0_X milestone Feb 10, 2016

cmsbuild added simulation-pending dqm-pending fastsim-pending pending-signatures tests-pending orp-pending comparison-pending labels Feb 10, 2016

makortel reviewed Feb 10, 2016
View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Feb 10, 2016

makortel reviewed Feb 10, 2016
View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Feb 10, 2016

cmsbuild added comparison-available and removed comparison-pending labels Feb 10, 2016

cmsbuild added tests-started and removed tests-pending labels Feb 15, 2016

cmsbuild added tests-approved and removed tests-started labels Feb 15, 2016

cmsbuild added comparison-available and removed comparison-pending labels Feb 15, 2016

cmsbuild added fastsim-approved and removed fastsim-pending labels Feb 16, 2016

cmsbuild added dqm-approved and removed dqm-pending labels Feb 16, 2016

cmsbuild added simulation-approved fully-signed and removed pending-signatures simulation-pending labels Feb 16, 2016

cmsbuild added orp-approved and removed orp-pending labels Feb 16, 2016

cmsbuild added a commit that referenced this pull request Feb 16, 2016

Merge pull request #13242 from makortel/clusterTPAssociation

d8c2841

Check TrackingParticle ProductID in cluster->TrackingParticle association

cmsbuild merged commit d8c2841 into cms-sw:CMSSW_8_0_X Feb 16, 2016

makortel mentioned this pull request Feb 17, 2016

Add GsfTracks to tracking validation #13324

Closed

cmsbuild mentioned this pull request Feb 18, 2016

Add GsfTracks to tracking validation #13348

Merged

makortel deleted the clusterTPAssociation branch October 20, 2016 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check TrackingParticle ProductID in cluster->TrackingParticle association #13242

Check TrackingParticle ProductID in cluster->TrackingParticle association #13242

makortel commented Feb 10, 2016

cmsbuild commented Feb 10, 2016

VinInn commented Feb 10, 2016

cmsbuild commented Feb 10, 2016

makortel Feb 10, 2016

makortel Feb 10, 2016

slava77 commented Feb 10, 2016

makortel commented Feb 10, 2016

cmsbuild commented Feb 10, 2016

cmsbuild commented Feb 10, 2016

lveldere commented Feb 11, 2016

cmsbuild commented Feb 15, 2016

cmsbuild commented Feb 15, 2016

cmsbuild commented Feb 15, 2016

lveldere commented Feb 16, 2016

deguio commented Feb 16, 2016

civanch commented Feb 16, 2016

cmsbuild commented Feb 16, 2016

davidlange6 commented Feb 16, 2016

davidlange6 commented Feb 16, 2016

		@@ -0,0 +1,68 @@
		#ifndef SimTracker_TrackerHitAssociation_ClusterTPAssociation_h
		#define SimTracker_TrackerHitAssociation_ClusterTPAssociation_h

Check TrackingParticle ProductID in cluster->TrackingParticle association #13242

Check TrackingParticle ProductID in cluster->TrackingParticle association #13242

Conversation

makortel commented Feb 10, 2016

cmsbuild commented Feb 10, 2016

VinInn commented Feb 10, 2016

cmsbuild commented Feb 10, 2016

makortel Feb 10, 2016

Choose a reason for hiding this comment

makortel Feb 10, 2016

Choose a reason for hiding this comment

slava77 commented Feb 10, 2016

makortel commented Feb 10, 2016

cmsbuild commented Feb 10, 2016

cmsbuild commented Feb 10, 2016

lveldere commented Feb 11, 2016

cmsbuild commented Feb 15, 2016

cmsbuild commented Feb 15, 2016

cmsbuild commented Feb 15, 2016

lveldere commented Feb 16, 2016

deguio commented Feb 16, 2016

civanch commented Feb 16, 2016

cmsbuild commented Feb 16, 2016

davidlange6 commented Feb 16, 2016

davidlange6 commented Feb 16, 2016