Relax a few unittests for double-precision reals by kinke · Pull Request #5346 · dlang/phobos

kinke · 2017-04-23T22:12:51Z

A loss-less roundtrip (to string <=> parse) apparently isn't guaranteed when using double-precision reals.

kinke · 2017-04-23T22:14:42Z

The issues were encountered with LDC for MSVC and ARM targets (double-precision reals).

quickfur · 2017-04-25T18:37:26Z

LGTM. But I'll defer to someone with stronger grasp of floating-point semantics.

tsbockman · 2017-04-25T22:03:27Z

Related: #4345

The round trip should be lossless; if it's not, why not fix it instead of relaxing the tests? Probably one or both of the conversion routines just isn't using enough decimal digits:

People generally assume that ceil(log10(pow(2.0, double.mant_dig))) == 16 is enough for a lossless round-trip, but actually ceil(log(pow(2.0, double.mant_dig - 1)) / log(10.0) + 1) == 18 are needed.

tsbockman · 2017-04-25T22:08:46Z

std/json.d

        const json0 = JSONValue(num0);
        const num1 = to!double(toJSON(json0));
-        version(Win32)
+        static if (realInDoublePrecision)


By doing this you may be relaxing the test on platforms where it currently works, inviting regressions.

Since when does DMD cover 64-bit reals? Most new Phobos releases come with a few new tests that fail exactly because they're not tested with 64-bit reals, so I keep on upstreaming relaxed unittests for LDC which actually supports and tests targets with double-precision reals.

If you consider the potential being-off-by-1-ulp as bug (for std.format and/or std.json?), the Win32 exception shouldn't have been here already in the first place. I'm just extending that exception to its true scope here.

Is there a list online somewhere of which platforms have is(double == real)?

Afaik, we use it for the 32/64-bit MSVC targets (i.e., on Windows), PowerPC and 32-bit ARM and possibly others, although real is still a distinct type (mangling).

I run these tests on Android/ARM, which is one of the platforms where real is 64-bit, and can confirm that this one only fails because of variability in the last digit, and that this relaxation patch gets it to pass again.

Is there a list online somewhere of which platforms have is(double == real)?

It would be easier to list those that don't. x86 (80-bit) and PPC (64-bit pair) for sure, I think IA64 (128-bit) is in that list too. Everyone else is a platform where real == double.

AArch64 has 128-bit quadruple reals. The PowerPC doubledouble is implemented in software AFAIK, that's why we use double-precision reals for PowerPC.

tsbockman · 2017-04-25T22:10:30Z

std/json.d

+    static if (realInDoublePrecision)
+        assert(test(-double.max / 2));
+    else
+        assert(test(-double.max));


What is the result of this test when it fails? What consequences could the failure have for the API user?

Answering my own question (from Relax std.json unittest some more): "test(-double.max) results in an overflow and thrown exception in std.conv.parse!(double, string)."

I guess that's not too bad; at least it doesn't just silently corrupt data.

This may have been a more severe issue (it's been a while since I relaxed them for LDC), I need to recheck.
Edit: Thanks for checking, didn't see your 2nd post before.

tsbockman · 2017-04-25T23:48:45Z

The failure of test(-double.max) is caused by a logic bug in std.conv.parse which is not platform dependant. (I can reproduce it on my AMD64 machine with DMD just by replacing real with double in a few places.)

Please give me a chance to fix the root cause rather than weakening the std.json unittest.

kinke · 2017-04-26T08:27:53Z

Please give me a chance to fix the root cause rather than weakening the std.json unittest.

;) Sure, thanks!

tsbockman · 2017-05-17T01:10:58Z

Unfortunately, fixing std.conv.parse turned out to be more trouble than I expected.

For decimal, getting a lossless round-trip without the use of a higher precision type seems difficult to impossible. (It should generally be within ~1 bit, though.)
The double.max problem is fixable, but it seems to me that a principled solution requires manipulating the rounding mode, which I cannot get working reliably across all three major compilers.
The whole function is kind of a mess, and should probably be re-written. I have done this, but I don't want to delay this trivial PR indefinitely waiting for it to be reviewed and merged. (Especially since I haven't even decided whether to submit my rewrite, at all.)

Given the above, I recommend that we go ahead and merge this as-is. (Sorry for the delay.)

joakim-noah · 2017-05-18T19:23:34Z

ping, looks like this is ready to go.

JackStouffer

LGTM

JackStouffer · 2017-05-24T20:08:13Z

@kinke Can you please rebase onto master so that github picks up the fact that jenkins is no longer required for a merge?

A loss-less roundtrip (to string <=> parse) apparently isn't guaranteed when using double-precision reals.

This was referenced Apr 23, 2017

Merge 2.074 front-end + libs ldc-developers/ldc#2076

Merged

Android and ARM regressions in master 1.2 ldc-developers/ldc#2024

Closed

quickfur added math Review:Needs Review labels Apr 25, 2017

tsbockman reviewed Apr 25, 2017

View reviewed changes

JackStouffer approved these changes May 24, 2017

View reviewed changes

JackStouffer added the Merge:auto-merge label May 24, 2017

kinke force-pushed the fixReal64 branch from 5362a0b to 69fd492 Compare May 24, 2017 21:50

dlang-bot added Severity:Bug Fix and removed Merge:auto-merge labels May 24, 2017

Relax a few unittests for double-precision reals

13ca4dd

A loss-less roundtrip (to string <=> parse) apparently isn't guaranteed when using double-precision reals.

kinke force-pushed the fixReal64 branch from 69fd492 to 13ca4dd Compare May 24, 2017 21:52

JackStouffer added Merge:auto-merge and removed Review:Needs Review labels May 24, 2017

dlang-bot merged commit 62a7e7c into dlang:stable May 24, 2017

kinke deleted the fixReal64 branch May 24, 2017 22:48

Uh oh!

Comments

Conversation

kinke commented Apr 23, 2017

Uh oh!

kinke commented Apr 23, 2017

Uh oh!

quickfur commented Apr 25, 2017

Uh oh!

tsbockman commented Apr 25, 2017

Uh oh!

tsbockman Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsbockman Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

joakim-noah Apr 26, 2017

Choose a reason for hiding this comment

Uh oh!

ibuclaw Apr 26, 2017

Choose a reason for hiding this comment

Uh oh!

kinke Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsbockman Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

tsbockman Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

kinke Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsbockman commented Apr 25, 2017

Uh oh!

kinke commented Apr 26, 2017

Uh oh!

tsbockman commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joakim-noah commented May 18, 2017

Uh oh!

JackStouffer left a comment

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented May 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tsbockman Apr 25, 2017 •

edited

Loading

kinke Apr 25, 2017 •

edited

Loading

tsbockman Apr 25, 2017 •

edited

Loading

kinke Apr 26, 2017 •

edited

Loading

kinke Apr 25, 2017 •

edited

Loading

tsbockman commented May 17, 2017 •

edited

Loading