Numbers 188.refactor unary functions #118

sumanth-rajkumar · 2022-07-21T15:13:02Z

This PR refactors Complex class using functional interfaces and static methods.
This allows reuse of the refactored static functions for Operations on List/Matrices of Complex Numbers (NUMBERS-186)

The changes retain binary backward compatibility.

Summary of changes

Introduced following interfaces for Complex Number Operations

ComplexUnaryOperator - unary operations on complex number represented as double real and imaginary parts
ComplexSink - interface that acts as a consumer for the complex result from double real and imaginary parts

Refactored all unary operations instance methods of Complex class as static functions in ComplexFunctions
The static functions use the functional interface signatures described above

…tions

aherbert

Hi @sumanth-rajkumar

The changes look good. There are a few formatting tweaks, notably to align the real and imaginary parts passed to the ComplexSink, and to make consistent indentation for the method arguments in test classes.

Note that z is commonly used for complex numbers. When there is no variable name clash then this is the preferred name. However note that when the value is not a complex number (as in TestUtils.assertSame with the ToDoubleFunction) then z should be avoided.

But the main change would be to add a new TestUtils method for a custom predicate:

    /**
     * Represents a predicate (boolean-valued function) of two {@code double}-valued
     * arguments. This is the {@code double}-consuming primitive type specialization
     * of {@link java.util.function.BiPredicate BiPredicate}.
     */
    @FunctionalInterface
    public interface DoubleBinaryPredicate {
        /**
         * Evaluates this predicate on the given argument.
         *
         * @param a the first input argument
         * @param b the second input argument
         * @return {@code true} if the input arguments match the predicate,
         * otherwise {@code false}
         */
        boolean test(double a, double b);
    }

    // Comments ...
    public static boolean assertSame(Complex c,
                                     String name,
                                     Predicate<Complex> operation1,
                                     DoubleBinaryPredicate operation2) {
        final boolean b1 = operation1.test(c);
        // Test operation2 produces the exact same result
        final boolean b2 = operation2.test(c.real(), c.imag());
        Assertions.assertEquals(b1, b2, () -> "Predicate operator mismatch: " + name);
        return b1;
    }

You will have to declare a new interface which you can put as an inner interface at the top of TestUtils.

Then refactor all the tests that use isNaN, isInfinite and isFinite to use this utility function.

aherbert · 2022-07-22T09:16:49Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/TestUtils.java

+        final double z = operation1.applyAsDouble(c);
+        // Test operation2 produces the exact same result
+        final double z2 = operation2.applyAsDouble(c.real(), c.imag());
+        Assertions.assertEquals(z, z2, () -> "Unary operator mismatch: " + name);


"To Double operator mismatch: "

aherbert · 2022-07-22T09:22:19Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/TestUtils.java

