From b64d792b3d5d82879df4c8fd911172922fa30a31 Mon Sep 17 00:00:00 2001 From: Schamschi Date: Sun, 14 Jul 2019 20:42:46 +0200 Subject: [PATCH 1/8] NUMBERS-131: Create class SimpleContinuedFraction --- .../fraction/SimpleContinuedFraction.java | 272 ++++++++++++++++++ .../fraction/SimpleContinuedFractionTest.java | 166 +++++++++++ 2 files changed, 438 insertions(+) create mode 100644 commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/SimpleContinuedFraction.java create mode 100644 commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/SimpleContinuedFractionTest.java diff --git a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/SimpleContinuedFraction.java b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/SimpleContinuedFraction.java new file mode 100644 index 000000000..76d4cc5f6 --- /dev/null +++ b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/SimpleContinuedFraction.java @@ -0,0 +1,272 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.numbers.fraction; + +import java.math.BigInteger; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Iterator; +import java.util.List; +import java.util.NoSuchElementException; + +/** + * A {@code BigInteger} based, mutable representation of a simple continued + * fraction, i.e. a continued fraction with partial numerators of 1 and partial + * denominators, or coefficients, that are positive integers except for the + * first coefficient, which can be any integer. + */ +class SimpleContinuedFraction { + /** + * The error message to display when attempting to perform an operation that + * requires coefficients to be present in the absence of any coefficients. + */ + private static final String COEFFICIENTS_EMPTY_ERROR_MESSAGE = "No coefficients present."; + + /** + * The coefficients of this continued fraction. + */ + private final List coefficients; + + /** + * A 2-element array containing the numerator followed by the denominator of + * the convergent corresponding to the last coefficient of this continued + * fraction, or, if no coefficients are currently stored, of the second of + * the two initial "theoretical" convergents 0/1 and 1/0 that don't + * correspond to any coefficient but are needed for the recursive formula + * that calculates the convergents to work. + */ + private BigInteger[] currentConvergent; + + /** + * A 2-element array containing the numerator followed by the denominator of + * the convergent corresponding to the second to last coefficient of this + * continued fraction, or, if less than two coefficients are currently + * stored, of one of the two initial "theoretical" convergents 0/1 and 1/0 + * that don't correspond to any coefficient but are needed for the recursive + * formula that calculates the convergents to work. + */ + private BigInteger[] previousConvergent; + + /** + * Creates an instance that does not store any coefficients and hence does + * not represent a number. + */ + SimpleContinuedFraction() { + coefficients = new ArrayList<>(); + previousConvergent = new BigInteger[]{BigInteger.ZERO, BigInteger.ONE}; + currentConvergent = new BigInteger[]{BigInteger.ONE, BigInteger.ZERO}; + } + + /** + * Adds a coefficient to the end of the simple continued fraction + * representation of this instance. If this instance does not yet have any + * coefficients stored, the new coefficient can be any integer, otherwise, + * it must be positive. + * @param coefficient the new coefficient + * @throws IllegalArgumentException if this instance already has coefficients + * stored and the argument is not positive + */ + void addCoefficient(BigInteger coefficient) { + if (!coefficients.isEmpty()) { + requirePositiveCoefficient(coefficient); + } + coefficients.add(coefficient); + BigInteger newNumerator = coefficient.multiply(currentConvergent[0]).add(previousConvergent[0]); + BigInteger newDenominator = coefficient.multiply(currentConvergent[1]).add(previousConvergent[1]); + previousConvergent = currentConvergent; + currentConvergent = new BigInteger[]{newNumerator, newDenominator}; + } + + /** + * Replaces the last coefficient in this instance's simple continued + * fraction representation with the specified value. If this instance has + * only one coefficient stored, the new value can be any integer, otherwise, + * it must be positive. + * @param coefficient the new coefficient + * @return the coefficient previously at the last position in this instance's + * simple continued fraction representation + * @throws IllegalArgumentException if this instance has more than one + * coefficient stored and the argument is not positive + * @throws IllegalStateException if this instance does not have any + * coefficients stored + */ + BigInteger setLastCoefficient(BigInteger coefficient) { + if (coefficients.size() > 1) { + requirePositiveCoefficient(coefficient); + } + BigInteger oldLastCoefficient; + try { + oldLastCoefficient = coefficients.set(coefficients.size() - 1, coefficient); + } catch (IndexOutOfBoundsException e) { + throw new IllegalStateException(COEFFICIENTS_EMPTY_ERROR_MESSAGE); + } + BigInteger diff = coefficient.subtract(oldLastCoefficient); + currentConvergent[0] = currentConvergent[0].add(diff.multiply(previousConvergent[0])); + currentConvergent[1] = currentConvergent[1].add(diff.multiply(previousConvergent[1])); + return oldLastCoefficient; + } + + /** + * Removes the last coefficient from this instance's simple continued + * fraction representation. + * @return the coefficient that was removed + * @throws IllegalStateException if this instance does not have any + * coefficients stored + */ + BigInteger removeLastCoefficient() { + BigInteger lastCoefficient; + try { + lastCoefficient = coefficients.remove(coefficients.size() - 1); + } catch (IndexOutOfBoundsException e) { + throw new IllegalStateException(COEFFICIENTS_EMPTY_ERROR_MESSAGE); + } + BigInteger newNumerator = currentConvergent[0].subtract(lastCoefficient.multiply(previousConvergent[0])); + BigInteger newDenominator = currentConvergent[1].subtract(lastCoefficient.multiply(previousConvergent[1])); + currentConvergent = previousConvergent; + previousConvergent = new BigInteger[]{newNumerator, newDenominator}; + return lastCoefficient; + } + + /** + * Returns a read-only view of the coefficients of this instance's simple + * continued fraction representation. + * @return the coefficients of this simple continued fraction + */ + List viewCoefficients() { + return Collections.unmodifiableList(coefficients); + } + + /** + * If this instance does not have any coefficients stored, this method + * returns the array {@code {1, 0}}. Otherwise, it returns a 2-element array + * containing the numerator followed by the denominator of the convergent + * corresponding to the last coefficient of this continued fraction. The + * convergent's denominator will always be positive. + * @return a representation of a convergent as described above + */ + BigInteger[] getCurrentConvergent() { + return Arrays.copyOf(currentConvergent, 2); + } + + /** + * If this instance does not have any coefficients stored, this method + * returns the array {@code {0, 1}}. If exactly one coefficient is stored, + * it returns the array {@code {1, 0}}. Otherwise, it returns a 2-element + * array containing the numerator followed by the denominator of the + * convergent corresponding to the second to last coefficient of this + * continued fraction. The convergent's denominator will always be positive. + * @return a representation of a convergent as described above + */ + BigInteger[] getPreviousConvergent() { + return Arrays.copyOf(previousConvergent, 2); + } + + /** + * Converts this continued fraction to a {@code BigFraction}. + * @return a {@code BigFraction} instance equivalent to this continued + * fraction, reduced to lowest terms + * @throws IllegalStateException if this instance does not have any + * coefficients stored + */ + BigFraction toBigFraction() { + if (coefficients.isEmpty()) { + throw new IllegalStateException(COEFFICIENTS_EMPTY_ERROR_MESSAGE); + } + return BigFraction.of(currentConvergent[0], currentConvergent[1]); + } + + /** + * Ascertains that an integer that is intended to be used as a coefficient + * other than the first one is positive, and throws an + * {@code IllegalArgumentException} with a corresponding error message if it + * is not. + * @param coefficient the prospective non-initial coefficient + * @throws IllegalArgumentException if the argument is not positive + */ + private static void requirePositiveCoefficient(BigInteger coefficient) { + if (coefficient.signum() != 1) { + throw new IllegalArgumentException("Only the initial coefficient may be non-positive: " + coefficient); + } + } + + /** + *

Generates an iterator that iterates over the coefficients of the + * simple continued fraction representation of the passed + * {@code BigFraction} object.

+ * + *

If [a0; a1, a2, …] is + * the simple continued fraction representation of the argument, then the + * ({@code i+1})th invocation of the returned iterator's {@link Iterator#next() + * next()} method returns a 3-element {@code BigInteger} array + * {ai, p, q}, where {@code p} and {@code q} + * are integers such that {@code q > 0} and p/q = [ai; + * ai+1, ai+2, …].

+ * + *

Note that the iterator returned by this method always generates the + * shorter of the two equivalent simple continued fraction representations + * of a rational number, i.e. the one where the last coefficient is not 1 + * unless the represented number itself is 1 (in which case the generated + * sequence of coefficients will simply be [1;] rather than [0; 1]). For + * example, when passed 31/24, the iterator will generate the sequence + * [1; 3, 2, 3], and not the equivalent sequence [1; 3, 2, 2, 1].

+ * @param fraction the fraction the coefficients of whose simple continued + * fraction representation should be iterated over + * @return an {@code Iterator} as described above + */ + static Iterator coefficientsOf(BigFraction fraction) { + final BigInteger initialDividend; + final BigInteger initialDivisor; + if (fraction.getDenominator().signum() == -1) { + initialDividend = fraction.getNumerator().negate(); + initialDivisor = fraction.getDenominator().negate(); + } else { + initialDividend = fraction.getNumerator(); + initialDivisor = fraction.getDenominator(); + } + + return new Iterator() { + private BigInteger dividend = initialDividend; + private BigInteger divisor = initialDivisor; + + @Override + public boolean hasNext() { + return !divisor.equals(BigInteger.ZERO); + } + + @Override + public BigInteger[] next() { + BigInteger[] quotientAndRemainder; + try { + quotientAndRemainder = dividend.divideAndRemainder(divisor); + } catch (ArithmeticException e) { + throw new NoSuchElementException(); + } + //simulate floor function for negative quotients to ensure non-negative remainder + if (quotientAndRemainder[1].signum() == -1) { + quotientAndRemainder[0] = quotientAndRemainder[0].subtract(BigInteger.ONE); + quotientAndRemainder[1] = quotientAndRemainder[1].add(divisor); + } + BigInteger[] result = new BigInteger[]{quotientAndRemainder[0], dividend, divisor}; + dividend = divisor; + divisor = quotientAndRemainder[1]; + return result; + } + }; + } +} diff --git a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/SimpleContinuedFractionTest.java b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/SimpleContinuedFractionTest.java new file mode 100644 index 000000000..fc0be5a72 --- /dev/null +++ b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/SimpleContinuedFractionTest.java @@ -0,0 +1,166 @@ +package org.apache.commons.numbers.fraction; + +import java.math.BigInteger; + +import java.util.Arrays; +import java.util.Iterator; +import java.util.NoSuchElementException; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +class SimpleContinuedFractionTest { + + private static void assertConvergent(long expectedNumerator, long expectedDenominator, BigInteger[] actual) { + Assertions.assertArrayEquals( + toBigIntegerArray(expectedNumerator, expectedDenominator), + actual + ); + } + + private static BigInteger[] toBigIntegerArray(long... arr) { + BigInteger[] result = new BigInteger[arr.length]; + for (int i = 0; i < arr.length; i++) { + result[i] = BigInteger.valueOf(arr[i]); + } + return result; + } + + @Test + void testAddCoefficient() { + final SimpleContinuedFraction testSubject = new SimpleContinuedFraction(); + + testSubject.addCoefficient(BigInteger.ONE); + assertConvergent(1, 1, testSubject.getCurrentConvergent()); + assertConvergent(1, 0, testSubject.getPreviousConvergent()); + + testSubject.addCoefficient(BigInteger.valueOf(2)); + assertConvergent(3, 2, testSubject.getCurrentConvergent()); + assertConvergent(1, 1, testSubject.getPreviousConvergent()); + + testSubject.addCoefficient(BigInteger.valueOf(3)); + assertConvergent(10, 7, testSubject.getCurrentConvergent()); + assertConvergent(3, 2, testSubject.getPreviousConvergent()); + + Assertions.assertThrows(IllegalArgumentException.class, + () -> testSubject.addCoefficient(BigInteger.valueOf(-4)) + ); + + Assertions.assertEquals( + Arrays.asList(BigInteger.ONE, BigInteger.valueOf(2), BigInteger.valueOf(3)), + testSubject.viewCoefficients() + ); + } + + @Test + void testSetLastCoefficient() { + final SimpleContinuedFraction testSubject = new SimpleContinuedFraction(); + + Assertions.assertThrows(IllegalStateException.class, + () -> testSubject.setLastCoefficient(BigInteger.ONE) + ); + + testSubject.addCoefficient(BigInteger.ONE); + BigInteger oldCoefficient = testSubject.setLastCoefficient(BigInteger.valueOf(-1)); + Assertions.assertEquals(BigInteger.ONE, oldCoefficient); + assertConvergent(-1, 1, testSubject.getCurrentConvergent()); + assertConvergent(1, 0, testSubject.getPreviousConvergent()); + + testSubject.addCoefficient(BigInteger.valueOf(2)); + oldCoefficient = testSubject.setLastCoefficient(BigInteger.valueOf(3)); + Assertions.assertEquals(BigInteger.valueOf(2), oldCoefficient); + assertConvergent(-2, 3, testSubject.getCurrentConvergent()); + assertConvergent(-1, 1, testSubject.getPreviousConvergent()); + + Assertions.assertThrows(IllegalArgumentException.class, + () -> testSubject.setLastCoefficient(BigInteger.valueOf(-3)) + ); + + Assertions.assertEquals( + Arrays.asList(BigInteger.valueOf(-1), BigInteger.valueOf(3)), + testSubject.viewCoefficients() + ); + } + + @Test + void testRemoveLastCoefficient() { + final SimpleContinuedFraction testSubject = new SimpleContinuedFraction(); + testSubject.addCoefficient(BigInteger.ZERO); + testSubject.addCoefficient(BigInteger.valueOf(4)); + testSubject.addCoefficient(BigInteger.valueOf(5)); + testSubject.addCoefficient(BigInteger.valueOf(2)); + + BigInteger removedCoefficient = testSubject.removeLastCoefficient(); + Assertions.assertEquals(BigInteger.valueOf(2), removedCoefficient); + assertConvergent(5, 21, testSubject.getCurrentConvergent()); + assertConvergent(1, 4, testSubject.getPreviousConvergent()); + + removedCoefficient = testSubject.removeLastCoefficient(); + Assertions.assertEquals(BigInteger.valueOf(5), removedCoefficient); + assertConvergent(1, 4, testSubject.getCurrentConvergent()); + assertConvergent(0, 1, testSubject.getPreviousConvergent()); + + removedCoefficient = testSubject.removeLastCoefficient(); + Assertions.assertEquals(BigInteger.valueOf(4), removedCoefficient); + assertConvergent(0, 1, testSubject.getCurrentConvergent()); + assertConvergent(1, 0, testSubject.getPreviousConvergent()); + + removedCoefficient = testSubject.removeLastCoefficient(); + Assertions.assertEquals(BigInteger.ZERO, removedCoefficient); + + Assertions.assertThrows(IllegalStateException.class, + testSubject::removeLastCoefficient + ); + + Assertions.assertTrue(testSubject.viewCoefficients().isEmpty()); + } + + @Test + void testConvergentAccessorsWithEmptyInstance() { + final SimpleContinuedFraction testSubject = new SimpleContinuedFraction(); + + assertConvergent(1, 0, testSubject.getCurrentConvergent()); + assertConvergent(0, 1, testSubject.getPreviousConvergent()); + } + + @Test + void testToBigFraction() { + final SimpleContinuedFraction testSubject = new SimpleContinuedFraction(); + + Assertions.assertThrows(IllegalStateException.class, + testSubject::toBigFraction + ); + + testSubject.addCoefficient(BigInteger.valueOf(3)); + Assertions.assertEquals(BigFraction.of(3, 1), testSubject.toBigFraction()); + + testSubject.addCoefficient(BigInteger.valueOf(2)); + Assertions.assertEquals(BigFraction.of(7, 2), testSubject.toBigFraction()); + } + + @Test + void testCoefficientsOf() { + final Iterator coefficientsIterator = SimpleContinuedFraction.coefficientsOf(BigFraction.of(-415, 93)); + BigInteger[] expectedCoefficients = toBigIntegerArray(-5, 1, 1, 6, 7); + BigFraction[] expectedFractions = new BigFraction[]{ + BigFraction.of(-415, 93), + BigFraction.of(93, 50), + BigFraction.of(50, 43), + BigFraction.of(43, 7), + BigFraction.of(7, 1) + }; + + for (int i = 0; i < expectedCoefficients.length; i++) { + Assertions.assertTrue(coefficientsIterator.hasNext()); + BigInteger[] next = coefficientsIterator.next(); + Assertions.assertEquals(3, next.length); + Assertions.assertEquals(expectedCoefficients[i], next[0]); + Assertions.assertEquals(1, next[2].signum()); + Assertions.assertEquals(expectedFractions[i], BigFraction.of(next[1], next[2])); + } + Assertions.assertFalse(coefficientsIterator.hasNext()); + Assertions.assertThrows(NoSuchElementException.class, + coefficientsIterator::next + ); + } +} From e9dcb40c3f96e1b0ae0108faab14b5cf5eac4fcc Mon Sep 17 00:00:00 2001 From: Schamschi Date: Wed, 17 Jul 2019 13:29:19 +0200 Subject: [PATCH 2/8] NUMBERS-131: Upgrade BigFraction.from(double, int) to from(double, BigInteger) --- .../commons/numbers/fraction/BigFraction.java | 107 +++++++++++++++++- .../numbers/fraction/BigFractionTest.java | 36 ++---- 2 files changed, 112 insertions(+), 31 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 7c8ce9c7a..ea2bca568 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 @@ -20,6 +20,7 @@ import java.math.BigDecimal; import java.math.BigInteger; import java.math.RoundingMode; +import java.util.Iterator; import org.apache.commons.numbers.core.ArithmeticUtils; /** @@ -301,7 +302,7 @@ public static BigFraction from(final double value) { * @throws ArithmeticException if the continued fraction failed to converge. * @return a new instance. * - * @see #from(double,int) + * @see #from(double,BigInteger) */ public static BigFraction from(final double value, final double epsilon, @@ -310,7 +311,15 @@ public static BigFraction from(final double value, } /** - * Create a fraction given the double value and maximum denominator. + * Approximates the given {@code double} value with a fraction such that + * no other fraction with a denominator smaller than or equal to the passed + * upper bound for the denominator will be closer to the {@code double} + * value. Furthermore, no other fraction with the same or a smaller + * denominator will be equally close to the {@code double} value unless the + * denominator limit is set to {@code 1} and the value to be approximated is + * an odd multiple of {@code 0.5}, in which case there will necessarily be + * two equally distant integers surrounding the {@code double} value, one of + * which will then be returned by this method. *

* References: *

    @@ -320,14 +329,102 @@ public static BigFraction from(final double value, * * @param value Value to convert to a fraction. * @param maxDenominator Maximum allowed value for denominator. - * @throws ArithmeticException if the continued fraction failed to converge. + * @throws IllegalArgumentException if the given {@code value} is NaN or + * infinite, or if {@code maxDenominator < 1} * @return a new instance. * * @see #from(double,double,int) */ public static BigFraction from(final double value, - final int maxDenominator) { - return from(value, 0, maxDenominator, 100); + final BigInteger maxDenominator) { + if (maxDenominator.signum() != 1) { + throw new IllegalArgumentException("Upper bound for denominator must be positive: " + maxDenominator); + } + + /* + * Required facts: + * + * 1. Every best rational approximation p/q of α (with q > 0), in the + * sense that |α - p/q| < |α - a/b| for all a/b ≠ p/q and 0 < b <= q, is + * a convergent or a semiconvergent of α's simple continued fraction + * expansion (Continued Fractions by A. Khinchin, theorem 15, p. 22) + * + * 2. Every convergent p/q from the second convergent onwards is a best + * rational approximation (even in the stronger sense that + * |qα - p| < |bα - a|), provided that the last coefficient is greater + * than 1 (theorem 17, p. 26, which ignores the fact that, if a_1 = 1, + * both the first and the second convergent will have a denominator of + * 1 and, should the variable k chosen to be 0, s = k + 1 will therefore + * also be conceivable as the order of the convergent equivalent to the + * expression x_0 / y_0 in addition to s <= k). + * + * 3. It follows that the first convergent is only a best rational + * approximation if a_1 >= 2, i.e. if the second convergent does not + * also have a denominator of 1, and if α ≠ [a_0; 2] (the exceptional + * case mentioned in theorem 17, where the first convergent a_0 will tie + * with the semiconvergent a_0 + 1 as a best rational approximation of α). + * + * 4. A semiconvergent [a_0; a_1, …, a_{n-1}, x], with n >= 1 and + * 0 < x <= a_n, is consequently only a best rational approximation if it + * is closer to α than the (n-1)th-order convergent [a_0; a_1, …, a_{n-1}], + * since the latter is definitely a best rational approximation (unless + * n = 1 and a_1 = 1, but then, the only possible integer value for x is + * a_1 = 1, which will yield the second convergent, which is always a + * best approximation). This is the case if and only if + * + * [a_n; a_{n+1}, a_{n+2}, …] - 2x < q_{n-2} / q_{n-1} + * + * where q_k is the denominator of the k-th-order convergent + * (https://math.stackexchange.com/questions/856861/semi-convergent-of-continued-fractions) + */ + BigFraction valueAsFraction = from(value); + + SimpleContinuedFraction approximation = new SimpleContinuedFraction(); + BigInteger[] currentConvergent; + BigInteger[] convergentBeforePrevious; + + Iterator coefficientsIterator = SimpleContinuedFraction.coefficientsOf(valueAsFraction); + BigInteger[] lastIterationResult; + + do { + convergentBeforePrevious = approximation.getPreviousConvergent(); + lastIterationResult = coefficientsIterator.next(); + approximation.addCoefficient(lastIterationResult[0]); + currentConvergent = approximation.getCurrentConvergent(); + } while (coefficientsIterator.hasNext() && currentConvergent[1].compareTo(maxDenominator) <= 0); + + if (currentConvergent[1].compareTo(maxDenominator) <= 0) { + return valueAsFraction; + } else { + // Calculate the largest possible value for the last coefficient so + // that the denominator will be within the given bounds + BigInteger[] previousConvergent = approximation.getPreviousConvergent(); + BigInteger lastCoefficientMax = maxDenominator.subtract(convergentBeforePrevious[1]).divide(previousConvergent[1]); + + /* + * Determine if the semiconvergent generated with this coefficient + * is a closer approximation than the previous convergent with the + * formula described in point 4. n >= 1 is guaranteed because the + * first convergent's denominator is 1 and thus cannot exceed the + * limit, and if x = 0 is inserted into the inequation, it will + * always be false because a_n >= 1 and k_{n-2} / k_{n-1} <= 1, so + * no need to make a special case for lastCoefficientMax = 0. + */ + boolean semiConvergentIsCloser = lastIterationResult[1] + .subtract(BigInteger.valueOf(2) + .multiply(lastCoefficientMax) + .multiply(lastIterationResult[2])) + .multiply(previousConvergent[1]) + .compareTo(convergentBeforePrevious[1] + .multiply(lastIterationResult[2])) < 0; + + if (semiConvergentIsCloser) { + return of(previousConvergent[0].multiply(lastCoefficientMax).add(convergentBeforePrevious[0]), + previousConvergent[1].multiply(lastCoefficientMax).add(convergentBeforePrevious[1])); + } else { + return of(previousConvergent[0], previousConvergent[1]); + } + } } /** 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 74aefc49b..586f8bf0e 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 @@ -105,34 +105,18 @@ public void testDoubleConstructor() throws Exception { // MATH-181 @Test - public void testDigitLimitConstructor() throws Exception { - assertFraction(2, 5, BigFraction.from(0.4, 9)); - assertFraction(2, 5, BigFraction.from(0.4, 99)); - assertFraction(2, 5, BigFraction.from(0.4, 999)); + public void testDigitLimitConstructor() { + assertFraction(2, 5, BigFraction.from(0.4, BigInteger.valueOf(9))); + assertFraction(2, 5, BigFraction.from(0.4, BigInteger.valueOf(99))); + assertFraction(2, 5, BigFraction.from(0.4, BigInteger.valueOf(999))); - assertFraction(3, 5, BigFraction.from(0.6152, 9)); - assertFraction(8, 13, BigFraction.from(0.6152, 99)); - assertFraction(510, 829, BigFraction.from(0.6152, 999)); - assertFraction(769, 1250, BigFraction.from(0.6152, 9999)); + assertFraction(5, 8, BigFraction.from(0.6152, BigInteger.valueOf(9))); + assertFraction(8, 13, BigFraction.from(0.6152, BigInteger.valueOf(99))); + assertFraction(510, 829, BigFraction.from(0.6152, BigInteger.valueOf(999))); + assertFraction(769, 1250, BigFraction.from(0.6152, BigInteger.valueOf(9999))); // MATH-996 - assertFraction(1, 2, BigFraction.from(0.5000000001, 10)); - } - - // MATH-1029 - @Test - public void testPositiveValueOverflow() { - Assertions.assertThrows(ArithmeticException.class, - () -> assertFraction((long) 1e10, 1, BigFraction.from(1e10, 1000)) - ); - } - - // MATH-1029 - @Test - public void testNegativeValueOverflow() { - Assertions.assertThrows(ArithmeticException.class, - () -> assertFraction((long) -1e10, 1, BigFraction.from(-1e10, 1000)) - ); + assertFraction(1, 2, BigFraction.from(0.5000000001, BigInteger.valueOf(10))); } @Test @@ -500,7 +484,7 @@ public void testMath340() { public void testSerial() { BigFraction[] fractions = { BigFraction.of(3, 4), BigFraction.ONE, BigFraction.ZERO, - BigFraction.of(17), BigFraction.from(Math.PI, 1000), + BigFraction.of(17), BigFraction.from(Math.PI, BigInteger.valueOf(1000)), BigFraction.of(-5, 2) }; for (BigFraction fraction : fractions) { From 8c0a0b964992ebe4623c5272af88cf9d760638f7 Mon Sep 17 00:00:00 2001 From: Schamschi Date: Thu, 18 Jul 2019 12:39:45 +0200 Subject: [PATCH 3/8] NUMBERS-131: Make BigFraction.from(double, double, int) always produce the smallest possible denominator --- .../commons/numbers/fraction/BigFraction.java | 102 ++++++++++++++++-- .../numbers/fraction/BigFractionTest.java | 10 +- 2 files changed, 98 insertions(+), 14 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 ea2bca568..8ed19f62e 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 @@ -287,7 +287,12 @@ public static BigFraction from(final double value) { } /** - * Create a fraction given the double value and maximum error allowed. + * Approximates the given {@code double} value with a fraction such that + * no other fraction within the given interval will have a smaller or equal + * denominator, unless {@code |epsilon| > 0.5}, in which case the integer + * closest to the value to be approximated will be returned (if there are + * two equally distant integers within the specified interval, either of + * them will be returned). *

    * References: *

      @@ -297,17 +302,102 @@ public static BigFraction from(final double value) { * * @param value Value to convert to a fraction. * @param epsilon Maximum error allowed. The resulting fraction is within - * {@code epsilon} of {@code value}, in absolute terms. - * @param maxIterations Maximum number of convergents. - * @throws ArithmeticException if the continued fraction failed to converge. + * {@code epsilon} of {@code value}, in absolute terms. + * @param maxIterations Maximum number of convergents. If this parameter is + * negative, no limit will be imposed. + * @throws ArithmeticException if {@code maxIterations >= 0} and the + * continued fraction failed to converge after this number of + * iterations + * @throws IllegalArgumentException if {@code value} is NaN or infinite, or + * if {@code epsilon} is NaN. * @return a new instance. * * @see #from(double,BigInteger) */ public static BigFraction from(final double value, - final double epsilon, + double epsilon, final int maxIterations) { - return from(value, epsilon, Integer.MAX_VALUE, maxIterations); + BigFraction valueAsFraction = from(value); + /* + * For every rational number outside the interval [α - 0.5, α + 0.5], + * there will be a rational number with the same denominator that lies + * within that interval (because repeatedly adding or subtracting 1 from + * any number is bound to hit the interval eventually). Limiting epsilon + * to ±0.5 thus ensures that, if a number with an absolute value greater + * than 0.5 is passed as epsilon, the integer closest to α is returned, + * and it also eliminates the need to make a special case for epsilon + * being infinite. + */ + epsilon = Math.min(Math.abs(epsilon), 0.5); + BigFraction epsilonAsFraction = from(epsilon); + + BigFraction lowerBound = valueAsFraction.subtract(epsilonAsFraction); + BigFraction upperBound = valueAsFraction.add(epsilonAsFraction); + + /* + * If [a_0; a_1, a_2, …] and [b_0; b_1, b_2, …] are the simple continued + * fraction expansions of the specified interval's boundaries, and n is + * an integer such that a_k = b_k for all k <= n, every real number + * within this interval will have a simple continued fraction expansion + * of the form + * + * [a_0; a_1, …, a_n, c_0, c_1, …] + * + * where [c_0; c_1, c_2 …] lies between [a_{n+1}; a_{n+2}, …] and + * [b_{n+1}; b_{n+2}, …] + * + * The objective is therefore to calculate a value for c_0 so that the + * denominator will grow by the smallest amount possible with the + * resulting number still being within the given interval. + */ + Iterator coefficientsOfLower = SimpleContinuedFraction.coefficientsOf(lowerBound); + Iterator coefficientsOfUpper = SimpleContinuedFraction.coefficientsOf(upperBound); + BigInteger lastCoefficientOfLower; + BigInteger lastCoefficientOfUpper; + + SimpleContinuedFraction approximation = new SimpleContinuedFraction(); + + boolean stop = false; + int iterationCount = 0; + do { + if (maxIterations >= 0 && iterationCount == maxIterations) { + throw new FractionException(FractionException.ERROR_CONVERSION, value, maxIterations); + } + if (coefficientsOfLower.hasNext()) { + lastCoefficientOfLower = coefficientsOfLower.next()[0]; + } else { + lastCoefficientOfLower = null; + } + if (coefficientsOfUpper.hasNext()) { + lastCoefficientOfUpper = coefficientsOfUpper.next()[0]; + } else { + lastCoefficientOfUpper = null; + } + if (lastCoefficientOfLower == null || + !lastCoefficientOfLower.equals(lastCoefficientOfUpper)) { + stop = true; + } else { + approximation.addCoefficient(lastCoefficientOfLower); + } + iterationCount++; + } while (!stop); + + if (lastCoefficientOfLower != null && lastCoefficientOfUpper != null) { + BigInteger finalCoefficient; + if (lastCoefficientOfLower.compareTo(lastCoefficientOfUpper) < 0) { + finalCoefficient = lastCoefficientOfLower; + if (coefficientsOfLower.hasNext()) { + finalCoefficient = finalCoefficient.add(BigInteger.ONE); + } + } else { + finalCoefficient = lastCoefficientOfUpper; + if (coefficientsOfUpper.hasNext()) { + finalCoefficient = finalCoefficient.add(BigInteger.ONE); + } + } + approximation.addCoefficient(finalCoefficient); + } + return approximation.toBigFraction(); } /** 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 586f8bf0e..2724e0979 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 @@ -75,12 +75,6 @@ public void testConstructor() { } catch (ArithmeticException ignored) { // expected } - try { - BigFraction.from(2.0 * Integer.MAX_VALUE, 1.0e-5, 100000); - Assertions.fail("Expecting ArithmeticException"); - } catch (ArithmeticException ignored) { - // expected - } } @Test @@ -120,12 +114,12 @@ public void testDigitLimitConstructor() { } @Test - public void testEpsilonLimitConstructor() throws Exception { + public void testEpsilonLimitConstructor() { assertFraction(2, 5, BigFraction.from(0.4, 1.0e-5, 100)); assertFraction(3, 5, BigFraction.from(0.6152, 0.02, 100)); assertFraction(8, 13, BigFraction.from(0.6152, 1.0e-3, 100)); - assertFraction(251, 408, BigFraction.from(0.6152, 1.0e-4, 100)); + assertFraction(171, 278, BigFraction.from(0.6152, 1.0e-4, 100)); assertFraction(251, 408, BigFraction.from(0.6152, 1.0e-5, 100)); assertFraction(510, 829, BigFraction.from(0.6152, 1.0e-6, 100)); assertFraction(769, 1250, BigFraction.from(0.6152, 1.0e-7, 100)); From 3a4a68de85e0731205496969e5101755b7aec07d Mon Sep 17 00:00:00 2001 From: Schamschi Date: Thu, 18 Jul 2019 12:43:14 +0200 Subject: [PATCH 4/8] NUMBERS-131: Delete obsolete factory method from(double, double, int, int), and re-insert introductory sentences in Javadoc --- .../commons/numbers/fraction/BigFraction.java | 138 +++--------------- 1 file changed, 21 insertions(+), 117 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 8ed19f62e..3e3dcf016 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 @@ -89,108 +89,6 @@ private BigFraction(BigInteger num, BigInteger den) { } } - /** - * Create a fraction given the double value and either the maximum - * error allowed or the maximum number of denominator digits. - * - *

      - * NOTE: This method is called with - * - EITHER a valid epsilon value and the maxDenominator set to - * Integer.MAX_VALUE (that way the maxDenominator has no effect) - * - OR a valid maxDenominator value and the epsilon value set to - * zero (that way epsilon only has effect if there is an exact - * match before the maxDenominator value is reached). - *

      - *

      - * It has been done this way so that the same code can be reused for - * both scenarios. However this could be confusing to users if it - * were part of the public API and this method should therefore remain - * PRIVATE. - *

      - * - * See JIRA issue ticket MATH-181 for more details: - * https://issues.apache.org/jira/browse/MATH-181 - * - * @param value Value to convert to a fraction. - * @param epsilon Maximum error allowed. - * The resulting fraction is within {@code epsilon} of {@code value}, - * in absolute terms. - * @param maxDenominator Maximum denominator value allowed. - * @param maxIterations Maximum number of convergents. - * @throws ArithmeticException if the continued fraction failed to converge. - */ - private static BigFraction from(final double value, - final double epsilon, - final int maxDenominator, - final int maxIterations) { - long overflow = Integer.MAX_VALUE; - double r0 = value; - long a0 = (long) Math.floor(r0); - - if (Math.abs(a0) > overflow) { - throw new FractionException(FractionException.ERROR_CONVERSION_OVERFLOW, value, a0, 1l); - } - - // check for (almost) integer arguments, which should not go - // to iterations. - if (Math.abs(a0 - value) < epsilon) { - return new BigFraction(BigInteger.valueOf(a0), - BigInteger.ONE); - } - - long p0 = 1; - long q0 = 0; - long p1 = a0; - long q1 = 1; - - long p2 = 0; - long q2 = 1; - - int n = 0; - boolean stop = false; - do { - ++n; - final double r1 = 1d / (r0 - a0); - final long a1 = (long) Math.floor(r1); - p2 = (a1 * p1) + p0; - q2 = (a1 * q1) + q0; - if (p2 > overflow || - q2 > overflow) { - // in maxDenominator mode, if the last fraction was very close to the actual value - // q2 may overflow in the next iteration; in this case return the last one. - if (epsilon == 0 && - Math.abs(q1) < maxDenominator) { - break; - } - throw new FractionException(FractionException.ERROR_CONVERSION_OVERFLOW, value, p2, q2); - } - - final double convergent = (double) p2 / (double) q2; - if (n < maxIterations && - Math.abs(convergent - value) > epsilon && - q2 < maxDenominator) { - p0 = p1; - p1 = p2; - q0 = q1; - q1 = q2; - a0 = a1; - r0 = r1; - } else { - stop = true; - } - } while (!stop); - - if (n >= maxIterations) { - throw new FractionException(FractionException.ERROR_CONVERSION, value, maxIterations); - } - - return q2 < maxDenominator ? - new BigFraction(BigInteger.valueOf(p2), - BigInteger.valueOf(q2)) : - new BigFraction(BigInteger.valueOf(p1), - BigInteger.valueOf(q1)); - } - /** *

      * Create a {@link BigFraction} equivalent to the passed {@code BigInteger}, ie @@ -287,12 +185,15 @@ public static BigFraction from(final double value) { } /** - * Approximates the given {@code double} value with a fraction such that - * no other fraction within the given interval will have a smaller or equal - * denominator, unless {@code |epsilon| > 0.5}, in which case the integer - * closest to the value to be approximated will be returned (if there are - * two equally distant integers within the specified interval, either of - * them will be returned). + * Create a fraction given the double value and maximum error allowed. + *

      + * This factory method approximates the given {@code double} value with a + * fraction such that no other fraction within the given interval will have + * a smaller or equal denominator, unless {@code |epsilon| > 0.5}, in which + * case the integer closest to the value to be approximated will be returned + * (if there are two equally distant integers within the specified interval, + * either of them will be returned). + *

      *

      * References: *

        @@ -401,15 +302,18 @@ public static BigFraction from(final double value, } /** - * Approximates the given {@code double} value with a fraction such that - * no other fraction with a denominator smaller than or equal to the passed - * upper bound for the denominator will be closer to the {@code double} - * value. Furthermore, no other fraction with the same or a smaller - * denominator will be equally close to the {@code double} value unless the - * denominator limit is set to {@code 1} and the value to be approximated is - * an odd multiple of {@code 0.5}, in which case there will necessarily be - * two equally distant integers surrounding the {@code double} value, one of - * which will then be returned by this method. + * Create a fraction given the double value and maximum denominator. + *

        + * This factory method approximates the given {@code double} value with a + * fraction such that no other fraction with a denominator smaller than or + * equal to the passed upper bound for the denominator will be closer to the + * {@code double} value. Furthermore, no other fraction with the same or a + * smaller denominator will be equally close to the {@code double} value + * unless the denominator limit is set to {@code 1} and the value to be + * approximated is an odd multiple of {@code 0.5}, in which case there will + * necessarily be two equally distant integers surrounding the {@code double} + * value, one of which will then be returned by this method. + *

        *

        * References: *

          From a4e473dc4a9cf83acf9ff0164b21a6098bc57145 Mon Sep 17 00:00:00 2001 From: Schamschi Date: Thu, 18 Jul 2019 12:53:40 +0200 Subject: [PATCH 5/8] NUMBERS-131: Use ternary operator for readability --- .../commons/numbers/fraction/BigFraction.java | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 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 3e3dcf016..d9ebc4b3f 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 @@ -264,16 +264,12 @@ public static BigFraction from(final double value, if (maxIterations >= 0 && iterationCount == maxIterations) { throw new FractionException(FractionException.ERROR_CONVERSION, value, maxIterations); } - if (coefficientsOfLower.hasNext()) { - lastCoefficientOfLower = coefficientsOfLower.next()[0]; - } else { - lastCoefficientOfLower = null; - } - if (coefficientsOfUpper.hasNext()) { - lastCoefficientOfUpper = coefficientsOfUpper.next()[0]; - } else { - lastCoefficientOfUpper = null; - } + lastCoefficientOfLower = coefficientsOfLower.hasNext() ? + coefficientsOfLower.next()[0] : + null; + lastCoefficientOfUpper = coefficientsOfUpper.hasNext() ? + coefficientsOfUpper.next()[0] : + null; if (lastCoefficientOfLower == null || !lastCoefficientOfLower.equals(lastCoefficientOfUpper)) { stop = true; From 9b7040f7102983e4decd5ea476ea73a9ece49688 Mon Sep 17 00:00:00 2001 From: Schamschi Date: Thu, 18 Jul 2019 20:56:44 +0200 Subject: [PATCH 6/8] NUMBERS-131: Add unit tests, refine Javadoc --- .../commons/numbers/fraction/BigFraction.java | 16 +++--- .../numbers/fraction/BigFractionTest.java | 57 +++++++++++++++++++ 2 files changed, 66 insertions(+), 7 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 d9ebc4b3f..1945a259f 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 @@ -203,7 +203,7 @@ public static BigFraction from(final double value) { * * @param value Value to convert to a fraction. * @param epsilon Maximum error allowed. The resulting fraction is within - * {@code epsilon} of {@code value}, in absolute terms. + * {@code |epsilon|} of {@code value}, in absolute terms. * @param maxIterations Maximum number of convergents. If this parameter is * negative, no limit will be imposed. * @throws ArithmeticException if {@code maxIterations >= 0} and the @@ -303,12 +303,13 @@ public static BigFraction from(final double value, * This factory method approximates the given {@code double} value with a * fraction such that no other fraction with a denominator smaller than or * equal to the passed upper bound for the denominator will be closer to the - * {@code double} value. Furthermore, no other fraction with the same or a - * smaller denominator will be equally close to the {@code double} value - * unless the denominator limit is set to {@code 1} and the value to be - * approximated is an odd multiple of {@code 0.5}, in which case there will - * necessarily be two equally distant integers surrounding the {@code double} - * value, one of which will then be returned by this method. + * {@code double} value. Furthermore, no other fraction with a denominator + * smaller than or equal to that of the returned fraction will be equally + * close to the {@code double} value unless the denominator limit is set to + * {@code 1} and the value to be approximated is an odd multiple of + * {@code 0.5}, in which case there will necessarily be two equally distant + * integers surrounding the {@code double} value, one of which will then be + * returned by this method. *

          *

          * References: @@ -321,6 +322,7 @@ public static BigFraction from(final double value, * @param maxDenominator Maximum allowed value for denominator. * @throws IllegalArgumentException if the given {@code value} is NaN or * infinite, or if {@code maxDenominator < 1} + * @throws NullPointerException if {@code maxDenominator} is {@code null} * @return a new instance. * * @see #from(double,double,int) 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 2724e0979..794f00f87 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 @@ -100,21 +100,63 @@ public void testDoubleConstructor() throws Exception { // MATH-181 @Test public void testDigitLimitConstructor() { + Assertions.assertThrows(IllegalArgumentException.class, + () -> BigFraction.from(4d, BigInteger.ZERO) + ); + Assertions.assertThrows(IllegalArgumentException.class, + () -> BigFraction.from(-2d, BigInteger.valueOf(-1)) + ); + Assertions.assertThrows(IllegalArgumentException.class, + () -> BigFraction.from(Double.NaN, BigInteger.valueOf(6)) + ); + Assertions.assertThrows(IllegalArgumentException.class, + () -> BigFraction.from(Double.POSITIVE_INFINITY, BigInteger.valueOf(23456789012345L)) + ); + Assertions.assertThrows(IllegalArgumentException.class, + () -> BigFraction.from(Double.NEGATIVE_INFINITY, BigInteger.ONE) + ); + + Assertions.assertThrows(NullPointerException.class, + () -> BigFraction.from(23d, null) + ); + assertFraction(2, 5, BigFraction.from(0.4, BigInteger.valueOf(9))); assertFraction(2, 5, BigFraction.from(0.4, BigInteger.valueOf(99))); assertFraction(2, 5, BigFraction.from(0.4, BigInteger.valueOf(999))); + assertFraction(3602879701896397L, 1L << 53, BigFraction.from(0.4, BigInteger.valueOf(1L << 54))); assertFraction(5, 8, BigFraction.from(0.6152, BigInteger.valueOf(9))); assertFraction(8, 13, BigFraction.from(0.6152, BigInteger.valueOf(99))); assertFraction(510, 829, BigFraction.from(0.6152, BigInteger.valueOf(999))); assertFraction(769, 1250, BigFraction.from(0.6152, BigInteger.valueOf(9999))); + assertFraction(2770614490758329L, 1L << 52, BigFraction.from(0.6152, BigInteger.valueOf(1L << 52))); // MATH-996 assertFraction(1, 2, BigFraction.from(0.5000000001, BigInteger.valueOf(10))); + + assertFraction(-5, 1, BigFraction.from(Math.nextDown(-4.5), BigInteger.ONE)); + assertFraction(-4, 1, BigFraction.from(Math.nextUp(-4.5), BigInteger.ONE)); + { + BigFraction f = BigFraction.from(-4.5, BigInteger.ONE); + Assertions.assertTrue(f.equals(BigFraction.of(-5)) || f.equals(BigFraction.of(-4)), "Expected: -5/1 or -4/1; Actual: " + f.toString()); + } } @Test public void testEpsilonLimitConstructor() { + Assertions.assertThrows(IllegalArgumentException.class, + () -> BigFraction.from(0.13579d, Double.NaN, 10) + ); + Assertions.assertThrows(IllegalArgumentException.class, + () -> BigFraction.from(Double.NaN, 1.0e-5, -10) + ); + Assertions.assertThrows(IllegalArgumentException.class, + () -> BigFraction.from(Double.POSITIVE_INFINITY, 1.0e-4, Integer.MAX_VALUE) + ); + Assertions.assertThrows(IllegalArgumentException.class, + () -> BigFraction.from(Double.NEGATIVE_INFINITY, 1.0e-3, 1) + ); + assertFraction(2, 5, BigFraction.from(0.4, 1.0e-5, 100)); assertFraction(3, 5, BigFraction.from(0.6152, 0.02, 100)); @@ -123,6 +165,21 @@ public void testEpsilonLimitConstructor() { assertFraction(251, 408, BigFraction.from(0.6152, 1.0e-5, 100)); assertFraction(510, 829, BigFraction.from(0.6152, 1.0e-6, 100)); assertFraction(769, 1250, BigFraction.from(0.6152, 1.0e-7, 100)); + + assertFraction(1, 3, BigFraction.from(1d / 3d, 0x0.6P-54, -1)); + assertFraction(3099251356470019L, 9297754069410058L, BigFraction.from(1d / 3d, 0x0.5P-54, -1)); + + assertFraction(1, 2, BigFraction.from(15d / 32d, 1d / 32d, -5)); + assertFraction(3, 4, BigFraction.from(49d / 64d, 1d / 64d, -5)); + assertFraction(1, 2, BigFraction.from(13d / 32d, 3d / 32d, 2)); + assertFraction(5, 16, BigFraction.from(5147d / 0x1P14, 27d / 0x1P14, 3)); + + assertFraction(-5, 1, BigFraction.from(Math.nextDown(-4.5), Double.POSITIVE_INFINITY, -1)); + assertFraction(-5, 1, BigFraction.from(Math.nextDown(-4.5), Double.NEGATIVE_INFINITY, -1)); + { + BigFraction f = BigFraction.from(-4.5, 0.5, -1); + Assertions.assertTrue(f.equals(BigFraction.of(-5)) || f.equals(BigFraction.of(-4)), "Expected: -5/1 or -4/1; Actual: " + f.toString()); + } } @Test From 615c5c918342611998edd956fec5b51a64fda1c6 Mon Sep 17 00:00:00 2001 From: Schamschi Date: Thu, 18 Jul 2019 22:04:03 +0200 Subject: [PATCH 7/8] NUMBERS-131: Eliminate hint of code duplication --- .../apache/commons/numbers/fraction/BigFraction.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 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 1945a259f..a67c76511 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 @@ -281,16 +281,16 @@ public static BigFraction from(final double value, if (lastCoefficientOfLower != null && lastCoefficientOfUpper != null) { BigInteger finalCoefficient; + Iterator iteratorOfSmallerLastCoefficient; if (lastCoefficientOfLower.compareTo(lastCoefficientOfUpper) < 0) { finalCoefficient = lastCoefficientOfLower; - if (coefficientsOfLower.hasNext()) { - finalCoefficient = finalCoefficient.add(BigInteger.ONE); - } + iteratorOfSmallerLastCoefficient = coefficientsOfLower; } else { finalCoefficient = lastCoefficientOfUpper; - if (coefficientsOfUpper.hasNext()) { - finalCoefficient = finalCoefficient.add(BigInteger.ONE); - } + iteratorOfSmallerLastCoefficient = coefficientsOfUpper; + } + if (iteratorOfSmallerLastCoefficient.hasNext()) { + finalCoefficient = finalCoefficient.add(BigInteger.ONE); } approximation.addCoefficient(finalCoefficient); } From cf54480eb238d9e3f722d9626842c864f7369d03 Mon Sep 17 00:00:00 2001 From: Schamschi Date: Fri, 19 Jul 2019 00:05:06 +0200 Subject: [PATCH 8/8] NUMBERS-131: Explicitly disallow null arguments in SimpleContinuedFraction methods --- .../fraction/SimpleContinuedFraction.java | 9 +++++++++ .../fraction/SimpleContinuedFractionTest.java | 19 +++++++++++++++---- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/SimpleContinuedFraction.java b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/SimpleContinuedFraction.java index 76d4cc5f6..9c4f52913 100644 --- a/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/SimpleContinuedFraction.java +++ b/commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/SimpleContinuedFraction.java @@ -24,6 +24,7 @@ import java.util.Iterator; import java.util.List; import java.util.NoSuchElementException; +import java.util.Objects; /** * A {@code BigInteger} based, mutable representation of a simple continued @@ -79,12 +80,15 @@ class SimpleContinuedFraction { * coefficients stored, the new coefficient can be any integer, otherwise, * it must be positive. * @param coefficient the new coefficient + * @throws NullPointerException if the argument is {@code null} * @throws IllegalArgumentException if this instance already has coefficients * stored and the argument is not positive */ void addCoefficient(BigInteger coefficient) { if (!coefficients.isEmpty()) { requirePositiveCoefficient(coefficient); + } else { + Objects.requireNonNull(coefficient); } coefficients.add(coefficient); BigInteger newNumerator = coefficient.multiply(currentConvergent[0]).add(previousConvergent[0]); @@ -101,6 +105,7 @@ void addCoefficient(BigInteger coefficient) { * @param coefficient the new coefficient * @return the coefficient previously at the last position in this instance's * simple continued fraction representation + * @throws NullPointerException if the argument is {@code null} * @throws IllegalArgumentException if this instance has more than one * coefficient stored and the argument is not positive * @throws IllegalStateException if this instance does not have any @@ -109,6 +114,8 @@ void addCoefficient(BigInteger coefficient) { BigInteger setLastCoefficient(BigInteger coefficient) { if (coefficients.size() > 1) { requirePositiveCoefficient(coefficient); + } else { + Objects.requireNonNull(coefficient); } BigInteger oldLastCoefficient; try { @@ -197,6 +204,7 @@ BigFraction toBigFraction() { * {@code IllegalArgumentException} with a corresponding error message if it * is not. * @param coefficient the prospective non-initial coefficient + * @throws NullPointerException if the argument is {@code null} * @throws IllegalArgumentException if the argument is not positive */ private static void requirePositiveCoefficient(BigInteger coefficient) { @@ -228,6 +236,7 @@ private static void requirePositiveCoefficient(BigInteger coefficient) { * @param fraction the fraction the coefficients of whose simple continued * fraction representation should be iterated over * @return an {@code Iterator} as described above + * @throws NullPointerException if the argument is {@code null} */ static Iterator coefficientsOf(BigFraction fraction) { final BigInteger initialDividend; diff --git a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/SimpleContinuedFractionTest.java b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/SimpleContinuedFractionTest.java index fc0be5a72..60f8f0530 100644 --- a/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/SimpleContinuedFractionTest.java +++ b/commons-numbers-fraction/src/test/java/org/apache/commons/numbers/fraction/SimpleContinuedFractionTest.java @@ -4,6 +4,7 @@ import java.util.Arrays; import java.util.Iterator; +import java.util.List; import java.util.NoSuchElementException; import org.junit.jupiter.api.Assertions; @@ -29,6 +30,7 @@ private static BigInteger[] toBigIntegerArray(long... arr) { @Test void testAddCoefficient() { final SimpleContinuedFraction testSubject = new SimpleContinuedFraction(); + List actualCoefficients = testSubject.viewCoefficients(); testSubject.addCoefficient(BigInteger.ONE); assertConvergent(1, 1, testSubject.getCurrentConvergent()); @@ -42,19 +44,22 @@ void testAddCoefficient() { assertConvergent(10, 7, testSubject.getCurrentConvergent()); assertConvergent(3, 2, testSubject.getPreviousConvergent()); + Assertions.assertThrows(NullPointerException.class, + () -> testSubject.addCoefficient(null) + ); Assertions.assertThrows(IllegalArgumentException.class, () -> testSubject.addCoefficient(BigInteger.valueOf(-4)) ); - Assertions.assertEquals( Arrays.asList(BigInteger.ONE, BigInteger.valueOf(2), BigInteger.valueOf(3)), - testSubject.viewCoefficients() + actualCoefficients ); } @Test void testSetLastCoefficient() { final SimpleContinuedFraction testSubject = new SimpleContinuedFraction(); + List actualCoefficients = testSubject.viewCoefficients(); Assertions.assertThrows(IllegalStateException.class, () -> testSubject.setLastCoefficient(BigInteger.ONE) @@ -72,13 +77,15 @@ void testSetLastCoefficient() { assertConvergent(-2, 3, testSubject.getCurrentConvergent()); assertConvergent(-1, 1, testSubject.getPreviousConvergent()); + Assertions.assertThrows(NullPointerException.class, + () -> testSubject.setLastCoefficient(null) + ); Assertions.assertThrows(IllegalArgumentException.class, () -> testSubject.setLastCoefficient(BigInteger.valueOf(-3)) ); - Assertions.assertEquals( Arrays.asList(BigInteger.valueOf(-1), BigInteger.valueOf(3)), - testSubject.viewCoefficients() + actualCoefficients ); } @@ -140,6 +147,10 @@ void testToBigFraction() { @Test void testCoefficientsOf() { + Assertions.assertThrows(NullPointerException.class, + () -> SimpleContinuedFraction.coefficientsOf(null) + ); + final Iterator coefficientsIterator = SimpleContinuedFraction.coefficientsOf(BigFraction.of(-415, 93)); BigInteger[] expectedCoefficients = toBigIntegerArray(-5, 1, 1, 6, 7); BigFraction[] expectedFractions = new BigFraction[]{