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

Ecal bad calib filter #20698

Merged
merged 15 commits into from
Oct 29, 2017
Merged

Ecal bad calib filter #20698

merged 15 commits into from
Oct 29, 2017

Conversation

dvannerom
Copy link
Contributor

Pull Request to add the EcalBadCalibFilter in the standard filters sequence

@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/PR-20698/1051

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-20698/1051/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20698/1051/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@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/PR-20698/1054

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

PhysicsTools/PatAlgos
RecoMET/METFilters

@perrotta, @cmsbuild, @monttj, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @imarches, @ahinzmann, @acaudron, @mmarionncern, @rappoccio, @jdamgov, @jdolen, @nhanvtran, @gpetruc, @gkasieczka, @schoef, @ferencek, @mverzett, @mariadalfonso, @pvmulder, @seemasharmafnal, @JyothsnaKomaragiri this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20698/1055

@slava77
Copy link
Contributor

slava77 commented Sep 29, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 29, 2017

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

std::cout << "Detid=" << eedet.rawId()
<< " ix=" << ix << " iy=" << iy<< " iz=" << iz
<< " energy=" << ene << " et=" << et << std::endl;
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

is this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant

this applies to all of this PR

@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/PR-20698/1670

@cmsbuild
Copy link
Contributor

Pull request #20698 was updated. @perrotta, @cmsbuild, @monttj, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Oct 27, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 27, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/24009/console Started: 2017/10/27 10:40

@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-20698/24009/summary.html

There are some workflows for which there are errors in the baseline:
10824.0 step 5
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 26
  • DQMHistoTests: Total histograms compared: 2786851
  • DQMHistoTests: Total failures: 121
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2786558
  • DQMHistoTests: Total skipped: 171
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 10 edm output root files, 26 DQM output files

@slava77
Copy link
Contributor

slava77 commented Oct 28, 2017

+1

for #20698 47fd4b9

  • ecalBadCalibFilter to be used for run2 processing.
  • jenkins tests pass
  • local tests with MC 10224 and data from JetHT run 302651 prompt AOD show that the filter is running at negligible cost in CPU. The selection rate in MC is below O(1-3) [0 events in 3K test], and around 0.2% in JetHT data [8/5000 selected]

I think that it should be safe to add this to 94X.
@dvannerom @gouskos please prepare a backport PR to 94X if you need this filter in 94X.

@gouskos
Copy link
Contributor

gouskos commented Oct 28, 2017

Thank you @slava77 Yes we need it in 94X - we will prepare it today

@gouskos
Copy link
Contributor

gouskos commented Oct 28, 2017

Hi @slava77 We created #21061 using the latest 94X IB but I see that the milestone is set again for CMSSW_10_0_X. Can you please advice how we should backport it ? Thank you.

@perrotta
Copy link
Contributor

@gouskos : when you submit your pull request, set the base as 9_4_X, instead of keeping the default (which is always master)

@gouskos
Copy link
Contributor

gouskos commented Oct 28, 2017

@perrotta Oh - yes - I am sorry . I am doing it now. So we can close the previous PR or I can change it on the fly ?

@gouskos
Copy link
Contributor

gouskos commented Oct 28, 2017

Hi @perrotta @slava77 please find here the correct PR (#21062) to backport the EcalBadCalib filter in 94X.

@davidlange6
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit e2c95d1 into cms-sw:master Oct 29, 2017
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.

10 participants