+    public static void assertAbs(double expected, Complex input) {
+        final double actual = assertSame(input, "abs", Complex::abs, ComplexFunctions::abs);
+        Assertions.assertEquals(expected, actual);
+        Assertions.assertEquals(actual, assertSame(input, "abs", Complex::abs, ComplexFunctions::abs));


This line is not required. You are repeating the computation from 2 lines above.

Although this method is used by two other classes I do not hink it generic enough to put in TestUtils. There are similar assertNorm and assertArgument functions in ComplexTest. I would put them into the Test class that uses them.

Note that in all these helper assert methods you repeat the computation and test it is exactly the same as the actual that was just computed. The method is very simple and could be put into the Test class commented as:

private static void assertAbs(double expected, Complex input) { final double actual = assertSame(input, "abs", Complex::abs, ComplexFunctions::abs); Assertions.assertEquals(expected, actual); }

aherbert · 2022-07-22T09:30:52Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/CStandardTest.java

        final Complex c1 = TestUtils.assertSame(zConj, name, operation1, operation2);
-        final Complex c2 = TestUtils.assertSame(z, name, operation1, operation2).conj();
+        Complex input = TestUtils.assertSame(z, name, operation1, operation2);


Missing final. Rename to result

aherbert · 2022-07-22T09:32:43Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/CStandardTest.java

@@ -469,7 +402,7 @@ private static void assertFunctionType(Complex z,
            !equals(t1.getImaginary(), t2.getImaginary())) {
            Assertions.fail(
                String.format("%s equality failed (z=%s, -z=%s). Expected: %s but was: %s (Unspecified sign = %s)",
-                              type, z, z.negate(), c1, c2, sign));


Undo formatting here

aherbert · 2022-07-22T09:34:34Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/CStandardTest.java

-            ComplexUnaryOperator<ComplexNumber> operation2,
-            Complex expected) {
-        assertComplex(z, name, operation1, operation2, expected, FunctionType.NONE, UnspecifiedSign.NONE);
+                                      String name, UnaryOperator<Complex> operation1,


By changing the indentation here it is difficult to understand the minor change to add the sign argument.

This class is indenting 8 spaces for trailing arguments on new lines.

aherbert · 2022-07-22T10:32:49Z

commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexFunctions.java

+        final double cosy = Math.cos(imaginary);
+        final double divisor = sinhx * sinhx + cosy * cosy;
+        return action.apply(sinhx * coshx / divisor,
+            siny * cosy / divisor);


Align real and imaginary passed to the apply function

aherbert · 2022-07-22T10:33:13Z

commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexFunctions.java

+        re /= 4;
+        im /= 2;
+        return action.apply(changeSign(re, real),
+            changeSign(im, imaginary));


Align real and imaginary passed to the apply function

aherbert · 2022-07-22T10:34:16Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/CReferenceTest.java

     */
    private static void assertOperation(String name,
-            UnaryOperator<Complex> operation, long maxUlps) {
+                                        UnaryOperator<Complex> operation1,


Keep the formatting at indented 8 spaces for consistency in the class

aherbert · 2022-07-22T10:36:43Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/CStandardTest.java

+     * @param type Function type.
+     */
+    private static void assertComplex(Complex z,
+                                      String name,


For consistency, wrap the arguments and use 8 spaces for trailing indentation

aherbert · 2022-07-22T10:37:25Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/CStandardTest.java

@@ -1049,8 +912,13 @@ void testDivide() {
     */
    @Test
    void testSqrt1() {
-        assertComplex(complex(-2.0, 0.0).sqrt(), complex(0.0, Math.sqrt(2)));
-        assertComplex(complex(-2.0, -0.0).sqrt(), complex(0.0, -Math.sqrt(2)));
+        Complex input = complex(-2.0, 0.0);


Rename input to z

codecov-commenter · 2022-07-22T19:04:07Z

Codecov Report

Merging #118 (ac33274) into complex-gsoc-2022 (5939b5e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                 Coverage Diff                  @@
##             complex-gsoc-2022     #118   +/-   ##
====================================================
  Coverage                99.12%   99.13%           
- Complexity                1662     1681   +19     
====================================================
  Files                       65       65           
  Lines                     4234     4257   +23     
  Branches                   834      834           
====================================================
+ Hits                      4197     4220   +23     
  Misses                      10       10           
  Partials                    27       27

Impacted Files	Coverage Δ
...ache/commons/numbers/complex/ComplexFunctions.java	`100.00% <ø> (ø)`
...va/org/apache/commons/numbers/complex/Complex.java	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us.

aherbert

Hi @sumanth-rajkumar

Thanks for the updates. I noticed that a lot of the javadoc for ComplexFunctions has been copied from the previous internal helper functions. This should be copied from the public API functions. This also applies to the abs function that was previously merged. Can you update these. In the case of 3 functions they have javadoc stating the method has been adapted from the Boost. For those function you can rename them using the prefix compute so we maintain the attribution.

You have updated formatting for trailing arguments on some existing functions to 8 spaces from the start of the line. This should be 12 spaces from the start of the line, or 8 spaces from the non-whitespace character of the previous line. Sorry for the confusion.

I think some usage of the TestUtils.assertSame with the DoubleBinaryPredicate is not required. You can revert these changes in CStandardTest.

aherbert · 2022-07-22T19:49:00Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/CStandardTest.java

@@ -335,8 +278,8 @@ private static void assertConjugateEquality(Complex z,
     * @param operation2 Operation on the complex real and imaginary parts.
     */
    private static void assertConjugateEquality(String name,
-            UnaryOperator<Complex> operation1,


When I said indent 8 spaces I meant from the first non-whitespace character on the previous line:

private static void assertConjugateEquality(String name, UnaryOperator<Complex> operation1, ComplexUnaryOperator<ComplexNumber> operation2) { // Edge cases. Inf/NaN are specifically handled in the C99 test cases

aherbert · 2022-07-22T19:50:01Z

commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexFunctions.java

-    /** {@code 1/2}. */
-    private static final double HALF = 0.5;
-    /** Base 10 logarithm of e divided by 2 (log10(e)/2). */
+    /** Base 10 logarithm of e divided by 2. */


This is still not the original as it lacks the computation in brackets

/** Base 10 logarithm of e divided by 2 (log10(e)/2). */

aherbert · 2022-07-22T19:51:35Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/CStandardTest.java

+     * @param operation operation on the Complex objects.
+     * @param condition1 Condition operation on the Complex object.
+     * @param condition2 Condition operation on the complex real and imaginary parts.
+     * @param operationName the operation name


The javadoc @param order should match the argument order. operationName is in the wrong place.

aherbert · 2022-07-22T19:54:41Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/CStandardTest.java

@@ -883,7 +785,7 @@ private static ArrayList<Complex> createZeroFinites() {
     */
    private static ArrayList<Complex> createNaNs() {
        final double[] values = {0, 1, nan};
-        return createCombinations(values, Complex::isNaN);
+        return createCombinations(values, "NaN", Complex::isNaN, ComplexFunctions::isNaN);


operation name is isNaN

aherbert · 2022-07-22T19:54:49Z

commons-numbers-complex/src/test/java/org/apache/commons/numbers/complex/CStandardTest.java

@@ -853,7 +755,7 @@ private static Complex complex(double real, double imaginary) {
     */
    private static ArrayList<Complex> createInfinites() {
        final double[] values = {0, 1, inf, negInf, nan};
-        return createCombinations(values, Complex::isInfinite);
+        return createCombinations(values, "Inf", Complex::isInfinite, ComplexFunctions::isInfinite);


operation name is isInfinite

aherbert · 2022-07-22T21:29:17Z

commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexFunctions.java

+            return action.apply(exp, imaginary);
+        }
+        return action.apply(exp * Math.cos(imaginary),
+                        exp * Math.sin(imaginary));


Indentation

aherbert · 2022-07-22T21:29:24Z

commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexFunctions.java

+                zeroOrInf = real;
+            }
+            return action.apply(zeroOrInf * Math.cos(imaginary),
+                            zeroOrInf * Math.sin(imaginary));


Indentation

aherbert · 2022-07-22T21:30:32Z

commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexFunctions.java

+        }
+        // No overflow of sinh/cosh
+        return action.apply(Math.sinh(real) * Math.cos(imaginary),
+                        Math.cosh(real) * Math.sin(imaginary));


Indentation

aherbert · 2022-07-22T21:30:39Z

commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexFunctions.java

+        }
+        // No overflow of sinh/cosh
+        return action.apply(Math.cosh(real) * Math.cos(imaginary),
+                        Math.sinh(real) * Math.sin(imaginary));


Indentation

aherbert · 2022-07-22T21:30:59Z

commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexFunctions.java

+        final double cosy = Math.cos(imaginary);
+        final double divisor = sinhx * sinhx + cosy * cosy;
+        return action.apply(sinhx * coshx / divisor,
+                        siny * cosy / divisor);


Indentation

aherbert · 2022-07-25T10:08:49Z

Squashed and merged in commit ab614de

sumanth-rajkumar added 10 commits July 20, 2022 16:03

NUMBERS-188 refactor complex log function using functional interfaces

bdd8732

NUMBERS-188 revert unintended change

aa6cf62

NUMBERS-188 applied requested changes

57eb5bf

NUMBERS-188 made javadoc changes

412cceb

NUMBERS-188 refactor complex log function using functional interfaces

6b172b7

NUMBERS-188 revert unintended change

6bf2c63

NUMBERS-188 applied requested changes

4b10761

NUMBERS-188 made javadoc changes

05db4ad

Merge branch 'complex_gsoc_2022' into NUMBERS-188.refactor_unary_func…

5a4b3cb

…tions

NUMBERS-188 refactored all unary operations in all classes

23ec5f2

aherbert requested changes Jul 22, 2022

View reviewed changes

sumanth-rajkumar added 2 commits July 22, 2022 14:53

NUMBERS-188 refactored boolean methods in test classes

abc0ed8

NUMBERS-188 refactored condition methods in test classes

66e57ff

aherbert requested changes Jul 22, 2022

View reviewed changes

NUMBERS-188 preserved javadoc consistency

ac33274

aherbert closed this Jul 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Numbers 188.refactor unary functions #118

Numbers 188.refactor unary functions #118

sumanth-rajkumar commented Jul 21, 2022

aherbert left a comment

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

codecov-commenter commented Jul 22, 2022 •

edited

aherbert left a comment

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert Jul 22, 2022

aherbert commented Jul 25, 2022

Numbers 188.refactor unary functions #118

Numbers 188.refactor unary functions #118

Conversation

sumanth-rajkumar commented Jul 21, 2022

aherbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 22, 2022 • edited

Codecov Report

aherbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aherbert commented Jul 25, 2022

codecov-commenter commented Jul 22, 2022 •

edited