Navigation Menu

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

set data processing bunch spacing configs to 50ns for HI #12246

Conversation

slava77
Copy link
Contributor

@slava77 slava77 commented Nov 3, 2015

  • backport of Central bunch spacing producer for 76x #11576 to 75X
  • force data processing setup for HI Run2 to 50 ns via bunch-spacing override [this is the important bug fix, the rest of the changes are mostly technical]
  • use bunchSpacingProducer override for 50 ns for pp run2 data processing setup "50nsRunsAfter253000" aka ppRun2at50ns scenario
  • add bunchSpacingProducer to the event content

The new pieces here will be propagated to 76X imminently.

Tests done in 7_5_4:

  • short matrix: to check that backport of bunchSpacingProducer code is done correctly (comparisons show no differences ==> OK)
  • run2 2015B and 2015C (134.70* and 134.80*) workflows to check that setup for these was correct already (comparisons show no differences ==> OK)
  • T0 setup processing with ppRun2at50ns of 2015B run to confirm that overrides act correctly (comparisons show no differences ==> OK)
  • T0 setup processing with HeavyIonsRun2 of 2015B run (50 ns) to confirm that overrides act correctly (comparisons show no differences ==> OK)
  • T0 setup processing with HeavyIonsRun2 of 2015C run (25 ns) to confirm that there are changes in ecal hits and downstream objects (comparisons show differences ==> OK)

@slava77
Copy link
Contributor Author

slava77 commented Nov 3, 2015

@cmsbuild please test

@cmsbuild cmsbuild added this to the Next CMSSW_7_5_X milestone Nov 3, 2015
@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2015

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2015

A new Pull Request was created by @slava77 (Slava Krutelyov) for CMSSW_7_5_X.

set data processing bunch spacing configs to 50ns for HI

It involves the following packages:

Configuration/DataProcessing
Configuration/EventContent
Configuration/StandardSequences
RecoEgamma/EgammaTools
RecoLocalCalo/EcalRecProducers
RecoLuminosity/LumiProducer
RecoParticleFlow/PFClusterProducer
Validation/Configuration

@civanch, @cvuosalo, @mdhildreth, @cmsbuild, @franzoni, @deguio, @slava77, @danduggan, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @mmarionncern, @Sam-Harper, @argiro, @makortel, @GiacomoSguazzoni, @rovere, @lgray, @Martin-Grunewald, @cerati, @VinInn, @dgulhan, @bachtis 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' or '@cmsbuild, please test' in the first line of a comment.
@Degano 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

cmsbuild commented Nov 3, 2015

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2015

@slava77
Copy link
Contributor Author

slava77 commented Nov 3, 2015

+1

for #12246 b127331

  • jenkins confirmed no changes (as observed in local tests)

@davidlange6
Copy link
Contributor

Slava- to be clear, you are adding the bunchSpacingProducer not only to the output, but to the release. Should be fine..

@slava77
Copy link
Contributor Author

slava77 commented Nov 4, 2015

@davidlange6
I'm not sure if your comment is about this PR (75X) or what's not fully done in 76X.
If about this PR, I may be missing a point of the comment (yes, the bunchSpacingProducer code was not in this release; the change is beyond just DP config changes)

@davidlange6
Copy link
Contributor

this PR - its adding the feature to 75x..

On Nov 4, 2015, at 7:51 PM, Slava Krutelyov notifications@github.com wrote:

@davidlange6
I'm not sure if your comment is about this PR (75X) or what's not fully done in 76X.
If about this PR, I may be missing a point of the comment (yes, the bunchSpacingProducer code was not in this release; the change is beyond just DP config changes)


Reply to this email directly or view it on GitHub.

@civanch
Copy link
Contributor

civanch commented Nov 4, 2015

+1

davidlange6 added a commit that referenced this pull request Nov 5, 2015
…ng-set-50ns

set data processing bunch spacing configs to 50ns for HI
@davidlange6 davidlange6 merged commit 8660fe9 into cms-sw:CMSSW_7_5_X Nov 5, 2015
@harmonicoscillator
Copy link
Contributor

@slava77
@ttrk and I have a stupid question - where can we find RecoLuminosity.LumiProducer.bunchSpacingProducer_cfi ? It does not appear to exist, even though the master reconstruction cff imports it... : https://github.com/cms-sw/cmssw/blob/CMSSW_7_5_X/Configuration/StandardSequences/python/Reconstruction_cff.py#L4

We're trying to test this PR on PbPb data and we're getting errors related to that python file.

@slava77
Copy link
Contributor Author

slava77 commented Nov 9, 2015

@richard-cms
It's auto-generated
see $CMSSW_RELEASE_BASE/cfipython/$SCRAM_ARCH/RecoLuminosity/LumiProducer/bunchSpacingProducer_cfi.py

@harmonicoscillator
Copy link
Contributor

@slava77 (please let me know if you'd like to discuss this via email or another place)

After some discussion with @matteosan1 and doing some checking ourselves, we really think that we would prefer to use the legacy Global ecal rechit reconstruction since we are not affected by OOT PU during PbPb running. We see that the configuration still exists here: https://github.com/cms-sw/cmssw/blob/CMSSW_7_5_X/RecoLocalCalo/EcalRecProducers/python/ecalGlobalUncalibRecHit_cfi.py

Can you describe the best way for us to use that method instead? Thanks as always for your assistance.

@yenjie @ttrk @dgulhan

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

6 participants