-
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: switch to Autumn18 models #26012
LowPtElectrons: switch to Autumn18 models #26012
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26012/8533
|
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. cms-bot commands are listed here |
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26012/8534
|
Please test with cms-sw/cmsdist#4728 |
The tests are being triggered in jenkins. |
Thank you @gudrutis ! |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
@bainbrid : performance numbers seem to change quite often for those low pt electrons. The comparisons you are listing in the PR descriptions clearly do not refer to the baseline (which already includes all those lotPtEle modules). They also differ somehow to what was evaluated previously for the ancestors of this PR, see for example #25753 (comment), for which the TTbar+PU workflow 11024.0 was used. In order to reduce the confusion, could you please specify how did you obtain the performance numbers listed in the PR description? In particular:
The RECO event size apparently differ significantly only in the Timing for the tight working point also seems rather inflated here with respect to the last evaluations that we made for #25679 (but you still have to define how was it computed here). The new model should only affect ID, not the seeds: when you specify better how all those numbers where derived we can draw some conclusion on it. |
@perrotta
Apologies for the confusion. Hopefully the comments below will clarify.
On 26 Feb 2019, at 11:53, perrotta ***@***.***> wrote:
@bainbrid <https://github.com/bainbrid> : performance numbers seem to change quite often for those low pt electrons. The comparisons you are listing in the PR descriptions clearly do not refer to the baseline (which already includes all those lotPtEle modules). They also differ somehow to what was evaluated previously for the ancestors of this PR, see for example #25753 (comment) <#25753 (comment)>, for which the TTbar+PU workflow 11024.0 was used.
I have not given the incremental difference w.r.t. the latest IB, but instead w.r.t. the nominal RECO chain. i.e. the latest version of the complete low pT electron chain adds 4.5% for standard workflows, which is consistent with numbers given previously, as I try to summarise below.
[Here](#25679 (comment)) you report a CPU increase for "up to GsfTracks" of 4.3%.
[Here](#25753 (comment)) I report numbers from myself, broken down according to "Up to GsfTracks" (4.41%) and the full chain (4.79%). (To give an example of the source of the largest increase, the lowPtGsfElectronSeeds module alone adds 3.31%.)
[Here](#25753 (comment)) you report numbers for the full chain _in addition_ to "Up to GsfTracks": an additional 52 ms/ev.
[Here](#26012 (comment)) is the latest summary from me, which shows an increase in CPU of 4.50% w.r.t. the nominal RECO, without low pT electrons. (The lowPtGsfElectronSeeds alone adds 3.46%.)
I would suggest all these numbers are consistent, at the level of 5-10%, within the context of statistical uncertainties and bug fixes, details below.
In order to reduce the confusion, could you please specify how did you obtain the performance numbers listed in the PR description? In particular:
Which workflow were you using for it? How many events?
I am using 11024, 100 events.
Which baseline are you comparing to?
The numbers I quote in this thread refer to a "baseline" that does not execute the low pT ele chain __at all__. i.e. the nominal RECO chain.
So the increases fold in the _total_ addition to RECO from the low pT electrons (and do not factorise into "up to GsfTracks" or "Full chain" as we have done for previously, for the different PRs).
Do the timing includes also Validation/DQM or only the reco/miniAOD steps?
Only RECO/miniAOD. (I think I remember being told to do this - is it correct?)
The RECO event size apparently differ significantly only in the recoCaloClusters_lowPtGsfElectronSuperClusters, and I expect that this is could be correlated to the bug fixes for it that were integrated in the meanwhile with #25974 <#25974>. On the other hand, that bug fix should have moved in the direction of slimming the SuperClusters, not the opposite: do you have an explantion for it?
I'm not sure I can draw the same conclusion. We were accessing freed memory (so who knows what that was), so I cannot say for certain whether the SuperCluster should increase or decrease. What I can say is that the SuperClusters distributions now look a lot healthier ...
Timing for the tight working point also seems rather inflated here with respect to the last evaluations that we made for #25679 <#25679> (but you still have to define how was it computed here). The new model should only affect ID, not the seeds: when you specify better how all those numbers where derived we can draw some conclusion on it.
This PR updates __all__ models, both Seeds and ID, from the Fall17 versions to the Autumn18 versions.
The corresponding working points have been updated, and were done so to rate balance w.r.t. what was observed for the Fall17 models. So the new models + Tight WPs gives a 4.5% increase in CPU w.r.t. the nominal RECO. And this should be compared with the number obtained for the Fall17 models, which is 4.8%, reported [here](#25753 (comment)).
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#26012 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABEfkszH2hgL1OzTndy79Hsxgb2xnNvrks5vRSAmgaJpZM4bPyek>.
|
Thank you @bainbrid : the key point is that you are normalizing timings on the RECO/miniAOD (good!) and the wf used is the same as in the previous evaluations (also good!). As a comparison, could you please provides the numbers also for the VL working point? Or should we rely on the evaluations that you provide in #26013 for it, even if measured on a different base release? |
On 26 Feb 2019, at 14:06, perrotta ***@***.***> wrote:
Thank you @bainbrid <https://github.com/bainbrid> : the key point is that you are normalizing timings on the RECO/miniAOD (good!) and the wf used is the same as in the previous evaluations (also good!).
As a comparison, could you please provides the numbers also for the VL working point? Or should we rely on the evaluations that you provide in #26013 <#26013> for it, even if measured on a different base release?
Yes, please rely on those listed in the conversion for #26013.
My thinking was:
- use master for the evaluation of the standard workflows
- use 10_2_X for the evaluation of the bParking era.
I hope this makes sense.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#26012 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABEfkhq6nFKrFGNgjDIx4fsLDj3X3M7Lks5vRT-BgaJpZM4bPyek>.
|
And what do you use for the baseline? 10_2 default in both cases, or 10_2_X baseline in one case and a "managed" master to remove even the tight wp from it for 10_6_X? |
All numbers are w.r.t. the __nominal RECO__ *without* low pT electrons.
i.e. "default 10_2_X".
In both cases.
(Similar to what I did for the Tight WP studies for master.)
… On 26 Feb 2019, at 14:17, perrotta ***@***.***> wrote:
And what do you use for the baseline? 10_2 default in both cases, or 10_2_X baseline in one case and a "managed" master to remove even the tight wp from it for 10_6_X?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#26012 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABEfksYN8HodqJ9zm4rhFnSdIg88DZGmks5vRUH6gaJpZM4bPyek>.
|
+1
|
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) |
+1 the needed external cms-sw/cmsdist#4728 has been merged has well |
This PR updates the models used by the lowPtGsfElectrons chain. The old models were "Fall17"; the new are "Autumn18". The Autumn18 models are available as externals in this (merged) PR:
cms-data/RecoEgamma-ElectronIdentification#13.
This PR also switches to a "Very Loose" working point for the low pT ElectronSeed module.
Commit 23e8326: Trivially moves some default configuration values (concerning the models) from the
fillDescriptions()
method of LowPtGsfElectronIDProducer to an explicit declaration inRecoEgamma/EgammaElectronProducers/python/lowPtGsfElectronID_cff.py
. This commit was (accidentally) added to 10_2_X before master in the ad31169 commit as part of Final BDT models based on 10.2 MC samples #25936.Commit 48fead2: Points to the new Autumn18 files and updates the corresponding L,M, and T working points
Commit 9f8467b: Adds the thresholds for a Very Loose WP and makes VL the new default for the bParking era. The back port of this commit can be found in LowPtElectrons: bug fixes + minor updates + Autumn18 models + new VL WP #26013.
The timing and footprint increases w.r.t. nominal when using the Autumn18 models and the default Tight working point used by all standard sequences are provided below.
Timing, standard workflows, Tight WP:
RECO footprint, standard workflows, Tight WP:
The lowPtGsfElectrons are not added to miniAOD for all standard workflows.
@slava77 @perrotta @mverzett @nancymarinelli @gkaratha