Closed Bug 892671 Opened 7 years ago Closed 7 years ago

Specify the tolerance in each new ES6 Math test

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file, 1 obsolete file)

...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.
Attached patch WIP 1 (obsolete) — Splinter Review
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.
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.
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!
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) {
         ...
Attached patch v2Splinter Review
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+
(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.
(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
Closed: 7 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.