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

Run3JetID #40710

Merged
merged 1 commit into from Feb 16, 2023
Merged

Run3JetID #40710

merged 1 commit into from Feb 16, 2023

Conversation

laurenhay
Copy link
Contributor

PR description:

This PR implements the new Run3 JetID criteria and makes them the default. It also creates a new modifier for general Run2 and for the Run3 BCDEprompt.

Relevant presentations and twiki's:

https://indico.cern.ch/event/1212177/contributions/5119544/attachments/2538364/4371070/JetIDrecommendations_Run3_01112022.pdf
https://indico.cern.ch/event/1223865/contributions/5171746/attachments/2561170/4424192/RunF_JetID_jmar_13_December_2022.pdf
https://twiki.cern.ch/twiki/bin/view/CMS/JetID13p6TeV

PR validation:

Tested a subset of workflows (136.88811 and jetmc) and they ran successfully.

@etzia @nurfikri89

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40710/34088

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2023

A new Pull Request was created by @laurenhay (Lauren Hay) for master.

It involves the following packages:

  • Configuration/Eras (operations)
  • PhysicsTools/NanoAOD (xpog)
  • PhysicsTools/SelectorUtils (reconstruction)

@perrotta, @rappoccio, @swertz, @vlimant, @clacaputo, @cmsbuild, @mandrenguyen, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel, @missirol, @gpetruc, @fabiocos, @Martin-Grunewald this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@swertz
Copy link
Contributor

swertz commented Feb 7, 2023

Thanks @laurenhay ! Could you please clarify the following regarding the added modifiers:

  • I don't understand the need for a run2_jme modifier, and I don't see why you only add it to the Run2_2018 era (and not to 2016/17). Wouldn't it be more consistent to introduce a run2_jme_2018 modifer, add that one to the Run2_2018 era, and then make the jetID changes based on the combination run2_jme_2016 | run2_jme_2017 | run_jme_2018?
  • I suppose the run3_jme_Winter22runsBCDEprompt modifier is only meant for the prompt reco of runs BCDE, while the re-reco of BCDE and the prompt of FG is considered the default, right? (ie, in practice we wouldn't use it in the re-NanoV12)

@mandrenguyen
Copy link
Contributor

type jetmet

@cmsbuild cmsbuild added the jetmet label Feb 7, 2023
@laurenhay
Copy link
Contributor Author

Hello @swertz.

  1. I went with just run2_jme because those criteria were the default before switching to Run3, but yes you're right that it would probably be more consistent to make it run2_jme_2018. I can make that change quickly if desired, just let me know.
  2. As far as I understand this is exactly the case. @etzia may be able to clarify more.

@clacaputo
Copy link
Contributor

please test

@etzia
Copy link
Contributor

etzia commented Feb 7, 2023

  1. Yes, indeed I verify it. However when the re-reco BCDE are available we will check if the default JetID criteria are valid as well.

@swertz
Copy link
Contributor

swertz commented Feb 7, 2023

Thanks the confirmation!

I went with just run2_jme because those criteria were the default before switching to Run3, but yes you're right that it would probably be more consistent to make it run2_jme_2018. I can make that change quickly if desired, just let me know.

Yes, please go ahead.

@@ -54,6 +54,18 @@
tightJetIdLepVeto.filterParams, version = "RUN2UL16CHS"
)

