-
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
swapping order of cuts to avoid expensive dxy/dz calc #26409
swapping order of cuts to avoid expensive dxy/dz calc #26409
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26409/9177
|
A new Pull Request was created by @Sam-Harper (Sam Harper) for master. It involves the following packages: RecoEgamma/PhotonIdentification @cmsbuild, @perrotta, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
float dxy = -999; | ||
auto iCand = aCCand.candidate; | ||
float dR2 = deltaR2(phoWrtVtxEta, phoWrtVtxPhi, iCand->eta(), iCand->phi()); | ||
if (dR2 > coneSizeDR * coneSizeDR || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional little gain can be achieved if you compute "coneSizeDR * coneSizeDR" and "dRveto * dRveto" only once when they are defined (as only their squares are used in this method)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we go down that route, we'll redo the entire class, its text book on how to write slow code. Yes there is much much more that could be done. I figure lets get the win, bake it in and then really fix this class later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not asking to rewrite the entire class, and of course I'm happy that you plan doing it eventually.
But if the purpose of this PR is to improve timing in the less invasive way, I think that don't having to make those multiplications multiple times per event can save a few cycles in an absolutely strightforward way and without any possible counterindication: I don't see why not to apply it also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well two reasons 1) everytime I touch something there is a potential for error and 2) I'm concerned that because this class is such a hot mess that this will be the start of many such changes.
Now I see from your intention that its not to go down that route, so I make those changes and call it a day?
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26409/9194
|
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+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 |
PR description:
The PhotonIDValueMap producer is a known slow module. Part of this is that it does expensive dxy/dz corrections (calling the slow ones too in miniAOD but thats another story).
This is a simple fix which speeds it up by doing a dR match first rather than a slow dxy./dz cut. Further optimisations are more than possible, however this is already significant and is obvious that it is safe.
I'm going to request this back to 102X because it'll speed up analysis user jobs significantly
PR validation:
A simple test job on running on (I think) EGamma Run2018 AOD and MINIAOD. This just running VID & scale and smearing in a job and writing out the result. So a typical analysis job.
AOD:
http://sharper.web.cern.ch/sharper/cgi-bin/igprof-navigator/forPRs/aodVIDRef
http://sharper.web.cern.ch/sharper/cgi-bin/igprof-navigator/forPRs/aodVIDSpeedup
time counts PhotonIDValueMapProducer::produce 186.44 -> 81.66
MINIAOD:
http://sharper.web.cern.ch/sharper/cgi-bin/igprof-navigator/forPRs/worstIsolRef
http://sharper.web.cern.ch/sharper/cgi-bin/igprof-navigator/forPRs/worstIsolSpeedup
time counts of PhotonIDValueMapProducer::produce 3,379.67 -> 433.96