Add sin,cos,floor,floorf,scalbn,copysign,copysignf #15

odlg · 2017-09-12T19:49:42Z

This adds sin, cos and dependencies.
I have seen some inaccuracies when using the libm comparison. For instance:
sin(-1_f32) should be equal to -0.84147096_f32
but when libm comparison is used it is -0.83888906_f32. Debugging points to doing a f64 as f32 introduces this.

When using these sin and cos functions in clean rust environments results are OK.

japaric

Thanks for the PR, @odlg.

This looks good to me but it's going to be very hard to land this if the results don't always match libm results though.

The discrepancy between the results doesn't seem to be small either -- I ran the test suite locally and some results are way off when the inputs are large. Though probably we shouldn't be testing functions like sin and cos for inputs beyond -2PI and 2PI.

sin(-1_f32) should be equal to -0.84147096_f32

It's weird that the result is off for that input. Could this be a bug in the openlibm implementation? libm seems to get the mathematically correct result.

P.S. It's ... interesting that openlibm computes sinf using double precision. I expect that will be heavyweight to compute on 32-bit architectures, specially ones without a FPU (e.g. microcontrollers).

japaric · 2017-09-19T20:29:33Z

src/ll.rs

+ unimplemented!()
+}
+
+


nit: extra newline

japaric · 2017-09-19T20:30:13Z

src/ll.rs

+}
+
+
+pub fn cosdf(x: f64) -> f32 {


I think this doesn't need to be public.

japaric · 2017-09-19T20:30:22Z

src/ll.rs

+}
+
+
+pub extern "C" fn ieee754_rem_pio2f(x: f32) -> (i32, f64) {


I think this doesn't need to be public.

japaric · 2017-09-19T20:30:30Z

src/ll.rs

+}
+
+
+pub extern "C" fn kernel_rem_pio2(x: [f64; 1], e0: i32, nx: i32, prec: i32) -> (i32, [f64; 3]) {


I think this doesn't need to be public.

japaric · 2017-09-19T20:31:29Z

src/ll.rs

+ (x.repr() & 0x7fffffffffffffff) | (y.repr() & 0x8000000000000000),
+ )
+}
+


nit: extra newline

japaric · 2017-09-19T20:31:44Z

src/ll.rs

+ assert_eq!(super::floor(-14207526.52111395f64), -14207527f64);
+ }
+
+


nit: extra newline

odlg · 2017-10-03T18:28:25Z

I have fixed some porting bugs, results are much better now.

I guess the minor differences are caused by comparing different libm implementations. At least reading JuliaMath/openlibm#30 makes me think so.

At one point I got this error running the tests:
---- ll::tests::sinf stdout ---- sinf - Args: x = -0.4352397 (0xbeded7bd) us: -0.421628 (0xbed7dfa0) libm: -0.42162776 (0xbed7df98) thread 'll::tests::sinf' panicked at '[quickcheck] TEST FAILED. Arguments: (-0.4352397 (0xbeded7bd))', /home/od/.cargo/registry/src/github.com-1ecc6299db9ec823/quickcheck-0.4.1/src/tester.rs:147:27

Making C openlibm doing this gives the same result as the rust port of openlibm. So:
Rust openlibm:
sinf(-0.4352397) = -0.421628
C openlibm:
sinf(-0.4352397) = -0.421628
glibc libm on my linux:
sinf(-0.4352397) = -0.42162776

japaric · 2017-11-04T17:11:54Z

@odlg sorry for taking so long to get back to this. I tried increasing the tolerance of the approximate equality comparison to 100 ULP (see patch bellow)

             #[cfg(test)]
             fn eq_repr(self, rhs: Self) -> bool {
+                const TOLERANCE_ULP: $repr_ty = 100;
+
                 if self.is_nan() && rhs.is_nan() {
                     true
                 } else {
                     let (lhs, rhs) = (self.repr(), rhs.repr());

-                    lhs == rhs || (lhs > rhs && lhs - rhs == 1) ||
-                    (rhs > lhs && rhs - lhs == 1)
+                    lhs.wrapping_sub(rhs) <= TOLERANCE_ULP
                 }
             }

but I got some "errors" in the order of 1000 ULPs or higher when randomly testing sinf and scalbn. Here's one such error:

---- ll::tests::sinf stdout ----
sinf - Args: x = -0.00390625 (0xbb800000)
  us:   -0.0039183237 (0xbb806548)
  libm: -0.00390624 (0xbb7fffd5)

0xbb806548 - 0xbb7fffd5 = 25971 ULPs

Any chance there could still be bugs in the sinf and scalbn implementations?

I also tried your sinf discrepancy example but I get the same result from libm and from libopenm on a amd64 machine:

