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.

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

Migrate Sim Code to New Random Service Interface #2392

Merged
merged 1 commit into from Feb 15, 2014

Conversation

wddgit
Copy link
Contributor

@wddgit wddgit commented Feb 10, 2014

Migrate most simulation code to use the new interface
of the random number generator service designed to work
with the multithreaded Framework. The main interface
change is to require a StreamID or LuminosityBlockIndex
argument to the getEngine function. In most cases,
this required moving code from the constructor to
the event or beginLuminosityBlock method. Then a pointer
to the engine was percolated or passed in some way
to the point where it was used.

Note OscarProducer was migrated earlier and there are a
few pieces of code I skipped over that will be migrated
in the near future. This pull request takes care of most
of the digitizers and the mixing modules.

Migrate most simulation code to use the new interface
of the random number generator service designed to work
with the multithreaded Framework. The main interface
change is to require a StreamID or LuminosityBlockIndex
argument to the getEngine function. In most cases,
this required moving code from the constructor to
the event or beginLuminosityBlock method. Then a pointer
to the engine was percolated or passed in some way
to the point where it was used.

Note OscarProducer was migrated earlier and there are a
few pieces of code I skipped over that will be migrated
in the near future. This pull request takes care of most
of the digitizers and the mixing modules.
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @wddgit (W. David Dagenhart) for CMSSW_7_1_X.

Migrate Sim Code to New Random Service Interface

It involves the following packages:

FWCore/Sources
FastSimulation/Tracking
IOPool/Input
IOPool/SecondaryInput
Mixing/Base
SimCalorimetry/CaloSimAlgos
SimCalorimetry/CastorSim
SimCalorimetry/EcalSimAlgos
SimCalorimetry/EcalSimProducers
SimCalorimetry/HcalSimAlgos
SimCalorimetry/HcalSimProducers
SimCalorimetry/HcalTestBeam
SimG4CMS/EcalTestBeam
SimGeneral/DataMixingModule
SimGeneral/MixingModule
SimGeneral/NoiseGenerators
SimGeneral/TrackingAnalysis
SimMuon/CSCDigitizer
SimMuon/DTDigitizer
SimMuon/GEMDigitizer
SimMuon/Neutron
SimMuon/RPCDigitizer
SimTracker/Common
SimTracker/SiPixelDigitizer
SimTracker/SiStripDigitizer
Validation/EcalDigis

@civanch, @ojeda, @deguio, @lveldere, @danduggan, @mdhildreth, @cmsbuild, @Dr15Jones, @rovere, @giamman, @Degano, @ktf, @nclopezo can you please review it and eventually sign? Thanks.
@cerati, @GiacomoSguazzoni, @rovere, @wmtan this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

@Dr15Jones
Copy link
Contributor

+1

2 similar comments
@lveldere
Copy link
Contributor

+1

@civanch
Copy link
Contributor

civanch commented Feb 11, 2014

+1

@ktf
Copy link
Contributor

ktf commented Feb 12, 2014

@deguio @ojeda can you please look into this?

@davidlange6
Copy link
Contributor

nothing to do with the changes in this pull request - but why redo(or do differently? the ecal digitization in the validation module? I would have expected that we should validate what is done in the digi part as to be validating what is then stored for reco/analysis

@wddgit
Copy link
Contributor Author

wddgit commented Feb 12, 2014

The only reason I included Validation/EcalDigis/src/EcalMixingModuleValidation changes in this pull request was that it uses the CaloHitResponse class that is also used by the digitization code. I had to modify the CaloHitResponse interface. That one class is the only DQM code changed in this pull request. That does not answer David Lange's valid question, but I do not think that question should delay signature of this pull request.

@deguio
Copy link
Contributor

deguio commented Feb 13, 2014

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

@davidlange6
Copy link
Contributor

except we won't get an answer to the question if we accept and forget…

On Feb 12, 2014, at 5:33 PM, W. David Dagenhart notifications@github.com wrote:

The only reason I included Validation/EcalDigis/src/EcalMixingModuleValidation changes in this pull request was that it uses the CaloHitResponse class that is also used by the digitization code. I had to modify the CaloHitResponse interface. That one class is the only DQM code changed in this pull request. That does not answer David Lange's valid question, but I do not think that question should delay signature of this pull request.


Reply to this email directly or view it on GitHub.

@Dr15Jones
Copy link
Contributor

@davidlange6 How about we open a Jira ticket?

@ktf
Copy link
Contributor

ktf commented Feb 14, 2014

Is anyone tracking the issue?

@civanch
Copy link
Contributor

civanch commented Feb 15, 2014

I would say that keeping this PR not merged is also a problem - too many sub-packages are affected. I believe it is possible open new unscheduled PR with the issue described - for me it would be equivalent to a Jira ticket.

ktf added a commit that referenced this pull request Feb 15, 2014
Multithreading fixes -- Migrate Sim Code to New Random Service Interface
@ktf ktf merged commit 0fd9f24 into cms-sw:CMSSW_7_1_X Feb 15, 2014
@nclopezo nclopezo modified the milestones: CMSSW_7_1_0_pre4, CMSSW_7_1_0_pre3 Feb 24, 2014
@nclopezo nclopezo modified the milestones: CMSSW_7_1_0_pre5, CMSSW_7_1_0_pre4 Mar 10, 2014
@wddgit wddgit deleted the threadedRandomNumbersDigitizers branch April 7, 2014 16:19
ggovi pushed a commit to ggovi/cmssw that referenced this pull request Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants