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

Add PoolSource parameter to specify run number for each lumi #12753

Merged
merged 1 commit into from Dec 23, 2015

Conversation

wmtan
Copy link
Contributor

@wmtan wmtan commented Dec 10, 2015

This PR implements a new parameter "setRunNumberForEachLumi" for PoolSource, which allows a user to specify a new run number for each consecutive lumi in a single run in the simulation input. The run may be split across multiple input files. There must be at least as many entries in the parameter as the number of lumis that are read.
Unit tests for this feature are also implemented.
While implementing this new parameter, I made four synergistic improvements not directly related to the new feature:

  1. The function ThinnedAssociationsHelper:updateFromInput was actually two different unrelated functions cobbled together in a suboptimal way that resulted in the need to pass unused arguments in some cases. So, I refactored this function into two much cleaner functions.
  2. I also added two new delegating constructors for class RootFIle.
  3. I also eliminated the need for constructing dummy arguments fo the RootFile constructors by using pointers instead of references, so that nullptr could be used instead of a default constructed dummy.
  4. I put the use of the two parameters that renumber runs into a helper class using the strategy pattern.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @wmtan for CMSSW_8_0_X.

It involves the following packages:

DataFormats/Provenance
IOPool/Input
IOPool/Streamer

@cmsbuild, @smuzaffar, @Dr15Jones, @davidlange6 can you please review it and eventually sign? Thanks.
@wddgit this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

  • +1|approve[d]|sign[ed]: L1/L2's to approve it
  • -1|reject[ed]: L1/L2's to reject it
  • assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
  • unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
  • hold: L1/all L2's/release manager to mark it as on hold
  • unhold: L1/user who put this PR on hold
  • merge: L1/release managers to merge this request
  • [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
  • [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

@wmtan wmtan changed the title Add PoolSOurce parameter to specify run number for each lumi Add PoolSource parameter to specify run number for each lumi Dec 10, 2015
@Dr15Jones
Copy link
Contributor

Please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/10260/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@@ -319,6 +365,8 @@ namespace edm {
->setComment("Size of ROOT TTree TBasket cache. Affects performance.");
desc.addUntracked<unsigned int>("setRunNumber", 0U)
->setComment("If non-zero, change number of first run to this number. Apply same offset to all runs. Allowed only for simulation.");
desc.addUntracked<std::vector<unsigned int> >("setRunNumberForEachLumi", std::vector<unsigned int>())
Copy link
Contributor

Choose a reason for hiding this comment

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

Talk with David Dagenhart about how to specify that this new parameter and the setRun can not be used simultaneously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There already is such protection, but I will talk with David to see if this protection can be done in the descriptions instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this protection can be done in the descriptions. Will do.

@cmsbuild
Copy link
Contributor

Pull request #12753 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @davidlange6 can you please check and sign again.

@wmtan
Copy link
Contributor Author

wmtan commented Dec 18, 2015

@Dr15Jones This PR has been redone using a strategy pattern to encapsulate the new (and some old) functionality to enhance maintainability, as per a suggestion from Chris Jones.

@wmtan wmtan force-pushed the SpecifyRunNumberForEachLumi branch from 0a7fee8 to 06ad69d Compare December 18, 2015 18:47
@cmsbuild
Copy link
Contributor

Pull request #12753 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @davidlange6 can you please check and sign again.

@wmtan
Copy link
Contributor Author

wmtan commented Dec 18, 2015

Corrected a minor typo.

@wmtan
Copy link
Contributor Author

wmtan commented Dec 18, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/10359/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@Dr15Jones
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

davidlange6 added a commit that referenced this pull request Dec 23, 2015
Add PoolSource parameter to specify run number for each lumi
@davidlange6 davidlange6 merged commit 9c253b5 into cms-sw:CMSSW_8_0_X Dec 23, 2015
@wmtan wmtan deleted the SpecifyRunNumberForEachLumi branch February 5, 2016 20:20
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

4 participants