892671 - Specify the tolerance in each new ES6 Math test

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

...so that we know how sloppy the implementations are. The current tests are too lenient and would not detect significant regressions in precision.

There is, too, room for improvement. acosh and asinh are quite sloppy on Windows. So is cbrt. By contrast, hypot is fine everywhere.

I have a patch.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

Comment 2

[Luke Wagner [:luke]]

On a related note:

V8 has a flag:

  --fast_math (faster (but maybe less accurate) math functions)
        type: bool  default: true

I was just encouraging the Ion guys to look into this and see if we should be trading precision for performance in the same places as well.

Comment 3

[4esn0k]

note:

java guarantees 1 ulp for many of the Math.* functions
> http://docs.oracle.com/javase/7/docs/api/java/lang/Math.html
> The computed result must be within 1 ulp of the exact result.

Comment 4

[Jason Orendorff [:jorendorff]]

David has a patch to improve the precision of many of these functions (see bug 717379). I need to land the test changes first.

Ideally, we would always give the one true answer, which is to say, "the Number value for" the mathematical result, as specified in ES5.1 section 8.5. Then there would be no need for tolerances at all; we could use assertEq().

https://people.mozilla.com/~jorendorff/es5.1-final.html#sec-8.5

Realistically, it stands to reason there'd be a precision/performance tradeoff, and precision also trades off with other stuff I could be working on.

Comment 5

[André Bargull [:anba]]

Can you make assertNear() always fail when the actual value is NaN? This would have caught this typo in my code -> https://github.com/anba/es6draft/commit/f67bbc9a06ca7d22c9cb5b289a9c4e9aac6deec0 . Thanks!

Comment 6

[Jason Orendorff [:jorendorff]]

Added:

     var assertNear = function assertNear(a, b, tolerance=1) {
         if (!Number.isFinite(b)) {
             fail("second argument to assertNear (expected value) must be a fini...
+        } else if (Number.isNaN(a)) {
+            fail("got NaN, expected a number near " + b);
         } else if (a === 1/0 || a === -1/0) {
         ...

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v2

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 790219 [details] [diff] [review]
v2

Review of attachment 790219 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming you did some sanity checks to make sure assertNear definitely fails if we return some bogus value..

::: js/src/tests/ecma_6/Math/shell.js
@@ +55,5 @@
> +        if (!Number.isFinite(b)) {
> +            fail("second argument to assertNear (expected value) must be a finite number");
> +        } else if (Number.isNaN(a)) {
> +            fail("got NaN, expected a number near " + b);
> +        } else if (a === 1/0 || a === -1/0) {

Can we use !Number.isFinite(a) here, now that Number.isNaN(a) is handled explicitly?

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> r=me assuming you did some sanity checks to make sure assertNear definitely
> fails if we return some bogus value..

Oh yes. I had more try server sanity checking than I wanted.

> > +        } else if (Number.isNaN(a)) {
> > +            fail("got NaN, expected a number near " + b);
> > +        } else if (a === 1/0 || a === -1/0) {
> 
> Can we use !Number.isFinite(a) here, now that Number.isNaN(a) is handled
> explicitly?

Yup.

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to Jason Orendorff [:jorendorff] from comment #9)
> Oh yes. I had more try server sanity checking than I wanted.

I would say my sanity was tested very thoroughly.

Comment 11

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a45ee4ead45f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a45ee4ead45f