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

Add GsfTracks to tracking validation #13324

Closed
wants to merge 9 commits into from

Conversation

makortel
Copy link
Contributor

This PR adds the monitoring of GsfTracks (electronGsfTracks collection) to tracking validation. The efficiency is defined wrt. TrackingParticles with abs(pdgId) == 11, and fake rate wrt. all TrackingParticles. On the same go, the definition of fake rate for conversions is also changed to be wrt. all TrackingParticles. In order to achieve these nicely, and to again properly support the separate label_tp_effic and label_tp_fake parameters of MultiTrackValidator, the following changes were made

  • The configuration of TrackingParticleNumberOfLayersProducer is fixed
  • MTV supports reading TrackingParticleRefVector instead of TrackingParticleCollection
    • It would have been easier to just use View<TrackingParticle>, but TrackToTrackingParticleAssociator interface does not support it, and changing it would have had much larger impact. (I'll probably try to do it anyway some day)
  • TrackingParticleConversionSelector is changed to produce TrackingParticleRefVector (and renamed and turned to edm::global)
  • TrackValidation_cffis migrated to use TrackingParticleRefVector instead of copies
  • SimToReco (TP->track) association is created wrt. label_tp_fake TPs instead of label_tp_eff, and label_tp_eff is required to be a subset of label_tp_fake (explicit check is added)
    • The SimToReco association is used in efficiency numerator, but also in duplicate rate numerator. For duplicates, the SimToReco must contain associations for TPs from label_tp_fake. For efficiency, these "extra" associations don't matter since the associations are checked only for label_tp_eff.

Tested in CMSSW_8_0_X_2016-02-15-1100 (with and without #13242), expecting changes under Tracking/TrackConversion (for fakes and other track-based histograms), and addition of Tracking/TrackGsf folder.

@rovere @VinInn

…or instead of TrackingParticleCollection

Something like this is necessary to be able to have separate sets of
TrackingParticles for efficiency and fake rate plots. It would be
easier to just use View<TrackingParticle>, but that would have
non-trivial impact on the track-TP associator/association interfaces,
so it is left to later time.
Duplicates are defined as "track associated to TP associated to > 1
tracks". The first association is done with recoToSim, and the second
with simToReco. If the efficiency and fake TP sets are different, this
leads to (unexpected) effects in duplicate. Naively one would assume
that the duplicate definition would be independet of the set of TPs
used for efficiencies.

The fix is to do the simToReco association wrt. the fake TPs. For
efficiency histograms it doesn't matter if the simToReco has entries
for TPs not in efficiency TP set.
@cmsbuild
Copy link
Contributor

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

It involves the following packages:

CommonTools/RecoAlgos
SimGeneral/TrackingAnalysis
Validation/RecoTrack

@civanch, @cvuosalo, @mdhildreth, @cmsbuild, @deguio, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@rappoccio, @battibass, @ahinzmann, @abbiendi, @GiacomoSguazzoni, @jhgoh, @VinInn, @jdolen, @istaslis, @rovere, @wmtford, @cerati, @trocino, @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

@@ -0,0 +1,3 @@
from SimGeneral.TrackingAnalysis.trackingParticleNumberOfLayersProducer_cfi import *
from Configuration.StandardSequences.Eras import eras
eras.fastSim.toModify(trackingParticleNumberOfLayersProducer, simHits=['famosSimHits:TrackerHits'])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dr15Jones Is this the way to combine fillDescriptions() and eras, or is there something better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider to define two different modules?

On Feb 17, 2016, at 1:01 PM, Matti Kortelainen notifications@github.com wrote:

In SimGeneral/TrackingAnalysis/python/trackingParticleNumberOfLayersProducer_cff.py:

@@ -0,0 +1,3 @@
+from SimGeneral.TrackingAnalysis.trackingParticleNumberOfLayersProducer_cfi import *
+from Configuration.StandardSequences.Eras import eras
+eras.fastSim.toModify(trackingParticleNumberOfLayersProducer, simHits=['famosSimHits:TrackerHits'])

@Dr15Jones Is this the way to combine fillDescriptions() and eras, or is there something better?


Reply to this email directly or view it on GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks David, I didn't think of that. I'll try it and push an update if there are no obstacles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the following I assume a goal of removing the introduced trackingParticleNumberOfLayersProducer_cff.py (that's my only problem with the approach above). With two different modules, it would be then up to the client(s) to do the customization for FastSim, either applying toReplaceWith() to the module itself, or modifying sequences+InputTags (I'm testing the latter approach because I feel that applying toReplaceWith() to an object elsewhere where the object is defined is a bit dirty). As long as there is one client (TrackValidation_cff) I think this is fine, but if there were at least two, this would lead to duplication.

Duplication could be removed in the _cff.py by applying toReplaceWith() to the module there. Then the only improvement I see would be that defaults for both FullSim and FastSim would be in a single file. Is this then what I should do?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is an issue to study how fillDescriptions() and eras could interact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dr15Jones Could give a pointer? I wasn't able to find it.

Would the piece above be good-enough interim solution or are there (significantly) better alternatives?

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out there wasn't one (I was thinking about the refToPSet one). I added #13326. For now, I think the _cff file with the same name as the _cfi is the best approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks. I'll leave it as it is then.

@slava77
Copy link
Contributor

slava77 commented Feb 17, 2016

@cmsbuild please test
@makortel the discussion thread was somewhat suggestive of upcoming updates to this PR, please clarify.

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

-1
Tested at: 5ea88e1
When I ran the RelVals I found an error in the following worklfows:
1306.0 step3

runTheMatrix-results/1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15/step3_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15.log
----- Begin Fatal Exception 18-Feb-2016 01:40:43 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Processing run: 1 lumi: 1 event: 1
   [1] Running path 'prevalidation_step'
   [2] Calling event method for module MultiTrackValidator/'trackValidatorConversion'
Exception Message:
Efficiency and fake TrackingParticle (refs) point to different collections (eff 0:0 fake 2:464). This is not supported. Efficiency TP set must be the same or a subset of the fake TP set.
----- End Fatal Exception -------------------------------------------------

1000.0 step1

DAS Error

1003.0 step1

DAS Error

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

@cmsbuild
Copy link
Contributor

Pull request #13324 was updated. @civanch, @cvuosalo, @mdhildreth, @cmsbuild, @deguio, @slava77, @vanbesien, @davidlange6 can you please check and sign again.

@makortel
Copy link
Contributor Author

I fixed the error in 1306.0 (didn't show up with short matrix).

@slava77 No further updates are coming (given @Dr15Jones' comment #13324 (comment)).

@slava77
Copy link
Contributor

slava77 commented Feb 19, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

+1

For #13324 5eb3f94

Adding GsfTracks to tracking validation.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_0_X_2016-02-18-2300 show only the differences expected for this PR, with examples shown below for workflow 25202.0_TTbar_13:

pu
numtrks
fakerate

@cvuosalo
Copy link
Contributor

An extended test of workflow 25202.0_TTbar_13 with 70 events against baseline CMSSW_8_0_X_2016-02-15-1100 shows only the differences expected for this PR, similar to the plots shown above.

@deguio
Copy link
Contributor

deguio commented Mar 1, 2016

+1

@civanch
Copy link
Contributor

civanch commented Mar 6, 2016

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2016

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

@VinInn
Copy link
Contributor

VinInn commented Mar 7, 2016

oops, did not realize I submitted for 8_0_0.
Can it be merged in 8_1_0 or shall I make a new PR?

@makortel
Copy link
Contributor Author

makortel commented Mar 7, 2016

@VinInn The 81X PR (#13348) is already merged. Given that it needs additional fixes (#13570 and #13579), it's probably better close this 80X one. If we nevertheless need/want this in 80X too, I'll reopen and add the fixes.

@makortel makortel closed this Mar 7, 2016
@VinInn
Copy link
Contributor

VinInn commented Mar 7, 2016

ok, let's move all developments to 8_1_X.

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

10 participants