Migrate PixelTrackProducer and HLT to new seeding framework #17170

makortel · 2017-01-13T10:32:04Z

This PR is a continuation of #16792 and #16635. It migrates the PixelTrackProducer to the new seeding framework, and migrates all instances of it and SeedGeneratorFromRegionHitsEDProducer in the HLT (with customizeHLTforCMSSW.py).

Tested in CMSSW_9_0_X_2017-01-11-2300, no changes expected.

@rovere @VinInn @felicepantaleo @mtosi @JanFSchulte

cmsbuild · 2017-01-13T10:32:25Z

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

It involves the following packages:

Calibration/IsolatedParticles
DQM/Integration
FastSimulation/Tracking
HLTrigger/Configuration
RecoHI/HiTracking
RecoMuon/L3MuonProducer
RecoPixelVertexing/PixelLowPtUtilities
RecoPixelVertexing/PixelTrackFitting
RecoPixelVertexing/PixelTriplets
RecoTauTag/HLTProducers
RecoTracker/Configuration
RecoTracker/IterativeTracking
RecoTracker/TkHitPairs
RecoTracker/TkTrackingRegions

@perrotta, @cmsbuild, @civanch, @lveldere, @silviodonato, @cvuosalo, @arunhep, @ssekmen, @mdhildreth, @dmitrijus, @Martin-Grunewald, @franzoni, @ghellwig, @cerminar, @slava77, @fwyzard, @mmusich, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @felicepantaleo, @abbiendi, @Martin-Grunewald, @threus, @geoff-smith, @battibass, @jhgoh, @HuguesBrun, @trocino, @yetkinyilmaz, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @tocheng, @jalimena, @mschrode, @dgulhan, @echapon, @batinkov, @mandrenguyen, @jazzitup, @calderona, @yenjie, @kurtejung, @gpetruc, @matt-komm, @rociovilar, @bachtis this is something you requested to watch as well.
@davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

Pixel tracking does not currently check cluster multiplicity

…astSim HLT

makortel · 2017-01-13T10:45:29Z

@cmsbuild, please test

cmsbuild · 2017-01-13T10:45:48Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17265/console Started: 2017/01/13 11:46

dmitrijus · 2017-01-16T13:07:38Z

+1
(some issues, but they are in your private test files)

slava77 · 2017-01-18T00:49:59Z

RecoTracker/TkHitPairs/plugins/HitPairEDProducer.cc

@@ -272,6 +271,10 @@ HitPairEDProducer::HitPairEDProducer(const edm::ParameterSet& iConfig):
  else
    throw cms::Exception("Configuration") << "HitPairEDProducer requires either produceIntermediateHitDoublets or produceSeedingHitSets to be True. If neither are needed, just remove this module from your sequence/path as it doesn't do anything useful";

+  auto clusterCheckTag = iConfig.getParameter<edm::InputTag>("clusterCheck");
+  if(clusterCheckTag.label() != "")
+    clusterCheckToken_ = consumes<bool>(clusterCheckTag);


isn't this technically equivalent to what it was before?
That is, if the label is empty, you get an uninitialized token.

I'd guess "no", but I don't know what consumes() and getByToken() do in practice if the product label is empty. My guess is that it does not lead to uninitialized token, but something else happens.

slava77 · 2017-01-18T00:53:58Z

RecoTracker/TkHitPairs/plugins/HitPairEDProducer.cc

-  edm::Handle<bool> hclusterCheck;
-  iEvent.getByToken(clusterCheckToken_, hclusterCheck);
+  bool clusterCheckOk = true;
+  if(!clusterCheckToken_.isUninitialized()) {


shouldn't this instead rely on a bool haveClusterCheck = clusterCheckTag.label() != "" (computed at construction).
Otherwise it seems like we allow other undefined behavior (can a token be uninitialized for a non-empty label? .. or if the product was not added to the event in a given event)

I have assumed that consumes() would always return an initialized token, and hence I "abused" the initialization status here to check if the clusterCheckTag label was empty.

If it is preferred to not to rely on that, I can add an explicit boolean to deliver that information (if people prefer I can also add an explicit configuration parameter instead of relying empty InputTag to deliver the intention to ignore cluster multiplicity check).

slava77 · 2017-01-18T01:49:28Z

+1

for #17170 8f717e9

changes are consistent with the PR description, with migration done in a similar way to earlier parts
jenkins tests pass and comparisons with baseline show no differences
local tetst with 10224 and 25202 show no differences in comparisons of physics quantities; changes in the running modules appear to be as expected
- step3 (reco) PSet is essentially unchanged (a few unused PSets are now gone with cleaner loading, e.g. process.PixelTrackReconstructionBlock)
- step2 (HLT) seeding modules are rearranged as expected from seeding migration with the total time roughly the same (see an example below for PAIter0)

       added       0.00 ms/ev ->        84.35 ms/ev hltPAIter0PixelTripletsHitTriplets
       added      0.00 ms/ev ->         0.05 ms/ev hltPAIter0PixelTripletsClusterCheck
       added      0.00 ms/ev ->         0.02 ms/ev hltPAIter0PixelTripletsTrackingRegions
       added     0.00 ms/ev ->         4.11 ms/ev hltPAIter0PixelTripletsHitDoublets
                     106.35 ms/ev ->        16.94 ms/ev hltPAIter0PixelTripletsSeeds

somewhat minor comments from #17170 (comment) can be addressed later

Martin-Grunewald · 2017-01-18T13:13:51Z

+1

makortel · 2017-01-24T13:38:01Z

@cerminar @franzoni @mmusich @ghellwig @arunhep @civanch @mdhildreth @ssekmen Could you please review and sign? Thanks.

franzoni · 2017-01-26T10:29:44Z

+1

@makortel , are you planning on importing Calibration.IsolatedParticles.isoTrack_cff from anywhere ?

makortel · 2017-01-26T10:35:10Z

@franzoni I have no idea what that file does or should do (or who uses or has been using it etc). I migrated it only because I found with git grep that it is using PixelTrackProducer, and since the migration was straightforward-enough, I didn't consider that it is apparently not used.

Are you suggesting to remove it? :)

mmusich · 2017-01-26T10:37:19Z

@makortel and @franzoni, this is used in the Hcal energy scale calibration via isolated tracks. Should not be removed. @bsunanda please comment

makortel · 2017-01-26T10:41:35Z

