329: Retain floating point precision when converting floats #331

daniel-shuy · 2021-02-16T17:21:47Z

Fixes #329

DefaultNumberSystem uses BigDecimal to preserve floating point precision for double arithmetic, but Float#doubleValue() does not preserve floating point precision, leading to precision gain for float arithmetic.

Added RationalNumber#of(float) to replace RationalNumber.of(float.doubleValue())
Convert Float to BigDecimal using new BigDecimal(float.toString()) instead of BigDecimal.valueOf(float.doubleValue()) (to preserve precision)

This change is

keilw · 2021-02-16T17:28:46Z

The cast of (float)number looks a little ugly, I thought Number still has a floatValue() method, or was that dropped recently?
@daniel-shuy you are not a JCP member at least on an Associate level, are you?
Because as mentioned in the Contribution-Guide the RI unlike most other modules requires a JCP agreement.

daniel-shuy · 2021-02-16T17:32:20Z

The cast of (float)number looks a little ugly, I thought Number still has a floatValue() method, or was that dropped recently?

I agree, I originally wanted to use Number#floatValue() as well as it is safer, but used (float)number because the existing code was using (double)number. I'll gladly change it if you wish.

@daniel-shuy you are not a JCP member at least on an Associate level, are you?
Because as mentioned in the Contribution-Guide the RI unlike most other modules requires a JCP agreement.

Ah, sorry, wasn't aware. I'll apply for a membership.

keilw

Aside from the cast which could be improved a bit, please @daniel-shuy join the JCP as Associate member if you can. Ask @andi-huber who also is an Associate member if you need help. Will keep this open for now, it does not sound ultra-critical for the upcoming 2.1.2 release of Indriya but happy to merge once the JCP membership is sorted (hoping there are no merge conflicts;-)

Float#doubleValue() does not preserve floating point precision

daniel-shuy · 2021-02-16T20:32:20Z

I've applied the suggestions and added tests

keilw · 2021-04-17T23:23:33Z

@daniel-shuy How is your JCP membership going?

daniel-shuy · 2021-04-19T05:04:09Z

@keilw I discussed with my employer and they wanted to join the JCP as an organization, so I was waiting for it to be approved before I can register under my employer. The organization membership has been approved, now I just need to register as an individual and request member association.

