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

Updating HLT DQM and validation for Hgg paths #19023

Merged
merged 1 commit into from Jul 11, 2017

Conversation

mplaner
Copy link
Contributor

@mplaner mplaner commented May 30, 2017

This PR adds the offline DQM for HLT as well as the HLT validation for diphoton triggers.

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

DQMOffline/Trigger
HLTriggerOffline/Higgs

@cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@battibass, @mtosi, @jhgoh, @calderona, @HuguesBrun, @trocino, @rociovilar this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

@dmitrijus
Copy link
Contributor

This needs greenlight from @mtosi :)

@mtosi
Copy link
Contributor

mtosi commented May 31, 2017

I think it needs the new updates by bhawna
in #18971

@mplaner
Copy link
Contributor Author

mplaner commented May 31, 2017

@mtosi
Yeah, I would like to wait until that PR is approved and then merge myself to make sure everything still works correctly for us.

I can do this now, if we're confident that there won't be any further changes to #18971.

@mtosi
Copy link
Contributor

mtosi commented Jun 6, 2017

I think bhawna PR is almost done (just waiting for the integration test)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2017

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

@mplaner
Copy link
Contributor Author

mplaner commented Jun 6, 2017

I updated and tested after merging PR #18971 into my local area. I've added the additional plots for subleading photon for doublePhoton triggers.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2017

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

@mtosi
Copy link
Contributor

mtosi commented Jun 8, 2017

@dmitrijus can we start the integration test, please ?

@mtosi mtosi mentioned this pull request Jun 8, 2017
@fwyzard
Copy link
Contributor

fwyzard commented Jun 8, 2017

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20421/console Started: 2017/06/08 14:12

@fwyzard
Copy link
Contributor

fwyzard commented Jun 8, 2017

tracked by #19142

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2017

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2017

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

Comparison Summary:

  • You potentially added 7 lines to the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1856393
  • DQMHistoTests: Total failures: 3150
  • DQMHistoTests: Total nulls: 65
  • DQMHistoTests: Total successes: 1853005
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@mtosi
Copy link
Contributor

mtosi commented Jun 12, 2017

what about
HLT_Diphoton30EB_18EB_R9Id_OR_IsoCaloId_AND_HE_R9Id_PixelVeto_Mass55_v*
HLT_Diphoton30PV_18PV_R9Id_AND_IsoCaloId_AND_HE_R9Id_NoPixelVeto_Mass55_v*
HLT_Diphoton30EB_18EB_R9Id_OR_IsoCaloId_AND_HE_R9Id_NoPixelVeto_Mass55_v*
?

@fwyzard
Copy link
Contributor

fwyzard commented Jul 11, 2017

The failing tests are unrelated to this PR

@fwyzard
Copy link
Contributor

fwyzard commented Jul 11, 2017

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 11, 2017

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

fwyzard added a commit to fwyzard/cmssw that referenced this pull request Jul 11, 2017
@fwyzard fwyzard mentioned this pull request Jul 11, 2017
@fwyzard
Copy link
Contributor

fwyzard commented Jul 11, 2017

Please merge #19684 which includes this and fixes conflicts.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@davidlange6
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 791292d into cms-sw:master Jul 11, 2017
@fwyzard
Copy link
Contributor

fwyzard commented Jul 11, 2017

David,
may I very politely ask "what the fuck ?" ?

I spent some time to go over all the HLT DQM PRs that were OK from the implementation point of view, merge all of them taking care of the conflicts, and write an explicit message on GitHub in each of them.

And the first thing you do is merge one of them at random messing up the conflict resolution I have already done.

.Andrea

@davidlange6
Copy link
Contributor

davidlange6 commented Jul 11, 2017 via email

@fwyzard
Copy link
Contributor

fwyzard commented Jul 11, 2017

No, I propose something much simpler:

  • we leave the individual PRs open for the moment
  • I merge them one at a time in a single PR, leaving the individual commits untouched, adding only the resolution of the merge conflicts and minor fixes
  • we do not re-review the "merged PR", since all the individual changes were already reviewd
  • once the tests pass, you merge the "merged PR"
  • this should automatically set the original PRs as "merged" as well

If you agree with the plan, I will re-do the "merged PR" based on the current 9.3.X.

@fwyzard
Copy link
Contributor

fwyzard commented Jul 11, 2017

