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 <alex.d.herb...@gmail.com>:
On 09/04/2020 10:54, Gilles Sadowski wrote:
Hi.

2020-04-09 11:28 UTC+02:00, Alex Herbert <alex.d.herb...@gmail.com>:
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
* <code>this<sup>exponent</sup></code>.
*
* @param exponent exponent to which this {@code Fraction} is to
be
raised.
* @return <code>this<sup>exponent</sup></code> 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<sup>{@code Integer.MAX_VALUE}</sup> (exclusive) to
+2<sup>{@code Integer.MAX_VALUE}</sup> (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 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


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

Reply via email to