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

Jump to bottom

Include option for 2018 Jetid working points #27184

Merged

cmsbuild merged 8 commits into cms-sw:master from knash:JetID2018

Aug 5, 2019

Contributor

knash commented Jun 12, 2019

PR description:

Update to include 2018 jetid version
https://twiki.cern.ch/twiki/bin/view/CMS/JetID13TeVRun2018

This just updates
PhysicsTools/SelectorUtils/interface/PFJetIDSelectionFunctor.h
and not
PhysicsTools/NanoAOD/python/jets_cff.py

PR validation:

Tested that the triggers for tight and tightlepveto seem to fire correctly and that the efficiency makes sense

knash added 2 commits

June 12, 2019 13:03


          add 2018 jetid

862a9c6

fix

c2c0af6

cmsbuild added this to the CMSSW_11_0_X milestone

cmsbuild added analysis-pending code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels

Contributor

cmsbuild commented Jun 12, 2019

The code-checks are being triggered in jenkins.

Contributor

cmsbuild commented Jun 12, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27184/10341

This PR adds an extra 16KB to repository

cmsbuild added code-checks-approved and removed code-checks-pending labels

Contributor

cmsbuild commented Jun 12, 2019

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

Contributor

perrotta commented Jun 12, 2019

please test

Contributor

cmsbuild commented Jun 12, 2019 •

edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/914/console Started: 2019/06/12 16:40

cmsbuild added tests-started and removed tests-pending labels

Contributor

cmsbuild commented Jun 12, 2019

-1

Tested at: c2c0af6

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10356f/914/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test runtestUtilAlgos had ERRORS
---> test runtestPhysicsToolsNanoAOD had ERRORS

Contributor

cmsbuild commented Jun 12, 2019

Comparison job queued.

cmsbuild added tests-rejected and removed tests-started labels

Contributor

cmsbuild commented Jun 12, 2019

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 1 differences found in the comparisons
DQMHistoTests: Total files compared: 33
DQMHistoTests: Total histograms compared: 3237932
DQMHistoTests: Total failures: 703
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3236895
DQMHistoTests: Total skipped: 334
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
Checked 137 log files, 14 edm output root files, 33 DQM output files

cmsbuild added comparison-available and removed comparison-pending labels

perrotta reviewed

View reviewed changes

Contributor

perrotta left a comment

Also a rebase is needed, see conflicting files

PhysicsTools/SelectorUtils/interface/PFJetIDSelectionFunctor.h Outdated

                     }
                   } // end if basic jet
+                  float etaB=2.4;
+                  // Cuts for |eta| < 2.6 for Summer18
+                  if (version_ != SUMMER18) etaB=2.6;

Contributor

perrotta Jun 17, 2019

Why not to define here etaB for all versions (with some else if statement, for example).
Then you won't have to redefine by hand inside (almost) every if branch

PhysicsTools/SelectorUtils/interface/PFJetIDSelectionFunctor.h Outdated

+                  float etaB=2.4;
+                  // Cuts for |eta| < 2.6 for Summer18
+                  if (version_ != SUMMER18) etaB=2.6;
+                  if((version_ != WINTER17 && version_ != WINTER17PUPPI && version_ != SUMMER18) ||  quality_ != TIGHT ) {if ( ignoreCut(indexCEF_)           || ( cef < cut(indexCEF_, double()) || std::abs(jet.eta()) > etaB ) ) passCut( ret, indexCEF_);}

Contributor

perrotta Jun 17, 2019

Splitting the line will improve readability (even better if you run clang format since now: it will be enforced anyhow)

Contributor

perrotta Jun 17, 2019

(The same apply to most of the lines below...)

PhysicsTools/SelectorUtils/interface/PFJetIDSelectionFunctor.h Outdated

+                    if ( ignoreCut(indexNEF_TR_)        || ( nef < cut(indexNEF_TR_, double())  || std::abs(jet.eta()) <= 2.6 || std::abs(jet.eta()) > 2.7)  ) passCut( ret, indexNEF_TR_);
+                    if ( ignoreCut(indexNCH_TR_)        || ( nch > cut(indexNCH_TR_, double())  || std::abs(jet.eta()) <= 2.6 || std::abs(jet.eta()) > 2.7)  ) passCut( ret, indexNCH_TR_);
+                    if ( quality_ == TIGHTLEPVETO )	{
+                    					if ( ignoreCut(indexMUF_TR_)        || ( muf < cut(indexMUF_TR_, double())  || std::abs(jet.eta()) <= 2.6 || std::abs(jet.eta()) > 2.7)  ) passCut( ret, indexMUF_TR_);

Contributor

perrotta Jun 17, 2019

Some better indentation here is mandatory.
Even better if you have already run clang-format

PhysicsTools/SelectorUtils/interface/PFJetIDSelectionFunctor.h Outdated

+              					push_back("CEF_TR");
+              					}
+                  }
+                  if( (version_ == WINTER17 || version_ == WINTER17PUPPI || version_ == SUMMER18) && quality_ == LOOSE ){
                     edm::LogWarning("BadJetIDVersion") << "Winter17 JetID version does not support the LOOSE operating point -- defaulting to TIGHT";

Contributor

perrotta Jun 17, 2019

The LogWarning message must take also "Summer18" into account now

PhysicsTools/SelectorUtils/interface/PFJetIDSelectionFunctor.h

		@@ -443,7 +503,7 @@ class PFJetIDSelectionFunctor : public Selector<pat::Jet> {
		} else if ( quality_ == TIGHT ) {
		set("CHF", 0.0);

Contributor

perrotta Jun 17, 2019

Indentation... or clang-format

Contributor

perrotta commented Jun 17, 2019

Given the changes observed in DQM in the jenkins tests outputs, and not in the reco output themselves, I have the impression that such PFJetIDSelectionFunctor is picked in the JETMET DQM modules, but not in the reco miniAOD ones.
Could you please shortly remind us how this PFJetIDSelectionFunctor enters in the game, and confirm that what reported in the jenkins tests outputs does make any sense?


          cosmetic fixes and fix to eta region typo

de306d2

cmsbuild removed the code-checks-approved label

Contributor

cmsbuild commented Aug 2, 2019

Comparison job queued.

cmsbuild added tests-approved and removed tests-started labels

Contributor

cmsbuild commented Aug 2, 2019

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 2492470
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2492151
DQMHistoTests: Total skipped: 317
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 132 log files, 14 edm output root files, 32 DQM output files

cmsbuild added comparison-available and removed comparison-pending labels

Contributor

perrotta commented Aug 3, 2019

+1

Only change since last reco signature in Include option for 2018 Jetid working points #27184 (comment) is that now the new working points are not propagated to nanoAOD any more.
Jenkins tests pass and show no differences wrt baseline; not even in nanoAOD, as expected

cmsbuild added reconstruction-approved and removed reconstruction-pending labels

Contributor

fabiocos commented Aug 5, 2019

@santocch you already signed the previous version of the PR before removing nanAOD, so I assume this is ok for you. Please comment otherwise

Contributor

fabiocos commented Aug 5, 2019

+1

Contributor

fabiocos commented Aug 5, 2019

merge

cmsbuild added orp-approved and removed orp-pending labels

cmsbuild merged commit de56c9c into cms-sw:master

Contributor

clelange commented Aug 5, 2019

@peruzzim @fgolf I guess this should be back-ported to at least 10_6_X, can you confirm? Also, any other branch?

cmsbuild mentioned this pull request

DD4Hep: Tracker overlaps TEC #27689

Merged

santocch commented Aug 6, 2019

+1

cmsbuild added analysis-approved fully-signed and removed analysis-pending pending-signatures labels

Contributor

cmsbuild commented Aug 6, 2019

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 be automatically merged.

This was referenced Sep 30, 2019

Backport jetid updates #27184 #27691 to 106X #28094

Merged

Backport jetid updates #27184 #27691 to 102X #28095

Merged

This was referenced Oct 9, 2019

NanoAODv6 release preparation #28119

Closed

Update jet ID to latest recommendations cms-nanoAOD/cmssw#424

Merged

NanoAODv6 updates, part 2 #28122

Merged

cmsbuild added a commit that referenced this pull request


          Merge pull request #28095 from knash/jetidbackport102X

1edd106

Backport jetid updates #27184 #27691 to 102X

cmsbuild added a commit that referenced this pull request


          Merge pull request #28094 from knash/jetidbackport106X

22c03d6

Backport jetid updates #27184 #27691 to 106X

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment