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

fixed ptMin threshold for PU subtractor #20482

Merged
merged 3 commits into from Sep 18, 2017

Conversation

kurtejung
Copy link
Contributor

When the PU subtractor code was upgraded to factorize the descriptions, ptMin was omitted - readding that ptMin value so HI Jet Reco doesn't ignore the value

@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-20482/656

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/PR-20482/656/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/PR-20482/656/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly (this will soon be required for PRs to be considered)

@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-20482/657

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @kurtejung (Kurt Jung) 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, @yslai, @nhanvtran, @gkasieczka, @schoef, @mariadalfonso, @seemasharmafnal this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@kurtejung
Copy link
Contributor Author

also want to ping @mandrenguyen and @mverwe about this

@@ -378,6 +379,7 @@ void PileUpSubtractor::fillDescriptions(edm::ConfigurationDescriptions& descript
desc.add<double> ("Ghost_EtaMax", 5);
desc.add<double> ("GhostArea", 0.01);
desc.add<int> ("Active_Area_Repeats", 1);
desc.add<double> ("jetPtMin", 10.);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this function (full PileUpSubtractor::fillDescriptions) actually doing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surprisingly not - I left it since it was already there but I can remove it. The RelVal and DQM run just fine without it.

@slava77
Copy link
Contributor

slava77 commented Sep 13, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 13, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22930/console Started: 2017/09/13 08:31

@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-20482/22930/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: 2645092
  • DQMHistoTests: Total failures: 207
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2644696
  • DQMHistoTests: Total skipped: 189
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 14 edm output root files, 26 DQM output files

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 14, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22988/console Started: 2017/09/15 04:34

@perrotta
Copy link
Contributor

Why did you remove the fillDescriptions() method?
Please put it back, if there is no reason to remove it

@kurtejung
Copy link
Contributor Author

kurtejung commented Sep 15, 2017

The fillDescriptions method doesn't seem to do much here. Please correct me if i'm wrong, but I think the parameter consistency check is not performed here since it did not complain previously about passing the code a ptMin parameter. See @slava77 's comment from a couple days ago.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@perrotta
Copy link
Contributor

@kurtejung what do you mean by "I think the parameter consistency check is not performed here since it did not complain previously about passing the code a ptMin parameter": please provide an example?

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20482/22988/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: 2646869
  • DQMHistoTests: Total failures: 227
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2646453
  • DQMHistoTests: Total skipped: 189
  • DQMHistoTests: Total Missing objects: 0
  • Checked 107 log files, 14 edm output root files, 26 DQM output files

@slava77
Copy link
Contributor

slava77 commented Sep 15, 2017 via email

@slava77
Copy link
Contributor

slava77 commented Sep 16, 2017

type bug-fix

@slava77
Copy link
Contributor

slava77 commented Sep 16, 2017

+1

for #20482 5934f2e

  • properly initialized jetPtMin; drops a dead-end implementation of PileUpSubtractor::fillDescriptions
  • jenkins tests pass and comparisons with baseline show changes in HI jet plots related to the change wf140 53_jetpt
  • if I'm not mistaken this PR also fixes occasional cases when the uninitialized value of jetPtMin leads to complete disappearance of the jets which was seen as randomly appearing differences in HI jet plots.

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

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit dbc259d into cms-sw:master Sep 18, 2017
mandrenguyen added a commit to mandrenguyen/cmssw that referenced this pull request Sep 27, 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.

None yet

5 participants