<Swing Dev> [15] RFR JDK-8220811: SpinnerNumberModel floating point rounding issue
Pankaj Bansal
pankaj.b.bansal at oracle.com
Tue Apr 28 10:56:14 UTC 2020
Hello Sergey,
<<Ok, can we split the change related to setting the data via text field
from the change of iteration over the number of steps using count? I
think it could be fixed separately.
We can separate the fix, but both would have to be pushed at same time.
We can first make changes for up/down button to use step/count, but it
will not work if the value is changed using textfield. Also, this will
be again changed a lot once we add support for changing the value via
textfield change using the step/count.
Regards,
Pankaj
On 16/04/20 4:48 PM, Sergey Bylokhov wrote:
> On 4/7/20 9:18 am, Pankaj Bansal wrote:
>> << 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.
>
> Ok, can we split the change related to setting the data via text field
> from the change of iteration over the number of steps using count?
> I think it could be fixed separately.
>
>>
>> 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