Adding angle utilities #26227

cvuosalo · 2019-03-20T22:03:59Z

PR description:

In many spots in CMSSW, code like the following appears:

if (pointphi >= twopi)
     pointphi -= twopi;
if(pointphi < 0)
     pointphi += twopi;

The intent is to constrain the angle to the range 0<= angle < 2pi, but the code, as in this example, sometimes doesn't handle the case of an |angle| >= 4pi.

DataFormats/Math/interface/deltaPhi.h provides a reduceRange() function for this purpose, but tests show this function is not as fast as it could be, and it suffers from sensitivity in its if statement to small deviations from pi.

This PR adds a new function make0to2pi, that can be significantly faster than reduceRange() and that greatly reduces value drift due to floating-point comparisons. This new function is added without changing the implementation of reduceRange() because changing reduceRange() could cause slight differences in program behavior. Usage of reduceRange() can later be replaced on a case-by-case basis where better performance and precision is required.

The Phi class in DataFormats/GeometryVector/interface/Phi.h represents an angle that maintains a value from -pi to pi. This PR enhances this class in a backwardly compatible fashion to allow the option of keeping the angle in the range from 0 to 2pi.

In addition, the generic angle utilities in the GeantUnits.h file are moved to deltaPhi.h because they are not Geant-specific and should be usable without subjecting code to Geant conventions.

PR validation:

A unit test program is included in this PR, and it verifies basic functions of the code and gives some rough performance measures.

Some performance results comparing reduceRange() and make0to2pi are shown below.

The entry for the Phi templates in the classes_def.xml file uses ClassVersion 3 because the revised template is effectively a new class. No problems with the ROOT dictionary for Phi were observed. Using baseline CMSSW_10_6_0_pre2 code without this PR, I wrote a Phi variable to a ROOT file. Then, with the new PR code, I could successfully read the Phi variable out of that same ROOT file, so there appears to be backwards compatibility in reading old ROOT files containing the Phi class. However, a scan of CMSSW finds no cases where Phi variables were written to ROOT files, so it doesn't look like the ability to read the old version of Phi variables in ROOT files is even needed.

No backport is needed.

cmsbuild · 2019-03-20T22:05:04Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-03-20T22:10:53Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26227/8851

This PR adds an extra 20KB to repository

cmsbuild · 2019-03-20T22:11:18Z

A new Pull Request was created by @cvuosalo (Carl Vuosalo) for master.

It involves the following packages:

DataFormats/Math

@cmsbuild, @perrotta, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @rovere 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

cvuosalo · 2019-03-20T22:17:46Z

@ianna: Please review and let us know if you approve.
@bsunanda: FYI

cvuosalo · 2019-03-20T22:26:28Z

@VinInn, @fwyzard: If you have time and would like to comment, we would appreciate your thoughts on the following questions. For angle values that need to be kept in the range [0, 2pi), what coding approach best balances reliability, developer convenience, and performance? Does this PR achieve this balance, or could it be improved?

cvuosalo · 2019-03-20T22:26:59Z

@cmsbuild please test

cmsbuild · 2019-03-20T22:27:24Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33694/console Started: 2019/03/20 23:27

cmsbuild · 2019-03-21T01:31:48Z

+1
Tested at: 533a9ee
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26227/33694/summary.html

cmsbuild · 2019-03-21T01:31:53Z

Comparison job queued.

cmsbuild · 2019-03-21T03:00:47Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26227/33694/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: 3114829
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3114631
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

VinInn · 2019-03-21T07:30:40Z

Sorry, there are already N version of it.
What's wrong with
https://cmssdt.cern.ch/dxr/CMSSW/source/DataFormats/Math/interface/deltaPhi.h#14
?

cmsbuild · 2019-05-15T13:58:56Z

+1
Tested at: 3265dea
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e3a6a9/218/summary.html

cmsbuild · 2019-05-15T13:59:01Z

Comparison job queued.

cmsbuild · 2019-05-15T17:22:20Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e3a6a9/218/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 5766 differences found in the comparisons
DQMHistoTests: Total files compared: 33
DQMHistoTests: Total histograms compared: 3212004
DQMHistoTests: Total failures: 7765
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3203905
DQMHistoTests: Total skipped: 334
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
Checked 137 log files, 14 edm output root files, 33 DQM output files

cvuosalo · 2019-05-16T03:06:23Z

As before, the comparison test differences are almost entirely in two Phase 2 simulation workflows which have different random sequences in the SIM step. These differences are not due to this PR.

I think all issues in this PR have been addressed, so I hope it can be soon approved.

cvuosalo · 2019-05-16T03:06:33Z

+1

perrotta · 2019-05-16T08:50:57Z

+1

Changes since last reco signature in Adding angle utilities #26227 (comment) are consequential to the review comments arrived later on
Worries about possible backward incompatibilities seem can be dissipated by the checks in Adding angle utilities #26227 (comment)

fabiocos · 2019-05-20T12:28:32Z

@civanch @kpedro88 do you have further comments?

civanch · 2019-05-21T17:44:31Z

+1

kpedro88 · 2019-05-22T19:18:08Z

+upgrade

cmsbuild · 2019-05-22T19:19:27Z

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)

kpedro88 · 2019-05-28T16:58:02Z

+1

slava77 · 2019-07-31T23:17:00Z

DataFormats/Math/interface/deltaPhi.h

