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.

Sign up for GitHub

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

Jump to bottom

LS-aware access to HcalChannelQuality #25351

Merged
merged 2 commits into from Dec 4, 2018
Merged

LS-aware access to HcalChannelQuality #25351

merged 2 commits into from Dec 4, 2018

Conversation

abdoulline
Copy link

@abdoulline abdoulline commented Nov 27, 2018 •
edited

Preparing to move to per-LS HcalChannelQuality conditions.
HCAL DQM will be updated in a separate PR by David Yu (@DryRun)

@cmsbuild cmsbuild added this to the CMSSW_10_4_X milestone Nov 27, 2018
@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-25351/7374

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @abdoulline (Salavat Abdullin) for master.

It involves the following packages:

RecoLocalCalo/HcalRecAlgos
RecoMET/METFilters

@perrotta, @monttj, @cmsbuild, @fgolf, @slava77, @peruzzim can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @ahinzmann, @mmarionncern, @argiro, @jdamgov, @jdolen, @nhanvtran, @gkasieczka, @clelange, @schoef, @mariadalfonso, @seemasharmafnal this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos 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 Nov 27, 2018 •
edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31869/console Started: 2018/11/27 08:26
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31872/console Started: 2018/11/27 08:34

@cmsbuild
Copy link
Contributor

+1
Tested at: ab580cb
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25351/31872/summary.html

@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-25351/31872/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 33
  • DQMHistoTests: Total histograms compared: 3131839
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3131633
  • DQMHistoTests: Total skipped: 204
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
  • Checked 137 log files, 14 edm output root files, 33 DQM output files

@DryRun DryRun mentioned this pull request Nov 28, 2018
Merged
@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2018

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25351/7428

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2018

Pull request #25351 was updated. @perrotta, @tocheng, @monttj, @cmsbuild, @franzoni, @fgolf, @slava77, @pohsun, @peruzzim, @lpernie can you please check and sign again.

@perrotta
Copy link
Contributor

perrotta commented Dec 2, 2018

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2018 •
edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31933/console Started: 2018/12/02 09:57

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2018

+1
Tested at: ce0bced
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25351/31933/summary.html

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2018

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 33
  • DQMHistoTests: Total histograms compared: 3131839
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3131634
  • DQMHistoTests: Total skipped: 204
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
  • Checked 137 log files, 14 edm output root files, 33 DQM output files

@pohsun
Copy link

pohsun commented Dec 2, 2018

+1

slava77
slava77 reviewed Dec 3, 2018
View reviewed changes
Copy link
Contributor

@slava77 slava77 left a comment

Choose a reason for hiding this comment

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

I also notice that HcalRecHitReflagger is a legacy module. This update can be a good opportunity to update it to e.g. edm::stream (or even edm::global, if it is possible)

RecoLocalCalo/HcalRecAlgos/test/HcalRecHitReflagger.cc
}

}
void HcalRecHitReflagger::beginRun(const Run& r, const EventSetup& c) { }
Copy link
Contributor

Choose a reason for hiding this comment

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

this (now empty) method can be removed

RecoLocalCalo/HcalRecAlgos/test/HcalRecHitReflagger.cc
HcalDetId id=HcalDetId(*i);
int status=(chanquality_.getValues(id))->getValue();
if ( (status & (1<<HcalChannelStatus::HcalCellDead))==0 ) continue;
badstatusmap[id]=status;
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this map reset?

RecoLocalCalo/HcalRecAlgos/test/HcalRecHitReflagger.cc
const HcalChannelQuality& chanquality_(*p.product());

std::vector<DetId> mydetids = chanquality_.getAllChannels();
for (std::vector<DetId>::const_iterator i = mydetids.begin();i!=mydetids.end();++i)
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like a pretty expensive operation to be done every event.
Some ESWatcher should be added to update only when necessary

@slava77
Copy link
Contributor

slava77 commented Dec 3, 2018

I somewhat slowly realized that RecoLocalCalo/HcalRecAlgos/test/HcalRecHitReflagger.cc is in a /test directory and is not used for production.
The review concerning this file is then advisory, I will not hold the PR signoff for that.

@slava77
Copy link
Contributor

slava77 commented Dec 3, 2018 •
edited

+1

for #25351 ce0bced

  • code changes are in line with the PR description; this is a somewhat technical update; the reco code modified in this PR is not used in production workflows.
  • jenkins tests pass and comparisons with the baseline show no differences

@abdoulline
Copy link
Author

abdoulline commented Dec 3, 2018 via email

@fabiocos
Copy link
Contributor

fabiocos commented Dec 4, 2018

+1

@fabiocos
Copy link
Contributor

fabiocos commented Dec 4, 2018

merge

@cmsbuild cmsbuild merged commit 1ba5eee into cms-sw:master Dec 4, 2018
@abdoulline abdoulline deleted the LS_based_ChannelQuality branch May 13, 2019 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slava77 slava77 slava77 left review comments

Assignees
No one assigned
Labels
alca-approved analysis-pending code-checks-approved comparison-available orp-approved pending-signatures reconstruction-approved tests-approved
Projects
None yet
Milestone
CMSSW_10_4_X
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@abdoulline @cmsbuild @perrotta @pohsun @slava77 @fabiocos