EXO regional tracking iteration #32806
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32806/20986
|
|
A new Pull Request was created by @aehart (Andrew Hart) for master. It involves the following packages: DQM/TrackingMonitor The following packages do not have a category, yet: RecoTracker/ExoRegionalTracking @perrotta, @andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @slava77, @jpata, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
| beamhalo = 28, | ||
| gsf = 29, | ||
| exoRegionalStep = 26, | ||
| reservedForUpgrades2 = 27, |
There was a problem hiding this comment.
when we discussed integration of what's now called displacedGeneralStep, the idea for the regional was that it can still reuse reservedForUpgrades2 and that the naming would be similar, displacedRegionalStep
There was a problem hiding this comment.
OK, I renamed the iteration exoRegionalStep → displacedRegionalStep and it now occupies the slot previously held by reservedForUpgrades2.
| if (!roi.valid()) | ||
| return true; | ||
| const math::XYZVector x(roi.vx(), roi.vy(), roi.vz()); | ||
| if ((minRadius_ >= 0.0 && (x - bs).rho() < minRadius_) || |
There was a problem hiding this comment.
do you really need the check of being a positive value ?
There was a problem hiding this comment.
You're right! The check has been removed.
| vector<math::XYZVector> &decayVertices) const { | ||
| for (const auto &genParticle : genParticles) { | ||
| if (genMatch_ == "HToSSTobbbb") { | ||
| if (genParticle.pdgId() != 5) |
There was a problem hiding this comment.
This is obviated by the removal of genMatch_, but in case you're curious, each decay vertex in this signal produces a b quark and b anti-quark, so we only need to look at the production point of the b quark (or the anti-quark, but not both). If abs were included, the vector of decay vertices would have duplicates.
| continue; | ||
| if (!genParticle.isHardProcess()) | ||
| continue; | ||
| if (abs(genParticle.mother()->pdgId()) != 9000006) |
There was a problem hiding this comment.
do you mind to add a comment just above this in order to help other users to identify the particle ? (it seems like a not SM ones ...but I might be wrong, I do not play with the pdgID since long time)
There was a problem hiding this comment.
Obviated by removal of genMatch_.
| if (abs(genParticle.mother()->pdgId()) != 9000006) | ||
| continue; | ||
| } else if (genMatch_ == "stopToBottom") { | ||
| if (abs(genParticle.pdgId()) != 5) |
There was a problem hiding this comment.
here you have the abs, for instance ... as I was expecting
There was a problem hiding this comment.
In this signal, each decay vertex produces a b quark or anti-quark and a charged lepton. So we have to look at the production points of both the quark and anti-quark in order to get all the decay vertices.
| const math::XYZVector bs(beamSpot->x0(), beamSpot->y0(), beamSpot->z0()); | ||
| event.getByToken(trackClustersToken_, trackClusters); | ||
| vector<math::XYZVector> decayVertices; | ||
| if (genMatch_ != "None") { |
There was a problem hiding this comment.
IIRC, we do have some examples of dependence on MC truth objects in reco modules.
Still, it is not really a welcome dependence and if possible, it would be better to decouple.
clearly, the default configuration is not supposed to use MC-specific inputs.
There was a problem hiding this comment.
It makes sense to decouple the MC truth dependence, which was only used for testing anyway, so I've removed everything related to genMatch_.
| } | ||
|
|
||
| const double ExoRegionProducer::getDiscriminatorValue(const ExoRegion &roi, const reco::BeamSpot &bs) const { | ||
| tensorflow::Tensor vertexTensor(tensorflow::DT_FLOAT, tensorflow::TensorShape({1, 40, 23})); |
There was a problem hiding this comment.
could you add a comment about these "magic numbers", please ?
There was a problem hiding this comment.
I added some comments and also attempted to assign the magic numbers to meaningfully named constants.
| tensorflow::Tensor vertexTensor(tensorflow::DT_FLOAT, tensorflow::TensorShape({1, 40, 23})); | ||
| auto vertex_map = vertexTensor.tensor<float, 3>(); | ||
| tensorflow::Tensor annulusTensor(tensorflow::DT_FLOAT, tensorflow::TensorShape({1, 10, 8})); | ||
| auto annulus_map = annulusTensor.tensor<float, 3>(); |
|
the .pb model files will have to be (eventually) removed from this PR commit history. |
| tensorflow::Tensor annulusTensor(tensorflow::DT_FLOAT, tensorflow::TensorShape({1, 10, 8})); | ||
| auto annulus_map = annulusTensor.tensor<float, 3>(); | ||
|
|
||
| for (int i = 0, map_i = 0; map_i < 40; i++, map_i++) { |
There was a problem hiding this comment.
wouldn't be better to have some constexpr for handling these "magic numbers" ?
There was a problem hiding this comment.
I'm familiar with constexpr, but I'm not sure how I could use it to handle the magic numbers in a clear way.
| trackClustersToken_ = | ||
| consumes<edm::View<reco::VertexCompositeCandidate> >(cfg.getParameter<edm::InputTag>("trackClusters")); | ||
| if (genMatch_ != "None") | ||
| genParticlesToken_ = consumes<edm::View<reco::GenParticle> >(cfg.getParameter<edm::InputTag>("genParticles")); |
There was a problem hiding this comment.
shouldn't you use the mayConsume ?
There was a problem hiding this comment.
shouldn't you use the
mayConsume?
No. The code below gets genParticlesToken_ under the same condition as the consumes declaration here, so consumes() is correct.
(and in practice mayConsume() currently does the same and consumes(), and to make it really functional could require interface changes, possibly similar to EventSetup mayConsume()).
There was a problem hiding this comment.
Obviated by removing genMatch_.
I was going to write the same, indeed (thanks @slava77 ) |
|
one should understand if this PR can go like it is, so enabling this extra step by default the quoted 7% increase seems reasonable, I do not know if one might think to have it run only on specific PDs (I hope not, but in case it could help) as far as I understand @aehart already drop his tracks from the PF, btw, I've just realized I forgot to say "thanks @aehart !" |
I expect that ability to turn this on and off until after one or two relval cycles will be necessary. 7% is still a pretty significant setback in the CPU budget and some careful scrutiny is needed. Reduction in CPU in some other tracking parts like deep core can be considered as a balance. |
|
-1 Failed Tests: UnitTests RelVals RelVals-INPUT AddOn Unit TestsI found errors in the following unit tests: ---> test unitTestsGroup_1 had ERRORS ---> test unitTestsGroup_4 had ERRORS ---> test TestFWCoreIntegrationStandalone had ERRORS ---> test testAlignmentOfflineValidation had ERRORS and more ... RelVals
Expand to see more relval errors ...RelVals-INPUT
Expand to see more relval errors ...
AddOn Tests
Expand to see more addon errors ... |
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b0836f/15212/summary.html Comparison SummarySummary:
|
|
+1 |
|
@aehart please note there is a conflict |
in addition to this please also remove the .pb files from this PR commit history Also, to follow up on #32806 (comment), for better self-documentation please update the readme file https://github.com/cms-data/RecoTracker-DisplacedRegionalTracking/blob/main/README.md with some brief details on the content of the .pb files and how they were obtained |
|
kindly ping @aehart |
|
-1 unattended for more than two months now |
PR description:
This PR introduces a new tracking iteration for Run 3, called the exoRegionalStep, based on ROIs targeting exotic physics scenarios. The ROIs are formed from tracks found by previous 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.
Several presentations have been made regarding the new iteration, most recently in the TRK POG meeting here:
https://indico.cern.ch/event/961478/#5-displaced-tracking-ml-develo
PR validation:
We see a useful increase in tracking efficiency for highly displaced tracks from at least two different exotic physics models (see above presentation for details). This is accompanied by a modest increase in the CPU time of just the tracking sequence of ~7%.
The usual code formatting checks have been also been run.