//#include <openlibm.h>
#include <math.h>
#include <stdio.h>

int main() {
  float x = -0.4352397;
  float y = sinf(x);

  printf("%.*e\n", y);
}

returns -4.216278e-01 for libm and -4.216278e-01 for openlibm. So, the same value.

I do agree that testing against openlibm would be ideal but it'll probably be complicated to get that C / asm code building for all the test targets. OTOH, we could reuse that build infrastructure to provide a C fall back for this crate (i.e. if asinf is not implemented in Rust you'll be able to use openlibm's C implementation through FFI).

japaric · 2017-11-04T17:20:25Z

Here's one such error:

And I'm getting sinf(-0.00390625) = -3.906240e-03 from both libm and openlibm on amd64.

odlg · 2017-11-08T16:53:09Z

I think I was partly fooled by differences between C printf("%f\n", y); and rust println!("{}", y);
Here are some more bug fixes, test does not find differences anymore. I have seen a single wrapping error by automatic testing in floor. It is also fixed.

japaric · 2017-11-08T18:01:26Z

Neat! Let's see what homu thinks.

@homunkulus r+

homunkulus · 2017-11-08T18:01:27Z

📌 Commit 0b78d9b has been approved by japaric

homunkulus · 2017-11-08T18:01:34Z

⌛ Testing commit 0b78d9b with merge a331420...

Add sin,cos,floor,floorf,scalbn,copysign,copysignf This adds sin, cos and dependencies. I have seen some inaccuracies when using the libm comparison. For instance: `sin(-1_f32) should be equal to -0.84147096_f32` but when libm comparison is used it is -0.83888906_f32. Debugging points to doing a f64 as f32 introduces this. When using these sin and cos functions in clean rust environments results are OK.

homunkulus · 2017-11-08T18:09:58Z

💔 Test failed - status-appveyor

odlg · 2017-11-09T15:52:10Z

@japaric Any ideas on what to do about this? Limit the range of which sinf and cosf are tested?
I am afraid we seeing different results from different libm implementations and thereby making it hard to add any functions to rust m lib as the results has to match any other libm implementation.

japaric · 2017-11-10T11:34:31Z

Limit the range of which sinf and cosf are tested?

Yes, to a few PIs around zero. (I think I may have mentioned this above)

I am afraid we seeing different results from different libm implementations and thereby making it hard to add any functions to rust m lib as the results has to match any other libm implementation.

It's OK to increase the tolerance (e.g. using the patch above) as long as the allowed tolerance is "reasonable". I'm not sure what would be reasonable but a difference of hundreds of ULPs between us and libm is likely indicating some bug (assuming the input is also reasonable).

homunkulus · 2017-11-15T23:31:54Z

⌛ Testing commit 0b78d9b with merge d25260a...

Add sin,cos,floor,floorf,scalbn,copysign,copysignf This adds sin, cos and dependencies. I have seen some inaccuracies when using the libm comparison. For instance: `sin(-1_f32) should be equal to -0.84147096_f32` but when libm comparison is used it is -0.83888906_f32. Debugging points to doing a f64 as f32 introduces this. When using these sin and cos functions in clean rust environments results are OK.

homunkulus · 2017-11-15T23:57:02Z

💔 Test failed - status-travis

Add sin,cos,floor,floorf,scalbn,copysign,copysignf This adds sin, cos and dependencies. I have seen some inaccuracies when using the libm comparison. For instance: `sin(-1_f32) should be equal to -0.84147096_f32` but when libm comparison is used it is -0.83888906_f32. Debugging points to doing a f64 as f32 introduces this. When using these sin and cos functions in clean rust environments results are OK.

homunkulus · 2017-11-30T10:48:10Z

⌛ Testing commit 0b78d9b with merge f3e4a5a...

homunkulus · 2017-11-30T10:56:35Z

💔 Test failed - status-appveyor

homunkulus · 2018-01-12T20:39:35Z

⌛ Testing commit 0b78d9b with merge 8f1241c...

Add sin,cos,floor,floorf,scalbn,copysign,copysignf This adds sin, cos and dependencies. I have seen some inaccuracies when using the libm comparison. For instance: `sin(-1_f32) should be equal to -0.84147096_f32` but when libm comparison is used it is -0.83888906_f32. Debugging points to doing a f64 as f32 introduces this. When using these sin and cos functions in clean rust environments results are OK.

homunkulus · 2018-01-12T20:46:50Z

💔 Test failed - status-appveyor

Add sin,cos,floor,floorf,scalbn,copysign,copysignf This adds sin, cos and dependencies. I have seen some inaccuracies when using the libm comparison. For instance: `sin(-1_f32) should be equal to -0.84147096_f32` but when libm comparison is used it is -0.83888906_f32. Debugging points to doing a f64 as f32 introduces this. When using these sin and cos functions in clean rust environments results are OK.

homunkulus · 2018-01-13T16:10:18Z

⌛ Testing commit 0b78d9b with merge b28ab67...

homunkulus · 2018-01-13T16:12:20Z

💔 Test failed - status-travis

odlg · 2018-01-21T12:58:12Z

@japaric Results are better now - one platform fails on doc-tests. Do you have an idea?

japaric · 2018-07-14T22:02:55Z

This crate has been deprecated in favor of the libm crate. This repository will be archived.

odlg added 4 commits September 12, 2017 21:03

Add sin,cos,floor,floorf,scalbn,copysign,copysignf

11ee969

Fix warnings

a1f5a8c

Use same settings

15c6bb0

Fix warnings

5c50df7

japaric reviewed Sep 19, 2017

View reviewed changes

Fix porting bugs

7b73b9c

odlg added 2 commits October 3, 2017 20:49

Make internal functions non public

b796865

Avoid allow unused_assignments

4ac351e

Bug fixes

0b78d9b

odlg added 6 commits January 15, 2018 19:00

Add tolerance for different libm

ac42fec

Attempt to make travis run

b0e694e

Upgrade quickcheck

17fa07e

Handle negative errors also

a3a7442

Limit range for sinf/cosf

f14e140

Limit range for sinf/cosf

ccfee52

japaric closed this Jul 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sin,cos,floor,floorf,scalbn,copysign,copysignf #15

Add sin,cos,floor,floorf,scalbn,copysign,copysignf #15

odlg commented Sep 12, 2017

japaric left a comment

japaric Sep 19, 2017

japaric Sep 19, 2017

japaric Sep 19, 2017

japaric Sep 19, 2017

japaric Sep 19, 2017

japaric Sep 19, 2017

odlg commented Oct 3, 2017 •

edited

Loading

japaric commented Nov 4, 2017

japaric commented Nov 4, 2017

odlg commented Nov 8, 2017

japaric commented Nov 8, 2017

homunkulus commented Nov 8, 2017

homunkulus commented Nov 8, 2017

homunkulus commented Nov 8, 2017

odlg commented Nov 9, 2017

japaric commented Nov 10, 2017

homunkulus commented Nov 15, 2017

homunkulus commented Nov 15, 2017

homunkulus commented Nov 30, 2017

homunkulus commented Nov 30, 2017

homunkulus commented Jan 12, 2018

homunkulus commented Jan 12, 2018

homunkulus commented Jan 13, 2018

homunkulus commented Jan 13, 2018

odlg commented Jan 21, 2018

japaric commented Jul 14, 2018

		}


		pub extern "C" fn ieee754_rem_pio2f(x: f32) -> (i32, f64) {

		}


		pub extern "C" fn kernel_rem_pio2(x: [f64; 1], e0: i32, nx: i32, prec: i32) -> (i32, [f64; 3]) {

		assert_eq!(super::floor(-14207526.52111395f64), -14207527f64);
		}

Add sin,cos,floor,floorf,scalbn,copysign,copysignf #15

Add sin,cos,floor,floorf,scalbn,copysign,copysignf #15

Conversation

odlg commented Sep 12, 2017

japaric left a comment

Choose a reason for hiding this comment

japaric Sep 19, 2017

Choose a reason for hiding this comment

japaric Sep 19, 2017

Choose a reason for hiding this comment

japaric Sep 19, 2017

Choose a reason for hiding this comment

japaric Sep 19, 2017

Choose a reason for hiding this comment

japaric Sep 19, 2017

Choose a reason for hiding this comment

japaric Sep 19, 2017

Choose a reason for hiding this comment

odlg commented Oct 3, 2017 • edited Loading

japaric commented Nov 4, 2017

japaric commented Nov 4, 2017

odlg commented Nov 8, 2017

japaric commented Nov 8, 2017

homunkulus commented Nov 8, 2017

homunkulus commented Nov 8, 2017

homunkulus commented Nov 8, 2017

odlg commented Nov 9, 2017

japaric commented Nov 10, 2017

homunkulus commented Nov 15, 2017

homunkulus commented Nov 15, 2017

homunkulus commented Nov 30, 2017

homunkulus commented Nov 30, 2017

homunkulus commented Jan 12, 2018

homunkulus commented Jan 12, 2018

homunkulus commented Jan 13, 2018

homunkulus commented Jan 13, 2018

odlg commented Jan 21, 2018

japaric commented Jul 14, 2018

odlg commented Oct 3, 2017 •

edited

Loading