Floating point precision not preserved when converting floats

[daniel-shuy]

Indriya retains floating point precision when converting doubles, but not floats.
eg.

System.out.println(Quantities.getQuantity(3.524, Units.GRAM).to(Units.KILOGRAM).getValue());

prints 0.003524, but

System.out.println(Quantities.getQuantity(3.524f, Units.GRAM).to(Units.KILOGRAM).getValue());

prints 0.0035239999294281006.

[andi-huber]

Hi @daniel-shuy - why is that an issue?

[daniel-shuy]

That's because the precision is not preserved. It should return 0.003524, like how it behaves with doubles.

DefaultNumberSystem uses BigDecimal internally to preserve floating point precision when converting doubles. It attempts to do the same for float, by converting to double. Unfortunately Float#doubleValue() does not preserve floating point precision.

I've created a PR (#331) to fix this.

[keilw]

@daniel-shuy according to https://jcp.org/en/participation/members/S you don't seem to be a JCP member, would you be willing to join because otherwise we cannot merge this PR?

[andi-huber]

@daniel-shuy - by reading into your PR, I can see you are converting a float (32bit IEEE 754) into its rounded decimal representation (thats the toString conversion) and then parse it with a BigDecimal, which represents floating point numbers in decimal notation. With this step you are already loosing precision. I believe you are chasing a red herring here. I cannot see any flaws in our current NumberSystem implementation.

If you think there is a bug, please demonstrate the issue with a test case.

[andi-huber]

I know that's a bit of a puzzler but eg. 0.1f or even 0.1d in Java does not represent the exact number 0.1, meaning (one tenth in decimal representation)!

[daniel-shuy]

@andi-huber well to be more exact, we actually don't want to gain precision (eg. we want 3.524 / 1000 to return 0.003524, not 0.0035239999294281006), let me correct my PR description.

You can test the behavior with this:

var number = Float.valueOf(3.524f);
System.out.println(BigDecimal.valueOf(number.doubleValue()));  // 3.5239999294281006
System.out.println(new BigDecimal(number.toString()));  // 3.524

But good point, I'll add some test cases.

[keilw]

+1 it would be nice to back a changed behavior (but also the existing one) with more JUnit tests.
While the test coverage of the API project is around 80% (could still be better but even at corporate clients that is usually decent) Indriya only got 60% so far, thus it could be better wherever people are willing to help.

[andi-huber]

My humble advice: please have a short excursion into floating point number representation according to IEEE 754, before writing any supposedly fixing code, which we are not going to merge.

[keilw]

@andi-huber Also added you as reviewer for the PR, if there are functional problems please discuss it there, I think maybe it could be better to call it a Draft PR for now, WDTY?
Changed to draft so it's not accidentially merged. Thanks also @desruisseaux for the additional input. If either of you feel the PR was unnecessary, feel free to close (at least @desruisseaux should be able to close it, not sure about Associate members but for most part I guess every contributor has the same right to close issues or PRs)

[desruisseaux]

Converting an IEEE 754 number with toString(), then parsing as a BigDecimal is the strategy applied by the JDK in BigDecimal.valueOf(double) method. That method is currently implemented as below:

    public static BigDecimal valueOf(double val) {
        return new BigDecimal(Double.toString(val));
    }

So replacing BigDecimal.valueOf(number.doubleValue()) by new BigDecimal(number.toString()) would give the same results and take in account the various types of Number (long, float, double, etc.). Note in particular that current Indriya implementation losts precision for the long type too (conversion from long to double is not always lossless), while the toString() representation avoid that.

Currently it is not even necessary to do a (number instanceof Double) check. Unless BigDecimal.valueOf(double) implementation is changed in future, just using unconditionally the toString() approach will work as well.

[daniel-shuy]

So replacing BigDecimal.valueOf(number.doubleValue()) by new BigDecimal(number.toString()) would give the same results

@desruisseaux unfortunately, when you pass a Float to BigDecimal.valueOf(double), the Float is unboxed and cast to a double, which has the same problem as Float#doubleValue():

public double doubleValue() {
    return (double)value;
}

Try running this code and you'll see what I mean:

var number = Float.valueOf(3.524f);
System.out.println(BigDecimal.valueOf(number));  // 3.5239999294281006
System.out.println(new BigDecimal(number.toString()));  // 3.524

[desruisseaux]

@daniel-shuy yes I know, that sentence was specifically about the Double case (it was not clear in my comment). I was trying to said that if the code use the toString() representation of Double type, there is no difference with BigDecimal.valueOf(double) current implementation. Consequently there is no need for the if (number instanceof Double) special case.

[daniel-shuy]

@desruisseaux ah sorry I misunderstood, that's a good point, that will greatly simplify the code, thanks!