Phase1 in FastSim of CMS #23363

angirar · 2018-05-29T06:53:07Z

New configurable tracker geometry for Phase0 and Phase1. Configurable track reconstruction sequence with new iterations added for Phase1. Iteration specific CA seeding (quadruplets and triplets) also in place.

cmsbuild · 2018-05-29T06:53:26Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-05-29T06:56:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23363/4874

cmsbuild · 2018-05-29T06:56:36Z

A new Pull Request was created by @angirar for master.

It involves the following packages:

Configuration/Eras
Configuration/StandardSequences
FastSimulation/SimplifiedGeometryPropagator
FastSimulation/Tracking
RecoPixelVertexing/PixelTriplets
RecoTracker/IterativeTracking
RecoTracker/TkHitPairs
RecoTracker/TkSeedingLayers

@perrotta, @ssekmen, @lveldere, @civanch, @mdhildreth, @cmsbuild, @franzoni, @slava77, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @mschrode, @gpetruc, @matt-komm, @ebrondol, @dgulhan this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

makortel

Thanks for the PR. It looks mostly good, but I found a couple of details more to be ironed out.

I believe it would be good to also add a 2017 FastSim workflow to runTheMatrix.py in this PR to allow testing of the developments.

makortel · 2018-05-29T07:03:31Z

RecoPixelVertexing/PixelTriplets/interface/CAHitQuadrupletGenerator.h

-#include "CAGraph.h"
-
+#include "RecoPixelVertexing/PixelTriplets/src/CAGraph.h"
+#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"


Please remove both includes as they are not needed here (likely the #include CAGraph.h needs to be moved to the .cc file).

makortel · 2018-05-29T07:04:42Z

RecoPixelVertexing/PixelTriplets/interface/CAHitQuadrupletGenerator.h

+    edm::ParameterSetDescription BPix;
+    edm::ParameterSetDescription FPix;
+    const bool isFastSim;
+    std::vector<unsigned> layerPairs;


None of these are used, so please remove.

makortel · 2018-05-29T07:05:12Z

RecoPixelVertexing/PixelTriplets/interface/CAHitTripletGenerator.h

-#include "CAGraph.h"
-
+#include "RecoPixelVertexing/PixelTriplets/src/CAGraph.h"
+#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"


Please remove both includes as they are not needed here (likely the #include CAGraph.h needs to be moved to the .cc file).

makortel · 2018-05-29T07:05:19Z

RecoPixelVertexing/PixelTriplets/interface/CAHitTripletGenerator.h

+    edm::ParameterSetDescription BPix;
+    edm::ParameterSetDescription FPix;
+    const bool isFastSim;
+    std::vector<unsigned> layerPairs;


None of these are used, so please remove.

makortel · 2018-05-29T07:07:23Z

RecoPixelVertexing/PixelTriplets/src/CAHitQuadrupletGenerator.cc

+caHardPtCut(cfg.getParameter<double>("CAHardPtCut")),
+layerList(cfg.getParameter<std::vector<std::string>>("layerList")),
+isFastSim(cfg.getParameter<bool>("isFastSim")),
+layerPairs(cfg.getParameter<std::vector<unsigned>>("layerPairs"))


As said earlier, these additions are not needed, please remove.

makortel · 2018-05-29T07:11:44Z

RecoTracker/IterativeTracking/python/LowPtQuadStep_cff.py

+        TTRHBuilder = cms.string('WithoutRefit'),
+        HitProducer = cms.string('TrackingRecHitProducer'),
+        ),
+    layerPairs = lowPtQuadStepHitDoublets.layerPairs.value(),


Move to the definition of _fastSim_lowPtQuadStepSeeds.

makortel · 2018-05-29T07:12:23Z

RecoTracker/IterativeTracking/python/LowPtTripletStep_cff.py

+        TTRHBuilder = cms.string('WithoutRefit'),
+        HitProducer = cms.string('TrackingRecHitProducer'),
+        ),
+    layerPairs = lowPtTripletStepHitDoublets.layerPairs.value(),


Move to the definition of _fastSim_lowPtTripletStepSeeds.

makortel · 2018-05-29T07:12:56Z

RecoTracker/TkHitPairs/interface/LayerHitMapCache.h

