-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
LowPtElectrons: bug fixes + minor updates + Autumn18 models + new VL WP #26013
Conversation
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.
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. cms-bot commands are listed here |
If preferred, I can merge this new PR in the existing one #25936. |
please test with cms-sw/cmsdist#4724 |
This PR replaces and updates #25936, which can therefore get closed now |
I’ve closed the other PR. I’ll update the description later today. |
please test with cms-sw/cmsdist#4724 |
The tests are being triggered in jenkins. |
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
This PR correctly backports the content of the pull requests #26012 and #25960 and #25974 and #25915. The difficult thing for you will be to invent a short sentence which can summarize all that: good luck! :-) |
I tried ... ;-) |
+1
|
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) |
+1 this code is not active by default, and does not interfere with existing production setups |
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:
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 theLowPtGsfElectronSeedProducer
.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:
RECO timing increase, bParking era only, Very Loose WP:
RECO footprint increase, bParking era only, Very Loose WP:
MINIAOD footprint increase, bParking era only, Very Loose WP:
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:
RECO footprint increase, bParking era only, Loose WP:
MINIAOD footprint increase, bParking era only, Loose WP: