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

Add low-pT electrons to MINIAOD, update ID, improve end user experience #31220

Merged
merged 10 commits into from
Dec 2, 2020

Conversation

bainbrid
Copy link
Contributor

@bainbrid bainbrid commented Aug 24, 2020

This PR builds on a previous PR: #31080

This PR requires: cms-data/RecoEgamma-ElectronIdentification#15

Updated and embedded electron ID

Embedded ElectronSeed BDT scores

  • Schedules the LowPtGsfElectronSeedValueMapProducer to run as part of the default RECO sequence (and within PAT producersLayer1 for the run2_miniAOD_UL modifier). This producer rekeys two floatValueMaps by reco::GsfElectron such that the BDT scores can be embedded in the pat::Electron
  • The contents of these two ValueMaps are embedded via the electronIDSources PSet with the keys 'unbiased' and 'ptbiased'.

Embedded reco::CandidatePtr to provide provenance information

  • Schedules the LowPtGSFToPackedCandidateLinker module to run as part of the slimming sequence.
  • The Linker module has been adapted to provide a ValueMap of edm::PackedCandidatePtrs such that the low-pT pat::Electron can be linked to a PFCandidate object via the following provenance chain pat::Electron-->reco::GsfTrack-->reco::Track and reco::PFCandidate-->reco::Track.

Selects electrons based on the ID score

  • Selector string: cut = cms.string("pt>1. && electronID('ID')>1.5"), which gives an efficiency / fake rate of 90% / 10%

Miscellaneous

  • Embeds the ID and the two ElectronSeed BDT scores in the pat::Electron
  • Embeds related RECO quantities (GsfTrack, etc) in the pat::Electron
  • Drops all unnecessary RECO collections from the MINIAOD Event Content
  • Add protection for run2_miniAOD_94XFall17 and run2_miniAOD_80XLegacy (no low-pT electrons in existing AOD)

@slava77 @guitargeek @crovelli @afiqaize @SohamBhattacharya @jpata @jainshilpi @lsoffi

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@slava77
Copy link
Contributor

slava77 commented Aug 24, 2020

... if the bParking era is specified.

why not for all workflows that has the lowPt electrons configured or in the inputs?

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31220/17878

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

PhysicsTools/PatAlgos

@perrotta, @jpata, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@jdamgov, @rappoccio, @gouskos, @jdolen, @ahinzmann, @smoortga, @riga, @schoef, @emilbols, @mariadalfonso, @JyothsnaKomaragiri, @nhanvtran, @gkasieczka, @clelange, @hatakeyamak, @ferencek, @gpetruc, @andrzejnovak, @peruzzim, @seemasharmafnal, @mmarionncern 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

@bainbrid
Copy link
Contributor Author

bainbrid commented Aug 24, 2020 via email

@slava77
Copy link
Contributor

slava77 commented Aug 24, 2020

Oh, I thought the intention was that the ID is updated only if the bParking era is specified. Perhaps I misunderstood. I can remove this condition if preferred.

I was assuming that lowPtGsfElectronID works for both b-parking and the regular low-pt electrons, as the former is essentially a superset.
If that's the case, then why not provide the ID for everyone, not just those running the bParking modifier?
But maybe I just missed the context/scope of the update.

@slava77
Copy link
Contributor

slava77 commented Aug 24, 2020

Oh, I thought the intention was that the ID is updated only if the bParking era is specified. Perhaps I misunderstood. I can remove this condition if preferred.

I was assuming that lowPtGsfElectronID works for both b-parking and the regular low-pt electrons, as the former is essentially a superset.
If that's the case, then why not provide the ID for everyone, not just those running the bParking modifier?
But maybe I just missed the context/scope of the update.

In the current state of the default miniAOD, the point is somewhat moot because patLowPtElectrons are not running, but the near term plan is to put them in. So, this part of the code perhaps should assume as if it were enabled for all.
Someone from EGM better confirm/clarify

@bainbrid
Copy link
Contributor Author

Ah, yes, my mistake! Currently we only produce low-pT electrons for MINIAOD if the bParking era is enabled, as implemented here.

So presuambly I can remove the bParking.toModify() and bParking.toReplaceWith() conditions in the lowPtElectronProducer_cff.py file in this PR, correct?

The other question is if we always want low pT electrons in MINIAOD?

If yes, then the bParking era will only toggle the lower pT threshold (0.5 vs 1.0 GeV) and the seeding BDT working point (Very Loose vs Tight), as done here.

@slava77
Copy link
Contributor

slava77 commented Aug 25, 2020

@cmsbuild please test workflow 10824.8,136.898

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 25, 2020

The tests are being triggered in jenkins.
Test Parameters:

@cmsbuild
Copy link
Contributor

+1
Tested at: 9c8780e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e7542/8910/summary.html
CMSSW: CMSSW_11_2_X_2020-08-24-2300
SCRAM_ARCH: slc7_amd64_gcc820

@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-8e7542/8910/summary.html

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

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-8e7542/10824.8_TTbar_13+2018_ParkingBPH+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA+Nano
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-8e7542/136.898_RunParkingBPH2018B+RunParkingBPH2018B+HLTDR2_2018+RECODR2_2018reHLT_skimParkingBPH_Offline+HARVEST2018

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2609656
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2609628
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 34 files compared)
  • DQMHistoSizes: changed ( 10224.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

@santocch
Copy link

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2020

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

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 30 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2529593
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2529563
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 148 log files, 37 edm output root files, 35 DQM output files

@slava77
Copy link
Contributor

slava77 commented Dec 1, 2020

@cms-sw/xpog-l2
please take a look at this, it's on a critical path towards the UL remini.
Thank you.

@mariadalfonso @gouskos
please clarify if you are planning to check this PR.
RSVP

@mariadalfonso
Copy link
Contributor

@cms-sw/xpog-l2
please take a look at this, it's on a critical path towards the UL remini.
Thank you.

@mariadalfonso @gouskos
please clarify if you are planning to check this PR.
RSVP

@slava77
we do not have extra testing set-up for the miniAOD.
from visual inspection of PhysicsTools/PatAlgos the changes look reasonable
and Jenkins shows changes only on the lowPtElectron value map

@slava77
Copy link
Contributor

slava77 commented Dec 2, 2020

@slava77
we do not have extra testing set-up for the miniAOD.
from visual inspection of PhysicsTools/PatAlgos the changes look reasonable
and Jenkins shows changes only on the lowPtElectron value map

Thank you for checking.
As mentioned originally, #31220 (comment), the xpog signature was requested to make sure that the substantial update to the event content is sound and is acceptable.
Here we have about 2% increase. (well, at the time of "assign xpog" it was a 20% increase).

@gouskos
Copy link
Contributor

gouskos commented Dec 2, 2020

+xpog

@santocch
Copy link

santocch commented Dec 2, 2020

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2020

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)

@silviodonato
Copy link
Contributor

+1

@silviodonato
Copy link
Contributor

@bainbrid Please have a look at #32376

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.

large start-of-job memory spikes