Skip to content

Add GEM unpacker in L1Repack FullMC and temporary load of GEM geometry to hltGetConfiguration #34788

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

Closed
srimanob wants to merge 6 commits into from
Closed

Add GEM unpacker in L1Repack FullMC and temporary load of GEM geometry to hltGetConfiguration #34788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bb52bb0
add GEM unpacker in L1REPACK and update hltGetConfig
Aug 5, 2021
eebc1ec
fix GEM producer for dd4hep wf
Aug 12, 2021
e96a035
rebase and edit config
Sep 4, 2021
96a960d
update for dd4hep
Sep 4, 2021
47b90a6
code clean up
Sep 15, 2021
6c2bedf
add apply(process)
Sep 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Configuration/StandardSequences/python/SimL1EmulatorRepack_FullMC_cff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ def _print(ignored):
unpackCSC = EventFilter.CSCRawToDigi.cscUnpacker_cfi.muonCSCDigis.clone(
InputObjects = cms.InputTag( 'rawDataCollector', processName=cms.InputTag.skipCurrentProcess()))

import EventFilter.GEMRawToDigi.muonGEMDigis_cfi
unpackGEM = EventFilter.GEMRawToDigi.muonGEMDigis_cfi.muonGEMDigis.clone(
InputLabel = cms.InputTag( 'rawDataCollector', processName=cms.InputTag.skipCurrentProcess()))

import EventFilter.EcalRawToDigi.EcalUnpackerData_cfi
unpackEcal = EventFilter.EcalRawToDigi.EcalUnpackerData_cfi.ecalEBunpacker.clone(
InputLabel = cms.InputTag( 'rawDataCollector', processName=cms.InputTag.skipCurrentProcess()))
Expand Down Expand Up @@ -84,6 +88,9 @@ def _print(ignored):
simCaloStage2Layer1Digis.ecalToken = 'unpackEcal:EcalTriggerPrimitives'
simCaloStage2Layer1Digis.hcalToken = 'simHcalTriggerPrimitiveDigis'

# GEM
simMuonGEMPadDigis.InputCollection = 'unpackGEM'

