Use constant for 180/π in to_degrees #47919

varkor · 2018-02-01T01:23:38Z

The current f32|f64.to_degrees implementation uses a division to calculate 180/π, which causes a loss of precision. Using a constant is still not perfect (implementing a maximally-precise algorithm would come with a high performance cost), but improves precision with a minimal change.

As per the discussion in #29944, this fixes #29944 (the costs of improving the precision further would not outweigh the gains).

rust-highfive · 2018-02-01T01:23:43Z

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

shepmaster · 2018-02-01T02:05:30Z

r? @rkruppe

you seemed in favor of this.

shepmaster · 2018-02-01T02:06:10Z

src/libcore/num/f32.rs

@@ -239,7 +239,9 @@ impl Float for f32 {
    /// Converts to degrees, assuming the number is in radians.
    #[inline]
    fn to_degrees(self) -> f32 {
-        self * (180.0f32 / consts::PI)
+        // Use a constant for better precision.
+        let pis_in_180 = 57.2957795130823208767981548141051703_f32;


Why not an actual const?

Yeah, that makes more sense stylistically.

shepmaster · 2018-02-01T02:06:40Z

src/libcore/num/f64.rs

@@ -237,7 +237,9 @@ impl Float for f64 {
    /// Converts to degrees, assuming the number is in radians.
    #[inline]
    fn to_degrees(self) -> f64 {
-        self * (180.0f64 / consts::PI)
+        // Use a constant for better precision.
+        let pis_in_180 = 57.2957795130823208767981548141051703_f64;


Shouldn't there be more digits here than in f32?

Both literals actually have more digits than necessary: they both have 36 significant figures, which was chosen to be consistent with the existing definitions of mathematical constants (for both f32 and f64).

hanna-kruppe · 2018-02-01T08:52:59Z

Curiously, f64::to_degrees already passes the test without this change and 180.0 / f64::consts::PI is exactly equal to the exact value given in #29944 -- which is of course double rounded (since it's a transcendental reduced to a finite decimal string and then converted to a short binary float). I wonder whether that double rounding is significant and we just need a different test case to exhibit the problem, or whether it means we don't need this change for f64::to_degrees at all.

varkor · 2018-02-01T10:23:42Z

I think in this case, double rounding should not be an issue, because the precision to which the number is rounded in decimal form is significantly higher than the floating-point precision. I imagine the change isn't actually necessary for f64, but probably makes sense for consistency.

hanna-kruppe · 2018-02-01T11:12:46Z

Double rounding can be an issue regardless of what intermediate precision you round to. Suppose the full binary expansion of a number looks like this:

...0100000100...
    ^  ^
    A  B

If we first round (half-to-even) to position B and then to position A, the result ends in 01. If we round to A directly without double rounding, we round up and the result ends in 10. This can happen regardless of how far apart A and B are, it just requires that the bits (A+1)...B are zeros.

varkor · 2018-02-01T14:15:35Z

If we truncate/round to B and then try flipping the last bit, if there's no difference in the value when rounded to A (before and after flipping the bit), then we can determine the first rounding caused no loss of precision relative to rounding to A, because regardless of the bits following B, the result would still be the same. Am I right in thinking this? This would suggest that the approximation in the value of the f32/f64 constants are as close to the correct value of 180/π as possible, even taking it being transcendental into consideration.

hanna-kruppe · 2018-02-01T16:41:55Z

I'm not quite sure if I understand correctly, but if you produce anything other than ...0100000000..., you're not rounding to bit B. So whatever you're describing, if it arrives at the correctly rounded result (...1000000000...), then it's not double rounding, i.e., it's not properly rounding to some precision and then afterwards rounding again to some smaller precision.

But this is a theroetical discussion. The actual interesting part is whether the f64 produced from the decimal literal in this code is the correct rounding of the actual value of 180/pi. I'm rather busy now so I don't know when I'll have the time to verify that.
@nagisa, @est31: does one of you have the tools and time to verify?

varkor · 2018-02-01T17:08:04Z

I guess my real question is that, this code compiles:

fn main() {
    let pis_in_180_below = 57.29577951308232087679815_f64;
    let pis_in_180_above = 57.29577951308232087679816_f64;
    // pis_in_180_below < pis_in_180 < pis_in_180_above (in terms of their literals)
    assert_eq!(pis_in_180_below, pis_in_180_above);
}

so assuming the floating-point literal parser is well-behaved, then any literal value (including a hypothetically-infinite sequence) between these two values is equal to the constant in this code. This assumes that the parser always produces the closest floating-point number to the literal provided (which seems like a reasonable assumption, given that Rust even allows literals with more precision than f64 recognises). However if that is true, then this constant is correct.

nagisa · 2018-02-01T17:08:45Z

In both previous and new code, the division is evaluated during the construction of LLVM-IR and obtains following representations (the hexidecimal form used is whatever the IEEE standard specifies):

f32 division = 0x404CA5DC00000000
f32 constant = 0x404CA5DC20000000
f64 division = 0x404CA5DC1A63C1F8
f64 constant = 0x404CA5DC1A63C1F8

So at least for f64, the result is equivalent.

hanna-kruppe · 2018-02-01T17:36:02Z

@nagisa Thanks! But I believe that just confirms what was observed earlier in this thread? The tricky question is whether you'd also get double 0x404CA5DC1A63C1F8 if you took the transcendental real numner 180/PI and rounded it to f64 precision directly.

@varkor's last comment, however, settles the question with less effort than what I had in mind. I'm convinced now the dedicated constant is both correct and unnecessary for f64.

hanna-kruppe · 2018-02-01T17:38:21Z

src/libcore/num/f64.rs

@@ -237,7 +237,9 @@ impl Float for f64 {
    /// Converts to degrees, assuming the number is in radians.
    #[inline]
    fn to_degrees(self) -> f64 {
-        self * (180.0f64 / consts::PI)
+        // Use a constant for better precision.
+        const PIS_IN_180: f64 = 57.2957795130823208767981548141051703_f64;


You mentioned consistency as reason to have the constant here as well but I'd rather not have an opaque magic number duplicated for no functional reason. I think it's better to leave the code in place and add a comment saying that 180.0f64 / consts::PI is correctly rounded.

... and probably call out that this differs from f32::to_degrees.

Okay, sure 👍

hanna-kruppe · 2018-02-01T18:17:29Z

Great, thanks! r=me after squashing

ranma42 · 2018-02-01T18:31:49Z

Would it make sense to compute the f64 constant and convert it to f32 in the f32 impl?
This would avoid having a magic number in the code.

The current `f32|f64.to_degrees` implementation uses a division to calculate 180/π, which causes a loss of precision. Using a constant is still not perfect (implementing a maximally-precise algorithm would come with a high performance cost), but improves precision with a minimal change.

hanna-kruppe · 2018-02-01T18:36:52Z

@ranma42 It's possible that this doesn't give the right result (because it's double rounding differently than the ways examined in previously in this discussion, which turned out to be fine) and it doesn't seem easier to verify that it gives the right result than to verify that the constant is correct.

hanna-kruppe · 2018-02-01T18:37:38Z

@bors r+

bors · 2018-02-01T18:37:39Z

📌 Commit e34c31b has been approved by rkruppe

kennytm · 2018-02-01T18:51:43Z

@bors rollup

…ppe Use constant for 180/π in to_degrees The current `f32|f64.to_degrees` implementation uses a division to calculate `180/π`, which causes a loss of precision. Using a constant is still not perfect (implementing a maximally-precise algorithm would come with a high performance cost), but improves precision with a minimal change. As per the discussion in rust-lang#29944, this fixes rust-lang#29944 (the costs of improving the precision further would not outweigh the gains).

Rollup of 10 pull requests - Successful merges: #46156, #47829, #47842, #47898, #47914, #47916, #47919, #47942, #47951, #47973 - Failed merges: #47753

rust-lang#47919 regressed some test cases for `f32::to_degrees`, while improving others. This change satisfies all previous test cases and should be more accurate than both previous implementations. The `f32` to `f64` cast is lossless, `f64::to_degrees` should provide more accuracy than a native `f32::to_degrees`, and conversion back to `f32` will be as accurate a conversion as possible. It can be hard to reason about floating-point accuracy, but given that this passes all the tests, I feel confident it's an improvement.

rust-highfive assigned shepmaster Feb 1, 2018

shepmaster reviewed Feb 1, 2018

View reviewed changes

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 1, 2018

hanna-kruppe suggested changes Feb 1, 2018

View reviewed changes

varkor force-pushed the to_degrees-precision branch 2 times, most recently from d07c26e to febba80 Compare February 1, 2018 17:48

varkor force-pushed the to_degrees-precision branch from febba80 to e34c31b Compare February 1, 2018 18:36

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2018

kennytm mentioned this pull request Feb 2, 2018

Rollup of 10 pull requests #47962

Merged

bors added a commit that referenced this pull request Feb 3, 2018

Auto merge of #47962 - kennytm:rollup, r=kennytm

8d04b8f

Rollup of 10 pull requests - Successful merges: #46156, #47829, #47842, #47898, #47914, #47916, #47919, #47942, #47951, #47973 - Failed merges: #47753

bors merged commit e34c31b into rust-lang:master Feb 3, 2018

varkor deleted the to_degrees-precision branch February 3, 2018 15:28

This was referenced Feb 28, 2018

Various improvements to FloatCore rust-num/num-traits#41

Merged

f32::FRAC_PI_6.to_degrees() is now less accurate #48617

Closed

varkor mentioned this pull request Feb 28, 2018

Fix an accuracy regression in f32::to_degrees #48622

Closed

cuviper mentioned this pull request Apr 10, 2018

Use constant for 180/π in f32::to_degrees rust-num/num-traits#61

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use constant for 180/π in to_degrees #47919

Use constant for 180/π in to_degrees #47919

varkor commented Feb 1, 2018

rust-highfive commented Feb 1, 2018

shepmaster commented Feb 1, 2018

shepmaster Feb 1, 2018

varkor Feb 1, 2018

shepmaster Feb 1, 2018

varkor Feb 1, 2018

hanna-kruppe commented Feb 1, 2018

varkor commented Feb 1, 2018

hanna-kruppe commented Feb 1, 2018 •

edited

Loading

varkor commented Feb 1, 2018 •

edited

Loading

hanna-kruppe commented Feb 1, 2018

varkor commented Feb 1, 2018 •

edited

Loading

nagisa commented Feb 1, 2018

hanna-kruppe commented Feb 1, 2018

hanna-kruppe Feb 1, 2018

hanna-kruppe Feb 1, 2018

varkor Feb 1, 2018

hanna-kruppe commented Feb 1, 2018

ranma42 commented Feb 1, 2018

hanna-kruppe commented Feb 1, 2018

hanna-kruppe commented Feb 1, 2018

bors commented Feb 1, 2018

kennytm commented Feb 1, 2018

Use constant for 180/π in to_degrees #47919

Use constant for 180/π in to_degrees #47919

Conversation

varkor commented Feb 1, 2018

rust-highfive commented Feb 1, 2018

shepmaster commented Feb 1, 2018

shepmaster Feb 1, 2018

Choose a reason for hiding this comment

varkor Feb 1, 2018

Choose a reason for hiding this comment

shepmaster Feb 1, 2018

Choose a reason for hiding this comment

varkor Feb 1, 2018

Choose a reason for hiding this comment

hanna-kruppe commented Feb 1, 2018

varkor commented Feb 1, 2018

hanna-kruppe commented Feb 1, 2018 • edited Loading

varkor commented Feb 1, 2018 • edited Loading

hanna-kruppe commented Feb 1, 2018

varkor commented Feb 1, 2018 • edited Loading

nagisa commented Feb 1, 2018

hanna-kruppe commented Feb 1, 2018

hanna-kruppe Feb 1, 2018

Choose a reason for hiding this comment

hanna-kruppe Feb 1, 2018

Choose a reason for hiding this comment

varkor Feb 1, 2018

Choose a reason for hiding this comment

hanna-kruppe commented Feb 1, 2018

ranma42 commented Feb 1, 2018

hanna-kruppe commented Feb 1, 2018

hanna-kruppe commented Feb 1, 2018

bors commented Feb 1, 2018

kennytm commented Feb 1, 2018

hanna-kruppe commented Feb 1, 2018 •

edited

Loading

varkor commented Feb 1, 2018 •

edited

Loading

varkor commented Feb 1, 2018 •

edited

Loading