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

Properly veto HGCal superclusters from entering regular PFBlocks - phase2 scenarios #27785

Merged
merged 1 commit into from
Sep 4, 2019

Conversation

hatakeyamak
Copy link
Contributor

PR description:

Fix vetoes of superclusters from HGCAL going into PFBlocks. As we set up the PF workflow now, we are keeping the endcap HGCAL part separate from the barrel and HF sections, and we don't want superclusters going into regular PFBlocks, but this vetoes was not implemented properly, causing many LogError messages.

This should also address issues reported here:
#22582

PR validation:

Compiles. Also made sure the superclusters from HGCAL are properly removed from PFBlocks in a phase2 matrix test 20434.0 (ttbar D41), while retaining superclusters from barrel.

if this PR is a backport please specify the original PR:

Not backport.

@bendavid @guitargeek @jpata @Sam-Harper (cannot find github name of newer egamma leaders...)

@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-27785/11473

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @hatakeyamak (Kenichi Hatakeyama) for master.

It involves the following packages:

RecoParticleFlow/PFProducer

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @lgray, @seemasharmafnal, @bachtis, @cbernet 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 Aug 16, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2049/console Started: 2019/08/16 10:05

@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-b90fac/2049/summary.html

Comparison Summary:

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

@hatakeyamak
Copy link
Contributor Author

I think the change is made as intended, but I don't quite understand some changes in btag and photons for phase2. Thinking... Any inputs will be appreciated.

@perrotta
Copy link
Contributor

@hatakeyamak , was there any progress in it?

Looking at the differences in the Phase2 workflows, there are less elements in the PFBlocks , which is exactly what this PR is intended to. Then this propagates into differences in other quantities like secondary vertices, impact parameters, which end up in the differences for photons and btag that you are mentioning. Is it really unexpected, due to the removal of those PFblocks from HGCal?

@kpedro88
Copy link
Contributor

assign upgrade

@cmsbuild
Copy link
Contributor

New categories assigned: upgrade

@kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks

@kpedro88
Copy link
Contributor

+upgrade
this is a real bug fix that causes minor changes in PF-related quantities in Phase2 workflows

@perrotta
Copy link
Contributor

perrotta commented Aug 28, 2019

@hatakeyamak , was there any progress in it?
Looking at the differences in the Phase2 workflows, there are less elements in the PFBlocks , which is exactly what this PR is intended to. Then this propagates into differences in other quantities like secondary vertices, impact parameters, which end up in the differences for photons and btag that you are mentioning. Is it really unexpected, due to the removal of those PFblocks from HGCal?

Sorry for slow progress. Something was keeping me occupied for ~1 week, but I am almost done.

I was hoping to use this opportunity to understand how changes like this propagate to downstream. Just for my education, how/where do you see:

  • there are less elements in the PFBlocks
  • differences in secondary vertices, impact parameters
    Are you looking at specific plots from automatic DQM/validation plots from matrix/jenkins tests?

Yes, I simply looked at the differences in the "comparison failed" section of the automatic tests results. The "reco comparison differences" looks at the differences between reco quantities in the root outputs.
I must say that even to me all those differences look rather reasonable, given the pruning of the PF blocks. But before signing for reco I would like to understand from you whether you think that all those changes make sense, since you expressed doubts on it. I agree that this is a real bug fix, and it must get fixed: however, while this PR is open I would also like to be sure that it does not point to some other bug somewhere else, which could get forgotten would this PR get merged without inspecting.

@perrotta
Copy link
Contributor

perrotta commented Sep 2, 2019

@hatakeyamak please either confirm that the changes observed in output look reasonable also to you, or just let us know when you think you can be done with your checks (and we'll put on hold this PR till then)

@hatakeyamak
Copy link
Contributor Author

hatakeyamak commented Sep 3, 2019

Thanks for your ping @perrotta .

I did some investigation on this for the last few days, and I think I am ready to sign this off. I still cannot fully connect this change to the changes in secondary vertices, but some changes in photons via changes in particleFlowEGamma resulting from intended changes in PFBlocks are clear.

This PR is certainly doing the right thing for PFBlocks, so I think we can sign this off. Sorry that this took long.

@slava77
Copy link
Contributor

slava77 commented Sep 3, 2019

@cmsbuild please test

to still have the test results available before merging

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 3, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2333/console Started: 2019/09/03 18:15

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 3, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 3, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 3, 2019

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 327 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2955700
  • DQMHistoTests: Total failures: 208
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2955151
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 145 log files, 15 edm output root files, 34 DQM output files

@perrotta
Copy link
Contributor

perrotta commented Sep 4, 2019

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 4, 2019

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

fabiocos commented Sep 4, 2019

+1

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