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

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

Merged

Conversation

bainbrid
Copy link
Contributor

@bainbrid bainbrid commented Feb 25, 2019

This PR provides incremental changes on top of #25887.

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

This PR supersedes #25936.


This PR provides four changes:

1) 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) Modifies ValueMaps to store BDT output per GsfTrack track.

This is 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) Use new "Autumn18" BDT models and provide updated L, M, and T working points.

This is the back port of #26012.

4) This PR switches to a Very Loose working point in the ElectronSeed module of the low pT electron chain. This change is only enabled for the bParking era.

This is the back port of #26012.


Timing and footprint studies for the Very Loose WP used by the bParking era:

The increases in timing and footprint w.r.t. nominal are provided below. In short:

  • the timing increases by 25%,
  • the MINIAOD footprint increases by 137%.

RECO timing increase, bParking era only, Very Loose WP:

The same excluding the first 1 events
  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
       added      +0.00%         0.00 ms/ev ->         0.50 ms/ev selectedPatLowPtElectrons
       added      +8.23%         0.00 ms/ev ->      1225.09 ms/ev lowPtGsfElectronSeeds
       added      +0.01%         0.00 ms/ev ->         1.45 ms/ev slimmedLowPtElectrons
       added      +0.02%         0.00 ms/ev ->         2.56 ms/ev lowPtElectronMatch
       added      +8.92%         0.00 ms/ev ->      1328.09 ms/ev lowPtGsfEleGsfTracks
       added      +0.04%         0.00 ms/ev ->         6.17 ms/ev patLowPtElectrons
       added      +2.00%         0.00 ms/ev ->       297.88 ms/ev lowPtGsfEleCkfTrackCandidates
       added      +0.07%         0.00 ms/ev ->         9.84 ms/ev lowPtGsfElePfTracks
       added      +1.55%         0.00 ms/ev ->       230.25 ms/ev lowPtGsfElePfGsfTracks
       added      +0.00%         0.00 ms/ev ->         0.15 ms/ev lowPtGsfElectronSeedValueMaps
       added      +0.04%         0.00 ms/ev ->         5.93 ms/ev lowPtGsfElectronID
       added      +0.53%         0.00 ms/ev ->        78.90 ms/ev lowPtGsfElectronCores
       added      +2.86%         0.00 ms/ev ->       425.63 ms/ev lowPtGsfElectrons
       added      +0.81%         0.00 ms/ev ->       120.77 ms/ev lowPtGsfElectronSuperClusters
                 +25.08%                            3732.71 ms/ev
  ---------- ------------     --------                  ----       ------------
Job total:  14.8827 s/ev ==> 18.9492 s/ev

RECO footprint increase, bParking era only, Very Loose WP:

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch
-----------------------------------------------------------------
      0.0 ->      1126.5       1127     NEWO   0.03     recoGsfElectronCores_lowPtGsfElectronCores__RECO
      0.0 ->      2121.7       2122     NEWO   0.06     recoConversions_gsfTracksOpenConversions_gsfTracksOpenConversions_RECO
   8870.8 ->      8425.3       -446     -5.2  -0.01     edmErrorSummaryEntrys_logErrorHarvester__RECO
      0.0 ->       601.4        601     NEWO   0.02     floatedmValueMap_lowPtGsfElectronID__RECO
      0.0 ->     18855.2      18855     NEWO   0.53     recoCaloClusters_lowPtGsfElectronSuperClusters__RECO
      0.0 ->      6125.4       6125     NEWO   0.17     recoSuperClusters_lowPtGsfElectronSuperClusters__RECO
      0.0 ->     27324.7      27325     NEWO   0.77     recoGsfTracks_lowPtGsfEleGsfTracks__RECO
      0.0 ->       773.9        774     NEWO   0.02     floatedmValueMap_lowPtGsfElectronSeedValueMaps_ptbiased_RECO
      0.0 ->       790.8        791     NEWO   0.02     floatedmValueMap_lowPtGsfElectronSeedValueMaps_unbiased_RECO
      0.0 ->     43295.5      43296     NEWO   1.22     recoGsfElectrons_lowPtGsfElectrons__RECO
