329: Retain floating point precision when converting floats

[daniel-shuy]

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]

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]

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.

[daniel-shuy]

I've applied the suggestions and added tests

[keilw]

@daniel-shuy How is your JCP membership going?

[daniel-shuy]

@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]

@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]

@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]

@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!

[andi-huber]

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]

@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]

@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]

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]

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]

@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]

@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.