+  // calling fmod to improve performance.
+
+  template <class valType>
+  inline constexpr valType make0To2pi(valType angle) {


commenting here instead of in #27637

about angle0to2pi
IMHO, it would be more consistent to move it up in this file to the reco namespace and rename to reduceRange0To2pi
Also, if it's implementation is really faster, replace the current reduceRange of [-pi,pi] with the faster variant.

@slava77 and @cvuosalo - can we think of other then reco namespace? It would be confusing if the DD code uses reco::

Concerning the possible replacement of reduceRange: Since make0To2pi checks for floating-point value drift over repeated calculations and reduceRange does not, there is a good possibility that comparison tests for replacing reduceRange would show small differences. make0To2pi should provide more correct results, but it would be required to show that the comparison test differences were correct, appropriate, and desirable, which might be a challenge. I did not replace reduceRange in this PR for this very reason.

Perhaps replacement could be done on a step-by-step basis by developers familiar with the applications getting changed so that the comparison differences would be easier to understand.

I'd suggest putting everything in the file in the delta_phi namespace to match the header file name, but I don't have a strong opinion about namespace naming.

About the small differences, it should be acceptable, considering the expected speedup and improved numerical precision (unless perhaps a PR comes after the deadline with an "urgent" tag and some unrelated changes inside).

cvuosalo added 2 commits March 19, 2019 23:58

Adding utilities for constraining angles to range 0 to 2pi

5dea682

Fix formatting

533a9ee

cmsbuild added this to the CMSSW_10_6_X milestone Mar 20, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Mar 20, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 20, 2019

cvuosalo closed this Mar 20, 2019

cvuosalo reopened this Mar 20, 2019

cmsbuild added tests-started and removed tests-pending labels Mar 20, 2019

cmsbuild added tests-approved and removed tests-started labels Mar 21, 2019

cmsbuild added comparison-available and removed comparison-pending labels Mar 21, 2019

cmsbuild added tests-approved and removed tests-started labels May 15, 2019

cmsbuild added comparison-available and removed comparison-pending labels May 15, 2019

cmsbuild added geometry-approved and removed geometry-pending labels May 16, 2019

cmsbuild added reconstruction-approved and removed reconstruction-pending labels May 16, 2019

cmsbuild added simulation-approved and removed simulation-pending labels May 21, 2019

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels May 22, 2019

cmsbuild added orp-approved and removed orp-pending labels May 28, 2019

cmsbuild merged commit 1007a44 into cms-sw:master May 28, 2019

slava77 mentioned this pull request Jul 31, 2019

Define and use cms units #27637

Merged

slava77 reviewed Jul 31, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding angle utilities #26227

Adding angle utilities #26227

cvuosalo commented Mar 20, 2019 •

edited

cmsbuild commented Mar 20, 2019

cmsbuild commented Mar 20, 2019

cmsbuild commented Mar 20, 2019

cvuosalo commented Mar 20, 2019

cvuosalo commented Mar 20, 2019

cvuosalo commented Mar 20, 2019

cmsbuild commented Mar 20, 2019 •

edited

cmsbuild commented Mar 21, 2019

cmsbuild commented Mar 21, 2019

cmsbuild commented Mar 21, 2019

VinInn commented Mar 21, 2019

cmsbuild commented May 15, 2019

cmsbuild commented May 15, 2019

cmsbuild commented May 15, 2019

cvuosalo commented May 16, 2019

cvuosalo commented May 16, 2019

perrotta commented May 16, 2019

fabiocos commented May 20, 2019

civanch commented May 21, 2019

kpedro88 commented May 22, 2019

cmsbuild commented May 22, 2019

kpedro88 commented May 28, 2019

slava77 Jul 31, 2019

ianna Aug 1, 2019

cvuosalo Aug 1, 2019

cvuosalo Aug 1, 2019

slava77 Aug 1, 2019

Adding angle utilities #26227

Adding angle utilities #26227

Conversation

cvuosalo commented Mar 20, 2019 • edited

PR description:

PR validation:

cmsbuild commented Mar 20, 2019

cmsbuild commented Mar 20, 2019

cmsbuild commented Mar 20, 2019

cvuosalo commented Mar 20, 2019

cvuosalo commented Mar 20, 2019

cvuosalo commented Mar 20, 2019

cmsbuild commented Mar 20, 2019 • edited

cmsbuild commented Mar 21, 2019

cmsbuild commented Mar 21, 2019

cmsbuild commented Mar 21, 2019

VinInn commented Mar 21, 2019

cmsbuild commented May 15, 2019

cmsbuild commented May 15, 2019

cmsbuild commented May 15, 2019

cvuosalo commented May 16, 2019

cvuosalo commented May 16, 2019

perrotta commented May 16, 2019

fabiocos commented May 20, 2019

civanch commented May 21, 2019

kpedro88 commented May 22, 2019

cmsbuild commented May 22, 2019

kpedro88 commented May 28, 2019

slava77 Jul 31, 2019

Choose a reason for hiding this comment

ianna Aug 1, 2019

Choose a reason for hiding this comment

cvuosalo Aug 1, 2019

Choose a reason for hiding this comment

cvuosalo Aug 1, 2019

Choose a reason for hiding this comment

slava77 Aug 1, 2019

Choose a reason for hiding this comment

cvuosalo commented Mar 20, 2019 •

edited

cmsbuild commented Mar 20, 2019 •

edited