Displaced regional tracking iteration #40643
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40643/33946
|
|
A new Pull Request was created by @aehart (Andrew Hart) for master. It involves the following packages:
@perrotta, @rappoccio, @pmandrik, @mandrenguyen, @emanueleusai, @ahmad3213, @cmsbuild, @clacaputo, @syuvivida, @davidlange6, @micsucmed, @fabiocos, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@aehart |
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3d74a3/30263/summary.html Comparison SummarySummary:
|
|
@aehart @slava77 @cms-sw/reconstruction-l2 if this is intended for the 2023 data reconstruction, i.e. for 13_0_X, please notice that the deadline for the last open pre-releease is in one week from now: since this is a rather large PR, if it is intended to be merged there it should be put on a fast lane for the review |
since a large fraction of this was reviewed in the past already, hopefully this review cycle will be relatively short. |
There was a problem hiding this comment.
for a few questions related to performance I was thinking if it's easier to enable the new iteration by default and just run the profiling tests with the pr tests by the bot.
This will also give a basic idea of the impact of this iteration on general tracking
| double rParam_; | ||
|
|
||
| // selection parameters | ||
| double minRadius_; | ||
| double discriminatorCut_; | ||
| vector<string> input_names_; | ||
| vector<string> output_names_; | ||
|
|
||
| edm::EDGetTokenT<reco::BeamSpot> beamSpotToken_; | ||
| edm::EDGetTokenT<edm::View<reco::VertexCompositeCandidate> > trackClustersToken_; |
There was a problem hiding this comment.
make these const (to be more explicit with thread safety, since this is a global module)
| sumOfCenters_(trackClusters.at(index).vertex()), | ||
| centerOfMass_(trackClusters.at(index).vertex()) { | ||
| const auto trackClusterPtr = &trackClusters.at(index); | ||
| constituents_.push_back(trackClusterPtr); |
There was a problem hiding this comment.
can you just pass the trackClusterPtr to this constructor? the pattern trackClusters.at(index) seems to be repeated for no good reason
There was a problem hiding this comment.
Yes, you're right. I now pass the pointer directly.
| displacedRegionalStepSeedLayersTripl = _mod.seedingLayersEDProducer.clone( | ||
| layerList = [ | ||
| #TOB | ||
| 'TOB1+TOB2+MTOB3','TOB1+TOB2+MTOB4', |
There was a problem hiding this comment.
how much loss in efficiency do you see by dropping TOB1+TOB2+MTOB4 ? this skips a layer and I guess it adds fakes
There was a problem hiding this comment.
Strangely, dropping TOB1+TOB2+MTOB4 seems to slightly increase the efficiency as well as decrease the fake rate for the DisplacedRegionalStep (see plots versus transverse vertex position below, which are from the same H→SS signal sample with PU that I used for the plots in the PR description).
My guess is that dropping this seed combination allows tracks to be built from combinations that produce built tracks with higher efficiency. I think this is consistent with the fact that the seed efficiency is lower (see below plot).
In any case, this seems like a good change, so I plan to commit it.
| displacedRegionalStepSeedLayersPair = _mod.seedingLayersEDProducer.clone( | ||
| layerList = ['TOB1+TEC1_pos','TOB1+TEC1_neg', | ||
| 'TEC1_pos+TEC2_pos','TEC1_neg+TEC2_neg', | ||
| 'TEC2_pos+TEC3_pos','TEC2_neg+TEC3_neg', | ||
| 'TEC3_pos+TEC4_pos','TEC3_neg+TEC4_neg', | ||
| 'TEC4_pos+TEC5_pos','TEC4_neg+TEC5_neg', | ||
| 'TEC5_pos+TEC6_pos','TEC5_neg+TEC6_neg', | ||
| 'TEC6_pos+TEC7_pos','TEC6_neg+TEC7_neg'], |
There was a problem hiding this comment.
what's the cost for timing from running the pairs
and how does it compare (fractionally) to the efficiency and fakes?
The MTV (validation) by seeding layer should be able to clarify the latter two (eff/fr).
There was a problem hiding this comment.
Find a summary of the timing below. I measure these with the tracking-only workflows 11634.1 (without PU) and 11834.1 (with PU), with the DisplacedRegionalStep added to step 3. Then for the numbers in the table:
- denominator = total time for the (tracking-only) RECO step
- numerator = total time for all modules with "displacedRegional" in the name
| without PU | with PU | |
|---|---|---|
| with pairs | (0.64 ± 0.07)% | (2.8 ± 0.3)% |
| without pairs | (0.51 ± 0.04)% | (1.9 ± 0.2)% |
Dropping all pair combinations decreases the efficiency of the DisplacedRegionalStep by ~25% for particles with vertices between about 1–10 cm, and increases the fake rate by roughly 5% across most of the range (see plots versus transverse vertex position below, which are from the same H→SS signal sample with PU that I used for the plots in the PR description).
I also looked at the fake rate by seeding layer from the MTV (see below), and it's clear that the triplets typically have higher fake rates than the pairs, which seems consistent with the plots, but I was not able to find efficiency by seeding layer.
There was a problem hiding this comment.
Dropping all pair combinations decreases the efficiency of the DisplacedRegionalStep by ~25% for particles with vertices between about 1–10 cm, and increases the fake rate by roughly 5% across most of the range (see plots versus transverse vertex position below, which are from the same H→SS signal sample with PU that I used for the plots in the PR description).
sounds like yet another motivation to seed tracks in the TIB/TID (pixelLess) for less displaced vertices.
BTW, the fakerate plots are vs track impact parameter while the effs are vs production vertex position. So, it's not as trivial to match the phase space
| fixedError = 1.0, | ||
| VertexCollection = "displacedRegionalStepSeedingVertices", | ||
| useFakeVertices = True, | ||
| ptMin = 0.6, |
There was a problem hiding this comment.
would a higher pt threshold be acceptable? e.g. 0.9.
This question is mainly to mitigate the computing cost and perhaps balance fakes vs true (triplets starting from 0.55 seems more OK than pairs)
There was a problem hiding this comment.
The efficiency drops for low-pT tracks as you would expect (see plots versus transverse momentum below, which are from the same H→SS signal sample with PU that I used for the plots in the PR description). The fake rate also goes up for low-pT tracks, which I guess is consistent with the triplet seeds having a higher fake rate than the pairs.
But I also measured the timing in the same way as in the above question about seed pairs, and it seems to bring down the CPU time a bit, though I would need to run over more than 100 events to tell if it is significant.
| without PU | with PU | |
|---|---|---|
ptMin = 0.6 |
(0.64 ± 0.07)% | (2.8 ± 0.3)% |
ptMin = 0.9 |
(0.60 ± 0.05)% | (2.6 ± 0.3)% |
Given the relatively small decrease in CPU time compared to the negative effects for low-pT tracks, I would prefer to keep the threshold where it is.
There was a problem hiding this comment.
Given the relatively small decrease in CPU time compared to the negative effects for low-pT tracks, I would prefer to keep the threshold where it is.
I agree.
Less than 10%, close to 5% relative gain in CPU seems not worth the loss in performance.
| src = 'displacedRegionalStepTrackCandidates', | ||
| AlgorithmName = 'displacedRegionalStep', | ||
| #Fitter = 'displacedRegionalStepFitterSmoother', | ||
| Fitter = 'displacedRegionalFlexibleKFFittingSmoother', |
There was a problem hiding this comment.
even though this is copied from the TOBTEC setup, most other iterations use just FlexibleKFFittingSmoother.
How much slower or worse in performance is that?
There was a problem hiding this comment.
I measured the timing in the same way as in the above question about seed pairs, both as-is and with
Fitter = 'FlexibleKFFittingSmoother',
There is essentially no effect on timing (see table below).
| without PU | with PU | |
|---|---|---|
| displacedRegionalFlexibleKFFittingSmoother | (0.64 ± 0.07)% | (2.8 ± 0.3)% |
| FlexibleKFFittingSmoother | (0.61 ± 0.06)% | (2.8 ± 0.3)% |
There is an efficiency drop, especially for highly displaced particles (see plots versus transverse vertex position below, which are from the same H→SS signal sample with PU that I used for the plots in the PR description), but maybe more concerning is the increase in fake rate for large impact parameters. It actually decreases for more prompt tracks, but the increase for displaced tracks is quite large.
There was a problem hiding this comment.
OK, evidently no need to change here either.
| # TRACK SELECTION AND QUALITY FLAG SETTING. | ||
| from RecoTracker.FinalTrackSelectors.TrackMVAClassifierPrompt_cfi import * | ||
| from RecoTracker.FinalTrackSelectors.TrackMVAClassifierDetached_cfi import * | ||
| displacedRegionalStepClassifier1 = TrackMVAClassifierDetached.clone( | ||
| src = 'displacedRegionalStepTracks', | ||
| mva = dict(GBRForestLabel = 'MVASelectorIter6_13TeV'), | ||
| qualityCuts = [-0.6,-0.45,-0.3] | ||
| ) | ||
|
|
||
| displacedRegionalStepClassifier2 = TrackMVAClassifierPrompt.clone( | ||
| src = 'displacedRegionalStepTracks', | ||
| mva = dict(GBRForestLabel = 'MVASelectorIter0_13TeV'), | ||
| qualityCuts = [0.0,0.0,0.0] | ||
| ) | ||
|
|
||
| from RecoTracker.FinalTrackSelectors.ClassifierMerger_cfi import * | ||
| displacedRegionalStep = ClassifierMerger.clone( | ||
| inputClassifiers=['displacedRegionalStepClassifier1','displacedRegionalStepClassifier2'] | ||
| ) | ||
|
|
||
| from Configuration.Eras.Modifier_trackingPhase1_cff import trackingPhase1 | ||
| trackingPhase1.toReplaceWith(displacedRegionalStep, displacedRegionalStepClassifier1.clone( | ||
| mva = dict(GBRForestLabel = 'MVASelectorTobTecStep_Phase1'), | ||
| qualityCuts = [-0.6,-0.45,-0.3] | ||
| )) |
There was a problem hiding this comment.
there is a plan to remove support to the old BDT.
It may be practical to directly clone from tobTecStep to reduce maintenance questions
There was a problem hiding this comment.
That's a good idea. I'll start refactoring this config file.
|
|
||
| // the POCA should at least be in the sensitive volume | ||
| GlobalPoint cxPt = cApp.crossingPoint(); | ||
| if ((cxPt.x() * cxPt.x() + cxPt.y() * cxPt.y()) > 120. * 120. || std::abs(cxPt.z()) > 300.) | ||
| continue; | ||
|
|
||
| if (cxPt.x() * cxPt.x() + cxPt.y() * cxPt.y() < 25.0) { |
There was a problem hiding this comment.
this seems ad hoc
and perhaps should be configurable.
| import FWCore.ParameterSet.Config as cms | ||
|
|
||
| # This modifier sets replaces the default pattern recognition with mkFit for displacedRegionalStep | ||
| trackingMkFitDisplacedRegionalStep = cms.Modifier() |
There was a problem hiding this comment.
please add this in Configuration/ProcessModifiers/python/trackingMkFitDevel_cff.py for regular testing
I agree. Unless this is intended only for a dedicated production (in which case we should add relval workflow), why not just enable it by default and see what happens. |
|
enable profiling |
| @@ -0,0 +1,66 @@ | |||
| #ifndef RecoTracker_DisplacedRegionalTracking_DisplacedVertexCluster_h | |||
| #define RecoTracker_DisplacedRegionalTracking_DisplacedVertexCluster_h | |||
There was a problem hiding this comment.
Is this class foreseen to be used outside of DisplacedRegionSeedingVertexProducer? If not, it would be better to move this header and the corresponding source file to the plugins subdirectory.
There was a problem hiding this comment.
It should only be used by the plugin, so I moved it into the plugins/ directory.
RecoTracker/DisplacedRegionalTracking/interface/DisplacedVertexCluster.h
Outdated
Show resolved
Hide resolved
| unsigned nThreads = cfg.getParameter<unsigned>("nThreads"); | ||
| string singleThreadPool = cfg.getParameter<string>("singleThreadPool"); |
There was a problem hiding this comment.
These parameters should be untracked, because they do not affect the physics results (right?).
But is the singleThreadPool even necessary? That overload of tensorflow::setThreading() just prints a warning and calls the two-argument overload
cmssw/PhysicsTools/TensorFlow/src/TensorFlow.cc
Lines 22 to 26 in 3fa9bc2
Is the nThreads intended to be used in production, or only in private tests?
There was a problem hiding this comment.
These parameters should be untracked, because they do not affect the physics results (right?).
Indeed they do not affect physics, so I have made nThreads untracked.
But is the
singleThreadPooleven necessary? That overload oftensorflow::setThreading()just prints a warning and calls the two-argument overloadcmssw/PhysicsTools/TensorFlow/src/TensorFlow.cc
Lines 22 to 26 in 3fa9bc2
I had not realized this had been deprecated, so I have removed singleThreadPool altogether.
Is the
nThreadsintended to be used in production, or only in private tests?
It's just for private tests right now. I'm not sure how a value greater than one would interact with the threading in CMSSW, so I don't change it from the default value of one in the config file ultimately meant for production.
cmsbuild
added
tests-pending
pdmv-pending
and removed
tests-approved
code-checks-approved
labels
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3d74a3/32223/summary.html Comparison SummarySummary:
|
|
@cms-sw/reconstruction-l2 @cms-sw/dqm-l2 @cms-sw/pdmv-l2 @cms-sw/upgrade-l2 even though it's mostly nice to have for 13_1_0, it would still be nice to try to sign off soon, perhaps even in time for pre4. |
|
+pdmv |
|
+1 |
|
@cms-sw/reconstruction-l2 @cms-sw/upgrade-l2 |
| 'TID2_pos+TID3_pos+TEC1_pos','TID2_neg+TID3_neg+TEC1_neg',#ring 1-2 (matched) | ||
| 'TID2_pos+TID3_pos+MTEC1_pos','TID2_neg+TID3_neg+MTEC1_neg',#ring 3 (mono) | ||
| #TEC RING 1-3 | ||
| #'TEC1_pos+TEC2_pos+TEC3_pos', 'TEC1_neg+TEC2_neg+TEC3_neg', |
There was a problem hiding this comment.
Is there a reason to keep commented code around?
|
+reconstruction |
|
+upgrade
|
|
+1 |
cmsbuild
added
operations-approved
fully-signed
orp-approved
and removed
operations-pending
pending-signatures
orp-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will be automatically merged. |
|
@aehart |
PR description:
This is a reprisal of #32806, and was discussed recently in the TRK POG meeting:
https://indico.cern.ch/event/1232392/#3-update-on-ml-seeded-displace
This PR introduces a new tracking iteration for Run 3, called the displacedRegionalStep, based on ROIs targeting exotic physics scenarios. The ROIs are formed from tracks found by existing iterations, and they are tagged with an MVA that helps to reduce the CPU time. The tagged ROIs are used as tracking regions for the new iteration.
Since the previous PR, I have updated RecoTracker/IterativeTracking/python/DisplacedRegionalStep_cff.py with changes made to TobTecStep_cff.py, which it was originally based on. There are further studies/optimizations to explore, but the iteration, as a process modifier, should be ready for integration now.
PR validation:
I re-ran the tracking, both without and with the new iteration, on a sample of H→SS→bbbb signal events that we used previously, where S is a long-lived scalar with a mass of 40 GeV and lifetime of 500 mm/c. The below plots are the resulting efficiencies vs. dxy. On the left is what was obtained previously, when #32806 was submitted, and the right plot is what we get now. Although the shapes of the efficiencies have changed significantly, the effect of the new iteration is qualitatively the same and even greater in magnitude, showing that it still behaves as expected.
EDIT: I measured the event sizes by running the 11834.0 workflow (TTbar_14TeV+2021PU) with 100 events, both with and without the DisplacedRegionalStep enabled. The results are in the following table:
From the DQM step of the same workflow, which was run on our local T3, I got the following mean CPU times: