remove sincosd #60
remove sincosd #60
Conversation
As a side effect also remove fmod (as now it is deadcode)
| } | ||
|
|
||
| /// Compute sine and cosine of x in degrees | ||
| pub fn sincosd(x: f64) -> (f64, f64) { |
There was a problem hiding this comment.
This repository was originally ported from the python version of geographiclib, but is looking more and more like the cpp version over time, especially as we've done performance related work.
Because I'm not as knowledgeable in the domain as the upstream author (Charles Karney), I almost always prefer code that looks like theirs. They've put a lot of thought into robustness and error checking.
With your PR, I see impressive perf improvements (5-10%), but if I leave the original method in tact, and replace only the sin/cos calls with a call to sin_cos, I see no measurable wins at all, which suggests to me all the perf wins we're getting are from deleting the error/robustness checks. I'm not sure that's a good idea.
That said, our existing impl looks pretty far from the cpp version. This repository was originally a port of the python implementation, but has grown to look more like the cpp one as time has gone on. It's possible we could get some perf wins by making our sincosd look more like the cpp version.
There was a problem hiding this comment.
Looking a little deeper, it looks like even the original version already compiles down to sincos in asm.
There was a problem hiding this comment.
but if I leave the original method in tact, and replace only the sin/cos calls with a call to sin_cos, I see no measurable wins at all, which suggests to me all the perf wins we're getting are from deleting the error/robustness checks. I'm not sure that's a good idea.
This probably has to do with inlined vs not inlined.
There was a problem hiding this comment.
My guess is that it's a specific compiler optimization like this: https://bugs.llvm.org/show_bug.cgi?id=13204
There was a problem hiding this comment.
I agree that minor changes in final values for major operations (like direct or inverse) using infinite or NaN values doesn't feel too concerning, though there's some elegance value in them coming out to exactly 0.
Inputs very close to +/-180° feel more likely to matter, since inputs in that range may be quite common. One of the things I struggled with when I was working on this project was the issue of subtle differences in behavior of Rust vs C++ trig operations. I never solidly pinned down whether the differences were due to one sometimes managing to tease out a little more precision than the other, or whether it's just different noise and both are basically random at the point where they differ. The same sort of question comes up for the very small behavior changes described here. As @michaelkirk says, though, my personal impulse is to stick as close to the Karney implementation as possible, except in cases where it's very clear that we should do otherwise. Unfortunately, I'm unlikely to get enough time to gain any real insight into whether the behavior changes here are positive, negative, or neutral.
Regardless of how @michaelkirk calls it, this PR is certainly an interesting experiment. Thank you for trying it out.
There was a problem hiding this comment.
I've made an alternative PR at #61. The perf gains are smaller, but it passes all the same tests as the original code.
There was a problem hiding this comment.
sin/cos calls with a call to sin_cos
Because of sin_cos implementation is just call of sin and cos: https://doc.rust-lang.org/src/std/f64.rs.html#785-787 . The idea is that llvm optimize it in proper way. But sometimes llvm can not do the same, as direct sincos call: llvm/llvm-project#76152
|
No worries. |
As a side effect also remove fmod (as now it is deadcode)
CHANGES.mdif knowledge of this change could be valuable to users.Two Regressions. One minor & One Major
When solving the direct problem, bad values
f64::INFINITE&f64::NANwon't result in the output being0, only extremely close to0. Not really worried about this, sort of falls in the case of "garbage in, garbage out".1/50k of the GeodTest.dat cases fails 😱 (by 6.9e-7 the % error is 3e-12). This (AFAIT) comes down to quadrant normalization. It down is seems that
sincosddiffers from.to_radians().sin_cos()by~1e-16when dealing with values extremely close to +/-180°.In my local testing
It is amusing at the
atan2of the 2 values is correct to withine-16, but just not the sin/cos.Technically a regression, by nearly the smallest order (I'm not joking, there are only 2 64bit floating point values between the two results). I'll let project management judge if that is deal breaking.