Cms phase1 fastsim 10.1.x #2

angirar · 2018-03-23T11:09:22Z

Phase1 tracking in FastSim

…ifier

…om SeedFinderSelector

makortel

Sorry it took me a bit more than a few days to find good time slot to go though this. I hope you don't mind me going a few steps a head and pointing other points as well than only your immediate question.

makortel · 2018-03-29T12:59:53Z

FastSimulation/SimplifiedGeometryPropagator/python/TrackerMaterial_cfi.py

-
+
+"""
+trackingPhase1.toModify(TrackerMaterialBlock, TrackerMaterial = dict(


Even if this is "commented out", the proper era to use is phase1Pixel. The tracking eras should be used to customize only track reconstruction.

I tried to use modifier phase1Pixel. However, I am not getting the desired phase1 geometry while adding the new layers in this fashion to the TrackerMaterial block. Can you point out what could be wrong here?

Technically with Run2_2017[_FastSim] era phase1Pixel should work as well as trackingPhase1.

Taking a closer look now, the BarrelLayer = cms.VPSet( below replaces the BarrelLayer, while you want to add an element at the end. Maybe something along the following would work?

phase1Pixel.toModify(TrackerMaterialBlock, TrackerMaterial = dict( BarrelLayer = TrackerMaterialBlock.TrackerMaterial.BarrelLayer + [ cms.PSet( limits = ... ... ) ] ) )

makortel · 2018-03-29T13:02:16Z

FastSimulation/Tracking/python/iterativeTk_cff.py

 from FastSimulation.Tracking.InitialStep_cff import *
+from FastSimulation.Tracking.DetachedQuadStep_cff import *
+from FastSimulation.Tracking.HighPtTripletStep_cff import *
+from FastSimulation.Tracking.LowPtQuadStep_cff import *


Given that the FastSimulation.Tracking.* have just dummy imports from RecoTracker.IterativeTracking, you could import from RecoTracker.IterativeTracking directly here.

makortel · 2018-03-29T13:02:41Z

FastSimulation/Tracking/python/iterativeTk_cff.py

    +JetCoreRegionalStep
    +generalTracksBeforeMixing)

+iterTracking_Phase1 = cms.Sequence(


Add leading underscore (i.e. _iterTracking_phase1).

Done. Added a leading underscore to iterTracking_phase1.

makortel · 2018-03-29T13:05:41Z

FastSimulation/Tracking/interface/SeedFinderSelector.h

    const MeasurementTracker * measurementTracker_;
    const std::string measurementTrackerLabel_;
-
+    SeedingLayerSetsBuilder* seedingLayers_;


Use unique_ptr.

makortel · 2018-03-29T13:11:19Z

FastSimulation/Tracking/src/SeedFinderSelector.cc

+	const DetLayer * fLayer = measurementTracker_->geometricSearchTracker()->detLayer(hits[i]->det()->geographicalId());
+        const DetLayer * sLayer = measurementTracker_->geometricSearchTracker()->detLayer(hits[i+1]->det()->geographicalId());
+	std::vector<BaseTrackerRecHit const *> fHits(1,static_cast<const BaseTrackerRecHit*>(hits[i]));
+	std::vector<BaseTrackerRecHit const *> sHits(1,static_cast<const BaseTrackerRecHit*>(hits[i+1]));


As FastTrackerRecHit inherits from BaseTrackerRecHit, in principle the "up-casting" is unnecessary (or does the compiler complain?).

A more modern way for 'fHits` construction would be

std::vector<BaseTrackerRecHit const *> fHits{hits[i]};

I actually did it exactly similar to how it was being done so far for the doublets check. https://github.com/cms-sw/cmssw/blob/master/FastSimulation/Tracking/src/SeedFinderSelector.cc#L83 and the compiler doesn't complain. But you are right, the modern way works too, so I have replaced them all.

makortel · 2018-03-29T13:48:56Z

RecoTracker/IterativeTracking/python/PixelPairStep_cff.py

    seedCollections = ["pixelPairStepSeedsA", "pixelPairStepSeedsB"],
 )
-trackingPhase1.toReplaceWith(pixelPairStepSeeds, _pixelPairStepSeedsMerged)
+#trackingPhase1.toReplaceWith(pixelPairStepSeeds, _pixelPairStepSeedsMerged)


(trackingPhase1 & ~fastSim).toReplaceWith(pixelPairStepSeeds, _pixelPairStepSeedsMerged)

Done. Thanks. I didn't know if we could use the modifiers this way too!
Right now I am not sure if we have to implement the merged seedcollections in FastSim as well, don't know how much purpose its going to serve.

Yeah, the not (~, and or |) are rather recent additions, to support exactly things like this.

You won't need the merger unless FastSim starts to simulate the pixel inefficiencies and this doublet mitigation strategy.

makortel · 2018-03-29T13:51:34Z

RecoTracker/TkSeedingLayers/src/SeedingLayerSetsBuilder.cc


-  for(auto& layer: theLayers) {
+  if(isFastSim)
+    return ret;


Rather than adding a new configuration parameter, I'd create a new member function, e.g. makeSeedingLayerSetsHits() to do the construction above that you can call from FastSim code (and from this function to avoid copy-paste).

I can revisit this later.

makortel · 2018-03-29T14:07:23Z

FastSimulation/Tracking/interface/SeedFinderSelector.h

+    edm::Event * event_;
 };

+namespace IHD {


You should not need any of this stuff as there is only a single construction of impl_. The complexity is needed in HitPairEDProducer because that EDProducer has to support multitude of cases.

Right... thanks. I have removed all of this now.

makortel · 2018-03-29T14:29:41Z

FastSimulation/Tracking/src/SeedFinderSelector.cc

+
+      SeedingLayerSetsHits & layers = *seedingLayer;
+
+      for(int i=0; i<(int)hits.size()-1; i++){


Shouldn't this loop be limited to i < 3?

Correct... But the expression also gives 3 since hits.size() = 4 inside the quad section. But I guess it doesn't have to look so complicated when it is so obvious. So I have changed "(int)hits.size()-1" to 3.

Ok, didn't think of that. In that case keeping the code but adding a comment explaining the situation would have been ok as well.

makortel · 2018-03-29T14:39:37Z

FastSimulation/Tracking/src/SeedFinderSelector.cc

+        const RecHitsSortedInPhi secondhm(sHits, trackingRegion_->origin(), sLayer);
+        HitDoublets res(firsthm,secondhm);
+	HitPairGeneratorFromLayerPair::doublets(*trackingRegion_,*fLayer,*sLayer,firsthm,secondhm,*eventSetup_,0,res);
+        impl_->produce(layers, pairCandidate, *trackingRegion_, std::move(res), *event_);


What you should do here instead is something along

// outside of the for-loop over hits IntermediateHitDoublets ihd(seedingLayer); auto filler = ihd.beginRegion(*trackingRegion_); // here filler.addDoublets(pairCandidate, std::move(res)); // after the loop std::vector<OrderedHitSeeds> quadrupletResult; CAHitQuadGenerator_->hitNtuplets(ihd, quadrupletResult, *eventSetup_, *seedingLayer);

Then I'm not sure if the hits would have to be added to the SeedingLayerSetsHits as well. I had imagined "yes", but I don't see now a reason why they would have to, as you pass the hits explicitly to HitPairGeneratorFromLayerPair, and CA operates purely on the doublets.

I did the IHD implementation this way and it seems to work with doublets being added properly to filler. The hitNtuplets() also works, control actually goes inside the function. However, quadrupletResult is always 'false'. I don't know what's going wrong but I think I know where the problem starts. So, inside this loop
https://github.com/cms-sw/cmssw/blob/master/RecoPixelVertexing/PixelTriplets/plugins/CellularAutomaton.cc#L24
with rootVertex=0, it fails to enter this next loop
https://github.com/cms-sw/cmssw/blob/master/RecoPixelVertexing/PixelTriplets/plugins/CellularAutomaton.cc#L29
and then whatever happens, finally numberOfFoundQuadruplets=0 always at line
https://github.com/cms-sw/cmssw/blob/master/RecoPixelVertexing/PixelTriplets/plugins/CAHitQuadrupletGenerator.cc#L229

Also, FastSim RecHits have to be added to SeedingLayerSetsHits as well since it is used by the CA class to create the graph structure here,
https://github.com/cms-sw/cmssw/blob/master/RecoPixelVertexing/PixelTriplets/plugins/CAHitQuadrupletGenerator.cc#L93
I have managed to do it properly this time and it works.

However, quadrupletResult is always 'false'. I don't know what's going wrong but I think I know where the problem starts.

Could you give me a recipe to reproduce? I couldn't come up what could go wrong just by staring the code for a while so I have to try myself. In the meantime I have commented further #3.

angirar added 14 commits March 22, 2018 21:31

Moving to 10.1.X release, Initial commit

1053dd1

Phase1 pixel tracker geometry

ffc64e6

New iterations in FastSim

bc8677b

Phase1 implementation

318eb8a

Preparing SeedingLayerSetsBuilder for use in FastSim

1ff6e1e

Adding FastTrackerRecHits to SeedingLayerSetsBuilder for addHits()

f88ef75

Fixing OwnedHits&& and other compilation errors

3df353b

Making tracker geometry and iterative sequence configurable using mod…

b372c68

…ifier

Making FastSim seeding configurable in InitialStep

fe4d12f

Configurable tracker geometry not working properly

a7fabef

Making SeedingLayerSetsHits pointer without FastSim RecHits

843fcf8

Writing templates for creating IHD

c68cd60

Finding match of layerPairs from SeedingLayerSetsHits and hitPairs fr…

cbb3bc5

…om SeedFinderSelector

Constructing IntermediateHitDoublets

26c5aa3

makortel reviewed Mar 29, 2018

View reviewed changes

makortel mentioned this pull request Apr 6, 2018

Cms phase1 fastsim 10.1.0.pre3 #3

Open



		"""
		trackingPhase1.toModify(TrackerMaterialBlock, TrackerMaterial = dict(


		SeedingLayerSetsHits & layers = *seedingLayer;

		for(int i=0; i<(int)hits.size()-1; i++){

Cms phase1 fastsim 10.1.x #2

Are you sure you want to change the base?

Cms phase1 fastsim 10.1.x #2

Uh oh!

Conversation

angirar commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

makortel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

angirar commented Mar 23, 2018 •

edited

Loading