Re: [numbers] Fraction

[Alex Herbert]

> On 9 Apr 2020, at 23:59, Gilles Sadowski  wrote:
> 
> Le jeu. 9 avr. 2020 à 23:20, Alex Herbert  > a écrit :
>> 
>> 
>> 
>>> On 9 Apr 2020, at 21:36, Gilles Sadowski  wrote:
>>> 
>>> Le jeu. 9 avr. 2020 à 22:20, Alex Herbert  a 
>>> écrit :
 
 
 
> On 9 Apr 2020, at 16:32, Gilles Sadowski  wrote:
> 
> 
> 
>> Given this I am thinking that using ZERO when possible is a better
>> option and avoid 0 / -1.
> 
> Hmm, then I'm both +0 and -0 (which is the same, right?)
> on this issue. ;-)
 
 Ironically the conversion to a double is a minor bug:
 
 Fraction.of(0, 1).doubleValue() == 0.0
 Fraction.of(0, -01).doubleValue() == -0.0
 
 IEEE754 arithmetic for 0.0 / -1.0 creates a -0.0.
 
 Do we want to support -0.0?
>>> 
>>> Why prevent it since it looks expected from the above call?
>> 
>> Well, in the against argument -0.0 is an artefact of the IEEE floating point 
>> format. It is not a real number.
>> 
>> If we allow 0 / -1 as a fraction to mean something then we should really 
>> support it fully which means carrying the sign of the denominator through 
>> arithmetic as would be done for -0.0 (from the top of my head):
>> 
>> -0.0 + -0.0 = -0.0
>> -0.0 + 0.0 = 0.0
>> 0.0 - -0.0 = 0.0
>> 0.0 - 0.0 = 0.0
>> 0.0 * 42 = 0.0
>> -0.0 * 42 = -0.0
>> 
>> And so on...
>> 
>> It is easier to exclude this representation from ever existing by changing 
>> the factory constructor to not allow it.
>> 
>> Note that Fraction.of(-0.0) creates 0 / 1. So the support for 0 / 1 is 
>> inconsistent with conversion to and from double:
>> 
>> Fraction.of(-0.0).doubleValue() == 0.0
>> Fraction.of(0, -1).doubleValue() == -0.0
>> 
>> I have checked and Fraction.of(0, 1).compareTo(Fraction.of(0, -1)) is 0. 
>> They evaluate to equal and have the same hash code. So this behaviour is 
>> different from Double.compareTo, Double.equals and Double.hashCode which 
>> distinguishes the two values.
>> 
>> If fraction represented a signed number using the signed numerator and an 
>> unsigned denominator, reduced to smallest form, then the representation of 
>> zero is fixed. It would be 0 / 1 as you cannot have -0 as an integer.
> 
> This seems to be the winning argument to transform all zero to
> canonical form.

I have updated Fraction and BigFraction to have a canonical form for Zero. Both 
types pass the same tests for the constructor using a double and the 
doubleValue() method does not return -0.0 for zero.

I tried to align the FractionTest and BigFractionTest to be more similar. This 
should ease maintenance going forward.

I found that the use of a negative denominator had broken the compareTo 
function. So I have fixed that.

The Fraction add/subtract methods were direct adaptions from the BigFraction 
method. This was copied over from CM 3:

public Fraction add(final int i) {
return new Fraction(numerator + i * denominator, denominator);
}

This fails to detect overflow. So I have added tests for this and fixed the 
implementation so it does not wrap the numerator (overflow).

All the fixes are small changes. One thing I noticed was that the constructor 
using a double is limited to a maximum denominator of Integer.MAX_VALUE. 
However with the support for Integer.MIN_VALUE as a denominator this 
constructor does not allow for example:

@Test
public void test() {
Fraction f1 = Fraction.of(-1, Integer.MIN_VALUE);
Fraction f2 = Fraction.from(f1.doubleValue(), Integer.MIN_VALUE);
Assertions.assertEquals(f1, f2);
}

I will raise a ticket for this and investigate fixing it. The same limitation 
is in BigFraction too.

Alex

Re: [numbers] Fraction

[Eric Barnhill]

Great +1

On Thu, Apr 9, 2020 at 3:59 PM Gilles Sadowski  wrote:

> Le jeu. 9 avr. 2020 à 23:20, Alex Herbert  a
> écrit :
> >
> >
> >
> > > On 9 Apr 2020, at 21:36, Gilles Sadowski  wrote:
> > >
> > > Le jeu. 9 avr. 2020 à 22:20, Alex Herbert 
> a écrit :
> > >>
> > >>
> > >>
> > >>> On 9 Apr 2020, at 16:32, Gilles Sadowski 
> wrote:
> > >>>
> > >>>
> > >>>
> >  Given this I am thinking that using ZERO when possible is a better
> >  option and avoid 0 / -1.
> > >>>
> > >>> Hmm, then I'm both +0 and -0 (which is the same, right?)
> > >>> on this issue. ;-)
> > >>
> > >> Ironically the conversion to a double is a minor bug:
> > >>
> > >> Fraction.of(0, 1).doubleValue() == 0.0
> > >> Fraction.of(0, -01).doubleValue() == -0.0
> > >>
> > >> IEEE754 arithmetic for 0.0 / -1.0 creates a -0.0.
> > >>
> > >> Do we want to support -0.0?
> > >
> > > Why prevent it since it looks expected from the above call?
> >
> > Well, in the against argument -0.0 is an artefact of the IEEE floating
> point format. It is not a real number.
> >
> > If we allow 0 / -1 as a fraction to mean something then we should really
> support it fully which means carrying the sign of the denominator through
> arithmetic as would be done for -0.0 (from the top of my head):
> >
> > -0.0 + -0.0 = -0.0
> > -0.0 + 0.0 = 0.0
> > 0.0 - -0.0 = 0.0
> > 0.0 - 0.0 = 0.0
> > 0.0 * 42 = 0.0
> > -0.0 * 42 = -0.0
> >
> > And so on...
> >
> > It is easier to exclude this representation from ever existing by
> changing the factory constructor to not allow it.
> >
> > Note that Fraction.of(-0.0) creates 0 / 1. So the support for 0 / 1 is
> inconsistent with conversion to and from double:
> >
> > Fraction.of(-0.0).doubleValue() == 0.0
> > Fraction.of(0, -1).doubleValue() == -0.0
> >
> > I have checked and Fraction.of(0, 1).compareTo(Fraction.of(0, -1)) is 0.
> They evaluate to equal and have the same hash code. So this behaviour is
> different from Double.compareTo, Double.equals and Double.hashCode which
> distinguishes the two values.
> >
> > If fraction represented a signed number using the signed numerator and
> an unsigned denominator, reduced to smallest form, then the representation
> of zero is fixed. It would be 0 / 1 as you cannot have -0 as an integer.
>
> This seems to be the winning argument to transform all zero to
> canonical form.
>
> > This issue has been created by the support for the sign in either part
> so that Integer.MIN_VALUE can be used as a denominator. This is a nice
> change to allow support for fractions up to 2^-31. But creates this signed
> zero issue.
> >
> > It leads me to think we should have a canonical representation of zero
> as 0 / 1 and prevent creation of 0 / -1 by careful management of class
> creation.
>
> +1
>
> Best,
> Gilles
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>

Re: [numbers] Fraction

[Gilles Sadowski]

Le jeu. 9 avr. 2020 à 23:20, Alex Herbert  a écrit :
>
>
>
> > On 9 Apr 2020, at 21:36, Gilles Sadowski  wrote:
> >
> > Le jeu. 9 avr. 2020 à 22:20, Alex Herbert  a 
> > écrit :
> >>
> >>
> >>
> >>> On 9 Apr 2020, at 16:32, Gilles Sadowski  wrote:
> >>>
> >>>
> >>>
>  Given this I am thinking that using ZERO when possible is a better
>  option and avoid 0 / -1.
> >>>
> >>> Hmm, then I'm both +0 and -0 (which is the same, right?)
> >>> on this issue. ;-)
> >>
> >> Ironically the conversion to a double is a minor bug:
> >>
> >> Fraction.of(0, 1).doubleValue() == 0.0
> >> Fraction.of(0, -01).doubleValue() == -0.0
> >>
> >> IEEE754 arithmetic for 0.0 / -1.0 creates a -0.0.
> >>
> >> Do we want to support -0.0?
> >
> > Why prevent it since it looks expected from the above call?
>
> Well, in the against argument -0.0 is an artefact of the IEEE floating point 
> format. It is not a real number.
>
> If we allow 0 / -1 as a fraction to mean something then we should really 
> support it fully which means carrying the sign of the denominator through 
> arithmetic as would be done for -0.0 (from the top of my head):
>
> -0.0 + -0.0 = -0.0
> -0.0 + 0.0 = 0.0
> 0.0 - -0.0 = 0.0
> 0.0 - 0.0 = 0.0
> 0.0 * 42 = 0.0
> -0.0 * 42 = -0.0
>
> And so on...
>
> It is easier to exclude this representation from ever existing by changing 
> the factory constructor to not allow it.
>
> Note that Fraction.of(-0.0) creates 0 / 1. So the support for 0 / 1 is 
> inconsistent with conversion to and from double:
>
> Fraction.of(-0.0).doubleValue() == 0.0
> Fraction.of(0, -1).doubleValue() == -0.0
>
> I have checked and Fraction.of(0, 1).compareTo(Fraction.of(0, -1)) is 0. They 
> evaluate to equal and have the same hash code. So this behaviour is different 
> from Double.compareTo, Double.equals and Double.hashCode which distinguishes 
> the two values.
>
> If fraction represented a signed number using the signed numerator and an 
> unsigned denominator, reduced to smallest form, then the representation of 
> zero is fixed. It would be 0 / 1 as you cannot have -0 as an integer.

This seems to be the winning argument to transform all zero to
canonical form.

> This issue has been created by the support for the sign in either part so 
> that Integer.MIN_VALUE can be used as a denominator. This is a nice change to 
> allow support for fractions up to 2^-31. But creates this signed zero issue.
>
> It leads me to think we should have a canonical representation of zero as 0 / 
> 1 and prevent creation of 0 / -1 by careful management of class creation.

+1

Best,
Gilles

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers] Fraction

[Alex Herbert]

> On 9 Apr 2020, at 21:36, Gilles Sadowski  wrote:
> 
> Le jeu. 9 avr. 2020 à 22:20, Alex Herbert  a écrit :
>> 
>> 
>> 
>>> On 9 Apr 2020, at 16:32, Gilles Sadowski  wrote:
>>> 
>>> 
>>> 
 Given this I am thinking that using ZERO when possible is a better
 option and avoid 0 / -1.
>>> 
>>> Hmm, then I'm both +0 and -0 (which is the same, right?)
>>> on this issue. ;-)
>> 
>> Ironically the conversion to a double is a minor bug:
>> 
>> Fraction.of(0, 1).doubleValue() == 0.0
>> Fraction.of(0, -01).doubleValue() == -0.0
>> 
>> IEEE754 arithmetic for 0.0 / -1.0 creates a -0.0.
>> 
>> Do we want to support -0.0?
> 
> Why prevent it since it looks expected from the above call?

Well, in the against argument -0.0 is an artefact of the IEEE floating point 
format. It is not a real number.

If we allow 0 / -1 as a fraction to mean something then we should really 
support it fully which means carrying the sign of the denominator through 
arithmetic as would be done for -0.0 (from the top of my head):

-0.0 + -0.0 = -0.0
-0.0 + 0.0 = 0.0
0.0 - -0.0 = 0.0
0.0 - 0.0 = 0.0
0.0 * 42 = 0.0
-0.0 * 42 = -0.0

And so on...

It is easier to exclude this representation from ever existing by changing the 
factory constructor to not allow it.

Note that Fraction.of(-0.0) creates 0 / 1. So the support for 0 / 1 is 
inconsistent with conversion to and from double:

Fraction.of(-0.0).doubleValue() == 0.0
Fraction.of(0, -1).doubleValue() == -0.0

I have checked and Fraction.of(0, 1).compareTo(Fraction.of(0, -1)) is 0. They 
evaluate to equal and have the same hash code. So this behaviour is different 
from Double.compareTo, Double.equals and Double.hashCode which distinguishes 
the two values.

If fraction represented a signed number using the signed numerator and an 
unsigned denominator, reduced to smallest form, then the representation of zero 
is fixed. It would be 0 / 1 as you cannot have -0 as an integer. This issue has 
been created by the support for the sign in either part so that 
Integer.MIN_VALUE can be used as a denominator. This is a nice change to allow 
support for fractions up to 2^-31. But creates this signed zero issue.

It leads me to think we should have a canonical representation of zero as 0 / 1 
and prevent creation of 0 / -1 by careful management of class creation.

> 
> Gilles
> 
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
> 

Re: [numbers] Fraction

[Gilles Sadowski]

Le jeu. 9 avr. 2020 à 22:20, Alex Herbert  a écrit :
>
>
>
> > On 9 Apr 2020, at 16:32, Gilles Sadowski  wrote:
> >
> >
> >
> >> Given this I am thinking that using ZERO when possible is a better
> >> option and avoid 0 / -1.
> >
> > Hmm, then I'm both +0 and -0 (which is the same, right?)
> > on this issue. ;-)
>
> Ironically the conversion to a double is a minor bug:
>
> Fraction.of(0, 1).doubleValue() == 0.0
> Fraction.of(0, -01).doubleValue() == -0.0
>
> IEEE754 arithmetic for 0.0 / -1.0 creates a -0.0.
>
> Do we want to support -0.0?

Why prevent it since it looks expected from the above call?

Gilles

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers] Fraction

[Alex Herbert]

> On 9 Apr 2020, at 16:32, Gilles Sadowski  wrote:
> 
> 
> 
>> Given this I am thinking that using ZERO when possible is a better
>> option and avoid 0 / -1.
> 
> Hmm, then I'm both +0 and -0 (which is the same, right?)
> on this issue. ;-)

Ironically the conversion to a double is a minor bug:

Fraction.of(0, 1).doubleValue() == 0.0
Fraction.of(0, -01).doubleValue() == -0.0

IEEE754 arithmetic for 0.0 / -1.0 creates a -0.0.

Do we want to support -0.0?

Re: [numbers] Fraction

[Alex Herbert]

> On 9 Apr 2020, at 16:32, Gilles Sadowski  wrote:
> 
> Le jeu. 9 avr. 2020 à 16:48, Alex Herbert  > a écrit :
>> 
>> 
>> On 09/04/2020 14:38, Gilles Sadowski wrote:
>>> Le jeu. 9 avr. 2020 à 14:09, Alex Herbert  a 
>>> écrit :
 
 On 09/04/2020 12:04, Alex Herbert wrote:
>>> So if we are to support 0/-1 then I will add this to the standard test
>>> cases to make sure it is correctly implemented.
 So I did this. No major functionality failures.
 
 However there is a discrepancy between multiply and other functions.
 
 Multiply will return the form 0 / 1 for zero when either argument is zero.
 
 
 Divide will return the current instance if the instance is zero.
 
 Add/subtract will return the current instance if the instance to add is
 zero.
 
 Pow will return the current instance if the instance is zero.
 
 
 So you get this:
 
 0 / -1   multiply   1 / 3== 0 / 1
 
 
 0 / -1   divide   1 / 3== 0 / -1
 
 
 0 / -1   add   0 / 1== 0 / -1
 
 0 / -1   pow   1== 0 / -1
 
 0 / -1   pow   2== 0 / -1
 
 
 In the later case you may actually expect pow(0 / -1, 2) to return 0 /
 1. The result would require a change of sign on the denominator when the
 power is even.
 
 
 So do we:
 
 1. return the canonical form 0 / 1 when the result is zero?
 
 2. return the appropriate instance that is zero when the result is zero?
 
 3. maintain the sign as if implemented using the arithmetic on the
 denominator?
 
 
 I do not see a situation where preserving the signedness of the zero
 denominator is important. This rules out 3.
>>> But didn't you imply above that
>>>   0 / -1   pow   2
>>> should return
>>>   0 / 1
>>> ?
>> 
>> If you computed pow(2) it as (0 * 0) / (-1 * -1) then the result is 0 /
>> 1. Likewise pow(3) is 0 / -1. But both are zero.
>> 
>> If you see the result as zero then the sign of the denominator does not
>> matter.
>> 
>> If you want to preserve the sign of the denominator then more work will
>> have to be done in certain methods. I do not think this is important.
> 
> So there is no issue that several representations come up
> during computations (?).

I do not think so. At lest I have not found an issue where the computation is 
broken.

> 
>> Note: This is in contrast to Complex where the sign of zero components
>> of the complex number have to be preserved to ensure compatibility with
>> ISO C99 standards.
>> 
>> So do we leave it to produce 0 / -1 or 0 / 1 depending on the
>> computation (as is done currently), fix it to preserve the sign
>> appropriately or fix it to ensure that zero is always a canonical 0 / 1?
> 
> Is there any advantage to a single zero representation?
> If not, I'd vote for the simplest code.
> 
>>> 
 It makes it cleaner in the code to always return Fraction.ZERO when
 operations result in zero. This has memory advantage by allowing the
 garbage collection of fractions that are zero when they go out of scope
 following computations.
>>> +1
>> 
>> Returning 0 / 1 for zero when possible is simple to do in the existing
>> methods that detect zero as one of the arguments.
> 
> if (this.isZero()) { return ZERO; }
> if (this.isZero()) { return this; }
> 
> Is one better than the other?
> If not, I'd vote for the former (referring to allowing garbage collection).

I will make this change as it makes the code more readable IMO. It is very 
clear the result is zero.

> 
 This is a weak argument as it is unlikely to
 save much memory in common usage. It is also possible to create zeros in
 computation and we do not want to go the route of switching new 0/1
 instances to ZERO.
>>> Why not?
>> It is extra work in the computations since computations do not detect
>> zeros in the result. They detect zeros in the arguments and can fast
>> rack the result.
> 
> I thought that we were indeed referring to the above case (this
> vs ZERO).
> 
>> For those that actually compute a result then the result would require
>> construction using a factory constructor that will detect and return
>> ZERO if the numerator is zero.
> 
> IIUC, this should be avoided, since ZERO will be returned at
> the next opportunity (e.g. if the new instance calls "multiply")
> without incurring any performance cost.

An interesting point. And if the ultimate output for the fraction is conversion 
to a double then it would not matter anyway.

So I will check all the conversions work with the 0 / -1 representation of zero.

> 
>> This would add another if statement to
>> all usage of the factory constructor and effect performance. However
>> performance impact is not likely to be high (see below).
>> 
>> It should solve the previous issue that you can represent zero as 0 /
>> -1. If all computations use a factory constructor then we can make 

Re: [numbers] Fraction

[Gilles Sadowski]

Le jeu. 9 avr. 2020 à 16:48, Alex Herbert  a écrit :
>
>
> On 09/04/2020 14:38, Gilles Sadowski wrote:
> > Le jeu. 9 avr. 2020 à 14:09, Alex Herbert  a 
> > écrit :
> >>
> >> On 09/04/2020 12:04, Alex Herbert wrote:
> > So if we are to support 0/-1 then I will add this to the standard test
> > cases to make sure it is correctly implemented.
> >> So I did this. No major functionality failures.
> >>
> >> However there is a discrepancy between multiply and other functions.
> >>
> >> Multiply will return the form 0 / 1 for zero when either argument is zero.
> >>
> >>
> >> Divide will return the current instance if the instance is zero.
> >>
> >> Add/subtract will return the current instance if the instance to add is
> >> zero.
> >>
> >> Pow will return the current instance if the instance is zero.
> >>
> >>
> >> So you get this:
> >>
> >> 0 / -1   multiply   1 / 3== 0 / 1
> >>
> >>
> >> 0 / -1   divide   1 / 3== 0 / -1
> >>
> >>
> >> 0 / -1   add   0 / 1== 0 / -1
> >>
> >> 0 / -1   pow   1== 0 / -1
> >>
> >> 0 / -1   pow   2== 0 / -1
> >>
> >>
> >> In the later case you may actually expect pow(0 / -1, 2) to return 0 /
> >> 1. The result would require a change of sign on the denominator when the
> >> power is even.
> >>
> >>
> >> So do we:
> >>
> >> 1. return the canonical form 0 / 1 when the result is zero?
> >>
> >> 2. return the appropriate instance that is zero when the result is zero?
> >>
> >> 3. maintain the sign as if implemented using the arithmetic on the
> >> denominator?
> >>
> >>
> >> I do not see a situation where preserving the signedness of the zero
> >> denominator is important. This rules out 3.
> > But didn't you imply above that
> >0 / -1   pow   2
> > should return
> >0 / 1
> > ?
>
> If you computed pow(2) it as (0 * 0) / (-1 * -1) then the result is 0 /
> 1. Likewise pow(3) is 0 / -1. But both are zero.
>
> If you see the result as zero then the sign of the denominator does not
> matter.
>
> If you want to preserve the sign of the denominator then more work will
> have to be done in certain methods. I do not think this is important.

So there is no issue that several representations come up
during computations (?).

> Note: This is in contrast to Complex where the sign of zero components
> of the complex number have to be preserved to ensure compatibility with
> ISO C99 standards.
>
> So do we leave it to produce 0 / -1 or 0 / 1 depending on the
> computation (as is done currently), fix it to preserve the sign
> appropriately or fix it to ensure that zero is always a canonical 0 / 1?

Is there any advantage to a single zero representation?
If not, I'd vote for the simplest code.

> >
> >> It makes it cleaner in the code to always return Fraction.ZERO when
> >> operations result in zero. This has memory advantage by allowing the
> >> garbage collection of fractions that are zero when they go out of scope
> >> following computations.
> > +1
>
> Returning 0 / 1 for zero when possible is simple to do in the existing
> methods that detect zero as one of the arguments.

if (this.isZero()) { return ZERO; }
if (this.isZero()) { return this; }

Is one better than the other?
If not, I'd vote for the former (referring to allowing garbage collection).

> >> This is a weak argument as it is unlikely to
> >> save much memory in common usage. It is also possible to create zeros in
> >> computation and we do not want to go the route of switching new 0/1
> >> instances to ZERO.
> > Why not?
> It is extra work in the computations since computations do not detect
> zeros in the result. They detect zeros in the arguments and can fast
> rack the result.

I thought that we were indeed referring to the above case (this
vs ZERO).

> For those that actually compute a result then the result would require
> construction using a factory constructor that will detect and return
> ZERO if the numerator is zero.

IIUC, this should be avoided, since ZERO will be returned at
the next opportunity (e.g. if the new instance calls "multiply")
without incurring any performance cost.

> This would add another if statement to
> all usage of the factory constructor and effect performance. However
> performance impact is not likely to be high (see below).
>
> It should solve the previous issue that you can represent zero as 0 /
> -1. If all computations use a factory constructor then we can make it
> possible to exclude the 0 / -1 representation completely.
>
> >
> >> The easiest change is to just update multiply to return the appropriate
> >> zero instance and not use Fraction.ZERO. The classes then are consistent
> >> across the methods. Return the appropriate fraction instance if it
> >> matches the result, otherwise compute the result.
> > I don't follow: How do you match before computing?
>
> I missed out that one of the function arguments match the result when
> zero is involved as an argument. All the arithmetic methods detect zeros
> in the arguments and quickly return.
>
> It would be 

Re: [numbers] Fraction

[Alex Herbert]

On 09/04/2020 14:38, Gilles Sadowski wrote:

Le jeu. 9 avr. 2020 à 14:09, Alex Herbert  a écrit :


On 09/04/2020 12:04, Alex Herbert wrote:

So if we are to support 0/-1 then I will add this to the standard test
cases to make sure it is correctly implemented.

So I did this. No major functionality failures.

However there is a discrepancy between multiply and other functions.

Multiply will return the form 0 / 1 for zero when either argument is zero.


Divide will return the current instance if the instance is zero.

Add/subtract will return the current instance if the instance to add is
zero.

Pow will return the current instance if the instance is zero.


So you get this:

0 / -1   multiply   1 / 3== 0 / 1


0 / -1   divide   1 / 3== 0 / -1


0 / -1   add   0 / 1== 0 / -1

0 / -1   pow   1== 0 / -1

0 / -1   pow   2== 0 / -1


In the later case you may actually expect pow(0 / -1, 2) to return 0 /
1. The result would require a change of sign on the denominator when the
power is even.


So do we:

1. return the canonical form 0 / 1 when the result is zero?

2. return the appropriate instance that is zero when the result is zero?

3. maintain the sign as if implemented using the arithmetic on the
denominator?


I do not see a situation where preserving the signedness of the zero
denominator is important. This rules out 3.

But didn't you imply above that
   0 / -1   pow   2
should return
   0 / 1
?


If you computed pow(2) it as (0 * 0) / (-1 * -1) then the result is 0 / 
1. Likewise pow(3) is 0 / -1. But both are zero.


If you see the result as zero then the sign of the denominator does not 
matter.


If you want to preserve the sign of the denominator then more work will 
have to be done in certain methods. I do not think this is important. 
Note: This is in contrast to Complex where the sign of zero components 
of the complex number have to be preserved to ensure compatibility with 
ISO C99 standards.


So do we leave it to produce 0 / -1 or 0 / 1 depending on the 
computation (as is done currently), fix it to preserve the sign 
appropriately or fix it to ensure that zero is always a canonical 0 / 1?






It makes it cleaner in the code to always return Fraction.ZERO when
operations result in zero. This has memory advantage by allowing the
garbage collection of fractions that are zero when they go out of scope
following computations.

+1


Returning 0 / 1 for zero when possible is simple to do in the existing 
methods that detect zero as one of the arguments.



This is a weak argument as it is unlikely to
save much memory in common usage. It is also possible to create zeros in
computation and we do not want to go the route of switching new 0/1
instances to ZERO.

Why not?
It is extra work in the computations since computations do not detect 
zeros in the result. They detect zeros in the arguments and can fast 
rack the result.


For those that actually compute a result then the result would require 
construction using a factory constructor that will detect and return 
ZERO if the numerator is zero. This would add another if statement to 
all usage of the factory constructor and effect performance. However 
performance impact is not likely to be high (see below).


It should solve the previous issue that you can represent zero as 0 / 
-1. If all computations use a factory constructor then we can make it 
possible to exclude the 0 / -1 representation completely.





The easiest change is to just update multiply to return the appropriate
zero instance and not use Fraction.ZERO. The classes then are consistent
across the methods. Return the appropriate fraction instance if it
matches the result, otherwise compute the result.

I don't follow: How do you match before computing?


I missed out that one of the function arguments match the result when 
zero is involved as an argument. All the arithmetic methods detect zeros 
in the arguments and quickly return.


It would be cleaner to try and enforce a canonical form for ZERO.

Looking at the code for the standard constructor using Fraction(num, 
den) there are already a few if statements in there. In most cases it 
then follows the path using the reduction with the greatest common 
divisor logic. This is bound to be the largest part of the execution 
time. Forcing all construction through the of(num, den) factory method 
and adding another if statement to detect a zero numerator should not 
impact performance too much. The class can avoid the factory constructor 
if the value has already been tested and known to be non zero. This is 
the case for methods that use a numerator and not a denominator (such as 
pow(int)).


Given this I am thinking that using ZERO when possible is a better 
option and avoid 0 / -1.



Also, did you have an opinion on dropping pow(long) and pow(BigInteger)? 
I do not think that they can compute a value when pow(int) cannot be used.






The ZERO constant then is reduced to a convenience as for the ONE 

Re: [numbers] Fraction

[Gilles Sadowski]

Le jeu. 9 avr. 2020 à 14:09, Alex Herbert  a écrit :
>
>
> On 09/04/2020 12:04, Alex Herbert wrote:
> >
> >>>
> >>> So if we are to support 0/-1 then I will add this to the standard test
> >>> cases to make sure it is correctly implemented.
>
> So I did this. No major functionality failures.
>
> However there is a discrepancy between multiply and other functions.
>
> Multiply will return the form 0 / 1 for zero when either argument is zero.
>
>
> Divide will return the current instance if the instance is zero.
>
> Add/subtract will return the current instance if the instance to add is
> zero.
>
> Pow will return the current instance if the instance is zero.
>
>
> So you get this:
>
> 0 / -1   multiply   1 / 3== 0 / 1
>
>
> 0 / -1   divide   1 / 3== 0 / -1
>
>
> 0 / -1   add   0 / 1== 0 / -1
>
> 0 / -1   pow   1== 0 / -1
>
> 0 / -1   pow   2== 0 / -1
>
>
> In the later case you may actually expect pow(0 / -1, 2) to return 0 /
> 1. The result would require a change of sign on the denominator when the
> power is even.
>
>
> So do we:
>
> 1. return the canonical form 0 / 1 when the result is zero?
>
> 2. return the appropriate instance that is zero when the result is zero?
>
> 3. maintain the sign as if implemented using the arithmetic on the
> denominator?
>
>
> I do not see a situation where preserving the signedness of the zero
> denominator is important. This rules out 3.

But didn't you imply above that
  0 / -1   pow   2
should return
  0 / 1
?

> It makes it cleaner in the code to always return Fraction.ZERO when
> operations result in zero. This has memory advantage by allowing the
> garbage collection of fractions that are zero when they go out of scope
> following computations.

+1

> This is a weak argument as it is unlikely to
> save much memory in common usage. It is also possible to create zeros in
> computation and we do not want to go the route of switching new 0/1
> instances to ZERO.

Why not?

> The easiest change is to just update multiply to return the appropriate
> zero instance and not use Fraction.ZERO. The classes then are consistent
> across the methods. Return the appropriate fraction instance if it
> matches the result, otherwise compute the result.

I don't follow: How do you match before computing?

>
> The ZERO constant then is reduced to a convenience as for the ONE constant.
>
> So my vote is option 2 for code consistency.
>
>
> Alex

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers] Fraction

[Alex Herbert]

On 09/04/2020 12:04, Alex Herbert wrote:




So if we are to support 0/-1 then I will add this to the standard test
cases to make sure it is correctly implemented.


So I did this. No major functionality failures.

However there is a discrepancy between multiply and other functions.

Multiply will return the form 0 / 1 for zero when either argument is zero.

Divide will return the current instance if the instance is zero.

Add/subtract will return the current instance if the instance to add is 
zero.


Pow will return the current instance if the instance is zero.


So you get this:

0 / -1   multiply   1 / 3    == 0 / 1

0 / -1   divide   1 / 3    == 0 / -1

0 / -1   add   0 / 1    == 0 / -1

0 / -1   pow   1    == 0 / -1

0 / -1   pow   2    == 0 / -1


In the later case you may actually expect pow(0 / -1, 2) to return 0 / 
1. The result would require a change of sign on the denominator when the 
power is even.



So do we:

1. return the canonical form 0 / 1 when the result is zero?

2. return the appropriate instance that is zero when the result is zero?

3. maintain the sign as if implemented using the arithmetic on the 
denominator?



I do not see a situation where preserving the signedness of the zero 
denominator is important. This rules out 3.


It makes it cleaner in the code to always return Fraction.ZERO when 
operations result in zero. This has memory advantage by allowing the 
garbage collection of fractions that are zero when they go out of scope 
following computations. This is a weak argument as it is unlikely to 
save much memory in common usage. It is also possible to create zeros in 
computation and we do not want to go the route of switching new 0/1 
instances to ZERO.


The easiest change is to just update multiply to return the appropriate 
zero instance and not use Fraction.ZERO. The classes then are consistent 
across the methods. Return the appropriate fraction instance if it 
matches the result, otherwise compute the result.


The ZERO constant then is reduced to a convenience as for the ONE constant.

So my vote is option 2 for code consistency.


Alex



-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers] Fraction

[Alex Herbert]

Fowarded to ML due to a reply-to error.


On 09/04/2020 11:52, Gilles Sadowski wrote:

2020-04-09 12:33 UTC+02:00, Alex Herbert :

On 09/04/2020 10:54, Gilles Sadowski wrote:

Hi.

2020-04-09 11:28 UTC+02:00, Alex Herbert :

Two oddities in fraction:

1. Zero can be represented as 0 / 1 or 0 / -1

I have not found any location where this manifest as a bug.

:-)


It is
handled by the arithmetic and toString methods and the denominator is
not negated in the method negate() and so negation of 0 / -1 
returns the

same value:

Fraction f1 = Fraction.of(0, -1);
Fraction f2 = f1.negate();
System.out.printf("%s/%s (%s) : %s/%s (%s)%n",
f1.getNumerator(), f1.getDenominator(), f1,
f2.getNumerator(), f2.getDenominator(), f2);

0/-1 (0) : 0/-1 (0)
0/-1 (0) : 0/-1 (0)

However any future issues could be avoided by detecting a zero 
numerator

in the constructors and setting the denominator as 1.

Wouldn't this mean a performance hit for the majority of
instances that are not zero?

Yes, but the difference may be marginal.

So if we are to support 0/-1 then I will add this to the standard test
cases to make sure it is correctly implemented.

+1

[We removed the requirement that the denominator must
be positive. It would have been strange to reinstate it for
the case where it least matters.]


2. There are a lot of pow(exponent) methods in BigFraction compared to
Fraction:

This is mandated by o.a.c.numbers.core.NativeOperations:

Fraction pow(int)

BigFraction pow(int)

But there are more in BigFraction and one returns a double:

BigFraction pow(long)
BigFraction pow(BigInteger)
double pow(double)

I see the long and BigInteger versions as specialisations of the
pow(int) method. The double variant can be implemented in Fraction 
using

the same method from BigFraction as:

/**
* Returns a {@code double} whose value is
* thisexponent.
*
* @param exponent exponent to which this {@code Fraction} is to
be
raised.
* @return thisexponent as a {@code
double}.
*/
public double pow(final double exponent) {
return Math.pow(numerator, exponent) /
Math.pow(denominator, exponent);
}

This now strikes me as dangerous.


So either we add this method to Fraction

-1


or drop it from BigFraction.

+1

Let users perform any "cast" to "double" that could be invalid.

OK.

I was wondering about:

BigFraction pow(long)
BigFraction pow(BigInteger)

These are implemented by computing:

pow(numerator, exponent) and pow(denominator, exponent) and then
creating a fraction from the result which will perform a reduction. When
the exponent absolutely must be a long or BigInteger (and not an int)
then I imagine that this would overflow BigInteger. On my machine this:

BigInteger.valueOf(2).pow(Integer.MAX_VALUE)

reports this:

java.lang.ArithmeticException: BigInteger would overflow supported range

Strange; this doesn't seem to be consistent with the Javadoc:[1]
---CUT---
Semantics of arithmetic operations exactly mimic those of Java's
integer arithmetic operators, as defined in The Java Language
Specification. For example, division by zero throws an
ArithmeticException, and division of a negative by a positive yields a
negative (or zero) remainder. All of the details in the Spec
concerning overflow are ignored, as BigIntegers are made as large as
necessary to accommodate the results of an operation.
---CUT---


You can overflow a BigInteger. Maybe the intention when they wrote that 
was that it would be infinite. But the implementation uses a single 
array to store the bits. So you are limited to a maximum array length. 
However the limit is actually lower. This is also in the javadoc:


BigInteger must support values in the range
-2{@code Integer.MAX_VALUE} (exclusive) to
+2{@code Integer.MAX_VALUE} (exclusive)
and may support values outside of that range.

The BigInteger.pow(int) method throws the above exception when the 
result requires a left bit shift above Integer.MAX_VALUE. So this fits 
with supporting up to 2^Integer.MAX_VALUE exclusive.


In the class there is a constant for the maximum length:

    /**
 * This constant limits {@code mag.length} of BigIntegers to the 
supported

 * range.
 */
    private static final int MAX_MAG_LENGTH = Integer.MAX_VALUE / 
Integer.SIZE + 1; // (1 << 26)


So it seems that currently BigInteger is supporting an integer 
containing up to Integer.MAX_VALUE bits.


I suggest removing the BigFraction.pow(long) and pow(BigInteger) methods 
as they cannot work when the pow(int) method would also fail.