@@ -71,9 +78,10 @@ class LayerHitMapCache {
    assert (key>=0);
    const RecHitsSortedInPhi * lhm = theCache.get(key);
    if (lhm==nullptr) {
-      auto tmp=new RecHitsSortedInPhi (region.hits(iSetup,layer), region.origin(), layer.detLayer());
+      //      auto tmp=new RecHitsSortedInPhi (region.hits(iSetup,layer), region.origin(), layer.detLayer());


Please remove commented line.

makortel · 2018-05-29T07:13:03Z

RecoTracker/TkHitPairs/interface/LayerHitMapCache.h

      tmp->theOrigin = region.origin();
-      theCache.add( key, tmp);
+      //      theCache.add( key, tmp);


Please remove commented line.

makortel · 2018-05-29T07:16:28Z

RecoTracker/TkSeedingLayers/src/SeedingLayerSetsBuilder.cc

@@ -181,11 +182,13 @@ std::string SeedingLayerSetsBuilder::LayerSpec::print(const std::vector<std::str
 SeedingLayerSetsBuilder::SeedingLayerSetsBuilder(const edm::ParameterSet & cfg, edm::ConsumesCollector&& iC):
  SeedingLayerSetsBuilder(cfg, iC)
 {}
-SeedingLayerSetsBuilder::SeedingLayerSetsBuilder(const edm::ParameterSet & cfg, edm::ConsumesCollector& iC)
+SeedingLayerSetsBuilder::SeedingLayerSetsBuilder(const edm::ParameterSet & cfg, edm::ConsumesCollector& iC):
+  isFastSim(cfg.getParameter<bool>("isFastSim"))


I would prefer that the flag and the FastSim hit collection name (or maybe even the token) are passed as arguments to the constructor.

Can you elaborate on this? Do you want me to create a new constructor which has the FastSim token as argument?

Yes (although now I think maybe passing the collection name would be more clean and in line with what the SeedingLayerSetsBuilder does in standard reco). Note that you can call this constructor first to do it's job, so it should be something along

SeedingLayerSetsBuilder::SeedingLayerSetsBuilder(const edm::ParameterSet & cfg, edm::ConsumesCollector& iC, const edm::InputTag& fastsimHitTag): SeedingLayerSetsBuilder(cfg, iC), fastSimrecHitsToken_(iC.consumes<FastTrackerRecHitCollection>(fastSimHitTag)) {}

I tried it this way but I keep getting the following error -
Exception Message:
A get using a EDGetToken with the C++ type 'edm::OwnVector<FastTrackerRecHit,edm::ClonePolicy >' was made using an uninitialized token.
Please check that the variable is being initialized from a 'consumes' call.

I'm investigating it.

Do you call that constructor from the FastSim side?

Yes I am. Infact, when I declared the constructor the way you have written above, it gives error : "mem-initializer for 'SeedingLayerSetsBuilder::fastSimrecHitsToken_' follows constructor delegation
fastSimrecHitsToken_(iC.consumes(fastsimHitTag))"
So instead I declared it simply as
SeedingLayerSetsBuilder::SeedingLayerSetsBuilder(const edm::ParameterSet & cfg, edm::ConsumesCollector& iC, const edm::InputTag& fastsimHitTag):
fastSimrecHitsToken_(iC.consumes(fastsimHitTag))
{}
which works, and I'm calling it from SeedFinderSelector like this - "SeedingLayerSetsBuilder s(cfg, consumesCollector, edm::InputTag("fastTrackerRecHits"));" after making the unique pointer of SeedingLayerSetsBuilder here https://github.com/angirar/cmssw/blob/9fb1e01e75e58cef6411dfae4d642918442b3d72/FastSimulation/Tracking/src/SeedFinderSelector.cc#L54
I know it goes inside the new constructor and comes back (by putting the cout statements) but still gives an error about 'uninitialized token'.

Right, sorry, it didn't occur to me that (of course) when delegating a constructor call one can't use the initializer list anymore.

Could you clarify what do you mean with

I'm calling it from SeedFinderSelector like this - "SeedingLayerSetsBuilder s(cfg, consumesCollector, edm::InputTag("fastTrackerRecHits"));" after making the unique pointer of SeedingLayerSetsBuilder here

? I interpret (possibly incorrectly) what you wrote as

seedingLayers_ = std::make_unique<SeedingLayerSetsBuilder>(cfg, consumesCollector); SeedingLayerSetsBuilder s(cfg, consumesCollector, edm::InputTag("fastTrackerRecHits"));

while you should use the FastSim-specific constructor for the unique_ptr

seedingLayers_ = std::make_unique<SeedingLayerSetsBuilder>(cfg, consumesCollector, edm::InputTag("fastTrackerRecHits"));

Otherwise the token member variable in seedingLayers_ object stays indeed uninitialized.

Ah right.. with little tweaking its working now. Thanks.

cmsbuild · 2018-05-30T10:42:31Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-05-30T10:45:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23363/4905

ssekmen · 2018-06-15T16:08:42Z

+1

civanch · 2018-06-17T09:08:22Z

+1

fabiocos · 2018-06-18T12:13:12Z

+operations

the addition of a new Era and the corresponding modification to the standard sequences appear consistent with the purpose of the PR

fabiocos · 2018-06-18T20:12:53Z

@fabozzi @prebello could you please check and sign it in case?

fabozzi · 2018-06-18T21:29:31Z

+1

cmsbuild · 2018-06-18T21:30:07Z

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 now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2018-06-19T08:00:49Z

+1

angirar added 9 commits May 29, 2018 06:40

New FastSim modifier skipping GEM sequence

1a4a743

Phase1 configurable geometry

4ddf7ca

Removing dummy iteration files from FastSim/Tracking

24e0020

Making tracking sequence configurable in FastSim

023b50c

Phase1 iterations in FastSim

73b0f63

Preparing CA generators for use in FastSim

308fe73

Making SeedingLayerSetsHits pointer for FastSim seeding

597ba53

Fix for doublets by adding to the cache

3f1780d

Phase1 seeding in FastSim

1b9eea2

cmsbuild added this to the CMSSW_10_2_X milestone May 29, 2018

cmsbuild added code-checks-pending comparison-pending fastsim-pending operations-pending orp-pending pending-signatures reconstruction-pending tests-pending labels May 29, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels May 29, 2018

makortel reviewed May 29, 2018

View reviewed changes

makortel mentioned this pull request May 29, 2018

GPU Cellular Automaton cms-patatrack/cmssw#48

Merged

Removing the commented lines from LayerHitMapCache.h

f80a58e

cmsbuild added code-checks-pending and removed code-checks-approved labels May 30, 2018

cmsbuild added dqm-approved and removed dqm-pending labels Jun 15, 2018

cmsbuild added fastsim-approved and removed fastsim-pending labels Jun 15, 2018

cmsbuild added simulation-approved and removed simulation-pending labels Jun 17, 2018

cmsbuild added operations-approved and removed operations-pending labels Jun 18, 2018

cmsbuild added fully-signed pdmv-approved and removed pdmv-pending pending-signatures labels Jun 18, 2018

cmsbuild added orp-approved and removed orp-pending labels Jun 19, 2018

cmsbuild merged commit 1dbf22e into cms-sw:master Jun 19, 2018

kpedro88 mentioned this pull request Jun 21, 2018

Run2-hcx178 Usage of the correct Hcal geometry in FastSim #23631

Merged

fwyzard mentioned this pull request Jul 8, 2018

Update the CA GPU code following the reorganisation in #23363 cms-patatrack/cmssw#93

Closed

fwyzard added a commit to cms-patatrack/cmssw that referenced this pull request Jul 8, 2018

Update CA GPU code following the reorganisation in cms-sw#23363

e0f4648

fwyzard added a commit to cms-patatrack/cmssw that referenced this pull request Jul 8, 2018

Update CA GPU code following the reorganisation in cms-sw#23363

3541658

zhenhu mentioned this pull request Sep 22, 2018

Backport of Phase1 Tracking in FastSim to 94X #24624

Merged

zhenhu mentioned this pull request Oct 8, 2018

fastsim 2017 PU and pmx wfs #24829

Merged

zhenhu mentioned this pull request Jan 18, 2019

Fastsim era 2018 and workflows #25683

Merged

angirar deleted the Phase1-Fastsim-CMS branch January 24, 2019 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phase1 in FastSim of CMS #23363

Phase1 in FastSim of CMS #23363

angirar commented May 29, 2018 •

edited

cmsbuild commented May 29, 2018

cmsbuild commented May 29, 2018

cmsbuild commented May 29, 2018

makortel left a comment

makortel May 29, 2018

makortel May 29, 2018

makortel May 29, 2018

makortel May 29, 2018

makortel May 29, 2018

makortel May 29, 2018

makortel May 29, 2018

makortel May 29, 2018

makortel May 29, 2018

makortel May 29, 2018

angirar Jun 4, 2018

makortel Jun 4, 2018

angirar Jun 5, 2018

makortel Jun 5, 2018

angirar Jun 6, 2018

makortel Jun 6, 2018

angirar Jun 6, 2018

cmsbuild commented May 30, 2018

cmsbuild commented May 30, 2018

ssekmen commented Jun 15, 2018

civanch commented Jun 17, 2018

fabiocos commented Jun 18, 2018

fabiocos commented Jun 18, 2018

fabozzi commented Jun 18, 2018

cmsbuild commented Jun 18, 2018

fabiocos commented Jun 19, 2018

Phase1 in FastSim of CMS #23363

Phase1 in FastSim of CMS #23363

Conversation

angirar commented May 29, 2018 • edited

cmsbuild commented May 29, 2018

cmsbuild commented May 29, 2018

cmsbuild commented May 29, 2018

makortel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented May 30, 2018

cmsbuild commented May 30, 2018

ssekmen commented Jun 15, 2018

civanch commented Jun 17, 2018

fabiocos commented Jun 18, 2018

fabiocos commented Jun 18, 2018

fabozzi commented Jun 18, 2018

cmsbuild commented Jun 18, 2018

fabiocos commented Jun 19, 2018

angirar commented May 29, 2018 •

edited