run2_jme.toModify(
Copy link
Contributor

Choose a reason for hiding this comment

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

This should then become run2_jme_2016 | run2_jme_2017 | run_jme_2018

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to (run2_jme_2017 & run2_jme_2018).toModify( since the run2_jme_2016 modifier still lives above it on like 51.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed 👍 but shouldn't it be | instead of &? Perhaps both do the same...

Copy link
Contributor

Choose a reason for hiding this comment

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

They don't. & makes an "and" between the modifiers, whereas | makes an "or".

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks, in that case it's the | we want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sorry about that. My thinking was mixed up. We do want the "or". Pushing after code checks momentarily.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2023

-1

Failed Tests: UnitTests RelVals RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4cd088/30465/summary.html
COMMIT: ce1a8b6
CMSSW: CMSSW_13_0_X_2023-02-07-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40710/30465/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 TestConfigDP had ERRORS

RelVals

  • 138.4138.4_PromptCollisions2021/step2_PromptCollisions2021.log
  • 138.5138.5_ExpressCollisions2021/step2_ExpressCollisions2021.log
  • 4.534.53_RunPhoton2012B/step3_RunPhoton2012B.log
Expand to see more relval errors ...

RelVals-INPUT

  • 4.64.6_MinimumBias2010A/step2_MinimumBias2010A.log
  • 136.72412136.72412_RunJetHT2016B_reminiaodUL/step2_RunJetHT2016B_reminiaodUL.log
  • 138.4138.4_PromptCollisions2021/step2_PromptCollisions2021.log
Expand to see more relval errors ...

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40710/34175

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

Pull request #40710 was updated. @perrotta, @rappoccio, @swertz, @vlimant, @clacaputo, @cmsbuild, @mandrenguyen, @fabiocos, @davidlange6 can you please check and sign again.

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4cd088/30614/summary.html
COMMIT: 60f488a
CMSSW: CMSSW_13_1_X_2023-02-13-2300/el8_amd64_gcc11
Additional Tests: NANO
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40710/30614/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 12 lines from the logs
  • Reco comparison results: 25 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555972
  • DQMHistoTests: Total failures: 29
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3555921
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 213 log files, 164 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

NANO Comparison Summary

Summary:

  • You potentially removed 6 lines from the logs
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 11
  • DQMHistoTests: Total histograms compared: 10829
  • DQMHistoTests: Total failures: 18
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 10811
  • DQMHistoTests: Total skipped: 0
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 10 files compared)
  • Checked 23 log files, 10 edm output root files, 11 DQM output files

Nano size comparison Summary:

Sample kb/ev ref kb/ev diff kb/ev ev/s/thd ref ev/s/thd diff rate mem/thd ref mem/thd
2500.31 2.232 2.232 0.000 ( +0.0% ) 9.60 9.38 +2.4% 1.527 1.417
2500.311 2.323 2.323 0.000 ( +0.0% ) 9.11 9.15 -0.5% 1.891 1.740
2500.312 2.277 2.277 0.000 ( +0.0% ) 9.37 9.35 +0.2% 1.884 1.732
2500.33 1.100 1.100 0.000 ( +0.0% ) 21.88 21.34 +2.5% 1.654 1.449
2500.331 1.394 1.394 0.000 ( +0.0% ) 16.03 15.98 +0.3% 1.797 1.620
2500.332 1.326 1.326 0.000 ( +0.0% ) 17.59 17.78 -1.1% 1.862 1.538
2500.401 2.139 2.139 -0.000 ( -0.0% ) 10.40 10.05 +3.5% 1.210 1.114
2500.501 1.711 1.711 -0.000 ( -0.0% ) 16.61 16.57 +0.2% 1.127 1.004
2500.511 1.124 1.124 -0.000 ( -0.0% ) 30.50 30.76 -0.8% 1.379 1.288
2500.601 2.050 2.050 -0.000 ( -0.0% ) 12.52 12.55 -0.2% 1.187 1.050

@swertz
Copy link
Contributor

swertz commented Feb 14, 2023

+1

I see that 13_0_X has been branched off, so I think we'll now need a backport there.

@clacaputo
Copy link
Contributor

+reconstruction

@perrotta
Copy link
Contributor

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

@swertz
Copy link
Contributor

swertz commented Feb 20, 2023

@laurenhay kind reminder to backport this to 13_0_X as well.

@laurenhay laurenhay mentioned this pull request Feb 21, 2023
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

9 participants