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

Jetid muon fraction fix #27691

Merged
merged 4 commits into from
Aug 22, 2019
Merged

Jetid muon fraction fix #27691

merged 4 commits into from
Aug 22, 2019

Conversation

knash
Copy link
Contributor

@knash knash commented Aug 5, 2019

PR description:

Upon investigation into the issue reported here:
https://hypernews.cern.ch/HyperNews/CMS/get/physTools/3669.html

It seems that the muon fraction was not properly implemented for all jet types. This PR fixes the issue.
This affects the "lepveto" jetid value stored in NanoAOD.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2019

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27691/11293

  • This PR adds an extra 16KB to repository

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

@perrotta
Copy link
Contributor

perrotta commented Aug 8, 2019

@knash , you need to apply the code-checks in order to have this PR tested, and the review can start.
Please do so, or just let us know which are the plans for this PR

@perrotta
Copy link
Contributor

perrotta commented Aug 8, 2019

By the way, is there any intention to backport to any other release?

@cmsbuild cmsbuild mentioned this pull request Aug 12, 2019
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@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-27691/11425

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

PhysicsTools/SelectorUtils

@perrotta, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@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 13, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/1999/console Started: 2019/08/13 14:08

@perrotta
Copy link
Contributor

+1

  • The implementation of the fix looks quite logical, and adds the missing muon fractions to the jet id's whenever missing
  • Jenkins tests pass and reco output is unafffected
  • The only visible difference is in the nanoAOD JetID outputs, which is expected: xpog will evaluate about it

@perrotta
Copy link
Contributor

By the way (@fabiocos @smuzaffar): no external is really needed here. It was just added for the tests in order to allow the PR build on top of the latest IB

@fabiocos
Copy link
Contributor

@perrotta yes, it was a temporary workaround waiting for next IB, thank you anyway for reminding

@perrotta
Copy link
Contributor

@fgolf,@peruzzim please have a look and sign, if you think so.
(I asked for your signature because the fix applies on nanoAOD output and I wanted to be sure that you were aware of it; on the other hand this is a real fix, and I suppose you can approve it rather easily)

@cmsbuild cmsbuild mentioned this pull request Aug 19, 2019
@peruzzim
Copy link
Contributor

+xpog

As said, this is a real bugfix. Do we have plans for backport?

@santocch
Copy link

+1

@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)

@kpedro88
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit bd91394 into cms-sw:master Aug 22, 2019
@ahinzmann
Copy link
Contributor

@peruzzim @knash Let's get this fix into the next Run2 MiniAOD starting soon?

@knash
Copy link
Contributor Author

knash commented Sep 27, 2019

@ahinzmann Right! let me start the backport now -- 106, 102?

@srimanob
Copy link
Contributor

@ahinzmann
What will be your target? 2016, and reminiaod of 2017 and 2018?

@fabiocos

@ahinzmann
Copy link
Contributor

Well every NanoAOD production is affected, though the number of analyses is likely very small, and there is a way to work around it matching e.g. ak4 and ak8 jets. So this is a small improvement. Up to you what campaign shall take it into account.

@peruzzim
Copy link
Contributor

Do I understand correctly that this only affects variables stored in NanoAOD? If so, I think we should prepare the backports and hold them until we update the rest of NanoAOD code for the next round of production. It is not far away in time, in this way we can update both things at the same time and avoid inconsistencies until then.

cmsbuild added a commit that referenced this pull request Oct 21, 2019
cmsbuild added a commit that referenced this pull request Oct 23, 2019
@peruzzim peruzzim mentioned this pull request Oct 23, 2019
5 tasks
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.

10 participants