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

update number/names of layers/disks for phase 1 upgrade #19118

Merged

Conversation

capalmer85
Copy link
Contributor

No description provided.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2017

A new Pull Request was created by @capalmer85 for master.

It involves the following packages:

DQM/PixelLumi

@cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

@capalmer85
Copy link
Contributor Author

If I say "please test", will the bot comply?

@capalmer85
Copy link
Contributor Author

@cmsbuild please test

@capalmer85
Copy link
Contributor Author

@dmitrijus, @vanbesien... ok... so I can't ask @cmsbuild to do anything.

@dmitrijus
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 13, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20533/console Started: 2017/06/13 15:29

@dmitrijus
Copy link
Contributor

Using std::cout/cerr is not allowed, please replace those with edm::LogWarning

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Pull request #19118 was updated. @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please check and sign again.

@capalmer85
Copy link
Contributor Author

@dmitrijus I changed the std::cout's. Sorry about that.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 22
  • DQMHistoTests: Total histograms compared: 1817824
  • DQMHistoTests: Total failures: 34661
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1782997
  • DQMHistoTests: Total skipped: 166
  • DQMHistoTests: Total Missing objects: 0
  • Checked 90 log files, 14 edm output root files, 22 DQM output files

@dmitrijus
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 15, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20598/console Started: 2017/06/15 16:37

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@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-19118/20598/summary.html

Comparison Summary:

  • You potentially added 3 lines to the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 22
  • DQMHistoTests: Total histograms compared: 1803136
  • DQMHistoTests: Total failures: 14913
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1788057
  • DQMHistoTests: Total skipped: 166
  • DQMHistoTests: Total Missing objects: 0
  • Checked 90 log files, 14 edm output root files, 22 DQM output files

@capalmer85
Copy link
Contributor Author

Still not merged?

@capalmer85
Copy link
Contributor Author

Why is this not merge? @davidlange6 Is this not running online? @dmitrijus

@dmitrijus
Copy link
Contributor

@capalmer85 This PR should be running on the online DQM.

I don't know why it has not been merged. The change seems trivial and should only affect the online DQM.

@davidlange6
Copy link
Contributor

+1
unfortunate no attempt is made to make this backwards compatible..hopefully it stays online only.

@cmsbuild cmsbuild merged commit f91659f into cms-sw:master Aug 28, 2017
@capalmer85
Copy link
Contributor Author

@davidlange6 if that feedback was given any time between mid-june and now, I would have worked on tried.

@Dr15Jones
Copy link
Contributor

It appears this pull request is causing the gcc7 builds to have linking problems:https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc700/CMSSW_9_3_X_2017-08-29-1100/DQM/PixelLumi

I believe the easiest fix would be to change

static size_t const kNumLayers = 5;
static size_t const kNumDisks = 12;
static size_t const kOffsetLayers = 0;
static size_t const kOffsetDisks = 4;

to

static size_t constexpr const kNumLayers = 5;
static size_t constexpr const kNumDisks = 12;
static size_t constexpr const kOffsetLayers = 0;
static size_t constexpr const kOffsetDisks = 4;

@smuzaffar
Copy link
Contributor

@Dr15Jones , https://github.com/cms-sw/cmssw/pull/20293/files fixes this. Should we use
static size_t constexpr const instead of static constexpr size_t

@Dr15Jones
Copy link
Contributor

From http://en.cppreference.com/w/cpp/language/constexpr

A constexpr specifier used in an object declaration implies const

so no need to change the other pull request.

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