You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by ah...@apache.org on 2020/04/16 12:18:24 UTC
[commons-numbers] 09/26: Fraction: Use canonical zero as 0 / 1
This is an automated email from the ASF dual-hosted git repository.
aherbert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-numbers.git
commit fa5b3eed90ac61ae4230ec0e69a9f10102d66d12
Author: Alex Herbert <ah...@apache.org>
AuthorDate: Mon Apr 13 23:51:46 2020 +0100
Fraction: Use canonical zero as 0 / 1
Change arithmetic to return ZERO where applicable.
Change the factory constructor to return ZERO when the fraction is zero.
This removes the possibility to create 0 / -1. There should be no
equivalent of 0.0 and -0.0 in double. Thus compareTo, equals and
doubleValue() should function as if 0 / 1 and 0 / -1 are identical.
---
.../commons/numbers/fraction/BigFraction.java | 47 ++++++++++++++++---
.../apache/commons/numbers/fraction/Fraction.java | 53 +++++++++++++++-------
.../commons/numbers/fraction/BigFractionTest.java | 23 ++++++++--
.../commons/numbers/fraction/CommonTestCases.java | 11 ++---
.../commons/numbers/fraction/FractionTest.java | 22 +++++++--
5 files changed, 120 insertions(+), 36 deletions(-)
diff --git a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
index 1e60668..a62ced0 100644
--- a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
+++ b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java
@@ -39,10 +39,10 @@ public final class BigFraction
NativeOperators<BigFraction>,
Serializable {
/** A fraction representing "0". */
- public static final BigFraction ZERO = of(0);
+ public static final BigFraction ZERO = new BigFraction(BigInteger.ZERO);
/** A fraction representing "1". */
- public static final BigFraction ONE = of(1);
+ public static final BigFraction ONE = new BigFraction(BigInteger.ONE);
/** Serializable version identifier. */
private static final long serialVersionUID = 20190701L;
@@ -59,6 +59,10 @@ public final class BigFraction
/**
* Private constructor: Instances are created using factory methods.
*
+ * <p>This constructor should only be invoked when the fraction is known
+ * to be non-zero; otherwise use {@link #ZERO}. This avoids creating
+ * the zero representation {@code 0 / -1}.
+ *
* @param num Numerator, must not be {@code null}.
* @param den Denominator, must not be {@code null}.
* @throws ArithmeticException if the denominator is zero.
@@ -134,6 +138,9 @@ public final class BigFraction
if (!Double.isFinite(value)) {
throw new IllegalArgumentException(NOT_FINITE + value);
}
+ if (value == 0) {
+ return ZERO;
+ }
final long overflow = Integer.MAX_VALUE;
double r0 = value;
@@ -231,6 +238,9 @@ public final class BigFraction
if (!Double.isFinite(value)) {
throw new IllegalArgumentException(NOT_FINITE + value);
}
+ if (value == 0) {
+ return ZERO;
+ }
final long bits = Double.doubleToLongBits(value);
final long sign = bits & 0x8000000000000000L;
@@ -320,6 +330,9 @@ public final class BigFraction
* @return a new instance.
*/
public static BigFraction of(final int num) {
+ if (num == 0) {
+ return ZERO;
+ }
return new BigFraction(BigInteger.valueOf(num));
}
@@ -330,6 +343,9 @@ public final class BigFraction
* @return a new instance.
*/
public static BigFraction of(final long num) {
+ if (num == 0) {
+ return ZERO;
+ }
return new BigFraction(BigInteger.valueOf(num));
}
@@ -342,6 +358,9 @@ public final class BigFraction
*/
public static BigFraction of(final BigInteger num) {
Objects.requireNonNull(num, "numerator");
+ if (num.signum() == 0) {
+ return ZERO;
+ }
return new BigFraction(num);
}
@@ -355,6 +374,9 @@ public final class BigFraction
* @throws ArithmeticException if {@code den} is zero.
*/
public static BigFraction of(final int num, final int den) {
+ if (num == 0) {
+ return ZERO;
+ }
return new BigFraction(BigInteger.valueOf(num), BigInteger.valueOf(den));
}
@@ -368,6 +390,9 @@ public final class BigFraction
* @throws ArithmeticException if {@code den} is zero.
*/
public static BigFraction of(final long num, final long den) {
+ if (num == 0) {
+ return ZERO;
+ }
return new BigFraction(BigInteger.valueOf(num), BigInteger.valueOf(den));
}
@@ -382,6 +407,9 @@ public final class BigFraction
* @throws ArithmeticException if the denominator is zero.
*/
public static BigFraction of(final BigInteger num, final BigInteger den) {
+ if (num.signum() == 0) {
+ return ZERO;
+ }
return new BigFraction(num, den);
}
@@ -423,7 +451,7 @@ public final class BigFraction
final int slashLoc = stripped.indexOf('/');
// if no slash, parse as single number
if (slashLoc == -1) {
- return BigFraction.of(new BigInteger(stripped.trim()));
+ return of(new BigInteger(stripped.trim()));
}
final BigInteger num = new BigInteger(stripped.substring(0, slashLoc).trim());
final BigInteger denom = new BigInteger(stripped.substring(slashLoc + 1).trim());
@@ -654,6 +682,7 @@ public final class BigFraction
return of(value);
}
+ // Direct constructor: cannot add an integer to a fraction to produce zero
return new BigFraction(numerator.add(denominator.multiply(value)), denominator);
}
@@ -728,6 +757,7 @@ public final class BigFraction
return of(value.negate());
}
+ // Direct constructor: cannot subtract an integer from a fraction to produce zero
return new BigFraction(numerator.subtract(denominator.multiply(value)), denominator);
}
@@ -756,6 +786,11 @@ public final class BigFraction
num = (numerator.multiply(value.denominator)).subtract((value.numerator).multiply(denominator));
den = denominator.multiply(value.denominator);
}
+
+ if (num.signum() == 0) {
+ return ZERO;
+ }
+
return new BigFraction(num, den);
}
@@ -857,7 +892,7 @@ public final class BigFraction
throw new FractionException(FractionException.ERROR_DIVIDE_BY_ZERO);
}
if (isZero()) {
- return this;
+ return ZERO;
}
return new BigFraction(numerator, denominator.multiply(value));
}
@@ -876,7 +911,7 @@ public final class BigFraction
throw new FractionException(FractionException.ERROR_DIVIDE_BY_ZERO);
}
if (isZero()) {
- return this;
+ return ZERO;
}
return multiply(value.reciprocal());
}
@@ -895,7 +930,7 @@ public final class BigFraction
return ONE;
}
if (isZero()) {
- return this;
+ return ZERO;
}
// Note: Raise the BigIntegers to the power and then reduce.
diff --git a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
index 5154eff..a9401db 100644
--- a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
+++ b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java
@@ -36,10 +36,10 @@ public final class Fraction
NativeOperators<Fraction>,
Serializable {
/** A fraction representing "0". */
- public static final Fraction ZERO = of(0);
+ public static final Fraction ZERO = new Fraction(0);
/** A fraction representing "1". */
- public static final Fraction ONE = of(1);
+ public static final Fraction ONE = new Fraction(1);
/** Serializable version identifier. */
private static final long serialVersionUID = 20190701L;
@@ -59,6 +59,10 @@ public final class Fraction
/**
* Private constructor: Instances are created using factory methods.
*
+ * <p>This constructor should only be invoked when the fraction is known
+ * to be non-zero; otherwise use {@link #ZERO}. This avoids creating
+ * the zero representation {@code 0 / -1}.
+ *
* @param num Numerator.
* @param den Denominator.
* @throws ArithmeticException if the denominator is {@code zero}.
@@ -87,15 +91,11 @@ public final class Fraction
q = den;
}
- // Will not throw
+ // Will not throw.
+ // Cannot return 0 as gcd(0, 0) has been eliminated.
final int d = ArithmeticUtils.gcd(p, q);
- if (d > 1) {
- p /= d;
- q /= d;
- }
-
- numerator = p;
- denominator = q;
+ numerator = p / d;
+ denominator = q / d;
}
}
@@ -257,6 +257,9 @@ public final class Fraction
public static Fraction from(final double value,
final double epsilon,
final int maxIterations) {
+ if (value == 0) {
+ return ZERO;
+ }
return new Fraction(value, epsilon, Integer.MAX_VALUE, maxIterations);
}
@@ -278,6 +281,9 @@ public final class Fraction
*/
public static Fraction from(final double value,
final int maxDenominator) {
+ if (value == 0) {
+ return ZERO;
+ }
return new Fraction(value, 0, maxDenominator, 100);
}
@@ -288,6 +294,9 @@ public final class Fraction
* @return a new instance.
*/
public static Fraction of(final int num) {
+ if (num == 0) {
+ return ZERO;
+ }
return new Fraction(num);
}
@@ -301,6 +310,9 @@ public final class Fraction
* @return a new instance.
*/
public static Fraction of(final int num, final int den) {
+ if (num == 0) {
+ return ZERO;
+ }
return new Fraction(num, den);
}
@@ -342,7 +354,7 @@ public final class Fraction
final int slashLoc = stripped.indexOf('/');
// if no slash, parse as single number
if (slashLoc == -1) {
- return Fraction.of(Integer.parseInt(stripped.trim()));
+ return of(Integer.parseInt(stripped.trim()));
}
final int num = Integer.parseInt(stripped.substring(0, slashLoc).trim());
final int denom = Integer.parseInt(stripped.substring(slashLoc + 1).trim());
@@ -469,6 +481,7 @@ public final class Fraction
* cannot be represented in an {@code int}.
*/
public Fraction add(final int value) {
+ // Direct constructor: cannot add an integer to a fraction to produce zero
return new Fraction(numerator + value * denominator, denominator);
}
@@ -496,6 +509,7 @@ public final class Fraction
* cannot be represented in an {@code int}.
*/
public Fraction subtract(final int value) {
+ // Direct constructor: cannot subtract an integer from a fraction to produce zero
return new Fraction(numerator - value * denominator, denominator);
}
@@ -600,8 +614,8 @@ public final class Fraction
// Make sure we don't overflow unless the result *must* overflow.
final int d1 = ArithmeticUtils.gcd(numerator, value.denominator);
final int d2 = ArithmeticUtils.gcd(value.numerator, denominator);
- return of(Math.multiplyExact(numerator / d1, value.numerator / d2),
- Math.multiplyExact(denominator / d2, value.denominator / d1));
+ return new Fraction(Math.multiplyExact(numerator / d1, value.numerator / d2),
+ Math.multiplyExact(denominator / d2, value.denominator / d1));
}
/**
@@ -615,7 +629,14 @@ public final class Fraction
* by an {@code int}.
*/
public Fraction divide(final int value) {
- return divide(of(value));
+ if (value == 0) {
+ throw new FractionException(FractionException.ERROR_DIVIDE_BY_ZERO);
+ }
+ if (isZero()) {
+ return ZERO;
+ }
+ // Multiply by reciprocal
+ return multiply(new Fraction(1, value));
}
/**
@@ -634,7 +655,7 @@ public final class Fraction
throw new FractionException(FractionException.ERROR_DIVIDE_BY_ZERO);
}
if (isZero()) {
- return this;
+ return ZERO;
}
return multiply(value.reciprocal());
}
@@ -653,7 +674,7 @@ public final class Fraction
return ONE;
}
if (isZero()) {
- return this;
+ return ZERO;
}
if (exponent < 0) {
diff --git a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
index bdc8003..8e4d90b 100644
--- a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
+++ b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/BigFractionTest.java
@@ -96,6 +96,22 @@ public class BigFractionTest {
() -> BigFraction.from(2.0 * Integer.MAX_VALUE, 1.0e-5, 100000));
}
+ @Test
+ public void testConstructorZero() {
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.from(0.0));
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.from(0.0, 1e-10, 100));
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.from(0.0, 100));
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.of(0));
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.of(0L));
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.of(BigInteger.ZERO));
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.of(0, 1));
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.of(0, -1));
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.of(0L, 1L));
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.of(0L, -1L));
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.of(BigInteger.ZERO, BigInteger.ONE));
+ Assertions.assertSame(BigFraction.ZERO, BigFraction.of(BigInteger.ZERO, BigInteger.ONE.negate()));
+ }
+
// MATH-179
@Test
public void testDoubleConstructor() throws Exception {
@@ -624,14 +640,15 @@ public class BigFractionTest {
* Assert the two fractions are equal. The contract of {@link Object#hashCode()} requires
* that the hash code must also be equal.
*
- * <p>This method must not be called with the same instance for both arguments. It is
- * intended to be used to test different objects that are equal have the same hash code.
+ * <p>Ideally this method should not be called with the same instance for both arguments.
+ * It is intended to be used to test different objects that are equal have the same hash code.
+ * However the same object may be constructed for different arguments using factory
+ * constructors, e.g. zero.
*
* @param f1 Fraction 1.
* @param f2 Fraction 2.
*/
private static void assertEqualAndHashCodeEqual(BigFraction f1, BigFraction f2) {
- Assertions.assertNotSame(f1, f2, "Do not call this assertion with the same object");
Assertions.assertEquals(f1, f2);
Assertions.assertEquals(f1.hashCode(), f2.hashCode(), "Equal fractions have different hashCode");
// Check the computation matches the result of Arrays.hashCode and the signum.
diff --git a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/CommonTestCases.java b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/CommonTestCases.java
index fd4a37c..5f7c9e7 100644
--- a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/CommonTestCases.java
+++ b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/CommonTestCases.java
@@ -127,7 +127,7 @@ final class CommonTestCases {
testCases.add(new UnaryOperatorTestCase(0, 1, 0, 1));
testCases.add(new UnaryOperatorTestCase(0, 2, 0, 1));
- testCases.add(new UnaryOperatorTestCase(0, -1, 0, -1));
+ testCases.add(new UnaryOperatorTestCase(0, -1, 0, 1));
testCases.add(new UnaryOperatorTestCase(1, 2, 1, 2));
testCases.add(new UnaryOperatorTestCase(2, 4, 1, 2));
testCases.add(new UnaryOperatorTestCase(-1, 2, -1, 2));
@@ -216,7 +216,7 @@ final class CommonTestCases {
testCases.add(new UnaryOperatorTestCase(-11, 23, 11, 23));
testCases.add(new UnaryOperatorTestCase(13, -24, -13, -24));
testCases.add(new UnaryOperatorTestCase(0, 1, 0, 1));
- testCases.add(new UnaryOperatorTestCase(0, -1, 0, -1));
+ testCases.add(new UnaryOperatorTestCase(0, -1, 0, 1));
return testCases;
}
@@ -249,9 +249,8 @@ final class CommonTestCases {
testCases.add(new UnaryOperatorTestCase(-50, 75, 2, 3));
testCases.add(new UnaryOperatorTestCase(Integer.MAX_VALUE - 1, Integer.MAX_VALUE, Integer.MIN_VALUE + 2, Integer.MAX_VALUE));
testCases.add(new UnaryOperatorTestCase(1, Integer.MIN_VALUE, -1, Integer.MIN_VALUE));
- // Negation of zero is a no-op
testCases.add(new UnaryOperatorTestCase(0, 1, 0, 1));
- testCases.add(new UnaryOperatorTestCase(0, -1, 0, -1));
+ testCases.add(new UnaryOperatorTestCase(0, -1, 0, 1));
// XXX Failed by "BigFraction" (whose implementation differs from "Fraction").
// These are tested explicitly in FractionTest.
@@ -342,7 +341,7 @@ final class CommonTestCases {
testCases.add(new BinaryOperatorTestCase(2, 3, 2, 3, 1, 1));
testCases.add(new BinaryOperatorTestCase(0, 3, 2, 3, 0, 1));
// Return the original zero representation
- testCases.add(new BinaryOperatorTestCase(0, -3, 2, 3, 0, -1));
+ testCases.add(new BinaryOperatorTestCase(0, -3, 2, 3, 0, 1));
testCases.add(new BinaryOperatorTestCase(
2, 7,
@@ -503,7 +502,7 @@ final class CommonTestCases {
testCases.add(new BinaryIntOperatorTestCase(2, 3, -13, 1594323, 8192));
testCases.add(new BinaryIntOperatorTestCase(0, 1, Integer.MAX_VALUE, 0, 1));
- testCases.add(new BinaryIntOperatorTestCase(0, -1, Integer.MAX_VALUE, 0, -1));
+ testCases.add(new BinaryIntOperatorTestCase(0, -1, Integer.MAX_VALUE, 0, 1));
return testCases;
}
diff --git a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
index 7205c6b..e62b577 100644
--- a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
+++ b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/FractionTest.java
@@ -16,6 +16,7 @@
*/
package org.apache.commons.numbers.fraction;
+import java.math.BigInteger;
import java.util.Arrays;
import org.apache.commons.numbers.core.TestUtils;
@@ -65,6 +66,16 @@ public class FractionTest {
Assertions.assertThrows(ArithmeticException.class, () -> Fraction.of(1, 0));
}
+ @Test
+ public void testConstructorZero() {
+ Assertions.assertSame(Fraction.ZERO, Fraction.from(0.0));
+ Assertions.assertSame(Fraction.ZERO, Fraction.from(0.0, 1e-10, 100));
+ Assertions.assertSame(Fraction.ZERO, Fraction.from(0.0, 100));
+ Assertions.assertSame(Fraction.ZERO, Fraction.of(0));
+ Assertions.assertSame(Fraction.ZERO, Fraction.of(0, 1));
+ Assertions.assertSame(Fraction.ZERO, Fraction.of(0, -1));
+ }
+
// MATH-179
@Test
public void testDoubleConstructor() throws Exception {
@@ -199,7 +210,7 @@ public class FractionTest {
Assertions.assertEquals(0.0, Fraction.ZERO.doubleValue());
Assertions.assertEquals(0.0, ZERO_P.doubleValue());
- Assertions.assertEquals(-0.0, ZERO_N.doubleValue());
+ Assertions.assertEquals(0.0, ZERO_N.doubleValue());
}
@Test
@@ -216,7 +227,7 @@ public class FractionTest {
Assertions.assertEquals(-e, Fraction.of(1, -3).floatValue());
Assertions.assertEquals(0.0f, ZERO_P.floatValue());
- Assertions.assertEquals(-0.0f, ZERO_N.floatValue());
+ Assertions.assertEquals(0.0f, ZERO_N.floatValue());
}
@Test
@@ -510,14 +521,15 @@ public class FractionTest {
* Assert the two fractions are equal. The contract of {@link Object#hashCode()} requires
* that the hash code must also be equal.
*
- * <p>This method must not be called with the same instance for both arguments. It is
- * intended to be used to test different objects that are equal have the same hash code.
+ * <p>Ideally this method should not be called with the same instance for both arguments.
+ * It is intended to be used to test different objects that are equal have the same hash code.
+ * However the same object may be constructed for different arguments using factory
+ * constructors, e.g. zero.
*
* @param f1 Fraction 1.
* @param f2 Fraction 2.
*/
private static void assertEqualAndHashCodeEqual(Fraction f1, Fraction f2) {
- Assertions.assertNotSame(f1, f2, "Do not call this assertion with the same object");
Assertions.assertEquals(f1, f2);
Assertions.assertEquals(f1.hashCode(), f2.hashCode(), "Equal fractions have different hashCode");
// Check the computation matches the result of Arrays.hashCode and the signum.