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

island photons for XeXe collision era + HI pp reference run era and relval wf (92X) #21070

Merged
merged 5 commits into from
Nov 7, 2017

Conversation

ttrk
Copy link
Contributor

@ttrk ttrk commented Oct 30, 2017

fixes the pp_on_XeXe_2017 era to run island photons and add them into event content
original 92X PR for pp_on_XeXe_2017 era : #20760
adds "Run2_2017_ppRef" era to run customized reco for HI pp reference run
adds a corresponding relval wf (149)
runs and stores photonIsolationHIProducer objects using the era
backport of #20929 and #21122

apply the era modifiers to the original definitions in RecoEcal/EgammaClusterProducers
as suggested here cms-sw#20929 (comment)
and here cms-sw#20929 (comment)
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @ttrk (Kaya Tatar) for CMSSW_9_2_X.

It involves the following packages:

RecoEcal/Configuration
RecoEcal/EgammaClusterProducers
RecoEgamma/Configuration
RecoEgamma/EgammaPhotonProducers
RecoHI/HiEgammaAlgos

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @jainshilpi, @varuns23, @rovere, @argiro, @yenjie, @jazzitup, @kurtejung, @lgray, @mandrenguyen, @dgulhan, @yetkinyilmaz this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 30, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/24040/console Started: 2017/10/30 09:42

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21070/24040/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 2160393
  • DQMHistoTests: Total failures: 14535
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2145686
  • DQMHistoTests: Total skipped: 172
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 9 edm output root files, 23 DQM output files

@perrotta
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_9_2_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_10_0_X is complete. This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

By the way, @ttrk et al.: Which are the plans for this PR? Do you need to have it actually backported to 92X? If so, when? (I.e. should we ask a 92X release with it as soon as merged?)

@ttrk
Copy link
Contributor Author

ttrk commented Oct 30, 2017

The PR for 92X is needed, because the current version does not work as it should. The timeline depends on the new PR that I mentioned here #20929 (comment).
If the pp ref run is to be taken with 92X, then I assume this 92X PR becomes more urgent to merge so that I can make the PR for pp reference era.
I think a new release for this 92X PR is not urgent, because there is no plan to re-reco xexe data with 92X, the xexe re-reco will be with 94X.

@mandrenguyen
Copy link
Contributor

Can we merge this so we submit a 2nd PR adding a pp at 5TeV era? The run is (might be?) around the corner.

@slava77
Copy link
Contributor

slava77 commented Nov 2, 2017

Can we merge this so we submit a 2nd PR adding a pp at 5TeV era? The run is (might be?) around the corner.

the issue with the backports is that chained integration is not really supported. So, if you need this plus something else, you will need to make a combined PR.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2017

Pull request #21070 was updated. @perrotta, @prebello, @kpedro88, @fabozzi, @cmsbuild, @franzoni, @slava77, @GurpreetSinghChahal, @davidlange6 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Nov 7, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/24223/console Started: 2017/11/07 09:17

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2017

Comparison job queued.

@perrotta
Copy link
Contributor

perrotta commented Nov 7, 2017

After going through all lines of this PR I verified that it is a faithful copy of the merging of the two PRs #20929 and #21122, which are meant to be backported in 92X

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 7, 2017

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-21070/24223/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 2160359
  • DQMHistoTests: Total failures: 44287
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2115900
  • DQMHistoTests: Total skipped: 172
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 9 edm output root files, 23 DQM output files

@perrotta
Copy link
Contributor

perrotta commented Nov 7, 2017

+1

  • Backport needed for the ppref run at 5 TeV
  • Configuration changes are driver by the corresponding era, and coincide with the ones already merged in the master

@kpedro88
Copy link
Contributor

kpedro88 commented Nov 7, 2017

+1

@fabozzi
Copy link
Contributor

fabozzi commented Nov 7, 2017

+1

@davidlange6
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 1fcd76c into cms-sw:CMSSW_9_2_X Nov 7, 2017
self.recoSeq=''
self.cbSc='pp'
self.addEI=True
self.isRepacked=True
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want a new 92X PR for this ? Also should there be new PRs for 94X and master branch ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I made a PR for 92X and for master.
There is no need for 94X because T0 is not going to switch to 94X and this scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

PRs with fixes are in #21255 and #21256

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.

8 participants