Specify the tolerance in each new ES6 Math test

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
mozilla26
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
...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.
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

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.
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!
(Assignee)

Comment 6

5 years ago
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) {
         ...
(Assignee)

Comment 7

5 years ago
Created attachment 790219 [details] [diff] [review]
v2
Attachment #774775 - Attachment is obsolete: true
Attachment #790219 - Flags: review?(jdemooij)
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?
Attachment #790219 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/a45ee4ead45f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.