Final BDT models based on 10.2 MC samples #25936

bainbrid · 2019-02-14T17:42:30Z

This PR provides incremental changes on top of #25887.

This PR includes fixes to the SuperCluster class included in #25960 and #25974.

This PR requires the following external: cms-data/RecoEgamma-ElectronIdentification#13

This PR provide three changes:

1) It includes fixes to the SuperCluster class.

The fixes are back ported from #25960 and #25974:

fix a memory use after free error caught by ASAN;
avoid creating and using unnecessary edm::Refs;
removes a single line of code that was responsible for duplicating edm::Refs to ECAL clusters.

The timing and footprint numbers below have been updated to reflect these fixes.

2) It provides the back port of #25915.

The change is very small and concerns the only the LowPtGsfElectronSeedValueMapsProducer class, which produces ValueMaps that store the discriminator values from the BDT models used in the LowPtGsfElectronSeedProducer.

The ValueMaps originally stored the BDT outputs per GsfElectrons, but we need them per GsfTrack. There is a near (but not exact) 1-to-1 correspondence between GsfElectrons and GsfTracks, so the ValueMaps will increase moderately in size (at the few tens of % level, not orders of magnitude!) and they will use a different key. Quantitive numbers on timing and footprint can be found in the conversation of #25915.

3) It uses new BDT models and working points.

The models are found in cms-data/RecoEgamma-ElectronIdentification#13.

The working points have been tuned to rate balance w.r.t. the old WPs. Updated numbers are provided below, based on 11024 TTbar (PU50).

Timing:

The same excluding the first 1 events
  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
       added      +0.00%         0.00 ms/ev ->         0.15 ms/ev selectedPatLowPtElectrons
       added      +8.21%         0.00 ms/ev ->      1222.58 ms/ev lowPtGsfElectronSeeds
       added      +0.00%         0.00 ms/ev ->         0.57 ms/ev slimmedLowPtElectrons
       added      +0.01%         0.00 ms/ev ->         0.85 ms/ev lowPtElectronMatch
       added      +2.22%         0.00 ms/ev ->       330.50 ms/ev lowPtGsfEleGsfTracks
       added      +0.01%         0.00 ms/ev ->         2.06 ms/ev patLowPtElectrons
       added      +0.43%         0.00 ms/ev ->        63.31 ms/ev lowPtGsfEleCkfTrackCandidates
       added      +0.07%         0.00 ms/ev ->        10.39 ms/ev lowPtGsfElePfTracks
       added      +0.38%         0.00 ms/ev ->        56.14 ms/ev lowPtGsfElePfGsfTracks
       added      +0.00%         0.00 ms/ev ->         0.05 ms/ev lowPtGsfElectronSeedValueMaps
       added      +0.02%         0.00 ms/ev ->         2.77 ms/ev lowPtGsfElectronID
       added      +0.13%         0.00 ms/ev ->        18.65 ms/ev lowPtGsfElectronCores
       added      +0.89%         0.00 ms/ev ->       132.60 ms/ev lowPtGsfElectrons
       added      +0.20%         0.00 ms/ev ->        29.43 ms/ev lowPtGsfElectronSuperClusters
                 +12.57%                            1870.05 ms/ev
  ---------- ------------     --------                  ----       ------------
Job total:  14.8827 s/ev ==> 16.9203 s/ev

RECO footprint:

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch
-----------------------------------------------------------------
      0.0 ->       312.3        312     NEWO   0.01     recoGsfElectronCores_lowPtGsfElectronCores__RECO
      0.0 ->       252.9        253     NEWO   0.01     floatedmValueMap_lowPtGsfElectronID__RECO
      0.0 ->     13477.6      13478     NEWO   0.38     recoCaloClusters_lowPtGsfElectronSuperClusters__RECO
      0.0 ->      2198.6       2199     NEWO   0.06     recoSuperClusters_lowPtGsfElectronSuperClusters__RECO
      0.0 ->      6474.3       6474     NEWO   0.18     recoGsfTracks_lowPtGsfEleGsfTracks__RECO
      0.0 ->       264.5        264     NEWO   0.01     floatedmValueMap_lowPtGsfElectronSeedValueMaps_ptbiased_RECO
      0.0 ->       262.8        263     NEWO   0.01     floatedmValueMap_lowPtGsfElectronSeedValueMaps_unbiased_RECO
      0.0 ->     14082.6      14083     NEWO   0.40     recoGsfElectrons_lowPtGsfElectrons__RECO
-------------------------------------------------------------
  3541675 ->     3578709      37035             1.0     ALL BRANCHES

MINIAOD footprint:

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch
-----------------------------------------------------------------
      0.0 ->     12017.7      12018     NEWO   15.22    recoCaloClusters_lowPtGsfElectronSuperClusters__RECO
      0.0 ->     19327.0      19327     NEWO   24.48    patElectrons_slimmedLowPtElectrons__RECO
      0.0 ->       345.5        346     NEWO   0.44     recoGsfElectronCores_lowPtGsfElectronCores__RECO
      0.0 ->       200.8        201     NEWO   0.25     floatedmValueMap_lowPtGsfElectronSeedValueMaps_ptbiased_RECO
      0.0 ->       195.3        195     NEWO   0.25     floatedmValueMap_lowPtGsfElectronID__RECO
      0.0 ->       199.4        199     NEWO   0.25     floatedmValueMap_lowPtGsfElectronSeedValueMaps_unbiased_RECO
      0.0 ->      6594.4       6594     NEWO   8.35     recoGsfTracks_lowPtGsfEleGsfTracks__RECO
      0.0 ->      2125.1       2125     NEWO   2.69     recoSuperClusters_lowPtGsfElectronSuperClusters__RECO
-------------------------------------------------------------
    78956 ->      120018      41062            52.0     ALL BRANCHES

cmsbuild · 2019-02-14T17:43:06Z

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 · 2019-02-15T09:16:59Z

Incremental differences on top of #25887 seem rather limited (codewise): some better evaluation of this PR can be provided when #25887 is merged and this one will be rebased.

A few preliminary observations:

PhysicsTools/PatAlgos/python/slimming/MicroEventContent_cff.py should add the new products only for the bParking era (as in Complete low pT electron chain (back port) #25887)
Differences in the ```fillDescriptions()''' method of LowPtGsfElectronIDProducer: linked to the explicit declaration now in RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronID_cff.py
It also includes the backport of Low pT electrons: modify value maps storing Seed BDT outputs #25915
Different thresholds in RecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronSeeds_cfi.py
Differences in RecoEgamma/EgammaElectronProducers/src/LowPtGsfElectronSeedHeavyObjectCache.cc which need to be synchronized with 25887

In the meanwhile, please prepare also the corresponding pull request for the master release with just the incremental changes

cmsbuild · 2019-02-19T13:22:34Z

Pull request #25936 was updated. @cmsbuild, @perrotta, @santocch, @slava77 can you please check and sign again.

bainbrid · 2019-02-19T13:23:54Z

@perrotta @Dr15Jones @slava77
The commits above are the back ports of #25960 and #25974.
I will update the comments at the top of the thread.

perrotta · 2019-02-19T13:42:37Z

Thank you @bainbrid
Please don't forget to keep the master in synchro with it

perrotta · 2019-02-21T10:36:32Z

Now #25887 is merged in 10_2
Please @bainbrid wait for the first IB available with it, and rebase this PR, so that only the incremental changes can be sorted out

…sfElectronRef

The code was making a temporary copy of a PFBrem, then holding onto the address of internal data after the temporary was deleted. This change avoids the unnecessary copy. The problem was found by the address sanitizer.

cmsbuild · 2019-02-21T17:02:54Z

Pull request #25936 was updated. @cmsbuild, @perrotta, @slava77 can you please check and sign again.

bainbrid · 2019-02-21T17:12:43Z

@perrotta Rebased on CMSSW_10_2_X_2019-02-21-1100

perrotta · 2019-02-21T17:27:03Z

Thank you @bainbrid
As soon as the needed external will become available we will launch the tests
By the way: what about the test workflow with the bParking era enabled?

bainbrid · 2019-02-21T18:04:31Z

@perrotta Can anyone help with that? I have no clue what is needed. Perhaps @kfjack given he helped with the RelVal step for the skimming code. Is something similar needed here?

perrotta · 2019-02-22T06:19:21Z

please test
(the bPatking era is not activated in the standard tests, therefore we can start them even if the external with the new model is not yet available)

cmsbuild · 2019-02-22T06:21:02Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33219/console Started: 2019/02/22 07:22

cmsbuild · 2019-02-22T09:45:09Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 31
DQMHistoTests: Total histograms compared: 3007400
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3007209
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

Description: cms-data/RecoEgamma-ElectronIdentification#13 Resolves: #4702 Needed for: cms-sw/cmssw#25936

mrodozov · 2019-02-22T11:16:13Z

please test with cms-sw/cmsdist#4724
right ?

cmsbuild · 2019-02-22T11:16:41Z

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

perrotta · 2019-02-22T11:29:17Z

Thank you @mrodozov !
In reality, standard tests are not expected to show any change, because the new model should only get accessed with the dedicated bParking era, not enabled by default. But it is good to test it anyhow, just to certify that it does not interfere with the normal workflows.

cmsbuild · 2019-02-22T14:17:40Z

+1
Tested at: 274449d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25936/33229/summary.html

cmsbuild · 2019-02-22T14:17:47Z

Comparison job queued.

cmsbuild · 2019-02-22T16:14:42Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 31
DQMHistoTests: Total histograms compared: 3007400
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3007209
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

perrotta · 2019-02-25T14:57:10Z

-1
This PR is replaced and updated by #26013: it should better get closed now

bainbrid · 2019-02-25T15:22:21Z

Ok, i will close.

cmsbuild added this to the CMSSW_10_2_X milestone Feb 14, 2019

cmsbuild added analysis-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Feb 14, 2019

slava77 mentioned this pull request Feb 18, 2019

Final BDT models based on 10.2 MC samples cms-data/RecoEgamma-ElectronIdentification#13

Merged

fabiocos mentioned this pull request Feb 21, 2019

Features for low pt electrons in B parking processing #25991

Closed

6 tasks

bainbrid and others added 6 commits February 21, 2019 17:22

store Seed BDT outputs in ValueMaps keyed by GsfTrackRef instead of G…

c3edf21

…sfElectronRef

first pass of seed BDTs trained with 10.2.X MC

60b856f

updated to latest ID MDT trained with 10.2.X MC

ad31169

remove duplicate edm::Ref to ECAL clusters

274449d

Fix memory use after delete in LowPtGsfElectronSCProducer

c8c6567

The code was making a temporary copy of a PFBrem, then holding onto the address of internal data after the temporary was deleted. This change avoids the unnecessary copy. The problem was found by the address sanitizer.

Avoid creating an unnecessary edm::Ref

71cfe25

bainbrid force-pushed the LowPtElectronsFull_102X_update branch from 1c2aef5 to 71cfe25 Compare February 21, 2019 17:02

cmsbuild removed the analysis-pending label Feb 21, 2019

cmsbuild removed the tests-pending label Feb 22, 2019

cmsbuild added comparison-available and removed comparison-pending labels Feb 22, 2019

mrodozov added a commit to cms-sw/cmsdist that referenced this pull request Feb 22, 2019

Update to new tag V01-01-03

07e4a67

Description: cms-data/RecoEgamma-ElectronIdentification#13 Resolves: #4702 Needed for: cms-sw/cmssw#25936

mrodozov mentioned this pull request Feb 22, 2019

Update to new tag V01-01-03 cms-sw/cmsdist#4724

Merged

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-approved labels Feb 22, 2019

cmsbuild added requires-external tests-started and removed tests-pending labels Feb 22, 2019

cmsbuild added tests-approved and removed tests-started labels Feb 22, 2019

cmsbuild added comparison-available and removed comparison-pending labels Feb 22, 2019

This was referenced Feb 25, 2019

LowPtElectrons: switch to Autumn18 models #26012

Merged

LowPtElectrons: bug fixes + minor updates + Autumn18 models + new VL WP #26013

Merged

cmsbuild added reconstruction-rejected and removed reconstruction-pending labels Feb 25, 2019

bainbrid closed this Feb 25, 2019

bainbrid deleted the LowPtElectronsFull_102X_update branch August 6, 2019 12:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Final BDT models based on 10.2 MC samples #25936

Final BDT models based on 10.2 MC samples #25936

bainbrid commented Feb 14, 2019 •

edited

cmsbuild commented Feb 14, 2019

perrotta commented Feb 15, 2019

cmsbuild commented Feb 19, 2019

bainbrid commented Feb 19, 2019

perrotta commented Feb 19, 2019

perrotta commented Feb 21, 2019

cmsbuild commented Feb 21, 2019

bainbrid commented Feb 21, 2019

perrotta commented Feb 21, 2019

bainbrid commented Feb 21, 2019

perrotta commented Feb 22, 2019

cmsbuild commented Feb 22, 2019 •

edited

cmsbuild commented Feb 22, 2019

mrodozov commented Feb 22, 2019

cmsbuild commented Feb 22, 2019 •

edited

perrotta commented Feb 22, 2019

cmsbuild commented Feb 22, 2019

cmsbuild commented Feb 22, 2019

cmsbuild commented Feb 22, 2019

perrotta commented Feb 25, 2019

bainbrid commented Feb 25, 2019

Final BDT models based on 10.2 MC samples #25936

Final BDT models based on 10.2 MC samples #25936

Conversation

bainbrid commented Feb 14, 2019 • edited

cmsbuild commented Feb 14, 2019

perrotta commented Feb 15, 2019

cmsbuild commented Feb 19, 2019

bainbrid commented Feb 19, 2019

perrotta commented Feb 19, 2019

perrotta commented Feb 21, 2019

cmsbuild commented Feb 21, 2019

bainbrid commented Feb 21, 2019

perrotta commented Feb 21, 2019

bainbrid commented Feb 21, 2019

perrotta commented Feb 22, 2019

cmsbuild commented Feb 22, 2019 • edited

cmsbuild commented Feb 22, 2019

mrodozov commented Feb 22, 2019

cmsbuild commented Feb 22, 2019 • edited

perrotta commented Feb 22, 2019

cmsbuild commented Feb 22, 2019

cmsbuild commented Feb 22, 2019

cmsbuild commented Feb 22, 2019

perrotta commented Feb 25, 2019

bainbrid commented Feb 25, 2019

bainbrid commented Feb 14, 2019 •

edited

cmsbuild commented Feb 22, 2019 •

edited

cmsbuild commented Feb 22, 2019 •

edited