improve precision in jacobianCurvilinearToCartesian #34103
Conversation
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34103/23294
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34103/23295
|
|
A new Pull Request was created by @slava77 (Slava Krutelyov) for master. It involves the following packages: DataFormats/GeometryVector @cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
| @@ -36,10 +36,8 @@ AlgebraicMatrix65 jacobianCurvilinearToCartesian(const GlobalVector& momentum, i | |||
| R(5, 5) = 1.; | |||
|
|
|||
| double p = pvec.mag(), p2 = p * p; | |||
There was a problem hiding this comment.
p2 = pvec.mag2(), overp = 1./sqrt(p2);
may be more precise and avoids one division below;
one may compute overpt to avoid one more division....
|
since when the jacobians moved to DataFormat???? |
it was done in #5753 |
Time flies and memory fades... |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c75df9/15935/summary.html Comparison SummarySummary:
|
|
+1 few differences are classified as failing results but histograms are identical. |
|
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
some changes are expected. The updated code is not numerically identical (it's somewhat implicitly in the title of the PR). I looked at the plots and the differences are small, as expected. |
|
+1 |
the transformation from
q/p, lambda, phi -> p{x,y,z}depends significantly on precision for high momentum tracks. While most of the math in thejacobianCurvilinearToCartesianis in double, the initial step in the momentum space relied on float precision atan to get the polar and azimuth angles, which is extracted only to immediately call sincos.The angle extraction is not necessary considering that the input momentum is in Cartesian coordinates.
Tested on 1000 muons generated in [0.5 ,1000] GeV range, comparing full double precision implementation with other alternatives at the state conversion of tracking seeds: the number of converted covariances with a fractional change larger than 1e-3 in any of the cov matrix elements goes down by a factor of just over 100.
A more precise alternative is to convert the input momentum from
GlobalVector(float precision) to a double-precision version. This, however has a slightly larger computational cost.@mtosi @VinInn @vmariani @mmusich