fix issue 15885 - float serialized to JSON loses precision

[ghost]

In toJSON�, a float is converted to a string using to!string(). A best approach is to use the "%.17g" specifier with format(). With this specifier, the string returned may include non signifiant digits but the original value can be retrieved.

Reference for this fix.

[dlang-bot]

Fix	Bugzilla	Description
✓	15885	float serialized to JSON loses precision

[tsbockman]

This will cause a massive loss of precision, because val here is always a 64-bit double. (See the definition of JSONValue starting on line 100.)

I notice also that most of the unit tests below are improperly written to test with float values instead of double.

[ghost]

oops, I thought the internal storage was of type float.

The problem is that if I use "%.17g", some tests don't pass. "%16.g" is OK.

[tsbockman]

If all that is needed is another digit or two, I think we should fix std.conv.to!string(double) itself, rather than changing toJSON(). Surely JSON isn't the only use case that needs precise double to string to double round trips?

EDIT: I still think std.conv.to!string(double) should be made precise, but having experimented with the code I see it will be a non-trivial - and possibly controversial - PR. So, let's do the easy fix here first.

[tsbockman]

The problem is that if I use "%.17g", some tests don't pass. "%16.g" is OK.

When adding more precision breaks things, something is wrong. (This has nothing to do with the big argument about excess precision on the forums, because there is a binary to decimal conversion involved.)

The first step here is to write better tests, then we can figure out how to fix toJSON() and/or std.conv.to!string(double) so that they pass.

[ghost]

Actually %.16g is enough since 53 (fracp num bits) * 2 log10 = 15.9545897702.
Trying to match to the representation of %.17g is not reasonable.

[tsbockman]

Actually %.16g is enough since 53 (fracp num bits) * 2 log10 = 15.9545897702.

Yes, 16 decimal digits should be enough. EDIT: Actually, it's not. See below.

But, you need to undo the changes you made to these lines. This PR is not ready until it can pass the original tests.

To 16 digits of precision, 0.23 is the correct double representation of 0.23. The values you wrote above are truncated to float precision, which is not right.

[tsbockman]

Actually %.16g is enough since 53 (fracp num bits) * 2 log10 = 15.9545897702.

Upon further study, this turns out to be wrong. The correct formula is actually this:

const allDig = cast(int)ceil(log(pow(2.0L, F.mant_dig - 1)) / log(10.0L) + 1);
assert(allDig == (F.dig + 2));

The proof:

module example;

import std.conv, std.format, std.math, std.meta, std.stdio;

void main()
{
    foreach (F; AliasSeq!(real, double, float))
    {
        writeln(F.stringof);

        const allDig = cast(int)ceil(log(pow(2.0L, F.mant_dig - 1)) / log(10.0L) + 1);
        assert(allDig == (F.dig + 2));
        writefln("\tallDig: %s", allDig);

        enum F mostDecDig = 1 + F.epsilon;
        // 16 for double: FAIL
        assert(to!F(format("%." ~ to!string(F.dig) ~ "g", mostDecDig)) != mostDecDig);
        // 17 for double: FAIL
        assert(to!F(format("%." ~ to!string(F.dig + 1) ~ "g", mostDecDig)) != mostDecDig);
        // 18 for double: PASS
        enum goodSpec = "%." ~ to!string(F.dig + 2) ~ "g";
        static if (!is(F == real)) // to!real(string) is not precise!
            assert(to!F(format(goodSpec, mostDecDig)) == mostDecDig);

        // This is a very subtle issue; even the D frontend gets it wrong!
        assert(mostDecDig != mixin(mostDecDig.stringof));
        writefln("\t" ~ goodSpec ~ " != %s", mostDecDig, mostDecDig.stringof);
    }
}

DPaste: https://dpaste.dzfl.pl/1bd14e5c3f83

[tsbockman]

This PR is not ready until it can pass the original tests.

I'm going to retract that statement; now that I understand the actual precision requirements, I think it's better to be lossless than to keep the old unit tests. (After all - they were labelled as overly simple by the author.)

@bbasile I have submitted a pull request to you making toJSON() fully precise for double and modifying the tests accordingly: https://github.com/BBasile/phobos/pull/1

[ghost]

@tsbockman: the tests don't pass anymore (on windows) since I've merged your pull. Either you can fix that, either I come back to %.16g or I close. Your call.

[tsbockman]

Your call.

I will fix it, but how about you just close this anyway and I'll open a new PR. That way you don't have to act as a go-between for me and the auto-tester.