<Swing Dev> [15] RFR JDK-8220811: SpinnerNumberModel floating point rounding issue
Pankaj Bansal
pankaj.b.bansal at oracle.com
Tue Apr 7 16:18:37 UTC 2020
<< Probably I missed the point but isn't it too many changes for this?
<<Number base, expectedNewValue, newMinimum, newMaximum; int currentStep, positeveCount, negativeCount;
<<I expected that we will have only one new "count" field, probably all changes are needed, I'll check closely.
Most of these changes are needed to support changing the value from text field directly instead of up/down button. Changing from textfield can change min/max allowed etc.
Regards,
Pankaj
-----Original Message-----
From: Sergey Bylokhov
Sent: Tuesday, April 7, 2020 9:46 PM
To: Pankaj Bansal <pankaj.b.bansal at oracle.com>; Volodin, Vladislav <vladislav.volodin at sap.com>
Cc: swing-dev at openjdk.java.net; Jason Mehrens <jason_mehrens at hotmail.com>; Alexey Ivanov <alexey.ivanov at oracle.com>
Subject: Re: <Swing Dev> [15] RFR JDK-8220811: SpinnerNumberModel floating point rounding issue
Probably I missed the point but isn't it too many changes for this?
Number base, expectedNewValue, newMinimum, newMaximum; int currentStep, positeveCount, negativeCount;
I expected that we will have only one new "count" field, probably all changes are needed, I'll check closely.
On 4/7/20 9:01 am, Pankaj Bansal wrote:
> I tried few things and I think this version is working fine. I tested this version with few tests and this seem to work in all cases. Please have a look.
The test from the first version of the fix missed?
>
> webrev: http://cr.openjdk.java.net/~pbansal/8220811/webrev02/
>
> Regards,
> Pankaj
>
> -----Original Message-----
> From: Volodin, Vladislav <vladislav.volodin at sap.com>
> Sent: Sunday, March 15, 2020 2:03 AM
> To: Pankaj Bansal <pankaj.b.bansal at oracle.com>
> Cc: swing-dev at openjdk.java.net; Jason Mehrens
> <jason_mehrens at hotmail.com>; Sergey Bylokhov
> <sergey.bylokhov at oracle.com>; Alexey Ivanov <alexey.ivanov at oracle.com>
> Subject: Re: <Swing Dev> [15] RFR JDK-8220811: SpinnerNumberModel
> floating point rounding issue
>
> Hello Pankaj,
>
> I am not the reviewer, but I agree with Jason. To me p1.equals(Double.NaN) looks confusing. Because people might think that it will be equivalent to p1 == Double.NaN, that will return false, when p1 is also NaN (https://urldefense.com/v3/__https://stackoverflow.com/questions/8819738/why-does-double-nan-double-nan-return-false__;!!GqivPVa7Brio!MCTIuSS1NcMZWK7ZWOAVUhgC-AoVMYwCGbgTiDOH5pO2PQfW7b7XN-hLRtqBuhi6XnVl$ ).
> I prefer to use the dedicated method such as "public static boolean isNaN(double v)". It looks self-descriptive.
>
> Meanwhile, I remember you sentence regarding the number of steps:
> > double min=-.15,max=0.15,stepsize=.05, the steps is calculated as 5. double min=-.15,max=0.20,stepsize=.05, the steps is calculated as 7 instead of 6.
> I checked this part with the code:
>
> Double min = -.15;
> Double max = 0.15;
> Double stepsize = .05;
> Double steps = (max - min) / stepsize;
>
> And I found out that in this case the number of steps will be 5,999999....., but we can compensate it with either Math.round, it will return 6, or we can add the "epsilon" value to "max", and count the number of steps as it is:
> Double max = 0.15 + Math.ulp(1.0); Steps count will be 6.00000238418579.
>
> Since there is no value in Double (and probably float) less than Math.ulp (or epsilon, if we use this term), it will be probably safe to use my approach. What do you think?
>
> Kind regards,
> Vlad
>
> On 14.03.20, 15:51, "Pankaj Bansal" <pankaj.b.bansal at oracle.com> wrote:
>
> Hello Jason,
>
> << I would assume newMinimum.equals(Double.NaN) and newMinimum.equals(Float.NaN) should always evaluate to false.
> It seems to work as expected.
> Double p1 = Double.NaN;
> Double p2 = 1.0;
> System.out.println(p1.equals(Double.NaN)); //prints true
> System.out.println(p2.equals(Double.NaN)); //prints false
>
> Regards,
> Pankaj
>
> -----Original Message-----
> From: Jason Mehrens <jason_mehrens at hotmail.com>
> Sent: Saturday, March 14, 2020 8:09 PM
> To: Pankaj Bansal <pankaj.b.bansal at oracle.com>; Sergey Bylokhov <sergey.bylokhov at oracle.com>; Alexey Ivanov <alexey.ivanov at oracle.com>; Volodin, Vladislav <vladislav.volodin at sap.com>
> Cc: swing-dev at openjdk.java.net
> Subject: Re: <Swing Dev> [15] RFR JDK-8220811: SpinnerNumberModel
> floating point rounding issue
>
> Pankaj,
>
> I would assume newMinimum.equals(Double.NaN) and newMinimum.equals(Float.NaN) should always evaluate to false. Perhaps you want to use Float.isNaN and Double.isNaN instead?
>
> Jason
>
> ________________________________________
> From: swing-dev <swing-dev-bounces at openjdk.java.net> on behalf of Pankaj Bansal <pankaj.b.bansal at oracle.com>
> Sent: Saturday, March 14, 2020 8:11 AM
> To: Sergey Bylokhov; Alexey Ivanov; Volodin, Vladislav
> Cc: swing-dev at openjdk.java.net
> Subject: Re: <Swing Dev> [15] RFR JDK-8220811: SpinnerNumberModel
> floating point rounding issue
>
> Hello Sergey,
>
> << It will differ for two cases:
> - The error will not be accumulated when the counter will move forward/backward, currently the result might different on each iteration.
> - Initial/Default value will never be skipped due to counter=0
>
> I tried to code according to my understanding of the idea. I have created a preliminary webrev to demonstrate what I am doing. This is nowhere final, so please ignore the optimizations. Please have a look.
> As I was thinking, the precision error is creating issue while creating the step count. I have to do lot of stuff to allow values to be changed by editing the textfield. There are some other issues also, like the double value is formatted according to the formatter and that is also causing problems.
>
> webrev: http://cr.openjdk.java.net/~pbansal/8220811/webrev01/
>
> Regards,
> Pankaj
>
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Wednesday, March 11, 2020 5:33 AM
> To: Pankaj Bansal <pankaj.b.bansal at oracle.com>; Alexey Ivanov <alexey.ivanov at oracle.com>; Volodin, Vladislav <vladislav.volodin at sap.com>
> Cc: swing-dev at openjdk.java.net
> Subject: Re: <Swing Dev> [15] RFR JDK-8220811: SpinnerNumberModel
> floating point rounding issue
>
> On 3/10/20 5:34 am, Pankaj Bansal wrote:
> > Hello Sergey/Vlad/Alexey,
> >
> > Sorry, I could not reply to this earlier. I have one doubt about this approach. Won't the calculation of stepCount itself suffer from floating point issue? I mean the user will pass min, max, stepsize, then wont the calculation of steps required to go from min to max will also suffer from same floating point issue? I think there can be an rounding of error of -1 or +1 in calculation of step count.
>
> It will differ for two cases:
> - The error will not be accumulated when the counter will move forward/backward, currently the result might different on each iteration.
> - Initial/Default value will never be skipped due to counter=0
>
> >
> > eg.
> >
> > int steps =0;
> >
> > for (double i=min+stepsize; i<=max; i+=stepsize)
> > steps++;
> >
> > double min=-.15,max=0.15,stepsize=.05, the steps is calculated as 5. double min=-.15,max=0.20,stepsize=.05, the steps is calculated as 7 instead of 6.
> >
> >
> > The reason is that, there is floating point error in first case, but it is not present in second case.
> >
> > I think the best we can do here is as Sergey suggested in his first
> > reply to use Math.fma to reduce the floating point error chances from
> > 2 to 1 or just close this as not an issue
> >
> > Regards,
> >
> > Pankaj
> >
> >
> > On 19/02/20 3:49 AM, Sergey Bylokhov wrote:
> >> I think it should work, the step will counts from the default value.
> >>
> >> So currently:
> >> 1. if the user set default value to X1 and then he iterates forward 100 times then he will get some X2. During this calculation, he could get "100" rounding issues.
> >> 2. If later the user decides iterates backward then most probably he will not get X1, and the amount of possible "rounding issues" will be 200.
> >>
> >> If the user will repeat steps 1. and 2. then each time the values will "float".
> >>
> >> If we will use counter then in the worst case we will get only two roundings per step: X1+step*100 = X2(if we will use fma we will get only one for every step).
> >>
> >> It will not solve all issues but at least will make the iteration "stable".
> >>
> >> On 2/17/20 1:59 am, Alexey Ivanov wrote:
> >>> Hi Vlad,
> >>>
> >>> The idea looks reasonable. However, it does not allow for manual editing. The cases where max and min values are not multiples of step would be hard to handle with this approach. For example: max = 10.05, min = 0.01, step = 0.1; how many ticks are there? What if the user enters 1.01015; the value should change to 1.11015 or 0.91015.
> >>>
> >>> On 13/02/2020 22:22, Volodin, Vladislav wrote:
> >>>> Hello Sergey, Alexey and Pankaj,
> >>>>
> >>>> I am reading the current discussion and I was thinking about an idea changing the code in the way that instead of working with float/double numbers we work with integer ticks. For example, the model remembers the min/max/step values and calculates a number of steps required to reach from min to max. All increment/decrement actions are done against the current ˋtickˋ value. If the current ˋtickˋ reaches 0 - we return min; if maxTick — we return max. And the current value can be always counted as (min + tick * step) if tick is neither zero, nor max tick count.
> >>>>
> >>>> At least if we deal with integer ticks, but all reading operations calculate on the fly, we will be able to control the representativeness of output.
> >>>>
> >>>> As always, I don’t know all the details and possible consequences, so feel free to ignore my email, if I am wrong.
> >>>>
> >>>> Kind regards,
> >>>> Vlad
> >>>>
> >>>> Sent from myPad
> >>>>
> >>>>> On 13. Feb 2020, at 22:34, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> wrote:
> >>>>>
> >>>>> On 2/12/20 8:21 am, Alexey Ivanov wrote:
> >>>>>> The bug report says that going from -0.15 to -0.10 does not allow going back to -0.15. This happens because the result of this sequence of operations cannot be represented exactly, or, in other words, because of rounding errors; or rather the result is less than the set minimal value.
> >>>>>> Can we set the value of the spinner to the set minimal value instead of disallowing the operation. I mean, after going up the displayed value is -0.10; going down by 0.05 gives the result which is less than the minimal value for the spinner, and thus going down is not allowed. What if we set the value of the spinner to its minimal value instead?
> >>>>> In this case, we will need to update all types including int.
> >>>>> Isn't it will be surprised that the spinner will show the value which is not calculated as "defaultValue + stepValue * stepCount"?
> >>>>>
> >>>>>
> >>>>> --
> >>>>> Best regards, Sergey.
> >>
> >>
> >
>
>
> --
> Best regards, Sergey.
>
>
--
Best regards, Sergey.
More information about the swing-dev
mailing list