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

adding good e/gamma flag to PackedCandidate for MiniAOD #20903

Merged
merged 1 commit into from
Oct 14, 2017

Conversation

Sam-Harper
Copy link
Contributor

Dear All,

This sets a flag in PackedCandidate to indicate whether candidate is a good e/gamma object or not. Here good means "gets E/gamma energy regresssions". All electrons are automatically good, the problem is for photons, theres no way at the miniAOD level to know if you should apply the regression to it or not short of trying to do a tight p4 match with the original photon which is error prone.

This small change will greatly help E/gamma and JetMET understand the effects of the regression on jets + met as well as making it a lot easier to apply new energy corrections. Given the ongoing issues with E/gamma energy regression corrections, its extremely useful to have a clean way to re apply them at the miniAOD level.

As an aside, had this feature already been here, the regression PR would have already gone though as my bug was in trying to apply the regression on miniAOD packed candidates.

I've already cleared this with XPOG.

I have one more minor check to do in the morning

@slava77
Copy link
Contributor

slava77 commented Oct 12, 2017

code-checks
[does it really take 3 hours to do it?]

@Sam-Harper
Copy link
Contributor Author

8 hours now, looks like its stuck

@perrotta
Copy link
Contributor

code-checks

@davidlange6
Copy link
Contributor

davidlange6 commented Oct 12, 2017 via email

@davidlange6
Copy link
Contributor

@smuzaffar - could you have a look at this PR?

@smuzaffar
Copy link
Contributor

just a bad luck with this PR. cmsbot updated the message that code-checks are running but then jenkins job failed https://cmssdt.cern.ch/jenkins/job/cms-bot/373333/console (which means code-checks projects was not triggered) and after that bot assumes that checks are already running. I will update a timeout of 60mins after that cmsbot should re-trigger the checks.

@cms-sw cms-sw deleted a comment from cmsbuild Oct 12, 2017
@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-20903/1415

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Sam-Harper for master.

It involves the following packages:

DataFormats/PatCandidates
PhysicsTools/PatAlgos

@perrotta, @cmsbuild, @monttj, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @jdolen, @imarches, @ahinzmann, @acaudron, @mmarionncern, @rappoccio, @rovere, @jdamgov, @JyothsnaKomaragiri, @nhanvtran, @gpetruc, @gkasieczka, @schoef, @ferencek, @mverzett, @mariadalfonso, @pvmulder, @seemasharmafnal, @cbernet this is something you requested to watch as well.
@davidlange6, @slava77 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 Oct 12, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23712/console Started: 2017/10/12 10:53

@Sam-Harper Sam-Harper changed the title adding good e/gamma flag to PackedCandidate adding good e/gamma flag to PackedCandidate for MiniAOD Oct 12, 2017
@cmsbuild
Copy link
Contributor

-1

Tested at: 1861b3b

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
70922db
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20903/23712/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20903/23712/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20903/23712/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
10824.0 step5

runTheMatrix-results/10824.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+ALCAFull_2018+HARVESTFull_2018/step5_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2018_GenSimFull+DigiFull_2018+RecoFull_2018+ALCAFull_2018+HARVESTFull_2018.log

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
70922db
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20903/23712/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20903/23712/git-merge-result

@cmsbuild
Copy link
Contributor

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@cmsbuild
Copy link
Contributor

-1

Tested at: 1861b3b

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
70922db
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20903/23719/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20903/23719/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20903/23719/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

  • RelVals:

When I ran the RelVals I found an error in the following worklfows:
10024.0 step5

runTheMatrix-results/10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/step5_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017.log

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
70922db
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20903/23719/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20903/23719/git-merge-result

@cmsbuild
Copy link
Contributor

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

@perrotta
Copy link
Contributor

Annoying...

@davidlange6 @smuzaffar : is anything we can do to get rid of those spurios step5 errors which are flooding the tests in these days? Thanks

@smuzaffar
Copy link
Contributor

please test
for now I have updated the PR tests to run cpu-2 parallel jobs. We will update this to make use of new workflow scheduler to run based on the actual resource utilization.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 12, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23735/console Started: 2017/10/12 18:02

@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-20903/23735/summary.html

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: 2767051
  • DQMHistoTests: Total failures: 104
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2766776
  • DQMHistoTests: Total skipped: 171
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 10 edm output root files, 26 DQM output files

@Sam-Harper
Copy link
Contributor Author

So I'm fully happy with this change. My unit test implies that I'm not clobbering anything else and that I can read/write it back correctly. And then I've now done a test on 5K DoublePhoton gun events to ensure that the bit is properly been set in the PackedCandidate producer.

I would really like this in if possible, it will really help out our understanding of the regression corrections going into particle flow. Thanks.

@slava77
Copy link
Contributor

slava77 commented Oct 13, 2017

+1

for #20903 1861b3b

  • one bit filled in PackedCandidates qualityFlags_ to denote quality of the original egamma as a PFCandidate
  • jenkins tests pass and comparisons show no differences

@davidlange6
Copy link
Contributor

merge

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

6 participants