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

Complete low pT electron chain (back port) #25887

Merged
merged 5 commits into from Feb 21, 2019

Conversation

bainbrid
Copy link
Contributor

@bainbrid bainbrid commented Feb 7, 2019

This PR is the back port of #25753.

It relies on cms-sw/cmsdist#4691 (produced for 102X).

In summary, the PR:

  • completes the low pT electron chain by adding new modules and extending in a minor way the functionality of the existing Seeding module;
  • does not introduce any new DataFormat and simply reuses existing ones;
  • adds new collections to the Event;
  • does not alter any of the default EGamma code and workflows,
  • integrates within the miniAOD workflows,
  • relies on a new BDT model for ID purposes, found in this PR.

The modified existing modules are:

  • LowPtGsfElectronSeedProducer: this producer is minimally extended to produce a ValueMap indexed by an ElectronSeedRef. The PreIds hold the discriminator outputs of the BDT models used by this producer but the PreIds cannot be accessed directly from the GsfElectrons and related interfaces, while the ElectronSeeds are accessible. Hence this ValueMap allows the LowPtGsfElectronSeedValueMaps module to link the BDT output from the Seeding module to electrons, details below.

The new modules are:

  • LowPtGsfElectronSCProducer: A new "tracker-driven" SuperCluster producer, seeded by extrapolating the low pT GsfTracks and brem trajectories to the ECAL.
  • LowPtGsfElectronCoreProducer: this straightforward module produces GsfElectronCore objects and borrows heavily from the standard EGamma code.
  • LowPtGsfElectronProducer: this straightforward module produces GsfElectron objects and borrows heavily from the standard EGamma code.
  • lowPtGsfElectronSeedValueMapsProducer: This module consumes GsfElectrons and the ValueMap from the LowPtGsfElectronSeedProducer module. It produces two ValueMap indexed by a GsfElectronRef that contain the BDT discriminator values.
  • LowPtGsfElectronIDProducer: this module consumes GsfElectrons and makes use of a BDT model to ID the electrons, i.e. discriminate genuine electrons from fakes. The XML description of the model can be found in this PR to the cms-data repository.
  • miniAOD integration: modifications to the PhysicsTools/PatAlgos package allow to produced a slimmed low pT electron collection.

Here is the new sequence.

Here are the output collections stored for the RECO and AOD data tiers.

Here and here are the collections stored in the miniAOD data tier.

@bainbrid
Copy link
Contributor Author

bainbrid commented Feb 7, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2019

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

It involves the following packages:

PhysicsTools/PatAlgos
RecoEgamma/Configuration
RecoEgamma/EgammaElectronProducers

@cmsbuild, @perrotta, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @jainshilpi, @rappoccio, @HeinerTholen, @varuns23, @seemasharmafnal, @mmarionncern, @imarches, @ahinzmann, @smoortga, @acaudron, @lgray, @jdolen, @drkovalskyi, @ferencek, @Sam-Harper, @rovere, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @clelange, @JyothsnaKomaragiri, @mverzett, @gpetruc, @mariadalfonso, @pvmulder this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

perrotta commented Feb 8, 2019

@mrodozov
I think that a similar PR as cms-sw/cmsdist/pull/4650 should be prepared for 10_2_X: can you please take care of it?

@perrotta
Copy link
Contributor

perrotta commented Feb 8, 2019

code-checks

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25887/8356

  • This PR adds an extra 56KB to repository

@bainbrid
Copy link
Contributor Author

bainbrid commented Feb 8, 2019

@mrodozov @perrotta
Is there anything I can do to help here?

@bainbrid
Copy link
Contributor Author

@mrodozov @slava77 @perrotta

Just a heads up for the future:

  1. we will update the models in cms-data for the 10.2.X release cycle
  2. this will be accompanied by 1-2 change files in cms-sw, again for the 10.2.X release cycle
  3. we will repeat the timing and footprint studies with the final models and working points

We cannot yet define precisely the timescale for the above, so we'd prefer to:

  • continue to merge ASAP this PR
  • and open a new (small) PR when 1-3 are ready.

Is this reasonable?

@perrotta
Copy link
Contributor

Hi @bainbrid : yes, that makes sense.

We can proceed with this PR as soon as the needed external becomes available in 10_2_X

@mrodozov @smuzaffar : could you please prepare a similar PR as cms-sw/cmsdist/pull/4650 for 10_2_X, or arrange with @bainbrid for it, if anything which is needed is missing? Then we can launch the tests here.

@bainbrid
Copy link
Contributor Author

@mrodozov @smuzaffar : could you please prepare a similar PR as cms-sw/cmsdist/pull/4650 for 10_2_X, or arrange with @bainbrid for it, if anything which is needed is missing? Then we can launch the tests here.

I think there is nothing missing - you can take the existing models used already for 105X.

@mrodozov
Copy link
Contributor

Use cms-sw/cmsdist#4691 to test this when you are rdy.

@perrotta
Copy link
Contributor

please test with cms-sw/cmsdist#4691
(thank you @mrodozov )

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 11, 2019

The tests are being triggered in jenkins.
Using externals from cms-sw/cmsdist#4691
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33077/console

@perrotta
Copy link
Contributor

backport of #25753

@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-25887/33077/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 3007400
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3007208
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
  • Checked 129 log files, 14 edm output root files, 31 DQM output files

@bainbrid
Copy link
Contributor Author

Hi all - just a gentle ping - is there anything left to do here?

@perrotta
Copy link
Contributor

+1

  • Correct backport of Complete low pT electron chain (2) #25753
    • Differences in RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectrons_cfi.py are due to the base class of the LowPtGsfElectronProducer which has been update after the 10_2_X cycle was closed
    • Differences in LowPtGsfElectronIDHeavyObjectCache.cc and LowPtGsfElectronSeedHeavyObjectCache.cc come from GBRForest implementation without TMVA #24432, which was only merged in 10_3_X
  • Jenkins tests pass and show no differences: in fact, nothing is expected if the bParking era is not requested
  • Verified running with correctly the bParking era enabled (while the proponents will take care of evaluating the physics performance with it)
  • It needs Update data-RecoEgamma-ElectronIdentification.spec cmsdist#4691

@santocch
Copy link

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_10_2_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_10_5_X is complete. This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

+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