Sorry, I've been putting it off because there hasn't been any activity in the discussion since (#332).

daniel-shuy · 2021-04-19T09:57:29Z

@keilw If I am joining the JCP under an organization, do I have to ask my employer to apply for membership to the JSR or can I do that individually?

keilw · 2021-04-19T10:13:06Z

@daniel-shuy No, Associate Membership works without asking your employer, see https://jcp.org/en/participation/membership the two bottom options. There is currently no cost for either, but if your employer is not interested to get involved then Associate sounds best and we have plenty of Associate members who contribute the same or sometimes even more than other Full members.

daniel-shuy · 2021-04-19T10:49:39Z

@keilw my employer wants to get involved, as I am contributing back to libraries that are used by the company. I've joined the JCP under my employer, and I've submitted a nomination to join the JSR, thanks!

Task-Url: #335

andi-huber · 2021-04-19T12:43:32Z

Please understand, I don't see a bug with the current implementation.

By all means, please, if you find an issue other than that, which is subject to the discussion (#332), of course I'll have a closer look.

daniel-shuy · 2021-04-19T14:10:09Z

@andi-huber yes, we've established its not a bug, but a potential improvement to make it less confusing for users of the library.

If not, I propose adding a note to warn users of the library that converting floats between different units can result in additional "noise" (while it is technically more accurate, the end user may not know that, and to them it is noise).

keilw · 2021-04-19T15:37:05Z

@daniel-shuy Float is usually less precise see https://stackoverflow.com/questions/27598078 I don't think we need to put any note for what is common knowledge and reason. About the PR @andi-huber please have a look if any of the proposed changes would do harm, e.g. creating RationalNumber from a float as opposed to a double could perform better or anything? Others like creating BigDecimal from a string sound reasonable because valueOf(double) also takes the string of a new Double object so you might as well use the string directly. About joining the JCP and EG (which technically does not exist in Maintenance mode, but there were others accepting contributors in that phase) if none of us finds a real problem in this PR then I also see no reason to delay or reject the request to join the JSR because some also make many contributions (especially @andi-huber) while others probably contribute just a few lines once or twice if they are too busy so even with a relatively small PR let's start and hopefully you enjoy it and find more time to support it.

andi-huber

This PR was intended under the premise of cutting off float's at their decimal representation accuracy limit. If that were our goal, I'd consider this contribution at its most parts valid.

But unfortunately the suggested changes break with the current scheme that is to properly extend floats to double before converting them to any higher precision container.

andi-huber · 2021-04-19T17:52:18Z

src/main/java/tech/units/indriya/function/DefaultNumberSystem.java

        }
        if(number instanceof Float) {
-            return RationalNumber.of(number.doubleValue()).reciprocal();
+            return RationalNumber.of(number.floatValue()).reciprocal();


I'd rather not introduce a RationalNumber.of(float) for the reasons discussed in (#332).

andi-huber · 2021-04-19T17:56:24Z

src/main/java/tech/units/indriya/function/DefaultNumberSystem.java

@@ -615,7 +615,7 @@ private BigDecimal toBigDecimal(Number number) {
            return BigDecimal.valueOf(number.longValue());
        }
        if(number instanceof Double || number instanceof Float) {
-            return BigDecimal.valueOf(number.doubleValue());
+            return new BigDecimal(number.toString());


I'd rather not convert floats to String and then to BigDecimal if the general scheme (as up for discussion in #332) is to first properly extend floats to double before converting to BigDecimal. If we agree on this scheme we should be consistent with it throughout the entire implementation.

andi-huber · 2021-04-19T17:57:50Z

src/main/java/tech/units/indriya/function/DefaultNumberSystem.java

@@ -668,7 +668,7 @@ private Number addWideAndNarrow(
            }

            if(narrow instanceof Double || narrow instanceof Float) {
-                return ((BigDecimal) wide).add(BigDecimal.valueOf(narrow.doubleValue()), Calculus.MATH_CONTEXT);
+                return ((BigDecimal) wide).add(new BigDecimal(narrow.toString()), Calculus.MATH_CONTEXT);


dito: consistency

andi-huber · 2021-04-19T17:58:06Z

src/main/java/tech/units/indriya/function/DefaultNumberSystem.java

-            return BigDecimal.valueOf(wide.doubleValue())
-                .add(BigDecimal.valueOf(narrow.doubleValue()));
+            return new BigDecimal(wide.toString())
+                .add(new BigDecimal(narrow.toString()));


dito: consistency

andi-huber · 2021-04-19T18:06:32Z

src/main/java/tech/units/indriya/function/RationalNumber.java

+	 *
+	 * @param number
+	 */
+	public static RationalNumber of(float number) {


As mentioned above, breaks the general scheme of first extending floats to double before converting them to BigDecimal or Rational.

andi-huber · 2021-04-19T18:16:53Z

src/main/java/tech/units/indriya/function/DefaultNumberSystem.java

        }

        if(narrow instanceof RationalNumber) {
            //TODO[220] can we do better than that, eg. by converting BigDecimal to RationalNumber
-            return BigDecimal.valueOf(wide.doubleValue())
+            return new BigDecimal(wide.toString())


At this point wide is potentially a float, so don't break consistency.

andi-huber · 2021-04-19T18:17:22Z

src/main/java/tech/units/indriya/function/DefaultNumberSystem.java

                    .multiply(((RationalNumber) narrow).bigDecimalValue());
        }

        if(narrow instanceof BigInteger) {
-            return BigDecimal.valueOf(wide.doubleValue())
+            return new BigDecimal(wide.toString())


dito: consistency

andi-huber · 2021-04-19T18:18:07Z

src/main/java/tech/units/indriya/function/DefaultNumberSystem.java

                    .multiply(new BigDecimal((BigInteger) narrow));
        }

        // at this point we know, that 'narrow' is one of {(Atomic)Long, (Atomic)Integer, Short, Byte}
-        return BigDecimal.valueOf(wide.doubleValue())
-                .multiply(BigDecimal.valueOf(narrow.longValue()));              
+        return new BigDecimal(wide.toString())


At this point wide is potentially a float, so don't break consistency.

andi-huber · 2021-04-19T18:18:27Z

src/main/java/tech/units/indriya/function/DefaultNumberSystem.java

@@ -821,7 +826,7 @@ private int compareWideVsNarrow(
            }

            if(narrow instanceof Double || narrow instanceof Float) {
-                return ((BigDecimal) wide).compareTo(BigDecimal.valueOf(narrow.doubleValue()));
+                return ((BigDecimal) wide).compareTo(new BigDecimal(narrow.toString()));


dito: consistency

andi-huber · 2021-04-19T18:18:36Z

src/main/java/tech/units/indriya/function/DefaultNumberSystem.java

@@ -837,22 +842,23 @@ private int compareWideVsNarrow(
        // at this point we know, that wide is one of {Double, Float}

        if(narrow instanceof Double || narrow instanceof Float) {
-            return Double.compare(wide.doubleValue(), narrow.doubleValue());
+            return new BigDecimal(wide.toString())
+			        .compareTo(new BigDecimal(narrow.toString()));


dito: consistency

andi-huber · 2021-04-19T18:34:32Z

If there is enough momentum, we surely can think of providing different NumberSystem implementations optimized for different use-cases, so users have options. But I'd rather have the default one, not optimized for usability or performance but rather for fail-safety.

andi-huber · 2021-04-19T19:21:14Z

Hi @daniel-shuy - on a personal note:

These technical discussions are sometimes time consuming, exhausting and have a tendency toward frustration, especially if precious spare time is going into it. In that context, I want to be clear, that you are very welcome as a new contributor.

daniel-shuy · 2021-04-20T12:26:14Z

@andi-huber thanks for taking the time to review the PR. After the discussion, I'm actually in agreement to not change the current behavior, due to the corner cases. I was actually trying to push for adding a note to warn users of the behavior, but as @keilw pointed out above, if a user of the library wants accuracy, then they shouldn't be using float in the first place.

I'm happy to drop this issue, but I'd still be glad to join the JSR, as I have other improvements I would like to contribute back.

keilw · 2021-04-20T12:53:52Z

@daniel-shuy Thanks a lot for the update and understanding. We're happy to accept your invitation to the JSR and EG as a contributor anticipating further activity or improvement. For the number systems @andi-huber mentioned, please also see this discusison item #328. It is not fully fleshed out with comments to actually vote on, but the idea is to use this new GH feature and offer 2 or 3 candidates like Commons Number or Decimal4J and implement either one or multiple plugins to replace the built-in RationalNumber where a different NumberSystem is required.

daniel-shuy mentioned this pull request Feb 16, 2021

Floating point precision not preserved when converting floats #329

Closed

keilw self-requested a review February 16, 2021 17:41

keilw requested changes Feb 16, 2021

View reviewed changes

keilw requested a review from andi-huber February 16, 2021 18:10

keilw marked this pull request as draft February 16, 2021 18:13

keilw requested a review from desruisseaux February 16, 2021 18:47

daniel-shuy added 4 commits February 17, 2021 04:24

Add tests

903a961

329: Retain floating point precision when converting floats

a05c13b

Float#doubleValue() does not preserve floating point precision

Use Number#doubleValue()/floatValue() instead of cast

faa269f

Remove unnecessary instanceof Double check

c0a5e9a

daniel-shuy force-pushed the bugfix/retain-precision-when-converting-floats branch from 88925bd to c0a5e9a Compare February 16, 2021 20:24

Simplify Double + Float multiplication

d05aff2

keilw referenced this pull request Apr 19, 2021

335: Improve and fix JavaDoc

8b70fd3

Task-Url: #335

andi-huber reviewed Apr 19, 2021

View reviewed changes

keilw closed this Apr 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

329: Retain floating point precision when converting floats #331

329: Retain floating point precision when converting floats #331

daniel-shuy commented Feb 16, 2021 •

edited

Loading

keilw commented Feb 16, 2021

daniel-shuy commented Feb 16, 2021

keilw left a comment

daniel-shuy commented Feb 16, 2021

keilw commented Apr 17, 2021

daniel-shuy commented Apr 19, 2021

daniel-shuy commented Apr 19, 2021

keilw commented Apr 19, 2021 •

edited

Loading

daniel-shuy commented Apr 19, 2021 •

edited

Loading

andi-huber commented Apr 19, 2021

daniel-shuy commented Apr 19, 2021

keilw commented Apr 19, 2021 •

edited

Loading

andi-huber left a comment

andi-huber Apr 19, 2021

andi-huber Apr 19, 2021

andi-huber Apr 19, 2021

andi-huber Apr 19, 2021

andi-huber Apr 19, 2021

andi-huber Apr 19, 2021

andi-huber Apr 19, 2021

andi-huber Apr 19, 2021

andi-huber Apr 19, 2021

andi-huber Apr 19, 2021

andi-huber commented Apr 19, 2021

andi-huber commented Apr 19, 2021

daniel-shuy commented Apr 20, 2021 •

edited

Loading

keilw commented Apr 20, 2021

329: Retain floating point precision when converting floats #331

329: Retain floating point precision when converting floats #331

Conversation

daniel-shuy commented Feb 16, 2021 • edited Loading

keilw commented Feb 16, 2021

daniel-shuy commented Feb 16, 2021

keilw left a comment

Choose a reason for hiding this comment

daniel-shuy commented Feb 16, 2021

keilw commented Apr 17, 2021

daniel-shuy commented Apr 19, 2021

daniel-shuy commented Apr 19, 2021

keilw commented Apr 19, 2021 • edited Loading

daniel-shuy commented Apr 19, 2021 • edited Loading

andi-huber commented Apr 19, 2021

daniel-shuy commented Apr 19, 2021

keilw commented Apr 19, 2021 • edited Loading

andi-huber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andi-huber commented Apr 19, 2021

andi-huber commented Apr 19, 2021

daniel-shuy commented Apr 20, 2021 • edited Loading

keilw commented Apr 20, 2021

daniel-shuy commented Feb 16, 2021 •

edited

Loading

keilw commented Apr 19, 2021 •

edited

Loading

daniel-shuy commented Apr 19, 2021 •

edited

Loading

keilw commented Apr 19, 2021 •

edited

Loading

daniel-shuy commented Apr 20, 2021 •

edited

Loading