GEM and ME0 realistic digitizer updates #18660
Conversation
|
A new Pull Request was created by @dildick (Sven Dildick) for master. It involves the following packages: IOMC/RandomEngine @perrotta, @smuzaffar, @civanch, @Dr15Jones, @mdhildreth, @dmitrijus, @cmsbuild, @kpedro88, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
| @@ -3,5 +3,5 @@ | |||
| me0RecHits = cms.EDProducer("ME0RecHitProducer", | |||
| recAlgoConfig = cms.PSet(), | |||
| recAlgo = cms.string('ME0RecHitStandardAlgo'), | |||
| me0DigiLabel = cms.InputTag("simMuonME0ReDigis"), | |||
| me0DigiLabel = cms.InputTag("simMuonME0PseudoReDigis"), | |||
There was a problem hiding this comment.
can we settle on some common naming so that (local) reco configs do not get perturbed frequently just because of some internal DIGI-step reorganization?
There was a problem hiding this comment.
@slava77 After this PR no more name changes are foreseen. The naming that is proposed is similar to the other muon digitizers simMuonME0Digis vs simMuonRPCDigis or simMuonGEMDigis,simMuonCSCDigis and simMuonDTDigis for realistic digis. simMuonME0PadDigis vs simMuonGEMPadDigis for pads. Pseudo digis now have the string Pseudo in them to distinguish them from real digis. Re-digitized pseudo digis for Muon TDR studies are called simMuonME0PseudoReDigis. This naming convention was agreed upon with other GEM/ME0 developers.
There was a problem hiding this comment.
For my education, could you briefly summarize "pseudo" and "re"? (pseudo means smeared, and "re" means?) -perhaps the word "sim" needs to be dropped at some point along this chain
There was a problem hiding this comment.
Pseudo means that the digis are of type ME0DigiPreReco. Real digis are of type ME0Digi. The pseudo digitization sequence for ME0 was introduced for performance studies (deciding layers, eta partitions, strips) for the Muon Upgrade TDR. This sequence will be taken out of the main digi step in the future. The real ME0 digi sequence is what will be used to build ME0 triggers and for continued ME0 performance studies.
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
On 5/10/17 11:57 AM, Sven Dildick wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In RecoLocalMuon/GEMRecHit/python/me0RecHits_cfi.py
<#18660 (comment)>:
> @@ -3,5 +3,5 @@
me0RecHits = cms.EDProducer("ME0RecHitProducer",
recAlgoConfig = cms.PSet(),
recAlgo = cms.string('ME0RecHitStandardAlgo'),
- me0DigiLabel = cms.InputTag("simMuonME0ReDigis"),
+ me0DigiLabel = cms.InputTag("simMuonME0PseudoReDigis"),
@slava77 <https://github.com/slava77> After this PR no more name changes
are foreseen. The naming that is proposed is similar to the other muon
digitizers |simMuonME0Digis| vs |simMuonRPCDigis| or
|simMuonGEMDigis|,|simMuonCSCDigis| and |simMuonDTDigis| for realistic
digis. |simMuonME0PadDigis| vs |simMuonGEMPadDigis| for pads. Pseudo
digis now have the string |Pseudo| in them to distinguish them from real
digis. Re-digitized pseudo digis for Muon TDR studies are called
|simMuonME0PseudoReDigis|. This naming convention was agreed upon with
other GEM/ME0 developers.
Is there a twiki or other well-established document that describes this?
The past few months looked a bit of a many-step walk that pushed product
name changes
to downstream applications for what's supposed to have the same physical
meaning
"digis useful for running standard reconstruction".
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18660 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbt0sLV4zrQewrmPcpMg6zr0h_iZRks5r4gi3gaJpZM4NWqbA>.
|
|
We can put together some slides as a reference. |
|
Comparison is ready Comparison Summary:
|
| #ifndef ME0OBJECTS_ME0DIGISIMLINK_H | ||
| #define ME0OBJECTS_ME0DIGISIMLINK_H | ||
|
|
||
| #include <map> |
There was a problem hiding this comment.
Only need #include <utility> (not map) for pair
| import FWCore.ParameterSet.Config as cms | ||
|
|
||
| # PSet of mixObjects that only keeps muon collections (and SimTracks with SimVertices) | ||
| mixObjects_dt_csc_rpc = cms.PSet( |
There was a problem hiding this comment.
Rather than duplicating this entire structure, I think it would be better to import/copy the regular mixObjects and just clear out the parts you don't want. This has the advantage of picking up any Era modifications in SimGeneral/MixingModule/python/mixObjects_cfi.py.
|
|
||
|
|
||
| # Add simMuonGEMDigis to the list of modules served by RandomNumberGeneratorService | ||
| def customize_random_GEMDigi(process): |
There was a problem hiding this comment.
These random number initializations are already done by Eras in IOMC/RandomEngine/python/IOMC_cff.py. They should not be duplicated here.
There was a problem hiding this comment.
@kpedro88 If I use the construct
# Add simMuonGEMDigis to the list of modules served by RandomNumberGeneratorService
def customize_random_GEMDigi(process):
from IOMC.RandomEngine.IOMC_cff import RandomNumberGeneratorService as rndsv
process.RandomNumberGeneratorService.simMuonGEMDigis = rndsv.simMuonGEMDigis
return process
in a driver command
cmsDriver.py SingleMuPt100_pythia8_cfi \ --conditions auto:phase2_realistic -n 10 \ --era Phase2C2_timing \ --eventcontent FEVTDEBUG \ -s GEN,SIM,DIGI:pdigi_valid \ --datatier GEN-SIM-DIGI \ --beamspot HLLHC \ --geometry Extended2023D4 \ --customise SimMuon/Configuration/customizeMuonDigi.customize_digi_addGEM_addME0_muon_only \ --python SingleMuPt100_2023tilted_GenSimFull.py \ --no_exec --fileout file:step1.root
then the driver crashes with
...
process = customize_random_GEMDigi(process)
File "/afs/cern.ch/work/d/dildick/public/GEM/ME0DigiIntegration/CMSSW_9_2_X_2017-05-08-2300/python/SimMuon/Configuration/customizeMuonDigi.py", line 65, in customize_random_GEMDigi
process.RandomNumberGeneratorService.simMuonGEMDigis = rndsv.simMuonGEMDigis
AttributeError: 'Service' object has no attribute 'simMuonGEMDigis'
How do I load the Phase2 RandomNumberGeneratorService before calling the customization?
There was a problem hiding this comment.
Or should I simply throw away the random number initialization and assume the initialization will be done by the --era command?
There was a problem hiding this comment.
Yes, you should assume the initialization will be done by the Era.
|
|
||
|
|
||
| # customize the full digitization sequence pdigi by adding GEMs | ||
| def customize_digi_addGEM_addGEM(process): |
There was a problem hiding this comment.
should this be called ..._addGEM_addME0?
| b=a+'output' | ||
| if hasattr(process,b): | ||
| getattr(process,b).outputCommands.append('keep *_g4SimHits_Muon*_*') | ||
| getattr(process,b).outputCommands.append('keep *_*Muon*_*_*') |
There was a problem hiding this comment.
you can combine these two lines into one extend
| for (ME0DigiCollection::const_iterator digiIt = range.first; digiIt!=range.second; ++digiIt) | ||
| { | ||
| /* | ||
| for(int i = 0; i < id.roll(); i++){ |
There was a problem hiding this comment.
delete commented-out code
| std::cout << "roll 8 numbers = " << countRoll8 << "\tndigi = " << ndigi8 << std::endl; | ||
|
|
||
| /* | ||
| for (edm::DetSetVector<StripDigiSimLink>::const_iterator itlink = thelinkDigis->begin(); itlink != thelinkDigis->end(); itlink++) |
There was a problem hiding this comment.
delete commented-out code
| , me0DigiSimLinkToken_(consumes < edm::DetSetVector<ME0DigiSimLink> > (pset.getParameter < edm::InputTag > ("me0DigiSimLinkToken"))) | ||
| , debug_(pset.getParameter<bool>("debugFlag")) | ||
| { | ||
| edm::Service < TFileService > fs; |
There was a problem hiding this comment.
I think you need to have usesResource("TFileService"); here
|
|
||
| if (MuCluster.size() != 0) | ||
| { | ||
| for (std::map<int, int>::iterator it = MuCluster.begin(); it != MuCluster.end(); ++it) |
There was a problem hiding this comment.
use range-based loop (also below)
| if (abs(particletype) == 13) | ||
| { | ||
| tof_mu_bx0->Fill(partTof); | ||
| MuCluster.insert(std::pair<int, int>(strip, particletype)); |
There was a problem hiding this comment.
use emplace() (also below)
|
@dildick please only reply to my review comments on GitHub if you have a question or objection |
| process.rpcRecHits, | ||
| cms.Sequence(process.simMuonRPCReDigis+process.rpcRecHits) | ||
| ) | ||
| return process |
There was a problem hiding this comment.
Are you removing this file? by accident?
There was a problem hiding this comment.
Not by accident. It is now included in SimMuon/Configuration/python/customizeMuonDigi.py which will contain all expert customizations for muon digi studies.
|
Comparison is ready Comparison Summary:
|
|
+1
|
|
+1 |
|
+1 |
|
@davidlange6 Do you have any more comments on this PR? |
|
@Dr15Jones @civanch @mdhildreth please sign again |
|
+1 |
|
+1 |
cmsbuild
added
fully-signed
simulation-approved
and removed
pending-signatures
simulation-pending
labels
|
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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
|
Can this pull request be signed please? |
|
+1 |
dildick
deleted the
from-CMSSW_9_2_X_2017-05-08-2300-ME0-realistic-digi-updates
branch
This pull request includes updates for the ME0 digtizer
Below is a list of things I changed:
real digis (
ME0Digi) --> nowsimMuonME0Digisreal pad digis (
ME0PadDigi) --> nowsimMuonME0PadDigisfake digis (
ME0DigiPreReco) --> nowsimMuonME0PseudoDigisfake re-digis (
ME0DigiPreReco) --> nowsimMuonME0PseudoReDigisnew ME0 digi sequence:
simMuonME0Digis + simMuonME0PadDigis + simMuonME0PseudoDigis + simMuonME0PseudoReDigiscustomizeGEMDigi,customizeME0DigiandcustomizeRPCDigiin different packages to a single python filecustomizeMuonDigi.py. IMO there was way too much code-duplication in those customizations. I believe I merged all background customizations correctly.ME0PadDigiProducerwas updated to use theME0DigiCollectioninstead ofME0DigiPreRecoCollectionSupporting information for the GEM and ME0 digitizers is being added here: https://twiki.cern.ch/twiki/bin/view/CMS/GEMDigitizationRoadMap
@calabria @mileva