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

Low pT electrons: modify value maps storing Seed BDT outputs #25915

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

bainbrid
Copy link
Contributor

@bainbrid bainbrid commented Feb 12, 2019

This PR is a small modification to the code base in the now-merged PR #25753.

The back port of this PR to 10.2.X is #25887.

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.

No externals are needed nor changed for this PR.

We will need to back port this PR to the 10.2.X release cycle.

To give a sense of scale, below are the timing and footprint metrics for the original code. We do not expect these to change dramatically.

Timing:

The same excluding the first 1 events
  delta/mean delta/orJob     original                   new       module name
  ---------- ------------     --------                  ----       ------------
       added      +0.00%         0.00 ms/ev ->         0.03 ms/ev lowPtGsfElectronSeedValueMaps
  ---------- ------------     --------                  ----       ------------

RECO footprint:

Compare packed values
-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
      0.0 ->        98.5         98     NEWO   0.00     floatedmValueMap_lowPtGsfElectronSeedValueMaps_ptbiased_RECO.
      0.0 ->        97.8         98     NEWO   0.00     floatedmValueMap_lowPtGsfElectronSeedValueMaps_unbiased_RECO.
 -----------------------------------------------------------------
  3748472 ->     3755317       6845             0.2     ALL BRANCHES

@cmsbuild cmsbuild added this to the CMSSW_10_5_X milestone Feb 12, 2019
@bainbrid
Copy link
Contributor Author

@nancymarinelli @gkaratha @mverzett are interested to follow this thread.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25915/8396

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

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

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

@slava77
Copy link
Contributor

slava77 commented Feb 12, 2019

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 12, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33094/console Started: 2019/02/12 17:32

@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-25915/33094/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3097440
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3097242
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@bainbrid
Copy link
Contributor Author

Hi @slava77 @perrotta
Are we able to merge this small change?
If yes, I can prepare the back port to 10.2.X.
I can perhaps add the final models too, all in a single new PR.

Related: will the existing #25887 PR for 10.2.X be merged soon?

@perrotta
Copy link
Contributor

Verified with a 11024.0 workflow, run with the bParking era enabled, that the increase in output size is quite limited, while no sizeable effect on the CPU timing is visible:

Compare packed values
-----------------------------------------------------------------
   or, B         new, B      delta, B   delta, %   deltaJ, %    branch 
-----------------------------------------------------------------
    200.0 ->       216.3         16      7.8   0.00     floatedmValueMap_lowPtGsfElectronSeedValueMaps_ptbiased_RECO.
    196.9 ->       212.8         16      7.8   0.00     floatedmValueMap_lowPtGsfElectronSeedValueMaps_unbiased_RECO.
-------------------------------------------------------------
  3707672 ->     3707723         51             0.0     ALL BRANCHES

@perrotta
Copy link
Contributor

+1

  • The pull request realizes what declared in the description, which has been verified to run correctly both in the usual configurations and in the bParking era ones
  • Jenkins tests pass

@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 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.

5 participants