The opposite approach (one PR per feature) is what we have been doing so far.
It works well for reviewing the changes, but is a mess for merging PRs that need to modify the same files, as you have seen.

@davidlange6
Copy link
Contributor

davidlange6 commented Jul 11, 2017 via email

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19023/21364/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: 2012093
  • DQMHistoTests: Total failures: 117
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2011810
  • DQMHistoTests: Total skipped: 166
  • DQMHistoTests: Total Missing objects: 0
  • Checked 93 log files, 14 edm output root files, 23 DQM output files

fwyzard added a commit to fwyzard/cmssw that referenced this pull request Jul 28, 2017
This PR includes a backport of the following PRs:
  - cms-sw#18172
  - cms-sw#18950
  - cms-sw#18959
  - cms-sw#18968
  - cms-sw#18971
  - cms-sw#19023
  - cms-sw#19046
  - cms-sw#19078
  - cms-sw#19119
  - cms-sw#19178
  - cms-sw#19290
  - cms-sw#19293
  - cms-sw#19294
  - cms-sw#19490
  - cms-sw#19499
  - cms-sw#19577
  - cms-sw#19585
  - cms-sw#19596
  - cms-sw#19599
  - cms-sw#19627
  - cms-sw#19689
  - cms-sw#19694
  - cms-sw#19703
  - cms-sw#19781
  - cms-sw#19794

plus the older ones, contained in DQMOffline/Trigger and HLTriggerOffline.

It synchronises with CMSSW_9_3_X
  - DQMServices/ClientConfig
  - DQMOffline/Configuration
  - DQMOffline/Trigger
  - HLTriggerOffline/Btag
  - HLTriggerOffline/Higgs
  - HLTriggerOffline/SUSYBSM
  - HLTriggerOffline/Tau
  - HLTriggerOffline/Top
fwyzard added a commit to fwyzard/cmssw that referenced this pull request Sep 3, 2017
This PR includes a backport of the following PRs:
  - cms-sw#18172
  - cms-sw#18950
  - cms-sw#18959
  - cms-sw#18968
  - cms-sw#18971
  - cms-sw#19023
  - cms-sw#19046
  - cms-sw#19078
  - cms-sw#19119
  - cms-sw#19178
  - cms-sw#19290
  - cms-sw#19293
  - cms-sw#19294
  - cms-sw#19490
  - cms-sw#19499
  - cms-sw#19577
  - cms-sw#19585
  - cms-sw#19596
  - cms-sw#19599
  - cms-sw#19627
  - cms-sw#19689
  - cms-sw#19694
  - cms-sw#19703
  - cms-sw#19781
  - cms-sw#19794

plus the older ones, contained in DQMOffline/Trigger and HLTriggerOffline.

It synchronises with CMSSW_9_3_X
  - DQMServices/ClientConfig
  - DQMOffline/Configuration
  - DQMOffline/Trigger
  - HLTriggerOffline/Btag
  - HLTriggerOffline/Higgs
  - HLTriggerOffline/SUSYBSM
  - HLTriggerOffline/Tau
  - HLTriggerOffline/Top
mtosi pushed a commit to mtosi/cmssw that referenced this pull request Oct 17, 2017
This PR includes a backport of the following PRs:
  - cms-sw#18172
  - cms-sw#18950
  - cms-sw#18959
  - cms-sw#18968
  - cms-sw#18971
  - cms-sw#19023
  - cms-sw#19046
  - cms-sw#19078
  - cms-sw#19119
  - cms-sw#19178
  - cms-sw#19290
  - cms-sw#19293
  - cms-sw#19294
  - cms-sw#19490
  - cms-sw#19499
  - cms-sw#19577
  - cms-sw#19585
  - cms-sw#19596
  - cms-sw#19599
  - cms-sw#19627
  - cms-sw#19689
  - cms-sw#19694
  - cms-sw#19703
  - cms-sw#19781
  - cms-sw#19794

plus the older ones, contained in DQMOffline/Trigger and HLTriggerOffline.

It synchronises with CMSSW_9_3_X
  - DQMServices/ClientConfig
  - DQMOffline/Configuration
  - DQMOffline/Trigger
  - HLTriggerOffline/Btag
  - HLTriggerOffline/Higgs
  - HLTriggerOffline/SUSYBSM
  - HLTriggerOffline/Tau
  - HLTriggerOffline/Top
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

9 participants