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

Egamma Fall17 V2 Photon IDs #25293

Merged
merged 5 commits into from Nov 28, 2018
Merged

Egamma Fall17 V2 Photon IDs #25293

merged 5 commits into from Nov 28, 2018

Conversation

guitargeek
Copy link
Contributor

This PR aims to finally integrate the updated MVA and cut based photon IDs. They are activated in MiniAOD so reco changes are expected. The NanoAOD integration will be done separately in NanoAOD development fork this time.

Shout out to @michelif, @Sam-Harper, @lsoffi and @swagata87!

@cmsbuild cmsbuild added this to the CMSSW_10_4_X milestone Nov 19, 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-25293/7313

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @guitargeek (Jonas Rembser) for master.

It involves the following packages:

PhysicsTools/PatAlgos
RecoEgamma/PhotonIdentification

@perrotta, @monttj, @cmsbuild, @fgolf, @slava77, @peruzzim can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @jainshilpi, @rappoccio, @HeinerTholen, @varuns23, @seemasharmafnal, @mmarionncern, @imarches, @ahinzmann, @smoortga, @acaudron, @lgray, @jdolen, @drkovalskyi, @ferencek, @Sam-Harper, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @clelange, @JyothsnaKomaragiri, @mverzett, @gpetruc, @mariadalfonso, @pvmulder 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 20, 2018 •
edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31758/console Started: 2018/11/20 14:09

@cmsbuild
Copy link
Contributor

+1
Tested at: 752da9c
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25293/31758/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-25293/31758/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 332 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3013311
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3013111
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 134 log files, 14 edm output root files, 32 DQM output files

@guitargeek guitargeek mentioned this pull request Nov 21, 2018
Closed
peruzzim
peruzzim reviewed Nov 21, 2018
View reviewed changes
RecoEgamma/PhotonIdentification/python/Identification/mvaPhotonID_Fall17_94X_V2_cff.py
@@ -0,0 +1,55 @@
from RecoEgamma.PhotonIdentification.Identification.mvaPhotonID_tools import *
#
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the import of os.path and the definition of weightFileBaseDir are missing here

Copy link
Contributor

Choose a reason for hiding this comment

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

same with category_cuts later in the same file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, that's absolutely right! And I noticed I messed up with this PR (I forgot #24131 was merged in 10_3_X), so I have to adapt the config files a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...actually we are talking about the backport #25313. Nothing is missing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@peruzzim for what I can see, they are all included within mvaPhotonID_tools, and indeed the unit tests using this fragment looks to work properly

@davidlange6
Copy link
Contributor

Your PR is unmergeable. Please have a look and possibly rebase it.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25293/7359

@cmsbuild
Copy link
Contributor

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

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 26, 2018 •
edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31842/console Started: 2018/11/26 11:39

@cmsbuild
Copy link
Contributor

+1
Tested at: 8ec3c83
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25293/31842/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
be75d46
824da84
4d72114
cb9fe52
a6d0e0c
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25293/31842/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25293/31842/git-merge-result

@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-25293/31842/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 331 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

@perrotta
Copy link
Contributor

+1

  • New photon ID made available for miniAOD poduction and analyses
  • Changes in the miniAOD outputs correspond to the additional cut and MVA based IDs

@guitargeek guitargeek mentioned this pull request Nov 26, 2018
Closed
@fabiocos
Copy link
Contributor

@perrotta "Changes in the miniAOD outputs" refers to the 331 differences in the comparisons?

@perrotta
Copy link
Contributor

Yes, they do:

/build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison
JR results differ 22 all_mini_OldVSNew_RunEGamma2018Awf136p85
JR results differ 19 all_mini_OldVSNew_TTbar14TeV2023D35wf27434p0
JR results differ 22 all_mini_OldVSNew_RunSinglePh2017Bwf136p788
JR results differ 22 all_mini_OldVSNew_RunPhoton2012Bwf4p53
JR results differ 12 all_mini_OldVSNew_ZMM13TeVwf1330p0
JR results differ 15 all_mini_OldVSNew_TTbar14TeV2023D21wf21234p0
JR results differ 14 all_mini_OldVSNew_TTbar13TeVPUppmx2018wf250202p181
JR results differ 17 all_mini_OldVSNew_TTbar13TeV2017wf10024p0
JR results differ 20 all_mini_OldVSNew_TTbar13TeV2018wf10824p0
JR results differ 18 all_mini_OldVSNew_TTbarPUwf25202p0
JR results differ 12 all_mini_OldVSNew_TTbarwf25p0
JR results differ 19 all_OldVSNew_RunJetHT2016EreMINIAODwf136p7611
JR results differ 13 all_mini_OldVSNew_TTbar13TeVPU2017wf10224p0
JR results differ 22 all_mini_OldVSNew_RunSinglePh2016Bwf136p731
JR results differ 18 all_OldVSNew_RunJetHT2017FreMINIAODwf136p8311
JR results differ 19 all_mini_OldVSNew_TTbar13TeV2019wf11624p0
JR results differ 12 all_mini_OldVSNew_ZMM13TeV2017wf10042p0
JR results differ 17 all_mini_OldVSNew_Higgs200ChargedTauswf9p0
JR results differ 18 all_mini_OldVSNew_TTbar14TeV2023D17wf20034p0
SUMMARY Reco comparison results: 331 differences found in the comparisons

@fabiocos
Copy link
Contributor

@peruzzim @fgolf I will move forward with the integration of this development, I understand that the propagation to NanoAOD will happen in a second step

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 18b524f into cms-sw:master Nov 28, 2018
@perrotta perrotta mentioned this pull request Dec 3, 2018
Merged
@guitargeek guitargeek mentioned this pull request Dec 4, 2018
Merged
@guitargeek guitargeek deleted the PhotonID_10_4_X branch December 12, 2018 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabiocos fabiocos fabiocos left review comments

@peruzzim peruzzim peruzzim left review comments

Assignees
No one assigned
Labels
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
7 participants
@guitargeek @cmsbuild @perrotta @davidlange6 @fabiocos @peruzzim @swagata87