They are based on ArithmeticUtils.pow. These methods are probably 
redundant by the same argument as well.





So I think we can drop the pow() methods using a long or BigInteger
argument too. In cases where you have to use them they will overflow.


Note: The methods are present in the CM 3.6 version of BigFraction but
Fraction did not have any pow methods.

A lot of fixes and improvements were brought in by
Heinrich Bohne a few months ago. [IIRC, some issues
which he had reported are still 

Re: [numbers] Fraction

[Gilles Sadowski]

2020-04-09 12:33 UTC+02:00, Alex Herbert :
>
> On 09/04/2020 10:54, Gilles Sadowski wrote:
>> Hi.
>>
>> 2020-04-09 11:28 UTC+02:00, Alex Herbert :
>>> Two oddities in fraction:
>>>
>>> 1. Zero can be represented as 0 / 1 or 0 / -1
>>>
>>> I have not found any location where this manifest as a bug.
>> :-)
>>
>>> It is
>>> handled by the arithmetic and toString methods and the denominator is
>>> not negated in the method negate() and so negation of 0 / -1 returns the
>>> same value:
>>>
>>> Fraction f1 = Fraction.of(0, -1);
>>> Fraction f2 = f1.negate();
>>> System.out.printf("%s/%s (%s) : %s/%s (%s)%n",
>>>   f1.getNumerator(), f1.getDenominator(), f1,
>>>   f2.getNumerator(), f2.getDenominator(), f2);
>>>
>>> 0/-1 (0) : 0/-1 (0)
>>> 0/-1 (0) : 0/-1 (0)
>>>
>>> However any future issues could be avoided by detecting a zero numerator
>>> in the constructors and setting the denominator as 1.
>> Wouldn't this mean a performance hit for the majority of
>> instances that are not zero?
>
> Yes, but the difference may be marginal.
>
> So if we are to support 0/-1 then I will add this to the standard test
> cases to make sure it is correctly implemented.

+1

[We removed the requirement that the denominator must
be positive.  It would have been strange to reinstate it for
the case where it least matters.]

>>
>>> 2. There are a lot of pow(exponent) methods in BigFraction compared to
>>> Fraction:
>>>
>>> This is mandated by o.a.c.numbers.core.NativeOperations:
>>>
>>> Fraction pow(int)
>>>
>>> BigFraction pow(int)
>>>
>>> But there are more in BigFraction and one returns a double:
>>>
>>> BigFraction pow(long)
>>> BigFraction pow(BigInteger)
>>> double pow(double)
>>>
>>> I see the long and BigInteger versions as specialisations of the
>>> pow(int) method. The double variant can be implemented in Fraction using
>>> the same method from BigFraction as:
>>>
>>>   /**
>>>* Returns a {@code double} whose value is
>>>* thisexponent.
>>>*
>>>* @param exponent exponent to which this {@code Fraction} is to
>>> be
>>> raised.
>>>* @return thisexponent as a {@code
>>> double}.
>>>*/
>>>   public double pow(final double exponent) {
>>>   return Math.pow(numerator,   exponent) /
>>>  Math.pow(denominator, exponent);
>>>   }
>> This now strikes me as dangerous.
>>
>>> So either we add this method to Fraction
>> -1
>>
>>> or drop it from BigFraction.
>> +1
>>
>> Let users perform any "cast" to "double" that could be invalid.
>
> OK.
>
> I was wondering about:
>
> BigFraction pow(long)
> BigFraction pow(BigInteger)
>
> These are implemented by computing:
>
> pow(numerator, exponent) and pow(denominator, exponent) and then
> creating a fraction from the result which will perform a reduction. When
> the exponent absolutely must be a long or BigInteger (and not an int)
> then I imagine that this would overflow BigInteger. On my machine this:
>
> BigInteger.valueOf(2).pow(Integer.MAX_VALUE)
>
> reports this:
>
> java.lang.ArithmeticException: BigInteger would overflow supported range

Strange; this doesn't seem to be consistent with the Javadoc:[1]
---CUT---
Semantics of arithmetic operations exactly mimic those of Java's
integer arithmetic operators, as defined in The Java Language
Specification. For example, division by zero throws an
ArithmeticException, and division of a negative by a positive yields a
negative (or zero) remainder. All of the details in the Spec
concerning overflow are ignored, as BigIntegers are made as large as
necessary to accommodate the results of an operation.
---CUT---

>
> So I think we can drop the pow() methods using a long or BigInteger
> argument too. In cases where you have to use them they will overflow.
>
>>
>>> Note: The methods are present in the CM 3.6 version of BigFraction but
>>> Fraction did not have any pow methods.
>> A lot of fixes and improvements were brought in by
>> Heinrich Bohne a few months ago.  [IIRC, some issues
>> which he had reported are still pending...]
> I remember the good work on the constructors and conversion to/from
> double. I did not look at reported issues. I was focussing on the
> current functionality.

I didn't mean any subliminal message, except for Heinrich
maybe. ;-)


Regards,
Gilles

[1] https://docs.oracle.com/javase/8/docs/api/java/math/BigInteger.html

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers] Fraction

[Alex Herbert]

On 09/04/2020 10:54, Gilles Sadowski wrote:

Hi.

2020-04-09 11:28 UTC+02:00, Alex Herbert :

Two oddities in fraction:

1. Zero can be represented as 0 / 1 or 0 / -1

I have not found any location where this manifest as a bug.

:-)


It is
handled by the arithmetic and toString methods and the denominator is
not negated in the method negate() and so negation of 0 / -1 returns the
same value:

Fraction f1 = Fraction.of(0, -1);
Fraction f2 = f1.negate();
System.out.printf("%s/%s (%s) : %s/%s (%s)%n",
  f1.getNumerator(), f1.getDenominator(), f1,
  f2.getNumerator(), f2.getDenominator(), f2);

0/-1 (0) : 0/-1 (0)
0/-1 (0) : 0/-1 (0)

However any future issues could be avoided by detecting a zero numerator
in the constructors and setting the denominator as 1.

Wouldn't this mean a performance hit for the majority of
instances that are not zero?


Yes, but the difference may be marginal.

So if we are to support 0/-1 then I will add this to the standard test 
cases to make sure it is correctly implemented.





2. There are a lot of pow(exponent) methods in BigFraction compared to
Fraction:

This is mandated by o.a.c.numbers.core.NativeOperations:

Fraction pow(int)

BigFraction pow(int)

But there are more in BigFraction and one returns a double:

BigFraction pow(long)
BigFraction pow(BigInteger)
double pow(double)

I see the long and BigInteger versions as specialisations of the
pow(int) method. The double variant can be implemented in Fraction using
the same method from BigFraction as:

  /**
   * Returns a {@code double} whose value is
   * thisexponent.
   *
   * @param exponent exponent to which this {@code Fraction} is to be
raised.
   * @return thisexponent as a {@code double}.
   */
  public double pow(final double exponent) {
  return Math.pow(numerator,   exponent) /
 Math.pow(denominator, exponent);
  }

This now strikes me as dangerous.


So either we add this method to Fraction

-1


or drop it from BigFraction.

+1

Let users perform any "cast" to "double" that could be invalid.


OK.

I was wondering about:

BigFraction pow(long)
BigFraction pow(BigInteger)

These are implemented by computing:

pow(numerator, exponent) and pow(denominator, exponent) and then 
creating a fraction from the result which will perform a reduction. When 
the exponent absolutely must be a long or BigInteger (and not an int) 
then I imagine that this would overflow BigInteger. On my machine this:


BigInteger.valueOf(2).pow(Integer.MAX_VALUE)

reports this:

java.lang.ArithmeticException: BigInteger would overflow supported range

So I think we can drop the pow() methods using a long or BigInteger 
argument too. In cases where you have to use them they will overflow.





Note: The methods are present in the CM 3.6 version of BigFraction but
Fraction did not have any pow methods.

A lot of fixes and improvements were brought in by
Heinrich Bohne a few months ago.  [IIRC, some issues
which he had reported are still pending...]
I remember the good work on the constructors and conversion to/from 
double. I did not look at reported issues. I was focussing on the 
current functionality.


Regards,
Gilles

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers] Fraction

[Gilles Sadowski]

Hi.

2020-04-09 11:28 UTC+02:00, Alex Herbert :
> Two oddities in fraction:
>
> 1. Zero can be represented as 0 / 1 or 0 / -1
>
> I have not found any location where this manifest as a bug.

:-)

> It is
> handled by the arithmetic and toString methods and the denominator is
> not negated in the method negate() and so negation of 0 / -1 returns the
> same value:
>
> Fraction f1 = Fraction.of(0, -1);
> Fraction f2 = f1.negate();
> System.out.printf("%s/%s (%s) : %s/%s (%s)%n",
>  f1.getNumerator(), f1.getDenominator(), f1,
>  f2.getNumerator(), f2.getDenominator(), f2);
>
> 0/-1 (0) : 0/-1 (0)
> 0/-1 (0) : 0/-1 (0)
>
> However any future issues could be avoided by detecting a zero numerator
> in the constructors and setting the denominator as 1.

Wouldn't this mean a performance hit for the majority of
instances that are not zero?

>
> 2. There are a lot of pow(exponent) methods in BigFraction compared to
> Fraction:
>
> This is mandated by o.a.c.numbers.core.NativeOperations:
>
> Fraction pow(int)
>
> BigFraction pow(int)
>
> But there are more in BigFraction and one returns a double:
>
> BigFraction pow(long)
> BigFraction pow(BigInteger)
> double pow(double)
>
> I see the long and BigInteger versions as specialisations of the
> pow(int) method. The double variant can be implemented in Fraction using
> the same method from BigFraction as:
>
>  /**
>   * Returns a {@code double} whose value is
>   * thisexponent.
>   *
>   * @param exponent exponent to which this {@code Fraction} is to be
> raised.
>   * @return thisexponent as a {@code double}.
>   */
>  public double pow(final double exponent) {
>  return Math.pow(numerator,   exponent) /
> Math.pow(denominator, exponent);
>  }

This now strikes me as dangerous.

>
> So either we add this method to Fraction

-1

> or drop it from BigFraction.

+1

Let users perform any "cast" to "double" that could be invalid.

> Note: The methods are present in the CM 3.6 version of BigFraction but
> Fraction did not have any pow methods.

A lot of fixes and improvements were brought in by
Heinrich Bohne a few months ago.  [IIRC, some issues
which he had reported are still pending...]

Regards,
Gilles

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

[Heinrich Bohne]

Sure, no problem, thanks for letting me know!

On 7/19/19 6:22 PM, Eric Barnhill wrote:

I'm looking forward to reviewing your code within my limited knowledge,
however I can't guarantee a quick time frame since Apache GSoC mentor
milestones are due next week and I think that could get time consuming.

Thanks for the contribution,
Eric

On Thu, Jul 18, 2019 at 4:13 PM Heinrich Bohne 
wrote:


So, I think the code I have so far is ripe for a pull request, so I
submitted one. I changed the contracts of the epsilon and
max-denominator factory methods, because the old specifications were not
very useful, especially that of the max-denominator method – the only
guarantee you could get from it was that /some/ fraction with a
denominator not greater than the specified maximum would be returned,
without any relation to the double value that should be approximated.

The simple continued fraction is now created from an exact BigFraction
representation of the double value rather than with floating-point
arithmetic on the double value itself, to preserve maximum precision
(the old epsilon algorithm produces a fraction of 1/3 when passed the
double value obtained from the expression 1.0/3.0 and an epsilon of 5 *
2^(-58), which is incorrect because the distance between the rational
number 1/3 and its closest double representative is larger than 5 *
2^(-58); I added a corresponding unit test).

The methods setLastCoefficient(BigInteger) and removeLastCoefficient()
in the new class SimpleContinuedFraction are unused. I wrote this class
before I implemented the algorithms, and I thought these methods might
be useful in the max-denominator method, but this turned out not to be
the case. However, from a design-perspective, these two methods
complement the functionality of addCoefficient(BigInteger), so I thought
their presence is still tolerable.

I solved the problem with the maxIterations argument in the epsilon
method by simply not limiting the number of iterations if this argument
is negative. Maybe this parameter was necessary in the old algorithm
which calculated the simple continued fraction with floating-point
arithmetic, to prevent an infinite loop or something.

On 7/17/19 10:20 AM, Heinrich Bohne wrote:

It just occured to me that you might have misunderstood my sentence:


I am even more confused by your suggestion seeing as it was
you who banned BigInteger from Fraction.addSub(Fraction, boolean) in
https://issues.apache.org/jira/browse/NUMBERS-79  , which, even though
you were not aware of it at that time, did not limit the method's
functionality in any way whatsoever

The "which" was referring to your removal of BigInteger, not to the use
of BigInteger prior to your edits, so what I meant to say was, by
removing BigInteger, you did not limit the method's functionality.


On 7/17/19 10:10 AM, Heinrich Bohne wrote:

The reason it was done was because Knuth proved
(as in mathematical proof) that a long is insufficient for certain
fraction
multiplications where both numerator and denominator are large ints; 65
rather than 64 bits are necessary and a long will not suffice.

You seem to have missed my comment in ticket
https://issues.apache.org/jira/browse/NUMBERS-79 , which you created –

I

don't have the book by D. Knuth, but I can only assume that the section
referenced by the code talks about unsigned integers, because by the
logic in the comment I left in the JIRA ticket, long values are
**always** sufficient in Fraction.addSub(Fraction, boolean).

But this is beside the point, I only mentioned it because I didn't
understand why you suggested to remove the BigFraction class, and
actually, I still don't, as the class BigFraction provides functionality
that Fraction cannot have, both with and without my suggested
alterations.


On 7/17/19 2:29 AM, Eric Barnhill wrote:

On Tue, Jul 16, 2019 at 2:41 PM Heinrich Bohne
wrote:


Do you think we really even need a BigFraction class at all in the

context

of these upgrades? Or should one of the Fraction factory methods just

take

BigInteger argumentsm and all fractions use the lazy dynamic
method of
calculation you are proposing?

I don't quite understand what you mean by this. The BigInteger class
provides flexibility and the ability to store and operate on
(practically) unlimited values, which Fraction does not have. The
Fraction class, on the other hand, is faster and more memory
efficient,
due to its use of primitive values, which is an advantage over
BigFraction.

That's fine.



I am even more confused by your suggestion seeing as it was
you who banned BigInteger from Fraction.addSub(Fraction, boolean) in
https://issues.apache.org/jira/browse/NUMBERS-79  , which, even

though

you were not aware of it at that time, did not limit the method's
functionality in any way whatsoever (the use of int rather than long
did, however, but this is now fixed).


I don't know what you mean by "functionality" but constructing a
BigInteger
for every fraction multiplication 

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

[Eric Barnhill]

I'm looking forward to reviewing your code within my limited knowledge,
however I can't guarantee a quick time frame since Apache GSoC mentor
milestones are due next week and I think that could get time consuming.

Thanks for the contribution,
Eric

On Thu, Jul 18, 2019 at 4:13 PM Heinrich Bohne 
wrote:

> So, I think the code I have so far is ripe for a pull request, so I
> submitted one. I changed the contracts of the epsilon and
> max-denominator factory methods, because the old specifications were not
> very useful, especially that of the max-denominator method – the only
> guarantee you could get from it was that /some/ fraction with a
> denominator not greater than the specified maximum would be returned,
> without any relation to the double value that should be approximated.
>
> The simple continued fraction is now created from an exact BigFraction
> representation of the double value rather than with floating-point
> arithmetic on the double value itself, to preserve maximum precision
> (the old epsilon algorithm produces a fraction of 1/3 when passed the
> double value obtained from the expression 1.0/3.0 and an epsilon of 5 *
> 2^(-58), which is incorrect because the distance between the rational
> number 1/3 and its closest double representative is larger than 5 *
> 2^(-58); I added a corresponding unit test).
>
> The methods setLastCoefficient(BigInteger) and removeLastCoefficient()
> in the new class SimpleContinuedFraction are unused. I wrote this class
> before I implemented the algorithms, and I thought these methods might
> be useful in the max-denominator method, but this turned out not to be
> the case. However, from a design-perspective, these two methods
> complement the functionality of addCoefficient(BigInteger), so I thought
> their presence is still tolerable.
>
> I solved the problem with the maxIterations argument in the epsilon
> method by simply not limiting the number of iterations if this argument
> is negative. Maybe this parameter was necessary in the old algorithm
> which calculated the simple continued fraction with floating-point
> arithmetic, to prevent an infinite loop or something.
>
> On 7/17/19 10:20 AM, Heinrich Bohne wrote:
> > It just occured to me that you might have misunderstood my sentence:
> >
> >> I am even more confused by your suggestion seeing as it was
> >> you who banned BigInteger from Fraction.addSub(Fraction, boolean) in
> >> https://issues.apache.org/jira/browse/NUMBERS-79  , which, even though
> >> you were not aware of it at that time, did not limit the method's
> >> functionality in any way whatsoever
> >
> > The "which" was referring to your removal of BigInteger, not to the use
> > of BigInteger prior to your edits, so what I meant to say was, by
> > removing BigInteger, you did not limit the method's functionality.
> >
> >
> > On 7/17/19 10:10 AM, Heinrich Bohne wrote:
> >>> The reason it was done was because Knuth proved
> >>> (as in mathematical proof) that a long is insufficient for certain
> >>> fraction
> >>> multiplications where both numerator and denominator are large ints; 65
> >>> rather than 64 bits are necessary and a long will not suffice.
> >>
> >> You seem to have missed my comment in ticket
> >> https://issues.apache.org/jira/browse/NUMBERS-79 , which you created –
> I
> >> don't have the book by D. Knuth, but I can only assume that the section
> >> referenced by the code talks about unsigned integers, because by the
> >> logic in the comment I left in the JIRA ticket, long values are
> >> **always** sufficient in Fraction.addSub(Fraction, boolean).
> >>
> >> But this is beside the point, I only mentioned it because I didn't
> >> understand why you suggested to remove the BigFraction class, and
> >> actually, I still don't, as the class BigFraction provides functionality
> >> that Fraction cannot have, both with and without my suggested
> >> alterations.
> >>
> >>
> >> On 7/17/19 2:29 AM, Eric Barnhill wrote:
> >>> On Tue, Jul 16, 2019 at 2:41 PM Heinrich Bohne
> >>> wrote:
> >>>
> > Do you think we really even need a BigFraction class at all in the
>  context
> > of these upgrades? Or should one of the Fraction factory methods just
>  take
> > BigInteger argumentsm and all fractions use the lazy dynamic
> > method of
> > calculation you are proposing?
>  I don't quite understand what you mean by this. The BigInteger class
>  provides flexibility and the ability to store and operate on
>  (practically) unlimited values, which Fraction does not have. The
>  Fraction class, on the other hand, is faster and more memory
>  efficient,
>  due to its use of primitive values, which is an advantage over
>  BigFraction.
> >>> That's fine.
> >>>
> >>>
>  I am even more confused by your suggestion seeing as it was
>  you who banned BigInteger from Fraction.addSub(Fraction, boolean) in
>  https://issues.apache.org/jira/browse/NUMBERS-79  , which, even
> though
>  you 

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

[Heinrich Bohne]

So, I think the code I have so far is ripe for a pull request, so I
submitted one. I changed the contracts of the epsilon and
max-denominator factory methods, because the old specifications were not
very useful, especially that of the max-denominator method – the only
guarantee you could get from it was that /some/ fraction with a
denominator not greater than the specified maximum would be returned,
without any relation to the double value that should be approximated.

The simple continued fraction is now created from an exact BigFraction
representation of the double value rather than with floating-point
arithmetic on the double value itself, to preserve maximum precision
(the old epsilon algorithm produces a fraction of 1/3 when passed the
double value obtained from the expression 1.0/3.0 and an epsilon of 5 *
2^(-58), which is incorrect because the distance between the rational
number 1/3 and its closest double representative is larger than 5 *
2^(-58); I added a corresponding unit test).

The methods setLastCoefficient(BigInteger) and removeLastCoefficient()
in the new class SimpleContinuedFraction are unused. I wrote this class
before I implemented the algorithms, and I thought these methods might
be useful in the max-denominator method, but this turned out not to be
the case. However, from a design-perspective, these two methods
complement the functionality of addCoefficient(BigInteger), so I thought
their presence is still tolerable.

I solved the problem with the maxIterations argument in the epsilon
method by simply not limiting the number of iterations if this argument
is negative. Maybe this parameter was necessary in the old algorithm
which calculated the simple continued fraction with floating-point
arithmetic, to prevent an infinite loop or something.

On 7/17/19 10:20 AM, Heinrich Bohne wrote:

It just occured to me that you might have misunderstood my sentence:


I am even more confused by your suggestion seeing as it was
you who banned BigInteger from Fraction.addSub(Fraction, boolean) in
https://issues.apache.org/jira/browse/NUMBERS-79  , which, even though
you were not aware of it at that time, did not limit the method's
functionality in any way whatsoever


The "which" was referring to your removal of BigInteger, not to the use
of BigInteger prior to your edits, so what I meant to say was, by
removing BigInteger, you did not limit the method's functionality.


On 7/17/19 10:10 AM, Heinrich Bohne wrote:

The reason it was done was because Knuth proved
(as in mathematical proof) that a long is insufficient for certain
fraction
multiplications where both numerator and denominator are large ints; 65
rather than 64 bits are necessary and a long will not suffice.


You seem to have missed my comment in ticket
https://issues.apache.org/jira/browse/NUMBERS-79 , which you created – I
don't have the book by D. Knuth, but I can only assume that the section
referenced by the code talks about unsigned integers, because by the
logic in the comment I left in the JIRA ticket, long values are
**always** sufficient in Fraction.addSub(Fraction, boolean).

But this is beside the point, I only mentioned it because I didn't
understand why you suggested to remove the BigFraction class, and
actually, I still don't, as the class BigFraction provides functionality
that Fraction cannot have, both with and without my suggested
alterations.


On 7/17/19 2:29 AM, Eric Barnhill wrote:

On Tue, Jul 16, 2019 at 2:41 PM Heinrich Bohne
wrote:


Do you think we really even need a BigFraction class at all in the

context

of these upgrades? Or should one of the Fraction factory methods just

take

BigInteger argumentsm and all fractions use the lazy dynamic
method of
calculation you are proposing?

I don't quite understand what you mean by this. The BigInteger class
provides flexibility and the ability to store and operate on
(practically) unlimited values, which Fraction does not have. The
Fraction class, on the other hand, is faster and more memory
efficient,
due to its use of primitive values, which is an advantage over
BigFraction.

That's fine.



I am even more confused by your suggestion seeing as it was
you who banned BigInteger from Fraction.addSub(Fraction, boolean) in
https://issues.apache.org/jira/browse/NUMBERS-79  , which, even though
you were not aware of it at that time, did not limit the method's
functionality in any way whatsoever (the use of int rather than long
did, however, but this is now fixed).


I don't know what you mean by "functionality" but constructing a
BigInteger
for every fraction multiplication uses up more memory and operations
than
necessary and scales poorly. BigIntegers are not fast.

However, I understand why the previous coders incorporated a
BigInteger and
I'm not sure that you do. The reason it was done was because Knuth
proved
(as in mathematical proof) that a long is insufficient for certain
fraction
multiplications where both numerator and denominator are large ints; 65
rather 

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

[Heinrich Bohne]

It just occured to me that you might have misunderstood my sentence:


I am even more confused by your suggestion seeing as it was
you who banned BigInteger from Fraction.addSub(Fraction, boolean) in
https://issues.apache.org/jira/browse/NUMBERS-79  , which, even though
you were not aware of it at that time, did not limit the method's
functionality in any way whatsoever


The "which" was referring to your removal of BigInteger, not to the use
of BigInteger prior to your edits, so what I meant to say was, by
removing BigInteger, you did not limit the method's functionality.


On 7/17/19 10:10 AM, Heinrich Bohne wrote:

The reason it was done was because Knuth proved
(as in mathematical proof) that a long is insufficient for certain
fraction
multiplications where both numerator and denominator are large ints; 65
rather than 64 bits are necessary and a long will not suffice.


You seem to have missed my comment in ticket
https://issues.apache.org/jira/browse/NUMBERS-79 , which you created – I
don't have the book by D. Knuth, but I can only assume that the section
referenced by the code talks about unsigned integers, because by the
logic in the comment I left in the JIRA ticket, long values are
**always** sufficient in Fraction.addSub(Fraction, boolean).

But this is beside the point, I only mentioned it because I didn't
understand why you suggested to remove the BigFraction class, and
actually, I still don't, as the class BigFraction provides functionality
that Fraction cannot have, both with and without my suggested
alterations.


On 7/17/19 2:29 AM, Eric Barnhill wrote:

On Tue, Jul 16, 2019 at 2:41 PM Heinrich Bohne
wrote:


Do you think we really even need a BigFraction class at all in the

context

of these upgrades? Or should one of the Fraction factory methods just

take

BigInteger argumentsm and all fractions use the lazy dynamic method of
calculation you are proposing?

I don't quite understand what you mean by this. The BigInteger class
provides flexibility and the ability to store and operate on
(practically) unlimited values, which Fraction does not have. The
Fraction class, on the other hand, is faster and more memory efficient,
due to its use of primitive values, which is an advantage over
BigFraction.

That's fine.



I am even more confused by your suggestion seeing as it was
you who banned BigInteger from Fraction.addSub(Fraction, boolean) in
https://issues.apache.org/jira/browse/NUMBERS-79  , which, even though
you were not aware of it at that time, did not limit the method's
functionality in any way whatsoever (the use of int rather than long
did, however, but this is now fixed).


I don't know what you mean by "functionality" but constructing a
BigInteger
for every fraction multiplication uses up more memory and operations
than
necessary and scales poorly. BigIntegers are not fast.

However, I understand why the previous coders incorporated a
BigInteger and
I'm not sure that you do. The reason it was done was because Knuth
proved
(as in mathematical proof) that a long is insufficient for certain
fraction
multiplications where both numerator and denominator are large ints; 65
rather than 64 bits are necessary and a long will not suffice. For me,
these cases are so extreme and likely so rare that we might as well let
them fail, report to the user that these cases need to be handled with
BigFraction and leave it there. It could easily be handled in a try
catch
block and such a block would be high performance.

That was the judgment I made and it is open to interpretation, provided
such interpretation agrees with Knuth's proof. We are entitled to our
own
opinions but not our own facts.

Anyway I think your approximation schemes sound good and implement them
however you see fit.

Eric




-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org





-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

[Heinrich Bohne]

The reason it was done was because Knuth proved
(as in mathematical proof) that a long is insufficient for certain fraction
multiplications where both numerator and denominator are large ints; 65
rather than 64 bits are necessary and a long will not suffice.


You seem to have missed my comment in ticket
https://issues.apache.org/jira/browse/NUMBERS-79 , which you created – I
don't have the book by D. Knuth, but I can only assume that the section
referenced by the code talks about unsigned integers, because by the
logic in the comment I left in the JIRA ticket, long values are
**always** sufficient in Fraction.addSub(Fraction, boolean).

But this is beside the point, I only mentioned it because I didn't
understand why you suggested to remove the BigFraction class, and
actually, I still don't, as the class BigFraction provides functionality
that Fraction cannot have, both with and without my suggested alterations.


On 7/17/19 2:29 AM, Eric Barnhill wrote:

On Tue, Jul 16, 2019 at 2:41 PM Heinrich Bohne
wrote:


Do you think we really even need a BigFraction class at all in the

context

of these upgrades? Or should one of the Fraction factory methods just

take

BigInteger argumentsm and all fractions use the lazy dynamic method of
calculation you are proposing?

I don't quite understand what you mean by this. The BigInteger class
provides flexibility and the ability to store and operate on
(practically) unlimited values, which Fraction does not have. The
Fraction class, on the other hand, is faster and more memory efficient,
due to its use of primitive values, which is an advantage over
BigFraction.

That's fine.



I am even more confused by your suggestion seeing as it was
you who banned BigInteger from Fraction.addSub(Fraction, boolean) in
https://issues.apache.org/jira/browse/NUMBERS-79  , which, even though
you were not aware of it at that time, did not limit the method's
functionality in any way whatsoever (the use of int rather than long
did, however, but this is now fixed).


I don't know what you mean by "functionality" but constructing a BigInteger
for every fraction multiplication uses up more memory and operations than
necessary and scales poorly. BigIntegers are not fast.

However, I understand why the previous coders incorporated a BigInteger and
I'm not sure that you do. The reason it was done was because Knuth proved
(as in mathematical proof) that a long is insufficient for certain fraction
multiplications where both numerator and denominator are large ints; 65
rather than 64 bits are necessary and a long will not suffice. For me,
these cases are so extreme and likely so rare that we might as well let
them fail, report to the user that these cases need to be handled with
BigFraction and leave it there. It could easily be handled in a try catch
block and such a block would be high performance.

That was the judgment I made and it is open to interpretation, provided
such interpretation agrees with Knuth's proof. We are entitled to our own
opinions but not our own facts.

Anyway I think your approximation schemes sound good and implement them
however you see fit.

Eric




-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

[Eric Barnhill]

On Tue, Jul 16, 2019 at 2:41 PM Heinrich Bohne 
wrote:

> > Do you think we really even need a BigFraction class at all in the
> context
> > of these upgrades? Or should one of the Fraction factory methods just
> take
> > BigInteger argumentsm and all fractions use the lazy dynamic method of
> > calculation you are proposing?
>
> I don't quite understand what you mean by this. The BigInteger class
> provides flexibility and the ability to store and operate on
> (practically) unlimited values, which Fraction does not have. The
> Fraction class, on the other hand, is faster and more memory efficient,
> due to its use of primitive values, which is an advantage over
> BigFraction.


That's fine.


> I am even more confused by your suggestion seeing as it was
> you who banned BigInteger from Fraction.addSub(Fraction, boolean) in
> https://issues.apache.org/jira/browse/NUMBERS-79 , which, even though
> you were not aware of it at that time, did not limit the method's
> functionality in any way whatsoever (the use of int rather than long
> did, however, but this is now fixed).
>

I don't know what you mean by "functionality" but constructing a BigInteger
for every fraction multiplication uses up more memory and operations than
necessary and scales poorly. BigIntegers are not fast.

However, I understand why the previous coders incorporated a BigInteger and
I'm not sure that you do. The reason it was done was because Knuth proved
(as in mathematical proof) that a long is insufficient for certain fraction
multiplications where both numerator and denominator are large ints; 65
rather than 64 bits are necessary and a long will not suffice. For me,
these cases are so extreme and likely so rare that we might as well let
them fail, report to the user that these cases need to be handled with
BigFraction and leave it there. It could easily be handled in a try catch
block and such a block would be high performance.

That was the judgment I made and it is open to interpretation, provided
such interpretation agrees with Knuth's proof. We are entitled to our own
opinions but not our own facts.

Anyway I think your approximation schemes sound good and implement them
however you see fit.

Eric

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

[Heinrich Bohne]

Do you think we really even need a BigFraction class at all in the context
of these upgrades? Or should one of the Fraction factory methods just take
BigInteger argumentsm and all fractions use the lazy dynamic method of
calculation you are proposing?


I don't quite understand what you mean by this. The BigInteger class
provides flexibility and the ability to store and operate on
(practically) unlimited values, which Fraction does not have. The
Fraction class, on the other hand, is faster and more memory efficient,
due to its use of primitive values, which is an advantage over
BigFraction. I am even more confused by your suggestion seeing as it was
you who banned BigInteger from Fraction.addSub(Fraction, boolean) in
https://issues.apache.org/jira/browse/NUMBERS-79 , which, even though
you were not aware of it at that time, did not limit the method's
functionality in any way whatsoever (the use of int rather than long
did, however, but this is now fixed).

What I meant by lazy calculation was that the helper method does not
return the complete simple continued fraction expansion of the given
double value at once, because not all coefficients are needed to
approximate the number if the denominator limit is exceeded before the
last convergent is calculated (or if the convergent is close enough to
the actual value, in epsilon mode). Rather, the coefficients can be
queried one-by-one through an iterator, so that they are only calculated
when they are really needed. I don't see how this could replace or help
with any other functionality of the two fraction classes, though. It's
not like any state associated with the fraction instance itself would be
lazily evaluated. The calculation of the continued fraction expansion is
only relevant for the creation of the instance.

In fact, I have already written the helper class including Javadoc and
unit tests. It turns out that I implemented more methods than I actually
need, but the unneeded methods don't hurt, so I've left them in the class.

I am also finished with implementing the algorithm for approximating a
double number with a denominator limit. The algorithm itself is not very
complicated, but what I found all the more difficult was explaining
_why_ it works without the code comments becoming a thesis, but still in
a way that a person who is not an expert with continued fractions will
understand (which includes me, so I should hopefully be able to tell …).
I'm using some sections from the book Continued Fractions by Aleksandr
Khinchin thatare available in the preview on Google Books as a
reference, in addition to a question from Math Stackexchange.

Now I'm going to delete the unit tests asserting that the factory method
throws an exception if the double value is outside the int range >D

By the way, I think I'll also add a method that allows just an epsilon
to be specified without a parameter maxIterations, because currently,
the only public method for an epsilon-approximation also requires a
maximum number of iterations to be specified, which I think is
ridiculous, not least because the number of iterations seems to me to be
an implementation detail which the user is not necessarily interested
in, and there is no reason not to guarantee that the method will return
an appropriate approximation.


On 7/16/19 6:52 PM, Eric Barnhill wrote:

Sorry for the delay, I was on vacation.

On Fri, Jul 5, 2019 at 2:09 PM Heinrich Bohne  wrote:


Hello!

I think a re-design of the factory method BigFraction.from(double,
double, int, int) is in order, because I see several problems with it:

First, having a separate fraction class intended to overcome the
limitations of the int range with a factory method (formerly a
constructor) for approximating double values that can only produce
denominators within the int range because it has been copy-pasted from
Fraction (where this code is still a constructor) seems a bit like a
joke. I think it would be more useful to have this method accept a
BigInteger as an upper bound for the denominator instead of an int.


Quite right! I wanted to look this up before replying. It absolutely makes
sense to use a BigInteger there.



Second, the method only calculates the convergents of the corresponding
continued fraction, but never its semi-convergents, so it doesn't
necessarily produce the best rational approximation of the double number
within the given bounds. For example, the test method
BigFractionTest.testDigitLimitConstructor() asserts that the method
calculates 3/5 as an approximation of 0.6152 with the upper bound for
the denominator set to 9, but 5/8 = 0.625 is closer to 0.6152 than 3/5 =
0.6. Since the method is already using continued fractions to
approximate fractional numbers, I think it would be a pity if it didn't
take advantage of them for all that they're worth.


Wow. That is indeed problematic, nice catch.



Finally, the documentation of the method rightfully acknowledges the
latter's confusing design, with the method's 

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

[Eric Barnhill]

Sorry for the delay, I was on vacation.

On Fri, Jul 5, 2019 at 2:09 PM Heinrich Bohne  wrote:

> Hello!
>
> I think a re-design of the factory method BigFraction.from(double,
> double, int, int) is in order, because I see several problems with it:
>
> First, having a separate fraction class intended to overcome the
> limitations of the int range with a factory method (formerly a
> constructor) for approximating double values that can only produce
> denominators within the int range because it has been copy-pasted from
> Fraction (where this code is still a constructor) seems a bit like a
> joke. I think it would be more useful to have this method accept a
> BigInteger as an upper bound for the denominator instead of an int.
>

Quite right! I wanted to look this up before replying. It absolutely makes
sense to use a BigInteger there.


>
> Second, the method only calculates the convergents of the corresponding
> continued fraction, but never its semi-convergents, so it doesn't
> necessarily produce the best rational approximation of the double number
> within the given bounds. For example, the test method
> BigFractionTest.testDigitLimitConstructor() asserts that the method
> calculates 3/5 as an approximation of 0.6152 with the upper bound for
> the denominator set to 9, but 5/8 = 0.625 is closer to 0.6152 than 3/5 =
> 0.6. Since the method is already using continued fractions to
> approximate fractional numbers, I think it would be a pity if it didn't
> take advantage of them for all that they're worth.
>

Wow. That is indeed problematic, nice catch.


>
> Finally, the documentation of the method rightfully acknowledges the
> latter's confusing design, with the method's general behavior being
> dependent on some of its arguments and the validity of these arguments
> also being dependent on each other. However, a better way to solve this
> problem than to simply hide the design from the public would be to
> improve it, e.g. by extracting the functionality that is common to both
> the "maxDenominator mode" and the epsilon mode (which is the calculation
> of the continued fraction), and separating the differences in the
> functionality of the two modes into distinct methods that call the
> common functionality.
>

Yes absolutely.


>
> My suggestion for the third point above would be to create a separate
> class (not necessarily public) that provides an interface for
> calculating simple continued fractions and their convergents (I see that
> there's an abstract class ContinuedFraction, but I don't think it will
> be useful, because all the methods only return double values, and the
> class also requires that all coefficients can be explicitly calculated
> based on their index). The class would ideally be able to calculate the
> continued fraction dynamically/lazily, because only a limited number of
> coefficients are needed to approximate a fractional number within given
> bounds.


That would be awesome.


> What I think could be useful is if the class stores a list of
> the coefficients internally in addition to the current and previous
> convergent (two consecutive convergents are needed to calculate the next
> one recursively based on the next coefficient), and has methods like
> addCoefficient(BigInteger) and removeLastCoefficient() for building a
> continued fraction, and also a static method like
> coefficientsOf(BigFraction) that returns an Iterator that
> computes the coefficients only as they are queried through the iterator,
> so that they can then be passed to addCoefficient(BigInteger).
>

+1


>
> The maxDenominator factory method could then just iterate over the
> coefficients of the continued fraction representation of the passed
> double and build the continued fraction from them until the denominator
> of the current convergent exceeds the upper bound,


yes.



> and the epsilon
> method could iterate over the coefficients of both the lower and upper
> bound's continued fraction representation until the coefficients start
> to differ, at which point it can build the continued fraction of the
> close enough approximation from all coefficients at once (this would
> also prevent any loss of precision when repeatedly performing arithmetic
> operations with floating-point values).
>

right.


>
> Furthermore, this code could not only be used by the approximation
> factory methods in BigFraction, but also by those in Fraction, possibly
> adjusted so that not only the denominator must be within a given bound,
> but also the numerator needs to be within the int range.
>
> Any opinions or objections?


All expressed very clearly. This will be a major upgrade for the package.

Do you think we really even need a BigFraction class at all in the context
of these upgrades? Or should one of the Fraction factory methods just take
BigInteger argumentsm and all fractions use the lazy dynamic method of
calculation you are proposing?

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

[Gilles Sadowski]

Hello Heinrich.

Le ven. 5 juil. 2019 à 23:09, Heinrich Bohne  a écrit :
>
> Hello!
>
> I think a re-design of the factory method BigFraction.from(double,
> double, int, int) is in order, because I see several problems with it:
>
> First, having a separate fraction class intended to overcome the
> limitations of the int range with a factory method (formerly a
> constructor) for approximating double values that can only produce
> denominators within the int range because it has been copy-pasted from
> Fraction (where this code is still a constructor) seems a bit like a
> joke. I think it would be more useful to have this method accept a
> BigInteger as an upper bound for the denominator instead of an int.
>
> Second, the method only calculates the convergents of the corresponding
> continued fraction, but never its semi-convergents, so it doesn't
> necessarily produce the best rational approximation of the double number
> within the given bounds. For example, the test method
> BigFractionTest.testDigitLimitConstructor() asserts that the method
> calculates 3/5 as an approximation of 0.6152 with the upper bound for
> the denominator set to 9, but 5/8 = 0.625 is closer to 0.6152 than 3/5 =
> 0.6. Since the method is already using continued fractions to
> approximate fractional numbers, I think it would be a pity if it didn't
> take advantage of them for all that they're worth.
>
> Finally, the documentation of the method rightfully acknowledges the
> latter's confusing design, with the method's general behavior being
> dependent on some of its arguments and the validity of these arguments
> also being dependent on each other. However, a better way to solve this
> problem than to simply hide the design from the public would be to
> improve it, e.g. by extracting the functionality that is common to both
> the "maxDenominator mode" and the epsilon mode (which is the calculation
> of the continued fraction), and separating the differences in the
> functionality of the two modes into distinct methods that call the
> common functionality.
>
> My suggestion for the third point above would be to create a separate
> class (not necessarily public) that provides an interface for
> calculating simple continued fractions and their convergents (I see that
> there's an abstract class ContinuedFraction, but I don't think it will
> be useful, because all the methods only return double values, and the
> class also requires that all coefficients can be explicitly calculated
> based on their index). The class would ideally be able to calculate the
> continued fraction dynamically/lazily, because only a limited number of
> coefficients are needed to approximate a fractional number within given
> bounds. What I think could be useful is if the class stores a list of
> the coefficients internally in addition to the current and previous
> convergent (two consecutive convergents are needed to calculate the next
> one recursively based on the next coefficient), and has methods like
> addCoefficient(BigInteger) and removeLastCoefficient() for building a
> continued fraction, and also a static method like
> coefficientsOf(BigFraction) that returns an Iterator that
> computes the coefficients only as they are queried through the iterator,
> so that they can then be passed to addCoefficient(BigInteger).
>
> The maxDenominator factory method could then just iterate over the
> coefficients of the continued fraction representation of the passed
> double and build the continued fraction from them until the denominator
> of the current convergent exceeds the upper bound, and the epsilon
> method could iterate over the coefficients of both the lower and upper
> bound's continued fraction representation until the coefficients start
> to differ, at which point it can build the continued fraction of the
> close enough approximation from all coefficients at once (this would
> also prevent any loss of precision when repeatedly performing arithmetic
> operations with floating-point values).
>
> Furthermore, this code could not only be used by the approximation
> factory methods in BigFraction, but also by those in Fraction, possibly
> adjusted so that not only the denominator must be within a given bound,
> but also the numerator needs to be within the int range.
>
> Any opinions or objections?

Thanks a lot for these suggestions.  It seems like a plan towards better
consistency, increased robustness and higher precision.

Best,
Gilles

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

[Heinrich Bohne]

Agreed, but it's not clear to me that it must be part of the public API.


Yes, I admit that the public API doesn't really have anything to do with
this, I realize that now. If my last email suggested otherwise, this was
unintentional.


On 7/1/19 11:52 PM, Gilles Sadowski wrote:

Hello.

Le lun. 1 juil. 2019 à 20:23, Heinrich Bohne  a écrit :

Is there a case where *not* knowing whether the fraction is reduced or
not is detrimental?

Hm, maybe you're right and specifying this (as well as where the sign is
stored) in the public API is not that important, as long as the
combination of returned numerator and denominator is valid. The only
benefit would have been that the user would have the certainty that the
returned values are already reduced to lowest terms, so that, in case
this matters to the user, they don't need to reduce the values
themselves. But whether this would outweigh the reduced flexibility of
the API is something that probably cannot be answered without the
ability to predict the future.

However, what *is* crucial is specifying these things in the
documentation of the private fields that hold the numerator and
denominator, because a lot of the class' implementation relies on this.

Sure.
The code doc should indicate all current assumptions.
By not leaking them into the public API, we keep the flexibility until
usage requires to go one way or another.


In fact, I noticed that BigFraction.hashCode() was broken before the
resolution of NUMBERS-119 and NUMBERS-125, because, while
BigFraction.equals(Object) reduced the numerators and denominators
before comparing them, hashCode() didn't.

Specifying the reduction to lowest terms of the fields numerator and
denominator is even more important in Fraction than in BigFraction,
because the magnitude of the values stored in these fields has an impact
on whether arithmetic operations overflow or don't.

Agreed, but it's not clear to me that it must be part of the public API.

Gilles


On 7/1/19 7:03 PM, Gilles Sadowski wrote:

Hello.

Le lun. 1 juil. 2019 à 12:35, Heinrich Bohne  a écrit :

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?

Yes, it would, but I can't envision a scenario where this would be
detrimental. Of course, I may be missing something.

Is there a case where *not* knowing whether the fraction is reduced or
not is detrimental?


But I don't follow; if it's an implementation detail, it should not appear
anywhere in the doc, and users should not rely on it.

What I meant is that it would be an implementation detail how the
numerator and denominator are stored. What the methods getNumerator()
and getDenominator() return should, of course, be specified
appropriately (for example, something like "returns the numerator of
this fraction's representation in lowest terms").

It just occurred to me that the question of where something should be
documented can also be applied to the sign being held by the numerator
rather than the denominator in case of negative fractions (which, if I'm
not mistaken, is currently only specified in
Fraction.getReducedFraction(int, int), a method that is targeted for
deletion in https://issues.apache.org/jira/browse/NUMBERS-125 ).

Good catch.
It's a similar issue: Is knowing where the sign is stored useful to users or
can it be considered an implementation detail (leaving more freedom to
change it)?
Wouldn't a method "isNegative()" be a better alternative?

Regards,
Gilles



On 7/1/19 9:32 AM, Gilles Sadowski wrote:

Hi.

Le lun. 1 juil. 2019 à 03:52, Heinrich Bohne  a écrit :

I've recently been wondering about the following:

With the resolution of NUMBERS-119
(https://issues.apache.org/jira/browse/NUMBERS-119), all constructors in
Fraction and BigFraction reduce the created fraction to lowest terms (in
the constructor Fraction(double, double, int, int), this is not obvious,
but because the convergents of a simple continued fraction are
automatically reduced to lowest terms when calculated with the recursive
formula used in the constructor, the fraction returned thereof is also
reduced to lowest terms).

This rises the following question: Would it not be better if this
reduction to lowest terms were made part of the contract of the two
fields storing the numerator and denominator (i.e. included in their
documentation), and, as a consequence, of the corresponding public
accessor methods?

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?


Then the fact that the fraction is reduced to lowest
terms when created from a constructor or a public factory method would
become an implementation detail irrelevant to the caller of the
method/constructor, which could also prevent confusion, seeing as the
numerator-denominator factory methods are the only ones that explicitly
state that the fraction is reduced to lowest terms, even though this is
true for all other factory methods as well.

The doc should be fixed.
But I 

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

[Gilles Sadowski]

Hello.

Le lun. 1 juil. 2019 à 20:23, Heinrich Bohne  a écrit :
>
> > Is there a case where *not* knowing whether the fraction is reduced or
> > not is detrimental?
>
> Hm, maybe you're right and specifying this (as well as where the sign is
> stored) in the public API is not that important, as long as the
> combination of returned numerator and denominator is valid. The only
> benefit would have been that the user would have the certainty that the
> returned values are already reduced to lowest terms, so that, in case
> this matters to the user, they don't need to reduce the values
> themselves. But whether this would outweigh the reduced flexibility of
> the API is something that probably cannot be answered without the
> ability to predict the future.
>
> However, what *is* crucial is specifying these things in the
> documentation of the private fields that hold the numerator and
> denominator, because a lot of the class' implementation relies on this.

Sure.
The code doc should indicate all current assumptions.
By not leaking them into the public API, we keep the flexibility until
usage requires to go one way or another.

> In fact, I noticed that BigFraction.hashCode() was broken before the
> resolution of NUMBERS-119 and NUMBERS-125, because, while
> BigFraction.equals(Object) reduced the numerators and denominators
> before comparing them, hashCode() didn't.
>
> Specifying the reduction to lowest terms of the fields numerator and
> denominator is even more important in Fraction than in BigFraction,
> because the magnitude of the values stored in these fields has an impact
> on whether arithmetic operations overflow or don't.

Agreed, but it's not clear to me that it must be part of the public API.

Gilles

>
> On 7/1/19 7:03 PM, Gilles Sadowski wrote:
> > Hello.
> >
> > Le lun. 1 juil. 2019 à 12:35, Heinrich Bohne  a 
> > écrit :
> >>> Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
> >>> for efficiency reason?
> >> Yes, it would, but I can't envision a scenario where this would be
> >> detrimental. Of course, I may be missing something.
> > Is there a case where *not* knowing whether the fraction is reduced or
> > not is detrimental?
> >
> >>> But I don't follow; if it's an implementation detail, it should not appear
> >>> anywhere in the doc, and users should not rely on it.
> >> What I meant is that it would be an implementation detail how the
> >> numerator and denominator are stored. What the methods getNumerator()
> >> and getDenominator() return should, of course, be specified
> >> appropriately (for example, something like "returns the numerator of
> >> this fraction's representation in lowest terms").
> >>
> >> It just occurred to me that the question of where something should be
> >> documented can also be applied to the sign being held by the numerator
> >> rather than the denominator in case of negative fractions (which, if I'm
> >> not mistaken, is currently only specified in
> >> Fraction.getReducedFraction(int, int), a method that is targeted for
> >> deletion in https://issues.apache.org/jira/browse/NUMBERS-125 ).
> > Good catch.
> > It's a similar issue: Is knowing where the sign is stored useful to users or
> > can it be considered an implementation detail (leaving more freedom to
> > change it)?
> > Wouldn't a method "isNegative()" be a better alternative?
> >
> > Regards,
> > Gilles
> >
> >
> >>
> >> On 7/1/19 9:32 AM, Gilles Sadowski wrote:
> >>> Hi.
> >>>
> >>> Le lun. 1 juil. 2019 à 03:52, Heinrich Bohne  a 
> >>> écrit :
>  I've recently been wondering about the following:
> 
>  With the resolution of NUMBERS-119
>  (https://issues.apache.org/jira/browse/NUMBERS-119), all constructors in
>  Fraction and BigFraction reduce the created fraction to lowest terms (in
>  the constructor Fraction(double, double, int, int), this is not obvious,
>  but because the convergents of a simple continued fraction are
>  automatically reduced to lowest terms when calculated with the recursive
>  formula used in the constructor, the fraction returned thereof is also
>  reduced to lowest terms).
> 
>  This rises the following question: Would it not be better if this
>  reduction to lowest terms were made part of the contract of the two
>  fields storing the numerator and denominator (i.e. included in their
>  documentation), and, as a consequence, of the corresponding public
>  accessor methods?
> >>> Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
> >>> for efficiency reason?
> >>>
>  Then the fact that the fraction is reduced to lowest
>  terms when created from a constructor or a public factory method would
>  become an implementation detail irrelevant to the caller of the
>  method/constructor, which could also prevent confusion, seeing as the
>  numerator-denominator factory methods are the only ones that explicitly
>  state that the fraction is 

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

[Heinrich Bohne]

Is there a case where *not* knowing whether the fraction is reduced or
not is detrimental?


Hm, maybe you're right and specifying this (as well as where the sign is
stored) in the public API is not that important, as long as the
combination of returned numerator and denominator is valid. The only
benefit would have been that the user would have the certainty that the
returned values are already reduced to lowest terms, so that, in case
this matters to the user, they don't need to reduce the values
themselves. But whether this would outweigh the reduced flexibility of
the API is something that probably cannot be answered without the
ability to predict the future.

However, what *is* crucial is specifying these things in the
documentation of the private fields that hold the numerator and
denominator, because a lot of the class' implementation relies on this.
In fact, I noticed that BigFraction.hashCode() was broken before the
resolution of NUMBERS-119 and NUMBERS-125, because, while
BigFraction.equals(Object) reduced the numerators and denominators
before comparing them, hashCode() didn't.

Specifying the reduction to lowest terms of the fields numerator and
denominator is even more important in Fraction than in BigFraction,
because the magnitude of the values stored in these fields has an impact
on whether arithmetic operations overflow or don't.

On 7/1/19 7:03 PM, Gilles Sadowski wrote:

Hello.

Le lun. 1 juil. 2019 à 12:35, Heinrich Bohne  a écrit :

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?

Yes, it would, but I can't envision a scenario where this would be
detrimental. Of course, I may be missing something.

Is there a case where *not* knowing whether the fraction is reduced or
not is detrimental?


But I don't follow; if it's an implementation detail, it should not appear
anywhere in the doc, and users should not rely on it.

What I meant is that it would be an implementation detail how the
numerator and denominator are stored. What the methods getNumerator()
and getDenominator() return should, of course, be specified
appropriately (for example, something like "returns the numerator of
this fraction's representation in lowest terms").

It just occurred to me that the question of where something should be
documented can also be applied to the sign being held by the numerator
rather than the denominator in case of negative fractions (which, if I'm
not mistaken, is currently only specified in
Fraction.getReducedFraction(int, int), a method that is targeted for
deletion in https://issues.apache.org/jira/browse/NUMBERS-125 ).

Good catch.
It's a similar issue: Is knowing where the sign is stored useful to users or
can it be considered an implementation detail (leaving more freedom to
change it)?
Wouldn't a method "isNegative()" be a better alternative?

Regards,
Gilles




On 7/1/19 9:32 AM, Gilles Sadowski wrote:

Hi.

Le lun. 1 juil. 2019 à 03:52, Heinrich Bohne  a écrit :

I've recently been wondering about the following:

With the resolution of NUMBERS-119
(https://issues.apache.org/jira/browse/NUMBERS-119), all constructors in
Fraction and BigFraction reduce the created fraction to lowest terms (in
the constructor Fraction(double, double, int, int), this is not obvious,
but because the convergents of a simple continued fraction are
automatically reduced to lowest terms when calculated with the recursive
formula used in the constructor, the fraction returned thereof is also
reduced to lowest terms).

This rises the following question: Would it not be better if this
reduction to lowest terms were made part of the contract of the two
fields storing the numerator and denominator (i.e. included in their
documentation), and, as a consequence, of the corresponding public
accessor methods?

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?


Then the fact that the fraction is reduced to lowest
terms when created from a constructor or a public factory method would
become an implementation detail irrelevant to the caller of the
method/constructor, which could also prevent confusion, seeing as the
numerator-denominator factory methods are the only ones that explicitly
state that the fraction is reduced to lowest terms, even though this is
true for all other factory methods as well.

The doc should be fixed.
But I don't follow; if it's an implementation detail, it should not appear
anywhere in the doc, and users should not rely on it.

Regards,
Gilles


Also, fractions returned from arithmetic operations are thereby also
implicitly specified as being reduced to lowest terms, rendering
mentions of this fact in these methods' documentation obsolete.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org





-

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

[Gilles Sadowski]

Hello.

Le lun. 1 juil. 2019 à 12:35, Heinrich Bohne  a écrit :
>
> > Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
> > for efficiency reason?
>
> Yes, it would, but I can't envision a scenario where this would be
> detrimental. Of course, I may be missing something.

Is there a case where *not* knowing whether the fraction is reduced or
not is detrimental?

>
> > But I don't follow; if it's an implementation detail, it should not appear
> > anywhere in the doc, and users should not rely on it.
>
> What I meant is that it would be an implementation detail how the
> numerator and denominator are stored. What the methods getNumerator()
> and getDenominator() return should, of course, be specified
> appropriately (for example, something like "returns the numerator of
> this fraction's representation in lowest terms").
>
> It just occurred to me that the question of where something should be
> documented can also be applied to the sign being held by the numerator
> rather than the denominator in case of negative fractions (which, if I'm
> not mistaken, is currently only specified in
> Fraction.getReducedFraction(int, int), a method that is targeted for
> deletion in https://issues.apache.org/jira/browse/NUMBERS-125 ).

Good catch.
It's a similar issue: Is knowing where the sign is stored useful to users or
can it be considered an implementation detail (leaving more freedom to
change it)?
Wouldn't a method "isNegative()" be a better alternative?

Regards,
Gilles


>
>
> On 7/1/19 9:32 AM, Gilles Sadowski wrote:
> > Hi.
> >
> > Le lun. 1 juil. 2019 à 03:52, Heinrich Bohne  a 
> > écrit :
> >> I've recently been wondering about the following:
> >>
> >> With the resolution of NUMBERS-119
> >> (https://issues.apache.org/jira/browse/NUMBERS-119), all constructors in
> >> Fraction and BigFraction reduce the created fraction to lowest terms (in
> >> the constructor Fraction(double, double, int, int), this is not obvious,
> >> but because the convergents of a simple continued fraction are
> >> automatically reduced to lowest terms when calculated with the recursive
> >> formula used in the constructor, the fraction returned thereof is also
> >> reduced to lowest terms).
> >>
> >> This rises the following question: Would it not be better if this
> >> reduction to lowest terms were made part of the contract of the two
> >> fields storing the numerator and denominator (i.e. included in their
> >> documentation), and, as a consequence, of the corresponding public
> >> accessor methods?
> > Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
> > for efficiency reason?
> >
> >> Then the fact that the fraction is reduced to lowest
> >> terms when created from a constructor or a public factory method would
> >> become an implementation detail irrelevant to the caller of the
> >> method/constructor, which could also prevent confusion, seeing as the
> >> numerator-denominator factory methods are the only ones that explicitly
> >> state that the fraction is reduced to lowest terms, even though this is
> >> true for all other factory methods as well.
> > The doc should be fixed.
> > But I don't follow; if it's an implementation detail, it should not appear
> > anywhere in the doc, and users should not rely on it.
> >
> > Regards,
> > Gilles
> >
> >> Also, fractions returned from arithmetic operations are thereby also
> >> implicitly specified as being reduced to lowest terms, rendering
> >> mentions of this fact in these methods' documentation obsolete.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

[Heinrich Bohne]

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?


Yes, it would, but I can't envision a scenario where this would be
detrimental. Of course, I may be missing something.


But I don't follow; if it's an implementation detail, it should not appear
anywhere in the doc, and users should not rely on it.


What I meant is that it would be an implementation detail how the
numerator and denominator are stored. What the methods getNumerator()
and getDenominator() return should, of course, be specified
appropriately (for example, something like "returns the numerator of
this fraction's representation in lowest terms").

It just occurred to me that the question of where something should be
documented can also be applied to the sign being held by the numerator
rather than the denominator in case of negative fractions (which, if I'm
not mistaken, is currently only specified in
Fraction.getReducedFraction(int, int), a method that is targeted for
deletion in https://issues.apache.org/jira/browse/NUMBERS-125 ).


On 7/1/19 9:32 AM, Gilles Sadowski wrote:

Hi.

Le lun. 1 juil. 2019 à 03:52, Heinrich Bohne  a écrit :

I've recently been wondering about the following:

With the resolution of NUMBERS-119
(https://issues.apache.org/jira/browse/NUMBERS-119), all constructors in
Fraction and BigFraction reduce the created fraction to lowest terms (in
the constructor Fraction(double, double, int, int), this is not obvious,
but because the convergents of a simple continued fraction are
automatically reduced to lowest terms when calculated with the recursive
formula used in the constructor, the fraction returned thereof is also
reduced to lowest terms).

This rises the following question: Would it not be better if this
reduction to lowest terms were made part of the contract of the two
fields storing the numerator and denominator (i.e. included in their
documentation), and, as a consequence, of the corresponding public
accessor methods?

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?


Then the fact that the fraction is reduced to lowest
terms when created from a constructor or a public factory method would
become an implementation detail irrelevant to the caller of the
method/constructor, which could also prevent confusion, seeing as the
numerator-denominator factory methods are the only ones that explicitly
state that the fraction is reduced to lowest terms, even though this is
true for all other factory methods as well.

The doc should be fixed.
But I don't follow; if it's an implementation detail, it should not appear
anywhere in the doc, and users should not rely on it.

Regards,
Gilles


Also, fractions returned from arithmetic operations are thereby also
implicitly specified as being reduced to lowest terms, rendering
mentions of this fact in these methods' documentation obsolete.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org





-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

[Gilles Sadowski]

Hi.

Le lun. 1 juil. 2019 à 03:52, Heinrich Bohne  a écrit :
>
> I've recently been wondering about the following:
>
> With the resolution of NUMBERS-119
> (https://issues.apache.org/jira/browse/NUMBERS-119), all constructors in
> Fraction and BigFraction reduce the created fraction to lowest terms (in
> the constructor Fraction(double, double, int, int), this is not obvious,
> but because the convergents of a simple continued fraction are
> automatically reduced to lowest terms when calculated with the recursive
> formula used in the constructor, the fraction returned thereof is also
> reduced to lowest terms).
>
> This rises the following question: Would it not be better if this
> reduction to lowest terms were made part of the contract of the two
> fields storing the numerator and denominator (i.e. included in their
> documentation), and, as a consequence, of the corresponding public
> accessor methods?

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?

> Then the fact that the fraction is reduced to lowest
> terms when created from a constructor or a public factory method would
> become an implementation detail irrelevant to the caller of the
> method/constructor, which could also prevent confusion, seeing as the
> numerator-denominator factory methods are the only ones that explicitly
> state that the fraction is reduced to lowest terms, even though this is
> true for all other factory methods as well.

The doc should be fixed.
But I don't follow; if it's an implementation detail, it should not appear
anywhere in the doc, and users should not rely on it.

Regards,
Gilles

> Also, fractions returned from arithmetic operations are thereby also
> implicitly specified as being reduced to lowest terms, rendering
> mentions of this fact in these methods' documentation obsolete.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

[Heinrich Bohne]

I've created a pull request:
https://github.com/apache/commons-numbers/pull/55

This pull request exterminates most the code duplication between the two
classes. There is still some duplication left, most notably in the
method testDigitLimitConstructor(), but I've found out that the
Fraction(double, double, int, int) constructor (and the corresponding
BigFraction constructor, which seems to have been copy-pasted from
Fraction) doesn't work properly anyway, because it doesn't always
generate the closest possible approximation within the given bounds.

For example, 5/8 = 0.625 is closer to 0.6152 than 3/5 = 0.6, but
apparently, the constructor returns 3/5 when 9 is passed as
maxDenominator, and since the test asserts this, the test is useless.

I saw that there's a class ContinuedFraction, maybe this class can
somehow be used within the constructor instead of duplicating broken
code between two classes, so I didn't bother extracting the above tests.

Also, I created a new branch and renamed the commits to include the
number of the JIRA ticket, which means that the revision numbers are now
different from those in the branch I previously referenced, but the
changes made in the commits are the same, in case anyone is confused.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

[Eric Barnhill]

>
> > If additional context is required it fails to meet the definition of
> > a unit test and is instead an integration test,  and the function being
> > tested may require rethinking.
>
> Depends what you define as a unit test. I'd say the unit was BigFraction
> or Fraction. An integration test is something that must be tested with
> coherant components working together to provide functionality. You are
> not doing that.
>

Well, I totally agree with both of you that this is the superior approach
for architecture and maintainability. Maybe I should think about adding a
bit more setup to my own unit tests.

I think it is not quite right to say that Fraction is the unit. I think
unit tests test atomic behaviors in the code that ideally can only fail one
way; those are the units. But this is just semantics.

So if you are both in agreement I can change to a +1.

Re: Re: Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

[Heinrich Bohne]

By the way, I've worked a bit on the draft in the meantime and pushed
the changes I've made so far, in case anyone is interested in
(re-)viewing them. Here's the link to the branch again:
https://github.com/Schamschi/commons-numbers/tree/FractionCommonTestCases

On 6/20/19 6:13 PM, Heinrich Bohne wrote:

Hello Eric,

I'm not sure if I understand what you mean by "context" when you say
that all context has to be within the unit test.Do you mean that the
test should not rely on the functionality of other
modules/methods/"units" than the one to be tested? If so, I agree with
you, but I don't think that this would be the case in this scenario.

The test methods in FractionTest would still only test the functionality
of Fraction, and those in BigFractionTest only that of BigFraction. It's
just that, in cases where similar/analogous functionality is tested,
they don't "come up" with test cases, i.e. combinations of input and
expected output values, themselves, but rather draw these from an
outside source that just happens to be queried by unit tests of a
different, entirely unrelated class as well.

The class FractionTest does not care that the class BigFractionTest
retrieves some of its test cases from the same source as FractionTest
does and vice versa. Likewise, the hypothetical class CommonTestCases
does not care what FractionTest or BigFractionTest do with the test
cases it provides, or if they use them at all. What matters is that
FractionTest and BigFractionTest each take responsibility for ensuring
that the test cases provided by CommonTestCases are applicable to the
functionality they want to test. Of course, this requires that
CommonTestCases clearly documents the test cases it provides. If, for
example, CommonTestCases were to provide test cases that are completely
useless to both FractionTest and BigFractionTest, then it would be the
responsibility of FractionTest and BigFractionTest to disregard them.

So the tests performed by FractionTest and BigFractionTest would not
rely on any functionality outside the unit they test, except for the
validity of the test cases. But erroneous test cases can happen
regardless of whether they are declared in the test class itself or
outside.


On 6/20/19 5:07 PM, Eric Barnhill wrote:

Sorry for the slow reply, I thought I sent this yesterday.

I agree from a code architecture standpoint such a refactoring makes
sense.
However from the perspective of unit tests it makes it no longer a unit
test.

IIUC it's best practice for a unit test that all context be within the
test. If additional context is required it fails to meet the
definition of
a unit test and is instead an integration test,  and the function being
tested may require rethinking.

This results in unit tests often being clunkily and awkwardly coded,
but I
think it is the way they are typically written and it has its reasons.

  So I am +0 .

On Thu, Jun 20, 2019, 02:01 Heinrich Bohne 
wrote:


A quick looks shows that the BigFractionTest does have test cases for

very large numbers. However the add, subtract, divide and multiply
tests
and a few others just use values that would work with Fraction.
Possibly
these can be moved to a shared common tests location too.

That's what I was thinking too – the draft was by no means intended
to be
complete, I just created it to give a general idea of how I would go
about
implementing this. I'll work some more on it before I create an
actual pull
request.


On 6/20/19 10:40 AM, Alex Herbert wrote:

On 20 Jun 2019, at 00:54, Heinrich Bohne 
wrote:

An awful lot of code is duplicated between FractionTest and
BigFractionTest. Often, the test cases in the two classes only
differ in
the types they use (e.g. Fraction vs. BigFraction), but the actual
values the tests use are the same.

I think this could be mitigated by adding a new class that stores the
values for these common test cases, and the classes FractionTest and
BigFractionTest retrieve the values from this class and only
implement
the test patterns.

I created a draft here:


https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f


Any opinions on this?

1. BigFraction should work the same way as Fraction when the
numbers are

the same

So collecting the common tests together makes sense. The change in the

PR looks good.

2. BigFraction should work with numbers that cannot be handled by

Fraction

A quick looks shows that the BigFractionTest does have test cases for

very large numbers. However the add, subtract, divide and multiply
tests
and a few others just use values that would work with Fraction.
Possibly
these can be moved to a shared common tests location too.

Then variants added using BigInteger arguments just to make sure
the Big

part of BigFraction is working.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

[Heinrich Bohne]

Hello Eric,

I'm not sure if I understand what you mean by "context" when you say
that all context has to be within the unit test.Do you mean that the
test should not rely on the functionality of other
modules/methods/"units" than the one to be tested? If so, I agree with
you, but I don't think that this would be the case in this scenario.

The test methods in FractionTest would still only test the functionality
of Fraction, and those in BigFractionTest only that of BigFraction. It's
just that, in cases where similar/analogous functionality is tested,
they don't "come up" with test cases, i.e. combinations of input and
expected output values, themselves, but rather draw these from an
outside source that just happens to be queried by unit tests of a
different, entirely unrelated class as well.

The class FractionTest does not care that the class BigFractionTest
retrieves some of its test cases from the same source as FractionTest
does and vice versa. Likewise, the hypothetical class CommonTestCases
does not care what FractionTest or BigFractionTest do with the test
cases it provides, or if they use them at all. What matters is that
FractionTest and BigFractionTest each take responsibility for ensuring
that the test cases provided by CommonTestCases are applicable to the
functionality they want to test. Of course, this requires that
CommonTestCases clearly documents the test cases it provides. If, for
example, CommonTestCases were to provide test cases that are completely
useless to both FractionTest and BigFractionTest, then it would be the
responsibility of FractionTest and BigFractionTest to disregard them.

So the tests performed by FractionTest and BigFractionTest would not
rely on any functionality outside the unit they test, except for the
validity of the test cases. But erroneous test cases can happen
regardless of whether they are declared in the test class itself or outside.


On 6/20/19 5:07 PM, Eric Barnhill wrote:

Sorry for the slow reply, I thought I sent this yesterday.

I agree from a code architecture standpoint such a refactoring makes sense.
However from the perspective of unit tests it makes it no longer a unit
test.

IIUC it's best practice for a unit test that all context be within the
test. If additional context is required it fails to meet the definition of
a unit test and is instead an integration test,  and the function being
tested may require rethinking.

This results in unit tests often being clunkily and awkwardly coded, but I
think it is the way they are typically written and it has its reasons.

  So I am +0 .

On Thu, Jun 20, 2019, 02:01 Heinrich Bohne  wrote:


A quick looks shows that the BigFractionTest does have test cases for

very large numbers. However the add, subtract, divide and multiply tests
and a few others just use values that would work with Fraction. Possibly
these can be moved to a shared common tests location too.

That's what I was thinking too – the draft was by no means intended to be
complete, I just created it to give a general idea of how I would go about
implementing this. I'll work some more on it before I create an actual pull
request.


On 6/20/19 10:40 AM, Alex Herbert wrote:

On 20 Jun 2019, at 00:54, Heinrich Bohne  wrote:

An awful lot of code is duplicated between FractionTest and
BigFractionTest. Often, the test cases in the two classes only differ in
the types they use (e.g. Fraction vs. BigFraction), but the actual
values the tests use are the same.

I think this could be mitigated by adding a new class that stores the
values for these common test cases, and the classes FractionTest and
BigFractionTest retrieve the values from this class and only implement
the test patterns.

I created a draft here:


https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f

Any opinions on this?

1. BigFraction should work the same way as Fraction when the numbers are

the same

So collecting the common tests together makes sense. The change in the

PR looks good.

2. BigFraction should work with numbers that cannot be handled by

Fraction

A quick looks shows that the BigFractionTest does have test cases for

very large numbers. However the add, subtract, divide and multiply tests
and a few others just use values that would work with Fraction. Possibly
these can be moved to a shared common tests location too.

Then variants added using BigInteger arguments just to make sure the Big

part of BigFraction is working.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org


-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org




-
To unsubscribe, e-mail: 

Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

[Alex Herbert]

On 20/06/2019 16:07, Eric Barnhill wrote:

Sorry for the slow reply, I thought I sent this yesterday.

I agree from a code architecture standpoint such a refactoring makes sense.
However from the perspective of unit tests it makes it no longer a unit
test.
It is still testing a unit. Just the test code is spread out over 
multiple files so it can be reused.


IIUC it's best practice for a unit test that all context be within the
test.


If the purpose of the test is to ensure two pieces of code do the same 
thing then perhaps a rewrite as a parameterized test. Unfortunately 
BigFraction and Fraction do not share a common interface that the test 
is checking. So a parameterized test is not as simple as passing in an 
instance of one or the other (or a reference to the factory constructor 
method). Putting all the test cases in a single place rather than 
duplicating them in two files seems like the most maintainable going 
forward.



If additional context is required it fails to meet the definition of
a unit test and is instead an integration test,  and the function being
tested may require rethinking.


Depends what you define as a unit test. I'd say the unit was BigFraction 
or Fraction. An integration test is something that must be tested with 
coherant components working together to provide functionality. You are 
not doing that.


I'd say that if you executed the all the tests in the Test file and it 
achieves 100% coverage of the class it is aimed at then it is a unit 
test. Even if your Test file uses code from other places.


In this instance the code from other places amounts to a list of 
arguments and expected results. Duplicating this data just to make the 
Test file standalone is less maintainable.




This results in unit tests often being clunkily and awkwardly coded, but I
think it is the way they are typically written and it has its reasons.

  So I am +0 .

On Thu, Jun 20, 2019, 02:01 Heinrich Bohne  wrote:


A quick looks shows that the BigFractionTest does have test cases for

very large numbers. However the add, subtract, divide and multiply tests
and a few others just use values that would work with Fraction. Possibly
these can be moved to a shared common tests location too.

That's what I was thinking too – the draft was by no means intended to be
complete, I just created it to give a general idea of how I would go about
implementing this. I'll work some more on it before I create an actual pull
request.


On 6/20/19 10:40 AM, Alex Herbert wrote:

On 20 Jun 2019, at 00:54, Heinrich Bohne  wrote:

An awful lot of code is duplicated between FractionTest and
BigFractionTest. Often, the test cases in the two classes only differ in
the types they use (e.g. Fraction vs. BigFraction), but the actual
values the tests use are the same.

I think this could be mitigated by adding a new class that stores the
values for these common test cases, and the classes FractionTest and
BigFractionTest retrieve the values from this class and only implement
the test patterns.

I created a draft here:


https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f

Any opinions on this?

1. BigFraction should work the same way as Fraction when the numbers are

the same

So collecting the common tests together makes sense. The change in the

PR looks good.

2. BigFraction should work with numbers that cannot be handled by

Fraction

A quick looks shows that the BigFractionTest does have test cases for

very large numbers. However the add, subtract, divide and multiply tests
and a few others just use values that would work with Fraction. Possibly
these can be moved to a shared common tests location too.

Then variants added using BigInteger arguments just to make sure the Big

part of BigFraction is working.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org


-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org




-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org




-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

[Eric Barnhill]

Sorry for the slow reply, I thought I sent this yesterday.

I agree from a code architecture standpoint such a refactoring makes sense.
However from the perspective of unit tests it makes it no longer a unit
test.

IIUC it's best practice for a unit test that all context be within the
test. If additional context is required it fails to meet the definition of
a unit test and is instead an integration test,  and the function being
tested may require rethinking.

This results in unit tests often being clunkily and awkwardly coded, but I
think it is the way they are typically written and it has its reasons.

 So I am +0 .

On Thu, Jun 20, 2019, 02:01 Heinrich Bohne  wrote:

> > A quick looks shows that the BigFractionTest does have test cases for
> very large numbers. However the add, subtract, divide and multiply tests
> and a few others just use values that would work with Fraction. Possibly
> these can be moved to a shared common tests location too.
>
> That's what I was thinking too – the draft was by no means intended to be
> complete, I just created it to give a general idea of how I would go about
> implementing this. I'll work some more on it before I create an actual pull
> request.
>
>
> On 6/20/19 10:40 AM, Alex Herbert wrote:
> >
> >> On 20 Jun 2019, at 00:54, Heinrich Bohne  wrote:
> >>
> >> An awful lot of code is duplicated between FractionTest and
> >> BigFractionTest. Often, the test cases in the two classes only differ in
> >> the types they use (e.g. Fraction vs. BigFraction), but the actual
> >> values the tests use are the same.
> >>
> >> I think this could be mitigated by adding a new class that stores the
> >> values for these common test cases, and the classes FractionTest and
> >> BigFractionTest retrieve the values from this class and only implement
> >> the test patterns.
> >>
> >> I created a draft here:
> >>
> https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f
> >>
> >> Any opinions on this?
> > 1. BigFraction should work the same way as Fraction when the numbers are
> the same
> >
> > So collecting the common tests together makes sense. The change in the
> PR looks good.
> >
> > 2. BigFraction should work with numbers that cannot be handled by
> Fraction
> >
> > A quick looks shows that the BigFractionTest does have test cases for
> very large numbers. However the add, subtract, divide and multiply tests
> and a few others just use values that would work with Fraction. Possibly
> these can be moved to a shared common tests location too.
> >
> > Then variants added using BigInteger arguments just to make sure the Big
> part of BigFraction is working.
> >
> >> -
> >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> >> For additional commands, e-mail: dev-h...@commons.apache.org
> >>
> >
> > -
> > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> > For additional commands, e-mail: dev-h...@commons.apache.org
> >
> >
>
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>

Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

[Heinrich Bohne]

A quick looks shows that the BigFractionTest does have test cases for very 
large numbers. However the add, subtract, divide and multiply tests and a few 
others just use values that would work with Fraction. Possibly these can be 
moved to a shared common tests location too.


That's what I was thinking too – the draft was by no means intended to be 
complete, I just created it to give a general idea of how I would go about 
implementing this. I'll work some more on it before I create an actual pull 
request.


On 6/20/19 10:40 AM, Alex Herbert wrote:



On 20 Jun 2019, at 00:54, Heinrich Bohne  wrote:

An awful lot of code is duplicated between FractionTest and
BigFractionTest. Often, the test cases in the two classes only differ in
the types they use (e.g. Fraction vs. BigFraction), but the actual
values the tests use are the same.

I think this could be mitigated by adding a new class that stores the
values for these common test cases, and the classes FractionTest and
BigFractionTest retrieve the values from this class and only implement
the test patterns.

I created a draft here:
https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f

Any opinions on this?

1. BigFraction should work the same way as Fraction when the numbers are the 
same

So collecting the common tests together makes sense. The change in the PR looks 
good.

2. BigFraction should work with numbers that cannot be handled by Fraction

A quick looks shows that the BigFractionTest does have test cases for very 
large numbers. However the add, subtract, divide and multiply tests and a few 
others just use values that would work with Fraction. Possibly these can be 
moved to a shared common tests location too.

Then variants added using BigInteger arguments just to make sure the Big part 
of BigFraction is working.


-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org





-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

[Alex Herbert]

> On 20 Jun 2019, at 00:54, Heinrich Bohne  wrote:
> 
> An awful lot of code is duplicated between FractionTest and
> BigFractionTest. Often, the test cases in the two classes only differ in
> the types they use (e.g. Fraction vs. BigFraction), but the actual
> values the tests use are the same.
> 
> I think this could be mitigated by adding a new class that stores the
> values for these common test cases, and the classes FractionTest and
> BigFractionTest retrieve the values from this class and only implement
> the test patterns.
> 
> I created a draft here:
> https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f
> 
> Any opinions on this?

1. BigFraction should work the same way as Fraction when the numbers are the 
same

So collecting the common tests together makes sense. The change in the PR looks 
good.

2. BigFraction should work with numbers that cannot be handled by Fraction

A quick looks shows that the BigFractionTest does have test cases for very 
large numbers. However the add, subtract, divide and multiply tests and a few 
others just use values that would work with Fraction. Possibly these can be 
moved to a shared common tests location too.

Then variants added using BigInteger arguments just to make sure the Big part 
of BigFraction is working.

> 
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
> 


-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers][fraction] pulling fraction-dev into master

[Eric Barnhill]

Changed are merged; in particular the travis updates were kept; if you are
working on Fraction kindly rebase.

On Wed, Jun 5, 2019 at 3:40 PM Eric Barnhill  wrote:

> For some months I worked on the Fraction class on a fraction-dev branch,
> now others are furthering it, but IIUC working off of master, plus it
> sounds like my edits are out of date in other ways.
>
> So within the next day, I will pull fraction-dev into master. I would
> request any other contributors contributing to Fraction, to merge these
> changes into their own work and rebase.
>
> I was at the final checkstyle edits of what I was working on, so hopefully
> it will not cause anyone more than minor conveniences. If it will cause you
> a larger inconvenience and you would rather work together to merge it,
> please post here. If you find after the fact it causes you a headache I can
> roll it back, I will keep the branch around for a while.
>
> Eric
>

Re: [numbers-fraction] merging changes

[Gilles Sadowski]

Hello Eric.

Le mar. 26 mars 2019 à 17:22, Eric Barnhill  a écrit :
>
> I'm rebasing fraction to master and the next merge is looking tricky.
> Gilles, am I correct that you have added an interface and some methods to
> Fraction, and pushed this to master?

I don't think so.  Last commit I made to "commons-numbers-fraction",
more than 3 months ago, is:
---CUT---
commit d0cff0d8f2a249e6c87df323fe635386749f62f4
Author: Gilles Sadowski 
Date:   Mon Dec 17 12:59:24 2018 +0100

NUMBERS-83: Replace calls to JDK deprecated API.

---CUT---

So I've no idea about what could be "tricky". 8-)

> But master does not yet have the of()
> and from() method name changes that we discussed while my branch does, also
> my branch has parse().
>
> If you like I can merge these two branches together. What I see remaining
> to do with Fraction is:
> - Write some unit tests for parse()
> - Figure out what we are doing with the Format classes that we are no
> longer using. Just delete them, knowing they are previous releases if
> someone wants to implement numbers-formats some day?

I'm for deleting them.
And if we ever need the code (e.g. to implement a more IO related
module) we can recover it from git...

Best,
Gilles

>
> Those are all the remaining changes I see.
>
> Eric

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Maven surefire plugin error

[Eric Barnhill]

Rebasing on master fixed the problem, thank you.

On Mon, Mar 4, 2019 at 3:39 PM Gilles Sadowski  wrote:

>
>
> I'd recommend that you regularly rebase from "master".
>
> Regards,
> Gilles
>
> >
> > > Here is an excerpt of the exception trace (from running "mvn -e test"):
> > > ---CUT---
> > > Caused by: java.lang.NullPointerException
> > >at
> org.apache.maven.surefire.shade.org.apache.commons.lang3.SystemUtils.isJavaVersionAtLeast
> > > (SystemUtils.java:1626)
> > >at
> org.apache.maven.plugin.surefire.AbstractSurefireMojo.getEffectiveJvm
> > > (AbstractSurefireMojo.java:2107)
> > >at
> org.apache.maven.plugin.surefire.AbstractSurefireMojo.getForkConfiguration
> > > (AbstractSurefireMojo.java:1976)
> > >at
> org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeProvider
> > > (AbstractSurefireMojo.java:)
> > >at
> org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeAfterPreconditionsChecked
> > > (AbstractSurefireMojo.java:954)
> > > ---CUT---
> > >
> > > Regards,
> > > Gilles
> > >
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>

Re: [numbers-fraction] Maven surefire plugin error

[Gilles Sadowski]

Le mar. 5 mars 2019 à 00:29, Alex Herbert  a écrit :
>
>
> > On 4 Mar 2019, at 23:10, Gilles Sadowski  wrote:
> >
> > Hi.
> >
> > Le lun. 4 mars 2019 à 23:21, Eric Barnhill  a écrit 
> > :
> >>
> >> I am getting a maven error for the surefire plugin, but don't see it listed
> >> as a dependency in numbers-fraction or numbers-core. I see it listed in
> >> numbers-parent, but with no version number. Any ideas what I need to do to
> >> get the tests running again?
> >
> > Version is probably inherited from CP (commons-parent).
> >
> >>
> >> I also get this error with other numbers modules so I suspect it is down to
> >> my setup, but just can't quite see what to modify. "It was all working last
> >> week..."
> >
> > Did you change the environment (IDE, JVM, maven)?
> >
> >> Error msg:
> >> ---
> >> Failed to execute goal
> >> org.apache.maven.plugins:maven-surefire-plugin:2.20.1:test (default-test)
> >> on project commons-numbers-fraction: Execution default-test of goal
> >> org.apache.maven.plugins:maven-surefire-plugin:2.20.1:test failed.
> >> NullPointerException -> [Help 1]
> >
> > This does not occur on "master" but only on the "fraction-dev" branch.
>
> I tried the ‘fraction-dev’ branch. My build from the root reactor gets past 
> the [numbers-fraction] module but fails on [numbers-arrays] with a 
> compilation failure.
>
> Since it is possibly a platform issue I'll try open JDK on linux tomorrow for 
> comparison.
>
> You could try updating the branch to commons-parent 47 as a test.

I'd recommend that you regularly rebase from "master".

Regards,
Gilles

>
> > Here is an excerpt of the exception trace (from running "mvn -e test"):
> > ---CUT---
> > Caused by: java.lang.NullPointerException
> >at 
> > org.apache.maven.surefire.shade.org.apache.commons.lang3.SystemUtils.isJavaVersionAtLeast
> > (SystemUtils.java:1626)
> >at org.apache.maven.plugin.surefire.AbstractSurefireMojo.getEffectiveJvm
> > (AbstractSurefireMojo.java:2107)
> >at 
> > org.apache.maven.plugin.surefire.AbstractSurefireMojo.getForkConfiguration
> > (AbstractSurefireMojo.java:1976)
> >at org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeProvider
> > (AbstractSurefireMojo.java:)
> >at 
> > org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeAfterPreconditionsChecked
> > (AbstractSurefireMojo.java:954)
> > ---CUT---
> >
> > Regards,
> > Gilles
> >

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Maven surefire plugin error

[Alex Herbert]

> On 4 Mar 2019, at 23:10, Gilles Sadowski  wrote:
> 
> Hi.
> 
> Le lun. 4 mars 2019 à 23:21, Eric Barnhill  a écrit :
>> 
>> I am getting a maven error for the surefire plugin, but don't see it listed
>> as a dependency in numbers-fraction or numbers-core. I see it listed in
>> numbers-parent, but with no version number. Any ideas what I need to do to
>> get the tests running again?
> 
> Version is probably inherited from CP (commons-parent).
> 
>> 
>> I also get this error with other numbers modules so I suspect it is down to
>> my setup, but just can't quite see what to modify. "It was all working last
>> week..."
> 
> Did you change the environment (IDE, JVM, maven)?
> 
>> Error msg:
>> ---
>> Failed to execute goal
>> org.apache.maven.plugins:maven-surefire-plugin:2.20.1:test (default-test)
>> on project commons-numbers-fraction: Execution default-test of goal
>> org.apache.maven.plugins:maven-surefire-plugin:2.20.1:test failed.
>> NullPointerException -> [Help 1]
> 
> This does not occur on "master" but only on the "fraction-dev" branch.

I tried the ‘fraction-dev’ branch. My build from the root reactor gets past the 
[numbers-fraction] module but fails on [numbers-arrays] with a compilation 
failure.

Since it is possibly a platform issue I'll try open JDK on linux tomorrow for 
comparison.

You could try updating the branch to commons-parent 47 as a test.

> Here is an excerpt of the exception trace (from running "mvn -e test"):
> ---CUT---
> Caused by: java.lang.NullPointerException
>at 
> org.apache.maven.surefire.shade.org.apache.commons.lang3.SystemUtils.isJavaVersionAtLeast
> (SystemUtils.java:1626)
>at org.apache.maven.plugin.surefire.AbstractSurefireMojo.getEffectiveJvm
> (AbstractSurefireMojo.java:2107)
>at 
> org.apache.maven.plugin.surefire.AbstractSurefireMojo.getForkConfiguration
> (AbstractSurefireMojo.java:1976)
>at org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeProvider
> (AbstractSurefireMojo.java:)
>at 
> org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeAfterPreconditionsChecked
> (AbstractSurefireMojo.java:954)
> ---CUT---
> 
> Regards,
> Gilles
> 
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
> 


-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Maven surefire plugin error

[Gilles Sadowski]

Hi.

Le lun. 4 mars 2019 à 23:21, Eric Barnhill  a écrit :
>
> I am getting a maven error for the surefire plugin, but don't see it listed
> as a dependency in numbers-fraction or numbers-core. I see it listed in
> numbers-parent, but with no version number. Any ideas what I need to do to
> get the tests running again?

Version is probably inherited from CP (commons-parent).

>
> I also get this error with other numbers modules so I suspect it is down to
> my setup, but just can't quite see what to modify. "It was all working last
> week..."

Did you change the environment (IDE, JVM, maven)?

> Error msg:
> ---
> Failed to execute goal
> org.apache.maven.plugins:maven-surefire-plugin:2.20.1:test (default-test)
> on project commons-numbers-fraction: Execution default-test of goal
> org.apache.maven.plugins:maven-surefire-plugin:2.20.1:test failed.
> NullPointerException -> [Help 1]

This does not occur on "master" but only on the "fraction-dev" branch.
Here is an excerpt of the exception trace (from running "mvn -e test"):
---CUT---
Caused by: java.lang.NullPointerException
at 
org.apache.maven.surefire.shade.org.apache.commons.lang3.SystemUtils.isJavaVersionAtLeast
(SystemUtils.java:1626)
at org.apache.maven.plugin.surefire.AbstractSurefireMojo.getEffectiveJvm
(AbstractSurefireMojo.java:2107)
at 
org.apache.maven.plugin.surefire.AbstractSurefireMojo.getForkConfiguration
(AbstractSurefireMojo.java:1976)
at org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeProvider
(AbstractSurefireMojo.java:)
at 
org.apache.maven.plugin.surefire.AbstractSurefireMojo.executeAfterPreconditionsChecked
(AbstractSurefireMojo.java:954)
---CUT---

Regards,
Gilles

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Maven surefire plugin error

[Alex Herbert]

I just did a fresh clone of commons-numbers and it runs ‘mvn test’ ok.

Apache Maven 3.5.0 (ff8f5e7444045639af65f6095c62210b5713f426; 
2017-04-03T20:39:06+01:00)
Maven home: /usr/local/Cellar/maven/3.5.0/libexec
Java version: 1.8.0_131, vendor: Oracle Corporation
Java home: /Library/Java/JavaVirtualMachines/jdk1.8.0_131.jdk/Contents/Home/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "10.14.3", arch: "x86_64", family: “mac"


The output listed maven-surefire-plugin:2.22.0

Your 2.20.1 version may indicate something is out of sync somewhere.

The effective pom view in eclipse has both versions in properties:

2.20.1
2.22.0

PluginManagement has:


  maven-surefire-plugin
  2.22.0


This seems to be coming from commons-parent 47.

Try a fresh clone and build. Or are you on a branch other than master?


> On 4 Mar 2019, at 22:21, Eric Barnhill  wrote:
> 
> I am getting a maven error for the surefire plugin, but don't see it listed
> as a dependency in numbers-fraction or numbers-core. I see it listed in
> numbers-parent, but with no version number. Any ideas what I need to do to
> get the tests running again?
> 
> I also get this error with other numbers modules so I suspect it is down to
> my setup, but just can't quite see what to modify. "It was all working last
> week..."
> 
> Error msg:
> ---
> Failed to execute goal
> org.apache.maven.plugins:maven-surefire-plugin:2.20.1:test (default-test)
> on project commons-numbers-fraction: Execution default-test of goal
> org.apache.maven.plugins:maven-surefire-plugin:2.20.1:test failed.
> NullPointerException -> [Help 1]

Re: [numbers] Fraction() and Knuth 4.5.1 -- overflow, BigInteger, long, and rounding

[Gilles]

Hi Eric.

On Mon, 3 Dec 2018 10:01:39 -0800, Eric Barnhill wrote:



Does this mean that computations can "unpredictably" overflow
(or throw an exception)?



The ArithmeticUtils() methods mulAndCheck and addAndCheck throw 
exceptions
if there is overflow during primitive operations. That is the "check" 
part

of the method name.



Is it acceptable, or should we enclose the problematic code in
a "try" block and redo the computation with "BigInteger" when
necessary?

What is the performance hit of using "BigFraction" rather than
"Fraction"?



I once used BigDecimal for a project, it is great code but the 
performance

is nothing close to using primitives.



Are there use-cases that would need the ultimate performance from
"Fraction" while not worry about overflow?



You would need a greatest common factor between the two fractions 
that was

larger than 64 bits.

Again, BigFraction is there for anyone worried about such a case and 
there
is no significant performance hit to switching over to BigFraction 
compared

to a Fraction class that was using BigInteger under the hood. But I
suspect  there would be a substantial performance gain if longs were 
being
used under the hood for the Fraction class for the more common use 
case of
smaller fractions. If it would be best practice, a bit of 
microbenchmarking

could be done to check.

A FractionOverflowException could be specifically tailored to this 
use case
and the error message can suggest using BigFraction. Or as you 
suggest, the
catch block could silently or with warning return a BigFraction. If 
we have
class inheritance straight, and both Fraction and BigFraction have 
the

exact same interface, this could be an elegant solution.


Maybe I misunderstood, but I thought that only intermediate results
would require "BigInteger"; if the result needs to be converted to
"BigFraction", than I'd favour raising an exception (with the advice
which you mention).
If later on someone comes up with a use-case for the alternate 
solution,

we can revisit.

Regards,
Gilles



Eric



-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers] Fraction() and Knuth 4.5.1 -- overflow, BigInteger, long, and rounding

[Eric Barnhill]

>
>
> Does this mean that computations can "unpredictably" overflow
> (or throw an exception)?
>

The ArithmeticUtils() methods mulAndCheck and addAndCheck throw exceptions
if there is overflow during primitive operations. That is the "check" part
of the method name.


> Is it acceptable, or should we enclose the problematic code in
> a "try" block and redo the computation with "BigInteger" when
> necessary?
>
> What is the performance hit of using "BigFraction" rather than
> "Fraction"?
>

I once used BigDecimal for a project, it is great code but the performance
is nothing close to using primitives.


> Are there use-cases that would need the ultimate performance from
> "Fraction" while not worry about overflow?
>

You would need a greatest common factor between the two fractions that was
larger than 64 bits.

Again, BigFraction is there for anyone worried about such a case and there
is no significant performance hit to switching over to BigFraction compared
to a Fraction class that was using BigInteger under the hood. But I
suspect  there would be a substantial performance gain if longs were being
used under the hood for the Fraction class for the more common use case of
smaller fractions. If it would be best practice, a bit of microbenchmarking
could be done to check.

A FractionOverflowException could be specifically tailored to this use case
and the error message can suggest using BigFraction. Or as you suggest, the
catch block could silently or with warning return a BigFraction. If we have
class inheritance straight, and both Fraction and BigFraction have the
exact same interface, this could be an elegant solution.

Eric

Re: [numbers] Fraction() and Knuth 4.5.1 -- overflow, BigInteger, long, and rounding

[Gilles]

On Fri, 30 Nov 2018 15:56:54 -0800, Eric Barnhill wrote:
Here is what I propose for the Fraction doc text regarding this 
issue:


 * Implement add and subtract. This algorithm is similar to that
 * described in Knuth 4.5.1. while making some concessions to
 * performance. Note Knuth 4.5.1 Exercise 7, which observes that
 * adding two fractions with 32-bit numerators and denominators
 * requires 65 bits in extreme cases. Here calculations are 
performed

 * with 64-bit longs and the BigFraction class is recommended for
numbers
 * that may grow large enough to be in danger of overflow.


Does this mean that computations can "unpredictably" overflow
(or throw an exception)?
Is it acceptable, or should we enclose the problematic code in
a "try" block and redo the computation with "BigInteger" when
necessary?

What is the performance hit of using "BigFraction" rather than
"Fraction"?
Are there use-cases that would need the ultimate performance from
"Fraction" while not worry about overflow?

Regards,
Gilles




On Fri, Nov 9, 2018 at 4:33 PM Eric Barnhill  
wrote:


Addendum to the above. In an exercise in the Knuth book Knuth does 
indeed

state that "If the inputs are n-bit binary numbers, 2N+1 bits may be
necessary to represent t." where t is a derived quantity that would 
take

some time to explain.

So that means in extreme cases, the needed precision to represent a
fraction operation with 32 bits ints is 65 bits, one more than a 
long has.


The present code solves this by using BigInteger briefly in the 
code,
which strikes me as an awfully big performance hit for what must 
surely be

very occasional and very  extreme cases.

I think the most sensible strategy would be to restrict the 
precision of

Fraction to longs, with user guidance to use BigFraction if there is
concern of overflow.

Eric







On Thu, Nov 8, 2018 at 11:11 AM Gary Gregory 


wrote:

I'm all for the Javadoc made to reflect the reality of the code. It 
is

fine
to have an additional section that points out Knuth and how we may 
want to

change things as a hint or request to contributors.

Gary

On Wed, Nov 7, 2018 at 10:52 AM Eric Barnhill 


wrote:

> I read Kunth's "Art of Computer Programming 4.5.1" that is 
referenced

many
> times in the doc as the guidance for the 
commons-math/commons-numbers
> Fraction class. It is an interesting read. Also, for all the 
times it is
> cited in the doc, it is interesting that Fraction doesn't really 
use it

as
> implemented. Here is one example.
>
> Knuth is concerned about overflow in multiplication and division,
because
> numerator of f1 is multiplied by denominator of f2 and so forth, 
so he

> suggests a technique called "mediant rounding" that allows for
intermediate
> quantities in fraction multiplication to be rounded.
>
> It is a clever technique and probably works well, however the 
current
> Fraction class cites this chapter, then implements multiplication 
with

> BigInteger instead, ignoring this suggestion.
>
> First of all, the doc should be clear that the code is NOT 
following

4.5.1,
> while it gives the opposite impression. And that's ok but the use 
of
> BigInteger creates additional inconsistency: Multiply and divide 
are

> accomplished using ArithmeticUtils.addAndCheck and
> ArithmeticUtils.mulAndCheck . These convert the relevant ints to 
longs,
> then perform the operation, then if the resulting long is greater 
than

the
> range of an int, throw an OverflowException. So some parts of 
Fraction

> check for overflow using longs and others use BigInteger.
>
> It seems to me that BigInteger is overkill here for the vast 
majority of

> practical uses of Fraction in a way that could be damaging for
performance.
> And furthermore, we already have a BigFraction class to handle 
cases

that
> require BigInteger.
>
> So, I propose rewriting the doc to say the opposite of what it 
currently
> says when appropriate, and get usages of BigInteger out of 
Fraction, use

> them only in BigFraction, and use the long-based ArithmeticUtils
methods to
> check for overflow and underflow in fraction addition and 
subtraction.

>
> Eric
>






-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers] Fraction() and Knuth 4.5.1 -- overflow, BigInteger, long, and rounding

[Eric Barnhill]

Here is what I propose for the Fraction doc text regarding this issue:

 * Implement add and subtract. This algorithm is similar to that
 * described in Knuth 4.5.1. while making some concessions to
 * performance. Note Knuth 4.5.1 Exercise 7, which observes that
 * adding two fractions with 32-bit numerators and denominators
 * requires 65 bits in extreme cases. Here calculations are performed
 * with 64-bit longs and the BigFraction class is recommended for
numbers
 * that may grow large enough to be in danger of overflow.


On Fri, Nov 9, 2018 at 4:33 PM Eric Barnhill  wrote:

> Addendum to the above. In an exercise in the Knuth book Knuth does indeed
> state that "If the inputs are n-bit binary numbers, 2N+1 bits may be
> necessary to represent t." where t is a derived quantity that would take
> some time to explain.
>
> So that means in extreme cases, the needed precision to represent a
> fraction operation with 32 bits ints is 65 bits, one more than a long has.
>
> The present code solves this by using BigInteger briefly in the code,
> which strikes me as an awfully big performance hit for what must surely be
> very occasional and very  extreme cases.
>
> I think the most sensible strategy would be to restrict the precision of
> Fraction to longs, with user guidance to use BigFraction if there is
> concern of overflow.
>
> Eric
>
>
>
>
>
>
>
> On Thu, Nov 8, 2018 at 11:11 AM Gary Gregory 
> wrote:
>
>> I'm all for the Javadoc made to reflect the reality of the code. It is
>> fine
>> to have an additional section that points out Knuth and how we may want to
>> change things as a hint or request to contributors.
>>
>> Gary
>>
>> On Wed, Nov 7, 2018 at 10:52 AM Eric Barnhill 
>> wrote:
>>
>> > I read Kunth's "Art of Computer Programming 4.5.1" that is referenced
>> many
>> > times in the doc as the guidance for the commons-math/commons-numbers
>> > Fraction class. It is an interesting read. Also, for all the times it is
>> > cited in the doc, it is interesting that Fraction doesn't really use it
>> as
>> > implemented. Here is one example.
>> >
>> > Knuth is concerned about overflow in multiplication and division,
>> because
>> > numerator of f1 is multiplied by denominator of f2 and so forth, so he
>> > suggests a technique called "mediant rounding" that allows for
>> intermediate
>> > quantities in fraction multiplication to be rounded.
>> >
>> > It is a clever technique and probably works well, however the current
>> > Fraction class cites this chapter, then implements multiplication with
>> > BigInteger instead, ignoring this suggestion.
>> >
>> > First of all, the doc should be clear that the code is NOT following
>> 4.5.1,
>> > while it gives the opposite impression. And that's ok but the use of
>> > BigInteger creates additional inconsistency: Multiply and divide are
>> > accomplished using ArithmeticUtils.addAndCheck and
>> > ArithmeticUtils.mulAndCheck . These convert the relevant ints to longs,
>> > then perform the operation, then if the resulting long is greater than
>> the
>> > range of an int, throw an OverflowException. So some parts of Fraction
>> > check for overflow using longs and others use BigInteger.
>> >
>> > It seems to me that BigInteger is overkill here for the vast majority of
>> > practical uses of Fraction in a way that could be damaging for
>> performance.
>> > And furthermore, we already have a BigFraction class to handle cases
>> that
>> > require BigInteger.
>> >
>> > So, I propose rewriting the doc to say the opposite of what it currently
>> > says when appropriate, and get usages of BigInteger out of Fraction, use
>> > them only in BigFraction, and use the long-based ArithmeticUtils
>> methods to
>> > check for overflow and underflow in fraction addition and subtraction.
>> >
>> > Eric
>> >
>>
>

Re: [numbers] Fraction() and Knuth 4.5.1 -- overflow, BigInteger, long, and rounding

[Eric Barnhill]

Addendum to the above. In an exercise in the Knuth book Knuth does indeed
state that "If the inputs are n-bit binary numbers, 2N+1 bits may be
necessary to represent t." where t is a derived quantity that would take
some time to explain.

So that means in extreme cases, the needed precision to represent a
fraction operation with 32 bits ints is 65 bits, one more than a long has.

The present code solves this by using BigInteger briefly in the code, which
strikes me as an awfully big performance hit for what must surely be very
occasional and very  extreme cases.

I think the most sensible strategy would be to restrict the precision of
Fraction to longs, with user guidance to use BigFraction if there is
concern of overflow.

Eric







On Thu, Nov 8, 2018 at 11:11 AM Gary Gregory  wrote:

> I'm all for the Javadoc made to reflect the reality of the code. It is fine
> to have an additional section that points out Knuth and how we may want to
> change things as a hint or request to contributors.
>
> Gary
>
> On Wed, Nov 7, 2018 at 10:52 AM Eric Barnhill 
> wrote:
>
> > I read Kunth's "Art of Computer Programming 4.5.1" that is referenced
> many
> > times in the doc as the guidance for the commons-math/commons-numbers
> > Fraction class. It is an interesting read. Also, for all the times it is
> > cited in the doc, it is interesting that Fraction doesn't really use it
> as
> > implemented. Here is one example.
> >
> > Knuth is concerned about overflow in multiplication and division, because
> > numerator of f1 is multiplied by denominator of f2 and so forth, so he
> > suggests a technique called "mediant rounding" that allows for
> intermediate
> > quantities in fraction multiplication to be rounded.
> >
> > It is a clever technique and probably works well, however the current
> > Fraction class cites this chapter, then implements multiplication with
> > BigInteger instead, ignoring this suggestion.
> >
> > First of all, the doc should be clear that the code is NOT following
> 4.5.1,
> > while it gives the opposite impression. And that's ok but the use of
> > BigInteger creates additional inconsistency: Multiply and divide are
> > accomplished using ArithmeticUtils.addAndCheck and
> > ArithmeticUtils.mulAndCheck . These convert the relevant ints to longs,
> > then perform the operation, then if the resulting long is greater than
> the
> > range of an int, throw an OverflowException. So some parts of Fraction
> > check for overflow using longs and others use BigInteger.
> >
> > It seems to me that BigInteger is overkill here for the vast majority of
> > practical uses of Fraction in a way that could be damaging for
> performance.
> > And furthermore, we already have a BigFraction class to handle cases that
> > require BigInteger.
> >
> > So, I propose rewriting the doc to say the opposite of what it currently
> > says when appropriate, and get usages of BigInteger out of Fraction, use
> > them only in BigFraction, and use the long-based ArithmeticUtils methods
> to
> > check for overflow and underflow in fraction addition and subtraction.
> >
> > Eric
> >
>

Re: [numbers] Fraction() and Knuth 4.5.1 -- overflow, BigInteger, long, and rounding

[Gary Gregory]

I'm all for the Javadoc made to reflect the reality of the code. It is fine
to have an additional section that points out Knuth and how we may want to
change things as a hint or request to contributors.

Gary

On Wed, Nov 7, 2018 at 10:52 AM Eric Barnhill 
wrote:

> I read Kunth's "Art of Computer Programming 4.5.1" that is referenced many
> times in the doc as the guidance for the commons-math/commons-numbers
> Fraction class. It is an interesting read. Also, for all the times it is
> cited in the doc, it is interesting that Fraction doesn't really use it as
> implemented. Here is one example.
>
> Knuth is concerned about overflow in multiplication and division, because
> numerator of f1 is multiplied by denominator of f2 and so forth, so he
> suggests a technique called "mediant rounding" that allows for intermediate
> quantities in fraction multiplication to be rounded.
>
> It is a clever technique and probably works well, however the current
> Fraction class cites this chapter, then implements multiplication with
> BigInteger instead, ignoring this suggestion.
>
> First of all, the doc should be clear that the code is NOT following 4.5.1,
> while it gives the opposite impression. And that's ok but the use of
> BigInteger creates additional inconsistency: Multiply and divide are
> accomplished using ArithmeticUtils.addAndCheck and
> ArithmeticUtils.mulAndCheck . These convert the relevant ints to longs,
> then perform the operation, then if the resulting long is greater than the
> range of an int, throw an OverflowException. So some parts of Fraction
> check for overflow using longs and others use BigInteger.
>
> It seems to me that BigInteger is overkill here for the vast majority of
> practical uses of Fraction in a way that could be damaging for performance.
> And furthermore, we already have a BigFraction class to handle cases that
> require BigInteger.
>
> So, I propose rewriting the doc to say the opposite of what it currently
> says when appropriate, and get usages of BigInteger out of Fraction, use
> them only in BigFraction, and use the long-based ArithmeticUtils methods to
> check for overflow and underflow in fraction addition and subtraction.
>
> Eric
>

Re: [numbers] Fraction() and Knuth 4.5.1 -- overflow, BigInteger, long, and rounding

[Gilles]

Hi.

On Wed, 7 Nov 2018 09:52:30 -0800, Eric Barnhill wrote:
I read Kunth's "Art of Computer Programming 4.5.1" that is referenced 
many

times in the doc as the guidance for the commons-math/commons-numbers
Fraction class. It is an interesting read. Also, for all the times it 
is
cited in the doc, it is interesting that Fraction doesn't really use 
it as

implemented. Here is one example.

Knuth is concerned about overflow in multiplication and division, 
because
numerator of f1 is multiplied by denominator of f2 and so forth, so 
he
suggests a technique called "mediant rounding" that allows for 
intermediate

quantities in fraction multiplication to be rounded.

It is a clever technique and probably works well, however the current
Fraction class cites this chapter, then implements multiplication 
with

BigInteger instead, ignoring this suggestion.

First of all, the doc should be clear that the code is NOT following 
4.5.1,

while it gives the opposite impression. And that's ok but the use of
BigInteger creates additional inconsistency: Multiply and divide are
accomplished using ArithmeticUtils.addAndCheck and
ArithmeticUtils.mulAndCheck . These convert the relevant ints to 
longs,
then perform the operation, then if the resulting long is greater 
than the
range of an int, throw an OverflowException. So some parts of 
Fraction

check for overflow using longs and others use BigInteger.

It seems to me that BigInteger is overkill here for the vast majority 
of
practical uses of Fraction in a way that could be damaging for 
performance.
And furthermore, we already have a BigFraction class to handle cases 
that

require BigInteger.

So, I propose rewriting the doc to say the opposite of what it 
currently
says when appropriate, and get usages of BigInteger out of Fraction, 
use
them only in BigFraction, and use the long-based ArithmeticUtils 
methods to
check for overflow and underflow in fraction addition and 
subtraction.


Eric


Thanks a lot for the fact-checking.

Gilles


-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org