-------------------------------------------------------------
  3541675 ->     3642021     100346             2.8     ALL BRANCHES

MINIAOD footprint increase, bParking era only, Very Loose WP:

-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch
-----------------------------------------------------------------
      0.0 ->     16480.1      16480     NEWO   20.87     recoCaloClusters_lowPtGsfElectronSuperClusters__RECO
      0.0 ->     57850.4      57850     NEWO   73.27     patElectrons_slimmedLowPtElectrons__RECO
      0.0 ->       985.8        986     NEWO   1.25     recoGsfElectronCores_lowPtGsfElectronCores__RECO
      0.0 ->       723.1        723     NEWO   0.92     floatedmValueMap_lowPtGsfElectronSeedValueMaps_ptbiased_RECO
      0.0 ->       555.0        555     NEWO   0.70     floatedmValueMap_lowPtGsfElectronID__RECO
      0.0 ->       742.8        743     NEWO   0.94     floatedmValueMap_lowPtGsfElectronSeedValueMaps_unbiased_RECO
      0.0 ->     25644.5      25644     NEWO   32.48     recoGsfTracks_lowPtGsfEleGsfTracks__RECO
      0.0 ->      5431.8       5432     NEWO   6.88     recoSuperClusters_lowPtGsfElectronSuperClusters__RECO
-------------------------------------------------------------
    78956 ->      187349     108393            137.3     ALL BRANCHES

Timing and footprint studies for the Loose WP used by the bParking era:

For the record: the numbers below are for the original Loose working point, which has been tuned to rate balance w.r.t. the old Fall17 models.

RECO timing increase, bParking era only, Loose WP:

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 increase, bParking era only, Loose WP:

-----------------------------------------------------------------
   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 increase, bParking era only, Loose WP:

-----------------------------------------------------------------
   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
Copy link
Contributor

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

It involves the following packages:

RecoEgamma/EgammaElectronProducers

@cmsbuild, @perrotta, @slava77 can you please review it and eventually sign? Thanks.
@jainshilpi, @Sam-Harper, @varuns23, @rovere, @lgray 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

@bainbrid
Copy link
Contributor Author

If preferred, I can merge this new PR in the existing one #25936.

@bainbrid
Copy link
Contributor Author

@perrotta
Copy link
Contributor

perrotta commented Feb 25, 2019

backport of #26012 and #25960 and #25974 and #25915

@perrotta
Copy link
Contributor

please test with cms-sw/cmsdist#4724

@perrotta
Copy link
Contributor

This PR replaces and updates #25936, which can therefore get closed now
Please update the PR description by adding what was in the other PR

@bainbrid
Copy link
Contributor Author

I’ve closed the other PR. I’ll update the description later today.

@perrotta
Copy link
Contributor

please test with cms-sw/cmsdist#4724

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 25, 2019

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

@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-26013/33275/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: 4
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3007206
  • 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
Copy link
Contributor

This PR correctly backports the content of the pull requests #26012 and #25960 and #25974 and #25915.
This is correctly reported in the PR description. Could you please update also the title, so that the whole PR content can be recalled with it, and not only the new VL wp?

The difficult thing for you will be to invent a short sentence which can summarize all that: good luck! :-)

@bainbrid bainbrid changed the title LowPtElectrons: Very Loose working point LowPtElectrons: bug fixes + minor updates + Autumn18 models + new VL WP Feb 26, 2019
@bainbrid
Copy link
Contributor Author

I tried ... ;-)

@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 CMSSW_10_2_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_10_6_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

this code is not active by default, and does not interfere with existing production setups

@cmsbuild cmsbuild merged commit c4c9bd4 into cms-sw:CMSSW_10_2_X Feb 28, 2019
@bainbrid bainbrid deleted the LowPtElectronsFull_102X_VeryLoose branch August 6, 2019 11:59
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

5 participants