<Swing Dev> [15] RFR JDK-8220811: SpinnerNumberModel floating point rounding issue

[Pankaj Bansal]

Hello Sergey,
Thanks for the review.

I have created a Test.java file for the points I have mentioned below.
http://cr.openjdk.java.net/~pbansal/8220811/Test.java

<< Could you please try to use Math.fma instead of direct using of xxx.toString+BigDecimal
I tried using this, but Math.fma will not solve our issue of precision here. Math.fma internally creates BigDecimal, but it uses the [1] type constructor to create the BigDecimal. The [1] constructor takes the primitive double value and it tries to represent the primitive double as precisely as possible and adds the extra precision. After the calculations in fma, the BigDecimal is rounded off to create double, but as it has very high precision, the issue in spinner is still there.
In my changes in webrev00, I have used the [2] form of constructor, which does not add extra precision and works fine for our case.
I have created a dummy patch if you would like to try out this change with Math.fma (http://cr.openjdk.java.net/~pbansal/8220811/dummy.patch)

[1] new BigDecimal(double d)
[2] new BigDecimal(String s)
https://stackoverflow.com/questions/7186204/bigdecimal-to-use-new-or-valueof



<< Are you sure that it is necessary to add a new constructor? As far as I understand it is possible to pass a float value via:
In java the implicit type promotion (auto-widening) is preferred over the auto boxing/unboxing. The SpinnerNumberModel has two constructors, one accepts all primitive "double" arguments [4] and other accepts Objects [3]. Now when SpinnerNumberModel is passed primitive "float" values, it looks for constructor which accepts all primitive floats. As such constructor is not available, it checks for alternatives. It has two alternates, either box the primitive "float" into object "Float" and call [3] or widen the primitive "float" to primitive  "double". As auto widening is preferred over auto boxing, [4] is called instead of [3]. As I mentioned in previous mail, implicitly casting primitive  float to primitive  double adds precision issues
So either we need to add the constructor which accepts "float", or we can remove the constructor which accepts "double". Both will work fine. I just decided to add float one.

There are few other wrong things in the class which are not causing any issue as of now. There is one constructor which accepts primitive int [5]. So if we use, primitives byte, short, int, it will call [5]. But if we try to use primitive "long", it will end up calling double constructor [4] as long can't be passes to int and as auto widening is preferred over auto boxing. It unnecessarily adds floating point calculations for long type. 
I think we should remove both [4] and [5] and keep only [3]. This should work for all the cases. But I am not sure if we would be removing some constructor, so I did not propose this.

[3]     public SpinnerNumberModel(Number value,
                               Comparable<?> minimum,
                               Comparable<?> maximum,
                               Number stepSize)
[4] public SpinnerNumberModel(double value, double minimum, double maximum, double stepSize) 
[5] public SpinnerNumberModel(int value, int minimum, int maximum, int stepSize)

Regards,
Pankaj

-----Original Message-----
From: Sergey Bylokhov 
Sent: Tuesday, February 11, 2020 7:47 AM
To: Pankaj Bansal <pankaj.b.bansal at oracle.com>; swing-dev at openjdk.java.net
Subject: Re: <Swing Dev> [15] RFR JDK-8220811: SpinnerNumberModel floating point rounding issue

Hi, Pankaj.

Could you please try to use Math.fma instead of direct using of xxx.toString+BigDecimal
  
if (value instanceof Double) {
     newValue = Math.fma(stepSize.doubleValue(), dir, value.doubleValue()); } else {
     newValue = Math.fma(stepSize.floatValue(), dir, value.floatValue()); }

Are you sure that it is necessary to add a new constructor? As far as I understand it is possible to pass a float value via:

public SpinnerNumberModel(Number value,
                           Comparable<?> minimum,
                           Comparable<?> maximum,
                           Number stepSize) {

On 2/7/20 12:25 am, Pankaj Bansal wrote:
> Hi All,
> 
> Please review the following fix for jdk15.
> 
> 
> Bug:
> 
> https://bugs.openjdk.java.net/browse/JDK-8220811
> 
> webrev:
> 
> http://cr.openjdk.java.net/~pbansal/8220811/webrev00/
> 
> Issue:
> 
> In case of JSpinner with double/float values,  sometime the spinner value does not change even though the value is within the min, max range.
> 
> Cause:
> 
> The bug is caused by errors in floating point math.
> 
> Eg, if we create a JSpinner with min=0.15, max=1.0, stepsize =.05, if current value is -.10, it is not possible to go to -.15 by pressing the decrement button.
> 
> a=-.10, b=-.05, the c=a+b is not equal to -.15. Instead is something like -.150000000345. This caused issues as this values is considered lower than -.15, which minimum value allowed for the JSpinner. So the value of spinner cannot be decreased to -.15, though it should be possible.
> 
> Fix:
> 
> The fix is different for double and float values.
> 
> For double values, just using the BigDecimal class to do the floating point math operations solves the issues. This change is needed for float values as well along with the change mentioned below.
> 
> For float, there is one addition issue. There is no constructor in SpinnerNumberModel, which will accept float values. There is a constructor which accepts double values. So, even if all float values are passed to SpinnerNumberModel, the constructor accepting double values is called. So, the float values are implicitly casted to double. This implicit casting causes issue because if float a=.95, double b = a, then b is not exactly equal to .95. Instead it is something like .950000045. So in case of float values, the issue starts on from the creation of SpinnerNumberModel. So a new constructor for float values is added to the SpinnerNumberModel class.
> 
> This fix will need CSR, I will get to it after the review is completed here.
> 
> 
> Regards,
> Pankaj Bansal
> 


--
Best regards, Sergey.