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

Jet ID update to the Run2017 prescription #22323

Merged
merged 4 commits into from Mar 9, 2018

Conversation

knash
Copy link
Contributor

@knash knash commented Feb 23, 2018

Update of the jetID criterion to the Run2017 prescription (TIGHT operating point).
https://twiki.cern.ch/twiki/bin/view/CMS/JetID13TeVRun2017

The LOOSE operating point is no longer supported, so this PR would give a warning and switch to TIGHT.

THIS PR only updates the PFJetIDSelectionFunctor.h and pfJetIDSelector_cfi.py defaults to define and select the "WINTER17" version. In theory there are other files that reference these operating points
(https://github.com/cms-sw/cmssw/search?utf8=%E2%9C%93&q=WINTER16&type=) that this PR could also update, but I am not sure if this is appropriate.

The new prescription does not require the definition of a Charged EM Fraction (CEF) cut for the operating point here, so this has been taken out of the jetid definition. The other options would be to keep it and set the CEF cut to at >=1 or set ignoreCut(indexCEF_).

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

PhysicsTools/SelectorUtils

@perrotta, @cmsbuild, @monttj, @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 Feb 24, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26292/console Started: 2018/02/24 11:15

@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-22323/26292/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-22323/136.8311_RunJetHT2017F_reminiaod+RunJetHT2017F_reminiaod+REMINIAOD_data2017+HARVEST2017_REMINIAOD_data2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 29
  • DQMHistoTests: Total histograms compared: 2500248
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2500071
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.12000000002 KiB( 23 files compared)
  • Checked 118 log files, 9 edm output root files, 29 DQM output files

@perrotta
Copy link
Contributor

This code looks like being pure analysis code
https://cmssdt.cern.ch/lxr/search?%21v=CMSSW_10_1_X_2018-02-27-2300&_filestring=&_string=pfJetIDSelector

It does not affect reco production, and therefore I refrain from proposing code optimization, even though the two almost fully identically copy/pasted constructor could be better factorized, for the sake of avoiding possible future mistakes.

Was the change agreed with or notified to the analyses affected, or at least endorsed by JetMET POG?
(Adding @rappoccio @ahinzmann @jdolen , the Jet POG contacts, for confirmation)
Do you have any presentation that showed its effect?

@ahinzmann
Copy link
Contributor

Hi! I would suggest to split into TIGHT and TIGHT_LEP_VETO as for the recommendation, including and excluding and including the electron/muon fraction cuts. It his way analyzers have the same options as we recommend on the TWiki.

@knash
Copy link
Contributor Author

knash commented Feb 28, 2018

Hey Andreas, right this was what I was planning on implementing originally but the previous iteration had not included the lepveto operating point so I was not sure if there was any demand.

However I see no reason not to include it as a new feature -- let me add the option and test .

@perrotta
Copy link
Contributor

perrotta commented Mar 1, 2018

This PR now has conflicts with PhysicsTools/SelectorUtils/interface/PFJetIDSelectionFunctor.h
Please fix them

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 8, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26705/console Started: 2018/03/08 20:28

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 8, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 8, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 8, 2018

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 29
  • DQMHistoTests: Total histograms compared: 2484711
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2484534
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.929999999964 KiB( 22 files compared)
  • Checked 118 log files, 9 edm output root files, 29 DQM output files

@perrotta
Copy link
Contributor

perrotta commented Mar 9, 2018

+1

  • This is analysis code
  • No effect expected on reco/miniAOD outputs, none observed

@perrotta
Copy link
Contributor

perrotta commented Mar 9, 2018

@knash : do you plan a 94X backport for it?

@fabiocos
Copy link
Contributor

fabiocos commented Mar 9, 2018

@knash I assume this is first of all needed for the 2017 analysis, so I consider natural a backport to 94X, where such a work is supposed to happen.

@fabiocos
Copy link
Contributor

fabiocos commented Mar 9, 2018

+1

@fabiocos
Copy link
Contributor

fabiocos commented Mar 9, 2018

merge

@cmsbuild cmsbuild merged commit 3ae82f0 into cms-sw:master Mar 9, 2018
@knash
Copy link
Contributor Author

knash commented Mar 9, 2018

Hello, yes a 94X backport would make sense here

@fabiocos
Copy link
Contributor

@knash are you going to provide it?

@knash
Copy link
Contributor Author

knash commented Mar 12, 2018

sure, so this would just be merging with the 9_4_X branch instead of master?

@perrotta
Copy link
Contributor

perrotta commented Mar 12, 2018 via email

@arizzi
Copy link
Contributor

arizzi commented Mar 18, 2018

it seems the backport to 94X wasn't made yet. Can you create it and link it here ?

@knash
Copy link
Contributor Author

knash commented Mar 19, 2018

Hello, I have created the backport here: #22671

Thanks,
Kevin

ktht added a commit to HEP-KBFI/cmssw that referenced this pull request Mar 20, 2018
Jet ID update to the Run2017 prescription
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

7 participants