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

Puppi pileupjetID #24287

Merged
merged 8 commits into from Aug 21, 2018
Merged

Puppi pileupjetID #24287

merged 8 commits into from Aug 21, 2018

Conversation

knash
Copy link
Contributor

@knash knash commented Aug 14, 2018

Update to pileupjetID algorithm to include an option to calculate the discriminator variables using Puppi weights.

There are a few effects, but one visible change is the fractional pt variables. previously these were calculated using the Puppi jet pt in the denominator and and the unweighted constituent sum in the numerator which leads to an undesirable >1 tail in the total pt fraction. See below

OFF

image

ON

image

There was no additional implementation beyond the feature addition (ie no standard workflows should change).

I think the changes in RecoJets/JetProducers/src/PileupJetIdAlgo.cc, Line 509-510 are appropriate but would like a second opinion @ahinzmann .

This was tested only using a modified version of the script:
RecoJets/JetProducers/test/testJetTools_onMiniAOD_cfg.py
There were a few small changes required for this script to run in 10_3_X which I am also pushing.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

RecoJets/JetProducers

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @jdolen, @ahinzmann, @rappoccio, @jdamgov, @yslai, @nhanvtran, @gkasieczka, @clelange, @schoef, @mariadalfonso, @seemasharmafnal 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

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 14, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/29825/console Started: 2018/08/14 22:36

@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-24287/29825/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 2891469
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2891277
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
  • Checked 129 log files, 14 edm output root files, 31 DQM output files

@cmsbuild
Copy link
Contributor

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

}
if((abs(icand->pdgId()) == 1) || (abs(icand->pdgId()) == 2)) multNeut += candPuppiWeight;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this line? PdgId 1,2 seem not to be used in PF? https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideParticleFlow#Particle_types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are HF had/gamma. The multNeut should reproduce patjet->neutralMultiplicity() exactly in the case that usePuppi is False.

So we have events like this (setting usePuppi to False)

Adding 130
Adding 130
Adding 22
Adding 22
Adding 22
Adding 22
Adding 22
Adding 22
multNeut = 8 ; patjet->neutralMultiplicity() = 8

and events like this

Adding 1
Adding 1
Adding 1
Adding 1
Adding 1
multNeut = 5 ; patjet->neutralMultiplicity() = 5

Copy link
Contributor

Choose a reason for hiding this comment

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

ok makes sense!

@perrotta
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@slava77
Copy link
Contributor

slava77 commented Aug 16, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 16, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/29880/console Started: 2018/08/16 15:42

@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-24287/29880/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 2983602
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2983404
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
  • Checked 129 log files, 14 edm output root files, 31 DQM output files

@slava77
Copy link
Contributor

slava77 commented Aug 17, 2018

+1

for #24287 e5fdda6

  • code changes are in line with the PR description; current defaults are unchanged
  • jenkins tests pass and comparisons with the baseline show no differences

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

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit fbd573d into cms-sw:master Aug 21, 2018
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