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

Adding E, R and Z ranges to Close by gun generator #26229

Closed
wants to merge 2 commits into from

Conversation

tonydp03
Copy link
Contributor

@tonydp03 tonydp03 commented Mar 21, 2019

PR description:

This pull request is made because we wanted to introduce a range in Energy, R and Z in the CloseByParticleGunProducer Workflow. Now in the configuration file the energy, R and Z ranges must be specified, while the ParticleGun will select a random Energy, R and Z in those ranges.

PR validation:

This pull request adds a couple of features (as described before) on top of the pull request #26065 made by @rovere.

@felicepantaleo
@clelange

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

1 similar comment
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26229/8855

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @tonydp03 (Tony Di Pilato) for master.

It involves the following packages:

IOMC/EventVertexGenerators
IOMC/ParticleGuns

@efeyazgan, @perrozzi, @civanch, @mdhildreth, @cmsbuild, @alberto-sanchez, @qliphy can you please review it and eventually sign? Thanks.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@tonydp03 tonydp03 changed the title Closeby particle z range Adding E, R and Z ranges to Close by gun generator Mar 21, 2019
@rovere
Copy link
Contributor

rovere commented Mar 21, 2019

Please add a working snippet in Configuration/Generator/python, so that this can be linked easily with runTheMatrix/cmsDriver. This was suggested in #26065

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26229/8856

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

Pull request #26229 was updated. @efeyazgan, @perrozzi, @civanch, @mdhildreth, @cmsbuild, @alberto-sanchez, @qliphy can you please check and sign again.

@efeyazgan
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 21, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33699/console Started: 2019/03/21 14:29

@cmsbuild
Copy link
Contributor

Comparison job queued.

@fabiocos
Copy link
Contributor

@tonydp03 I concur with the comment of @rovere , the proposal for Configuration/Generator does not work as it is now. You should add a generator fragment, then in case you may add a full test configuration in the test area of the package (if deemed useful).

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26229/8885

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

Pull request #26229 was updated. @efeyazgan, @perrozzi, @civanch, @mdhildreth, @cmsbuild, @alberto-sanchez, @qliphy can you please check and sign again.

@tonydp03
Copy link
Contributor Author

In the last commit I modified the snippet according to your suggestions @rovere . It should work now @fabiocos .

@fabiocos
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 25, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33742/console Started: 2019/03/25 13:46

@fabiocos
Copy link
Contributor

please abort

@cmsbuild
Copy link
Contributor

Jenkins tests are aborted.

@fabiocos
Copy link
Contributor

@tonydp03 sorry, quickly looking at the generator fragment, it now looks ok, apart for the name (please use _cfi.py as for all other fragments). Anyway it looks you used cms-merge-topic to prepare this PR, which in general should be avoided. I see here 7 new files, but in practice 6 of them are from #26065, am I correct? I suggest that you close this PR and open a new one, where you just add the _cfi.py configuration fragment on top of the latest 10_6_X branch

@tonydp03
Copy link
Contributor Author

@fabiocos Indeed, but I modified the CloseByParticleGunProducer files in #26065 to handle a variable range in energy, R and z (that was the goal of the pull request), that's why I used cms-merge-topic.
In addition, since it had been asked in #26065 as well, I added the snippet according to the new changes.

@fabiocos
Copy link
Contributor

@tonydp03 fine, but please propose a clean PR with the snippet and the modification to the gun, without other cms-merge-topic

@tonydp03 tonydp03 closed this Mar 28, 2019
@tonydp03
Copy link
Contributor Author

Addressed in #26277.

@tonydp03 tonydp03 deleted the ClosebyParticle_ZRange branch June 28, 2019 16:05
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

5 participants