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

Clean up tau related nanoAOD content #33513

Merged

Conversation

swozniewski
Copy link
Contributor

PR description:

Removal of deprecated/redundant tau-related nanoAOD content.
Old decay mode ID is renamed in order to make it clearer that it is not the standard use case anymore.
Deprecated IDs are also removed from the selection which causes differences in the tau related distributions. A dR03 ID is added to the selection which adds some new taus, but altogether the number decreases.

Backport to 10_6_X for nanoAOD v9 intended.

PR validation:

Inspected some distributions by hand which change as stated above.
Code/format checks passed, unit tests passed, matrix tests passed

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33513/22258

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

PhysicsTools/NanoAOD

@cmsbuild, @mariadalfonso, @gouskos, @fgolf can you please review it and eventually sign? Thanks.
@gpetruc, @swertz this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@swozniewski swozniewski changed the title Cmssw 11 3 x tau pog nano ao dclean up Clean up tau related nanoAOD content Apr 23, 2021
@mariadalfonso
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b82149/14547/summary.html
COMMIT: 14218f2
CMSSW: CMSSW_12_0_X_2021-04-23-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33513/14547/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test runtestPhysicsToolsNanoAOD had ERRORS

RelVals-INPUT

  • 136.7722136.7722_RunJetHT2016H_nano+RunJetHT2016H_nano+NANOEDM2016_80X+HARVESTNANOAOD2016_80X/step2_RunJetHT2016H_nano+RunJetHT2016H_nano+NANOEDM2016_80X+HARVESTNANOAOD2016_80X.log
  • 136.7952136.7952_RunJetHT2017C_94Xv2NanoAODINPUT+RunJetHT2017C_94Xv2NanoAODINPUT+NANOEDM2017_94XMiniAODv2+HARVESTNANOAOD2017_94XMiniAODv2/step2_RunJetHT2017C_94Xv2NanoAODINPUT+RunJetHT2017C_94Xv2NanoAODINPUT+NANOEDM2017_94XMiniAODv2+HARVESTNANOAOD2017_94XMiniAODv2.log
  • 136.8521136.8521_RunJetHT2018A_nano+RunJetHT2018A_nano+NANOEDM2018_102Xv1+HARVESTNANOAOD2018_102Xv1/step2_RunJetHT2018A_nano+RunJetHT2018A_nano+NANOEDM2018_102Xv1+HARVESTNANOAOD2018_102Xv1.log
Expand to see more relval errors ...

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 107 differences found in the comparisons
  • DQMHistoTests: Total files compared: 38
  • DQMHistoTests: Total histograms compared: 2877046
  • DQMHistoTests: Total failures: 12
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2877011
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 37 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 160 log files, 37 edm output root files, 38 DQM output files
  • TriggerResults: no differences found

@mariadalfonso
Copy link
Contributor

@swozniewski
please have a look

    raise ValueError(message)
ValueError: Duplicate insert of member rawAntiEle

@swozniewski
Copy link
Contributor Author

I somehow missed this in the log of my local unit test. Looks like some era handling is still incorrect. I will check.

@swozniewski
Copy link
Contributor Author

@mariadalfonso I'm facing some strange behaviour of the modifiers here. I verified that the failing part of the test is run with the 94XMiniAODv1 modifier (only) and it still ends up in this line https://github.com/cms-sw/cmssw/pull/33513/files#diff-d54676262d2e5326ee3455e57747fd476b12be16993a8eb0f4794e8771f6526fR161 triggering the error event though only other modifiers are used in the loop. For some reason, putting an extra check of the validity of the modifiers by hand (which in my understanding should be done by the modifiers themselves before modifying) cures the test. I put my diff below. Maybe you can verify my observation or see what the problem is.

diff --git a/PhysicsTools/NanoAOD/python/taus_cff.py b/PhysicsTools/NanoAOD/python/taus_cff.py
index ce5ebf105f6..47f1101f479 100644
--- a/PhysicsTools/NanoAOD/python/taus_cff.py
+++ b/PhysicsTools/NanoAOD/python/taus_cff.py
@@ -146,6 +146,13 @@ _variables80X =  cms.PSet(
     _mvaIsoVars2015
 )
 
+print "92X", run2_nanoAOD_92X._isChosen()
+print "94XMiniAODv1", run2_nanoAOD_94XMiniAODv1._isChosen()
+print "94XMiniAODv2", run2_nanoAOD_94XMiniAODv2._isChosen()
+print "94X2016", run2_nanoAOD_94X2016._isChosen()
+print "102Xv1",  run2_nanoAOD_102Xv1._isChosen()
+print "106Xv1", run2_nanoAOD_106Xv1._isChosen()
+
 tauTable.variables = _variablesMiniV2
 
 for era in [run2_nanoAOD_94XMiniAODv1,]:
@@ -157,6 +164,8 @@ run2_miniAOD_80XLegacy.toModify(tauTable,
 )
 for era in [run2_nanoAOD_92X, run2_nanoAOD_94XMiniAODv2, \
             run2_nanoAOD_94X2016, run2_nanoAOD_102Xv1, run2_nanoAOD_106Xv1]:
+    if not era._isChosen():
+        continue
     era.toModify(tauTable,
                  variables = cms.PSet(tauTable.variables, _mvaAntiEVars, _mvaIsoVars2015Reduced, _mvaIsoVars2017v1, _mvaIsoVars2017v2)
     )

@swozniewski
Copy link
Contributor Author

ah, I guess the PSet entered as an keyword argument is created no matter whether it is used by the modifier later on and that's where it fails already...

Comment on lines 158 to 162
for era in [run2_nanoAOD_92X, run2_nanoAOD_94XMiniAODv2, \
run2_nanoAOD_94X2016, run2_nanoAOD_102Xv1, run2_nanoAOD_106Xv1]:
era.toModify(tauTable,
variables = cms.PSet(tauTable.variables, _mvaAntiEVars, _mvaIsoVars2015Reduced, _mvaIsoVars2017v1, _mvaIsoVars2017v2)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

these lines are overwritten after, expect for the run2_nanoAOD_94XMiniAODv1 as is missed in this list.
also the run2_nanoAOD_94XMiniAODv1 is the one that make things crush.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, run2_nanoAOD_94XMiniAODv1 is not in the list on purpose because it had an independent treatment before already. See my last comment in the main thread concerning explanation and solution of the problem.

@swozniewski
Copy link
Contributor Author

I added the imports of the missing modifiers and checked with one of the relval tests that failed before.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33513/22475

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2021

Pull request #33513 was updated. @cmsbuild, @mariadalfonso, @gouskos, @fgolf can you please check and sign again.

@gouskos
Copy link
Contributor

gouskos commented May 4, 2021

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b82149/14850/summary.html
COMMIT: 9aef44e
CMSSW: CMSSW_12_0_X_2021-05-04-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33513/14850/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 103 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2662646
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2662617
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -3.612 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 136.8523 ): -3.612 KiB Physics/NanoAODDQM
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: no differences found

@mariadalfonso
Copy link
Contributor

+xpog

major cleanup of the selection in view of UL mini run2_nanoAOD_106Xv2 and future

  1. list of variable much more reduced now
    https://cms-nanoaod-integration.web.cern.ch/integration/33513/
    gain in size about 2-3%

  2. no change to the previous eras

@cmsbuild
Copy link
Contributor

cmsbuild commented May 6, 2021

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

@mariadalfonso
Copy link
Contributor

@swozniewski
please prepare the backport

@qliphy
Copy link
Contributor

qliphy commented May 7, 2021

+1

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