@mmusich Thanks for the information. I presume the configs for "Hcal energy scale calibration via isolated tracks" are outside CMSSW as git grep isoTrack_cff returns nothing (or there's deeper python code than just load/import preventing grep to find it). Is there an easy way to test that the calibration job still works?

franzoni · 2017-01-26T10:50:11Z

I am not suggesting to remove anything - my questions was to understand the implications of the change (which based on the release content alone, are none).
@bsunanda may help us.
Thanks.

mmusich · 2017-01-26T10:57:21Z

I guess this is used to define the trigger paths (at the end of the file: HLT_IsoTrackHE_v15 and HLT_IsoTrackHB_v14 ) used to seed the calibration ALCARECO HcalCalIsoTrk. I must admit I am not sure how this enters the HLT menu without dependencies in CMSSW, but since it has been only relatively recently (c40ee88) changed to fix a change in the L1 seeds, I can only conclude it is still in use. Sunanda will certainly fill us in with more details.

makortel · 2017-01-27T09:09:51Z

@slava77 Regarding the exploitation of an uninitialized token in HitPairEDProducer to communicate from constructor to produce() whether a certain product should be read or not, I'd first like to get an answer from @Dr15Jones if consumes() can return an uninitialized token in some circumstance(s). (IIUC this is your main worry?)

Dr15Jones · 2017-01-27T14:54:53Z

consumes() never returns an uninitialized token.

makortel · 2017-01-27T14:56:25Z

Thanks Chris. Slava, do you nevertheless want an explicit boolean member?

slava77 · 2017-01-27T15:44:20Z

On 1/27/17 6:56 AM, Matti Kortelainen wrote: Thanks Chris. Slava, do you nevertheless want an explicit boolean member?

It looks like the logic will not change the way it is now. So, a bool is not required.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17170 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbsQ67zBpZ733VjmmpUyHyaW1L0uGks5rWgWZgaJpZM4LivG2>.

makortel closed this Jan 13, 2017

cmsbuild added this to the Next CMSSW_9_0_X milestone Jan 13, 2017

cmsbuild added alca-pending comparison-pending dqm-pending fastsim-pending hlt-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jan 13, 2017

makortel added 13 commits January 13, 2017 11:34

Migrate seeding to new seeding framework in customizeHLTforCMSSW

b087dac

Always clone LowPtClusterShapeSeedComparitor

e883189

Replace assert with an exception

a73b1b5

Add GlobalTrackingRegionEDProducer

52501d2

Make cluster multiplicity check optional in HitPairEDProducer

a15a3c0

Pixel tracking does not currently check cluster multiplicity

Add HITrackingRegionForPrimaryVtxEDProducer

6f54989

Add CandidateSeededTrackingRegionsEDProducer

43aeb02

Make PixelQuadrupletMergerEDProducer to work with PixelTrackProducer

a5ae39b

Migrate PixelTrackProducer to the new seeding framework

b2242d4

Migrate customiseFor*HLTPixelTracksByCellularAutomaton

f8fe5eb

Add PixelTrackProducer migration to customizeHLTforCMSSW

9bce3aa

Remove added region/doublet/triplet producers for pixel tracks from F…

2e95095

…astSim HLT

Migrate customizeHLTTrackingForPhaseI2017

9944fe1

makortel reopened this Jan 13, 2017

makortel force-pushed the reworkSeeding_pixelTracks_lastpiece branch from 25c2b8c to 9944fe1 Compare January 13, 2017 10:45

makortel changed the title ~~Migrate PixelTrackProducer to new seeding framework, and migrate HLT~~ Migrate PixelTrackProducer and HLT to new seeding framework Jan 13, 2017

cmsbuild added dqm-approved and removed dqm-pending labels Jan 16, 2017

slava77 reviewed Jan 18, 2017

View reviewed changes

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jan 18, 2017

cmsbuild added hlt-approved and removed hlt-pending labels Jan 18, 2017

cmsbuild added alca-approved and removed alca-pending labels Jan 26, 2017

davidlange6 merged commit 3318f06 into cms-sw:CMSSW_9_0_X Jan 26, 2017

Martin-Grunewald mentioned this pull request Jan 27, 2017

Remove HLT from FastSim (90X) #17183

Merged

This was referenced Feb 3, 2017

Avoid sequence expansion in customizeHLTTrackingForPhaseI2017 #17406

Closed

Cleanup (following #17098) for HLT ConfDB parsing #17438

Merged

Fix pixelTracks imports #17470

Merged

makortel deleted the reworkSeeding_pixelTracks_lastpiece branch February 12, 2018 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate PixelTrackProducer and HLT to new seeding framework #17170

Migrate PixelTrackProducer and HLT to new seeding framework #17170

makortel commented Jan 13, 2017

cmsbuild commented Jan 13, 2017

makortel commented Jan 13, 2017

cmsbuild commented Jan 13, 2017 •

edited

Loading

dmitrijus commented Jan 16, 2017

slava77 Jan 18, 2017

makortel Jan 18, 2017

slava77 Jan 18, 2017

makortel Jan 18, 2017

slava77 commented Jan 18, 2017

Martin-Grunewald commented Jan 18, 2017

makortel commented Jan 24, 2017

franzoni commented Jan 26, 2017

makortel commented Jan 26, 2017

mmusich commented Jan 26, 2017

makortel commented Jan 26, 2017

franzoni commented Jan 26, 2017

mmusich commented Jan 26, 2017

makortel commented Jan 27, 2017

Dr15Jones commented Jan 27, 2017

makortel commented Jan 27, 2017

slava77 commented Jan 27, 2017 via email

Migrate PixelTrackProducer and HLT to new seeding framework #17170

Migrate PixelTrackProducer and HLT to new seeding framework #17170

Conversation

makortel commented Jan 13, 2017

cmsbuild commented Jan 13, 2017

makortel commented Jan 13, 2017

cmsbuild commented Jan 13, 2017 • edited Loading

dmitrijus commented Jan 16, 2017

slava77 Jan 18, 2017

Choose a reason for hiding this comment

makortel Jan 18, 2017

Choose a reason for hiding this comment

slava77 Jan 18, 2017

Choose a reason for hiding this comment

makortel Jan 18, 2017

Choose a reason for hiding this comment

slava77 commented Jan 18, 2017

Martin-Grunewald commented Jan 18, 2017

makortel commented Jan 24, 2017

franzoni commented Jan 26, 2017

makortel commented Jan 26, 2017

mmusich commented Jan 26, 2017

makortel commented Jan 26, 2017

franzoni commented Jan 26, 2017

mmusich commented Jan 26, 2017

makortel commented Jan 27, 2017

Dr15Jones commented Jan 27, 2017

makortel commented Jan 27, 2017

slava77 commented Jan 27, 2017 via email

cmsbuild commented Jan 13, 2017 •

edited

Loading