# Finally, pack the new L1T output back into RAW
from EventFilter.L1TRawToDigi.caloStage2Raw_cfi import caloStage2Raw as packCaloStage2
from EventFilter.L1TRawToDigi.gmtStage2Raw_cfi import gmtStage2Raw as packGmtStage2
Expand All @@ -105,6 +112,7 @@ def _print(ignored):
stage2L1Trigger.toReplaceWith(SimL1EmulatorTask, cms.Task(unpackRPC
, unpackDT
, unpackCSC
, unpackGEM
, unpackEcal
, unpackHcal
#, simEcalTriggerPrimitiveDigis
Expand Down
14 changes: 14 additions & 0 deletions HLTrigger/Configuration/python/customizeHLTforCMSSW.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
# helper fuctions
from HLTrigger.Configuration.common import *

# dd4hep
from Configuration.ProcessModifiers.dd4hep_cff import dd4hep

# add one customisation function per PR
# - put the PR number into the name of the function
# - add a short comment
Expand Down Expand Up @@ -129,10 +132,21 @@ def customiseFor2018Input(process):
return process


#temporary solution to add GEM geometry for hltGetConfiguration
def customiseFor34788(process):
"""Add GEM geometry to output from hltGetConfiguration"""
process.load("Geometry.GEMGeometryBuilder.gemGeometryDB_cfi")

Copy link
Contributor

@missirol missirol Sep 15, 2021

Choose a reason for hiding this comment

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

If this function needs to be used via hltGetConfiguration --customise [..].customiseForRun3GEMGeometry34788 (which will write process = customiseForRun3GEMGeometry34788(process)), then the function body is missing return process.

In general, if the function is run from inside customizeHLTforCMSSW, passing it via the --customise option of hltGetConfiguration is not needed; if that's done, the function will be run twice (first by customizeHLTforCMSSW, then by the call added via --customise).

Also (but this is just an opinion), if the function is meant to be temporary, its name should just contain the PR number (i.e. customiseFor34788), plus a description as comment in the function body (which is already done). If the function is not temporary (but this is a much less common occurrence), then one should probably not use the PR number, like for other functions in this file (e.g. customisePixelGainForRun2Input).

Copy link
Contributor Author

@srimanob srimanob Sep 15, 2021

Choose a reason for hiding this comment

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

Fixed. Thx.

return process

# CMSSW version specific customizations
def customizeHLTforCMSSW(process, menuType="GRun"):

# add call to action function in proper order: newest last!
# process = customiseFor12718(process)

# temporary for GEM
if menuType in ["GRun","HIon","PIon","PRef"]:
(~dd4hep).makeProcessModifier(customiseFor34788).apply(process)

Copy link
Contributor

@missirol missirol Sep 15, 2021

Choose a reason for hiding this comment

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

I'm not very familiar with the syntax of makeProcessModifier, but I don't see how this is modifying the process (assuming that is the intention). I would have expected

(~dd4hep).makeProcessModifier(customiseForRun3GEMGeometry34788).apply(process)

Also, I was curious to know if the check on menuType is necessary. If not, it's maybe better to remove it.

Copy link
Contributor

@missirol missirol Sep 15, 2021

Choose a reason for hiding this comment

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

It would also be best to move the import call outside the function.

Copy link
Contributor Author

@srimanob srimanob Sep 15, 2021

Choose a reason for hiding this comment

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

AFAIK, makeProcessModifier acts on the process as a whole, so we don't need process again.

For the menu, as I test, it will not work with the Fake menu, i.e. Run1 or Run2 config. I assume because GEM does not exist. This is to provide a temporary solution until GEM unpacker will integrate with CSC (I hope soon), then the whole of this can be removed.

I can move import away, but then it will need to be at the beginning, or just before the function. It is fine to me.

Copy link
Contributor

@makortel makortel Sep 16, 2021

Choose a reason for hiding this comment

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

AFAIK, makeProcessModifier acts on the process as a whole, so we don't need process again.

That is true for cfi/cff fragments that get load into the process via process.load() (which is the stage where cms.Process calls the apply()). In this case the ProcessModifier is created in a customize function, in which case the ProcessModifier does not automatically propagate to the process, and therefore, as @missirol pointed out, the ProcessModifier.apply() needs to be called explicitly.

Copy link
Contributor Author

@srimanob srimanob Sep 16, 2021

Choose a reason for hiding this comment

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

Thanks @makortel
I've updated the PR.

Copy link
Contributor Author

@srimanob srimanob Sep 16, 2021

Choose a reason for hiding this comment

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

If not dd4hep. When DD4hep is used, I will get

two EventSetup Producers want to deliver type="GEMGeometry" label=""
 from record MuonGeometryRecord. The two providers are 
1) type="GEMGeometryESModule" label=""
2) type="GEMGeometryESModule" label="gemGeometry"

Copy link
Contributor

@fwyzard fwyzard Sep 16, 2021

Choose a reason for hiding this comment

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

It's a "not": if the dd4hep modifier is not active, then call customiseFor34788

Copy link
Contributor

@Martin-Grunewald Martin-Grunewald Sep 16, 2021

Choose a reason for hiding this comment

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

That will be a pain when we move the ES module into the HLT menus in ConfDb, as that is not dd4hep aware. Why this different treatment dd4hep vs non-dd4hep?

Copy link
Contributor

@Martin-Grunewald Martin-Grunewald Sep 16, 2021

Choose a reason for hiding this comment

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

Also, process load should avoid the duplication as you load exactly the same into the process (twice).

Copy link
Contributor Author

@srimanob srimanob Sep 16, 2021

Choose a reason for hiding this comment

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

I guess that when--procModifier dd4hep is used, it calls this. But I have to check with Geometry.
Apart from GEM ES module, why we don't see issues from other ES modules (in HLT menu in ConfDB) that should be called in the same way.

return process