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

Changes to jet pileup subtraction #29948

Merged
merged 25 commits into from Jun 14, 2020
Merged

Conversation

stepobr
Copy link
Contributor

@stepobr stepobr commented May 21, 2020

PR description:

Changes related to pileup subtraction in jets. These existed for a long while and were used by Heavy Ions group and now migrating to central repo. This also fixes a bug in pileup subtraction, that appeared after HE phase 1 upgrade.

PR validation:

Tested using runTheMatrix.py

@bi-ran @mandrenguyen

@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-29948/15586

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@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-29948/15587

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @stepobr (Stepan Obraztsov) for master.

It involves the following packages:

RecoHI/HiJetAlgos
RecoJets/JetProducers

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @jdolen, @ahinzmann, @schoef, @jazzitup, @jdamgov, @yslai, @nhanvtran, @yenjie, @gkasieczka, @clelange, @kurtejung, @mariadalfonso, @mandrenguyen, @dgulhan, @seemasharmafnal, @yetkinyilmaz this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented May 21, 2020

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 21, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/6491/console Started: 2020/05/22 00:22

@cmsbuild
Copy link
Contributor

+1
Tested at: 501a13d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49ee57/6491/summary.html
CMSSW: CMSSW_11_1_X_2020-05-21-1100
SCRAM_ARCH: slc7_amd64_gcc820

@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-29948/15995

@cmsbuild
Copy link
Contributor

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

@stepobr
Copy link
Contributor Author

stepobr commented Jun 10, 2020

after adding PFTowers to comparisons, we can see that this PR introduced some differences at the edges of acceptance, based on plots from the 2011 HI workflow 140.53
Is this migration to a larger eta expected?

@slava77 Thank you for the checks, the changes are expected because of the switch from calo geometry to a fixed grid. If I compare the etas for the actual tower centers and for the fixed grid the difference is indeed in the last bins.

@slava77
Copy link
Contributor

slava77 commented Jun 10, 2020

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 10, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/6937/console Started: 2020/06/10 18:13

@cmsbuild
Copy link
Contributor

+1
Tested at: e06ba5b
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49ee57/6937/summary.html
CMSSW: CMSSW_11_2_X_2020-06-09-2300
SCRAM_ARCH: slc7_amd64_gcc820

@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-49ee57/6937/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 14 differences found in the comparisons
  • DQMHistoTests: Total files compared: 36
  • DQMHistoTests: Total histograms compared: 2780497
  • DQMHistoTests: Total failures: 140
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2780307
  • DQMHistoTests: Total skipped: 50
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
  • Checked 152 log files, 16 edm output root files, 36 DQM output files

@slava77
Copy link
Contributor

slava77 commented Jun 12, 2020

+1

for #29948 e06ba5b

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show differences only in PFTowers (as described in ) and akPu*PFJets (after a change in the MultipleAlgoIterator::calculateOrphanInput algorithm [overloaded with new method]) in HI workflows
    e.g in wf 140.56

wf140 56_akPu3PF_deltaR

@stepobr if there is a link to a presentation or another document that describes expected changes in the MultipleAlgoIterator, it would help to link it in the PR description to improve self-documentation

@cmsbuild
Copy link
Contributor

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. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 69ec082 into cms-sw:master Jun 14, 2020
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

6 participants