Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Migrate module configuration in RecoTracker{SpecialSeedGenerators} to use default cfipython #34009

Merged
merged 4 commits into from
Jun 14, 2021
Merged

Migrate module configuration in RecoTracker{SpecialSeedGenerators} to use default cfipython #34009

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
875f645
replace EDProducer with clone from cfipy and drop type spec
jeongeun Jun 8, 2021
f985502
revert to use PSet
jeongeun Jun 11, 2021
e19f471
fix
jeongeun Jun 14, 2021
d4faa16
fix align
jeongeun Jun 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForBeamHalo_cff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
from RecoTracker.TkSeedingLayers.PixelLayerPairs_cfi import *
#get the module combinatorialbeamhaloseedfinder
from RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForBeamHalo_cfi import *
import RecoTracker.TkSeedingLayers.seedingLayersEDProducer_cfi as _mod

beamhaloTrackerSeedingLayers = cms.EDProducer("SeedingLayersEDProducer",
layerInfo,
layerList = layerList
beamhaloTrackerSeedingLayers = _mod.seedingLayersEDProducer.clone(
layerList = layerList,
**layerInfo
)
6 changes: 3 additions & 3 deletions RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForBeamHalo_cfi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from RecoTracker.TkTrackingRegions.GlobalTrackingRegion_cfi import *
from RecoLocalTracker.SiStripClusterizer.SiStripClusterChargeCut_cfi import *

layerInfo = cms.PSet(
layerInfo = dict(
TID = cms.PSet(
matchedRecHits = cms.InputTag("siStripMatchedRecHits","matchedRecHit"),
useRingSlector = cms.bool(False),
Expand Down Expand Up @@ -41,7 +41,7 @@
),
)

layerList = cms.vstring(
layerList = [
'FPix1_pos+FPix2_pos',
'FPix1_neg+FPix2_neg',
'TID2_pos+TID3_pos',
Expand All @@ -60,7 +60,7 @@
'MTEC7_pos+MTEC8_pos',
'MTEC8_neg+MTEC9_neg',
'MTEC8_pos+MTEC9_pos'
)
]

beamhaloTrackerSeeds = cms.EDProducer("CtfSpecialSeedGenerator",
SeedMomentum = cms.double(15.0), ##initial momentum in GeV !!!set to a lower value for slice test data
Expand Down
30 changes: 16 additions & 14 deletions RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmicsP5_cff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,30 +20,32 @@
from RecoTracker.TransientTrackingRecHit.TransientTrackingRecHitBuilder_cfi import *
from RecoTracker.TransientTrackingRecHit.TransientTrackingRecHitBuilderWithoutRefit_cfi import *
from RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForCosmics_cfi import *
import RecoTracker.TkSeedingLayers.seedingLayersEDProducer_cfi as _mod

# seeding layers
combinatorialcosmicseedingtripletsP5 = cms.EDProducer("SeedingLayersEDProducer",
layerInfo,
layerList = cms.vstring('MTOB4+MTOB5+MTOB6',
combinatorialcosmicseedingtripletsP5 = _mod.seedingLayersEDProducer.clone(
layerList = ['MTOB4+MTOB5+MTOB6',
'MTOB3+MTOB5+MTOB6',
'MTOB3+MTOB4+MTOB5',
'TOB2+MTOB4+MTOB5',
'MTOB3+MTOB4+MTOB6',
'TOB2+MTOB4+MTOB6')
'TOB2+MTOB4+MTOB6'],
**layerInfo
)
combinatorialcosmicseedingpairsTOBP5 = cms.EDProducer("SeedingLayersEDProducer",
layerInfo,
layerList = cms.vstring('MTOB5+MTOB6',
'MTOB4+MTOB5')
combinatorialcosmicseedingpairsTOBP5 = _mod.seedingLayersEDProducer.clone(
layerList = ['MTOB5+MTOB6',
'MTOB4+MTOB5'],
**layerInfo
)
combinatorialcosmicseedingpairsTECposP5 = cms.EDProducer("SeedingLayersEDProducer",
layerList = cms.vstring('TEC1_pos+TEC2_pos',
combinatorialcosmicseedingpairsTECposP5 = _mod.seedingLayersEDProducer.clone(
layerList = ['TEC1_pos+TEC2_pos',
'TEC2_pos+TEC3_pos',
'TEC3_pos+TEC4_pos',
'TEC4_pos+TEC5_pos',
'TEC5_pos+TEC6_pos',
'TEC6_pos+TEC7_pos',
'TEC7_pos+TEC8_pos',
'TEC8_pos+TEC9_pos'),
'TEC8_pos+TEC9_pos'],
TEC = cms.PSet(
minRing = cms.int32(5),
matchedRecHits = cms.InputTag("siStripMatchedRecHits","matchedRecHit"),
Expand All @@ -54,15 +56,15 @@
maxRing = cms.int32(7)
)
)
combinatorialcosmicseedingpairsTECnegP5 = cms.EDProducer("SeedingLayersEDProducer",
layerList = cms.vstring('TEC1_neg+TEC2_neg',
combinatorialcosmicseedingpairsTECnegP5 = _mod.seedingLayersEDProducer.clone(
layerList = ['TEC1_neg+TEC2_neg',
'TEC2_neg+TEC3_neg',
'TEC3_neg+TEC4_neg',
'TEC4_neg+TEC5_neg',
'TEC5_neg+TEC6_neg',
'TEC6_neg+TEC7_neg',
'TEC7_neg+TEC8_neg',
'TEC8_neg+TEC9_neg'),
'TEC8_neg+TEC9_neg'],
TEC = cms.PSet(
minRing = cms.int32(5),
matchedRecHits = cms.InputTag("siStripMatchedRecHits","matchedRecHit"),
Expand Down
7 changes: 4 additions & 3 deletions ...alSeedGenerators/python/CombinatorialSeedGeneratorForCosmicsRegionalReconstruction_cff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import FWCore.ParameterSet.Config as cms

from RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForCosmicsRegionalReconstruction_cfi import *
import RecoTracker.TkSeedingLayers.seedingLayersEDProducer_cfi as _mod

regionalCosmicTrackerSeedingLayers = cms.EDProducer("SeedingLayersEDProducer",
layerInfo,
layerList = layerList
regionalCosmicTrackerSeedingLayers = _mod.seedingLayersEDProducer.clone(
layerList = layerList,
**layerInfo
)
28 changes: 14 additions & 14 deletions ...alSeedGenerators/python/CombinatorialSeedGeneratorForCosmicsRegionalReconstruction_cfi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from RecoLocalTracker.SiStripClusterizer.SiStripClusterChargeCut_cfi import *

layerInfo = cms.PSet(
layerInfo = dict(
TOB = cms.PSet(
TTRHBuilder = cms.string('WithTrackAngle'),
clusterChargeCut = cms.PSet(refToPSet_ = cms.string('SiStripClusterChargeCutNone'))
Expand All @@ -15,19 +15,19 @@
maxRing = cms.int32(7)
)
)
layerList = cms.vstring('TOB6+TOB5',
'TOB6+TOB4',
'TOB6+TOB3',
'TOB5+TOB4',
'TOB5+TOB3',
'TOB4+TOB3',
'TEC1_neg+TOB6',
'TEC1_neg+TOB5',
'TEC1_neg+TOB4',
'TEC1_pos+TOB6',
'TEC1_pos+TOB5',
'TEC1_pos+TOB4'
)
layerList = ['TOB6+TOB5',
'TOB6+TOB4',
'TOB6+TOB3',
'TOB5+TOB4',
'TOB5+TOB3',
'TOB4+TOB3',
'TEC1_neg+TOB6',
'TEC1_neg+TOB5',
'TEC1_neg+TOB4',
'TEC1_pos+TOB6',
'TEC1_pos+TOB5',
'TEC1_pos+TOB4'
]
from RecoTracker.TkSeedGenerator.SeedFromConsecutiveHitsCreator_cfi import SeedFromConsecutiveHitsCreator as _SeedFromConsecutiveHitsCreator
CosmicSeedCreator = _SeedFromConsecutiveHitsCreator.clone(
ComponentName = 'CosmicSeedCreator',
Expand Down
2 changes: 1 addition & 1 deletion RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmics_cfi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from RecoTracker.TkTrackingRegions.GlobalTrackingRegion_cfi import *
from RecoLocalTracker.SiStripClusterizer.SiStripClusterChargeCut_cfi import *

layerInfo = cms.PSet(
layerInfo = dict(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are cleaning, you could further align inside the PSet's in layerInfo

MTIB = cms.PSet(
TTRHBuilder = cms.string('WithTrackAngle'),
clusterChargeCut = cms.PSet(refToPSet_ = cms.string('SiStripClusterChargeCutNone')),
Expand Down
7 changes: 4 additions & 3 deletions RecoTracker/SpecialSeedGenerators/python/SimpleCosmicBONSeeder_cff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import FWCore.ParameterSet.Config as cms

from RecoTracker.SpecialSeedGenerators.SimpleCosmicBONSeeder_cfi import *
import RecoTracker.TkSeedingLayers.seedingLayersEDProducer_cfi as _mod

simpleCosmicBONSeedingLayers = cms.EDProducer("SeedingLayersEDProducer",
layerInfo,
layerList = cms.vstring(*layerList)
simpleCosmicBONSeedingLayers= _mod.seedingLayersEDProducer.clone(
layerList = cms.vstring(*layerList),
**layerInfo
)
32 changes: 28 additions & 4 deletions RecoTracker/SpecialSeedGenerators/python/SimpleCosmicBONSeeder_cfi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import FWCore.ParameterSet.Config as cms

import RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForCosmics_cfi

def makeSimpleCosmicSeedLayers(*layers):
layerList = cms.vstring()
if 'ALL' in layers:
Expand Down Expand Up @@ -29,8 +27,34 @@ def makeSimpleCosmicSeedLayers(*layers):
#print "SEEDING LAYER LIST = ", layerList
return layerList

layerInfo = RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForCosmics_cfi.layerInfo.clone(
TEC = dict(useSimpleRphiHitsCleaner = False)
layerInfo = dict(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not keeping cloning from RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForCosmics_cfi, instead of all this copy/paste?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@perrotta : I got an error when I keep L32-33 as below, so I did simply copy/paste the layerInfo in this SimpleCosmicVONSeeder_cfi.py file.
Please let me know if there's the most suitable way you suggest here.

  File "/afs/cern.ch/work/j/jelee/ServiceWork/NewTask/0607/CMSSW_12_0_X_2021-06-06-2300/python/RecoTracker/SpecialSeedGenerators/SimpleCosmicBONSeeder_cfi.py", line 30, in <module>
    layerInfo = RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForCosmics_cfi.layerInfo.clone(
AttributeError: 'dict' object has no attribute 'clone'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then probably the simplest solution is to revert the change in https://github.com/cms-sw/cmssw/pull/34009/files#diff-0dbbc624124b7539ebca54a0a8940ad7ca30d06b687c5bbb825c30a51e16dd7eR9

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@makortel , perhaps you can help here...

I would like to avoid copy/pasting all the PSet's below, which were already defined in RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmics_cfi.py. This would be inconvenient in case of modifications in the source config, that couldn't be automatically propagated to the copy/pasted ones.

After investigating a bit how do dictionaries and unpacking operatoris work in python, I came to the conclusion that the simplest way to avoid it is to simply revert the changes for layerInfo here and in https://github.com/cms-sw/cmssw/pull/34009/files#diff-0dbbc624124b7539ebca54a0a8940ad7ca30d06b687c5bbb825c30a51e16dd7eR9

But perhaps python allows some more elegant and correct solution, that allows keep defining layerInfo as a dict in the source file, and propagate to the derived config with the possibility to modify a parameter (which is what causes the issue here). Do you have any suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With dict you can use copy module, i.e. along

import copy
dict_copy = copy.deepcopy(dict_orig)

For this specific case I'd like to remind that we recently allowed passing PSet as the first argument(s) for clone(). I think you could keep the layerInfo definition here as it is, and change SimpleCosmicBONSeeder_cff.py to something along

simpleCosmicBONSeedingLayers = _mod.seedingLayersEDProducer.clone(
    layerInfo,
    layerList = layerList
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @makortel
And what about modifying a parameter in an included PSet, as it was done originally in https://github.com/cms-sw/cmssw/pull/34009/files#diff-f633f65b880b125f9f08753ab0640fef555f76d83ce539433c8bcfeb3baecdc5L33 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow your question. The original code has

layerInfo = RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForCosmics_cfi.layerInfo.clone(
    TEC = dict(useSimpleRphiHitsCleaner = False)
)

that could stay as it is (the modification is in a clone() call after all).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Matti, thank you!

Then, if I understand it correctly, the suggestion for @jeongeun is:

  • Revert the change in RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmics_cfi.py and let the layerInfo as a PSet
  • Modify simpleCosmicBONSeedingLayers as you write above
  • Revert also the definition of layerInfo here in SimpleCosmicBONSeeder_cfi.py, with a clone and only the modified parameter replaced in it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you all!
To be consistent, I will revert (dict -> cms.PSet)all the changes in these 3 cfi.py files RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForBeamHalo_cfi.py,
RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmics_cfi.py, RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmicsRegionalReconstruction_cfi.py
Then, I will revert **layerInfo -> layerInfo as it is.

MTIB = cms.PSet(
TTRHBuilder = cms.string('WithTrackAngle'),
clusterChargeCut = cms.PSet(refToPSet_ = cms.string('SiStripClusterChargeCutNone')),
rphiRecHits = cms.InputTag("siStripMatchedRecHits","rphiRecHit")
),
TIB = cms.PSet(
matchedRecHits = cms.InputTag("siStripMatchedRecHits","matchedRecHit"),
TTRHBuilder = cms.string('WithTrackAngle'),
clusterChargeCut = cms.PSet(refToPSet_ = cms.string('SiStripClusterChargeCutNone'))
),
MTOB = cms.PSet(
TTRHBuilder = cms.string('WithTrackAngle'), clusterChargeCut = cms.PSet(refToPSet_ = cms.string('SiStripClusterChargeCutNone')),
rphiRecHits = cms.InputTag("siStripMatchedRecHits","rphiRecHit")
),
TOB = cms.PSet(
matchedRecHits = cms.InputTag("siStripMatchedRecHits","matchedRecHit"),
TTRHBuilder = cms.string('WithTrackAngle'), clusterChargeCut = cms.PSet(refToPSet_ = cms.string('SiStripClusterChargeCutNone'))
),
TEC = cms.PSet(
useSimpleRphiHitsCleaner = cms.bool(False),
minRing = cms.int32(5),
matchedRecHits = cms.InputTag("siStripMatchedRecHits","matchedRecHit"),
useRingSlector = cms.bool(False),
TTRHBuilder = cms.string('WithTrackAngle'), clusterChargeCut = cms.PSet(refToPSet_ = cms.string('SiStripClusterChargeCutNone')),
rphiRecHits = cms.InputTag("siStripMatchedRecHits","rphiRecHit"),
maxRing = cms.int32(7)
),
)
layerList = makeSimpleCosmicSeedLayers('ALL'),

Expand Down