Numbers 188.refactor complex log function #117

sumanth-rajkumar · 2022-07-15T21:57:56Z

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
ComplexConstructor - interface to create a generic complex result from double real and imaginary parts

As a first step, refactored log and log10 instance methods of Complex class as static functions in ComplexFunctions
The static functions use the functional interface signatures described above

resync with apache master

codecov-commenter · 2022-07-15T22:00:54Z

Codecov Report

Merging #117 (142fa7b) into complex-gsoc-2022 (bd454ef) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                 Coverage Diff                  @@
##             complex-gsoc-2022     #117   +/-   ##
====================================================
  Coverage                99.12%   99.12%           
- Complexity                1653     1662    +9     
====================================================
  Files                       64       65    +1     
  Lines                     4225     4234    +9     
  Branches                   834      834           
====================================================
+ Hits                      4188     4197    +9     
  Misses                      10       10           
  Partials                    27       27

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

darkma773r · 2022-07-16T12:45:13Z

...ons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexConstructor.java

+ * @param <R> Generic
+ */
+@FunctionalInterface
+public interface ComplexConstructor<R> {


The word "constructor" has a very specific meaning in Java. Perhaps we could use "ComplexFactory" here instead?

I agree the term constructor is not ideal. The function is acting as both a Consumer (to receive the complex result) and optionally a Function (to convert (a +ib) to z). In both cases the operation is a terminating operation where the primitive (r, i) pair finally gets used and potentially converted to an object. So there is a lot to cover in a name. Switching to Factory implies that the function will always make something (as does constructor). However some use cases could return Void. So I think this also needs to be captured in the name. Perhaps ComplexResult?

What do you think of using the name pair "source"/"sink". I could then picture the following:

// interface for obtaining complex values public interface ComplexSource { double getReal(); double getImaginary(); default real() { return getReal(); } default imag() { return getImaginary(); } } // interface for accepting complex values, optionally returning a result @FunctionalInterface public interface ComplexSink<R> { R accept(double real, double imaginary); }

These interfaces could then be used as follows:

// existing Complex class public final class Complex implements ComplexSource { // ... } // TBD complex value container class public class ComplexBuffer implements ComplexSource, ComplexSink<ComplexBuffer> { // indexing methods similar to java.nio.Buffer public int position(); public ComplexBuffer position(int pos); // get the values at the current position public double getReal() { ... } public double getImaginary() { ... } // set the values at the current position ComplexBuffer accept(double real, double imaginary) { // ... return this; } }

aherbert

Hi @sumanth-rajkumar

Thanks for the new PR. There are many items to address. Please also see my comments from the previous PR as some of them are the same (you have copied code from that PR without addressing the previous feedback).

aherbert · 2022-07-16T07:27:12Z

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

@@ -2905,224 +2776,7 @@ private static Complex atanh(final double real, final double imaginary,
     * @return {@code x^2 + y^2 - 1}.
     */
    private static double x2y2m1(double x, double y) {


This method is an internal method used in atanh. It should not be refactored within this PR and should not be public in ComplexFunctions.

This method is also used in log which is why I moved it to ComplexFunctions. Should I just make it public in Complex and use it from there in ComplexFunctions or is there something else I can do?

I did not see that. Just make it package private and add a TODO note to make it private in the future.

aherbert · 2022-07-16T07:30:04Z

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

@@ -3297,7 +2951,7 @@ private static boolean equals(double x, double y) {
     * @return {@code true} if {@code d} is negative.
     */
    private static boolean negative(double d) {


Add a TODO note to this method to remove it in future refactoring. IIUC it is only used in one place if all the functions are moved to ComplexFunctions (the remaining call site is in the ofPolar constructor).

aherbert · 2022-07-16T07:41:43Z

...s-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexUnaryOperator.java

+package org.apache.commons.numbers.complex;
+
+/**
+ * Represents a complex operation that accepts a complex number of type ComplexDouble and produces a ComplexDouble result.


This javadoc requires improvement. ComplexDouble does not exist. For example:

* Represents a unary operation on a Cartesian form of a complex number \( a + ib \) * where \( a \) and \( b \) are real numbers represented as two {@code double} * parts. The operation creates a complex number result; the result is supplied * to a terminating consumer function which may return an object representation * of the complex result. * * <p>This is a functional interface whose functional method is * {@link #apply(double, double, ComplexConstructor)}. * * @param <R> The type of the complex result * @since 1.1

aherbert · 2022-07-16T07:41:55Z

...s-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexUnaryOperator.java

+     * @return the object created by the supplied constructor.
+     */
+    R apply(double r, double i, ComplexConstructor<R> out);
+


Remove trailing line

aherbert · 2022-07-16T07:42:44Z

...s-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexUnaryOperator.java

+
+    /**
+     * Represents an operator that accepts real and imaginary parts of a complex number and a complex constructor to produce and return the result.
+     * @param r real


Javadoc style for [numbers] is to avoid using The for all arguments and have a trailing period . The argument descriptions should be improved. Please try and wrap the javadoc to 100 characters. Look through the rest of Complex for reference.

... Real part \( a \) of the complex number \(a +ib \). ... Imaginary part \( b \) of the complex number \(a +ib \). ... Terminating consumer for the complex result, used to construct a result of type {@code R}.

aherbert · 2022-07-16T12:46:55Z

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

+     * @return The argument of this complex number.
+     * @see Math#atan2(double, double)
+     */
+    public static double arg(double r, double i) {


This should be called by Complex for consistency.

aherbert · 2022-07-16T12:48:05Z

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

+    /**
+     * Returns the
+     * <a href="http://mathworld.wolfram.com/NaturalLogarithm.html">
+     * natural logarithm</a> of this complex number using its real and imaginary parts.


References to this complex number should be updated to the complex number

aherbert · 2022-07-16T12:48:59Z

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

+     * @return The logarithm of this complex number.
+     */
+    private static <R> R log(DoubleUnaryOperator log, double logOfeOver2, double logOf2,
+                                     double real, double imaginary, ComplexConstructor<R> constructor) {


Fix the trailing argument indentation

aherbert · 2022-07-16T12:51:46Z

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

+        if (!equals(t1.getReal(), t2.getReal()) ||
+            !equals(t1.getImaginary(), t2.getImaginary())) {
+            Assertions.fail(
+                String.format("Operation failed (z=%s). Expected: %s but was: %s (Unspecified sign = %s)",


This formatting message includes c1 as an argument but the string format does not require it.

aherbert · 2022-07-16T12:54:22Z

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

+ */
+class ComplexNumber {
+
+    private final double real;


Comment these for completeness.

aherbert

This is much improved.

Can you rename the ComplexResult to ComplexSink and remove use of the word constructor and constructs from all of ComplexFunctions. The result argument should be named action.

I think all the assertion functions that accept two operations should in javadoc:

describe the assertion to the expected
state the two operations are exactly the same

I've suggested using an assertSame function in TestUtils to test the two functions are the same and return the Complex. This should simplify converting all of ComplexTest to test both implementations when the assertions on the result z are custom.

It may be useful for the other assertion functions in CReferenceTest etc do it this way too. This essentially offloads the check the two functions are exactly equal elsewhere. The assertions can then just test the result complex z satisfies the equals requirements (essentially just reusing the current test code).

aherbert · 2022-07-18T06:16:43Z

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

+ * @since 1.1
+ */
+@FunctionalInterface
+public interface ComplexResult<R> {


Rename to ComplexSink.

For consistency the use of a + ib should be in brackets: \( (a + i b) \)

There are a few place where the brackets are not used in Complex (these should be changed in future); most cases seem to have the enclosing brackets.

aherbert · 2022-07-18T06:31:18Z

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

+package org.apache.commons.numbers.complex;
+
+/**
+ * Represents a Terminating consumer for the complex result, used to construct a result of type {@code R}.


terminating in lower case. Since we are replacing the name result with sink the description should avoid the word result. The interface may be used for example as the operation to add numbers to an extendable list:

// list returns itself list.apply(x, y).apply(r, i).

I would keep the doc simple (which can be expanded later):

Represents a data sink for a complex number \( (a + i b) \); operations return a result of type {@code R}.

aherbert · 2022-07-18T06:34:25Z

...s-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexUnaryOperator.java

+public interface ComplexUnaryOperator<R> {
+
+    /**
+     * Represents an operator that accepts real and imaginary parts of a complex number and a complex constructor to produce and return the result.


and supplies the complex result to the provided consumer

aherbert · 2022-07-18T06:36:40Z

...s-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexUnaryOperator.java

+     * @param r Real part \( a \) of the complex number \(a +ib \).
+     * @param i Imaginary part \( b \) of the complex number \(a +ib \).
+     * @param out Terminating consumer for the complex result, used to construct a result of type {@code R}.
+     * @return the object created by the supplied constructor.


@param out Consumer for the complex result @return the object returned by the provided consumer

aherbert · 2022-07-18T06:42:48Z

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

+        final Complex z = operation1.apply(c);
+        final ComplexNumber y = operation2.apply(c.getReal(), c.getImaginary(), ComplexNumber::new);
+
+        assertEquals(() -> "UnaryOperator " + name + "(" + c + "): real", expected.real(), z.real(), maxUlps);


These messages on the object are written in OO form (as per the other messages in the class):

c + "." + name + "(): e.g. c = (3 + 4i), name = sin (3, 4).sin():

The procedural form should reflect that:

name + c: e.g. c = (3 + 4i), name = sin sin(3, 4):

aherbert · 2022-07-18T08:04:26Z

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

@@ -1419,6 +1419,9 @@ void testLog10() {
            final Complex z = Complex.ofCartesian(rng.nextDouble() * 2, rng.nextDouble() * 2);
            final Complex lnz = z.log();
            final Complex log10z = z.log10();
+            final ComplexNumber log10z2 = ComplexFunctions.log10(z.getReal(), z.getImaginary(), ComplexNumber::new);


There is no test for ComplexFunctions.log.

In this case it may be convenient to add a method to TestUtils to assert two operations are the same, then return the result. You can then do further assertions on the result.

static Complex assertSame(Complex c, UnaryOperator<Complex> operation1, ComplexUnaryOperator<ComplexNumber> operation2, String name) { final Complex z = operation1.apply(c); // Test operation2 produces the exact same result operation2.apply(c.real(), c.imag(), (x, y) -> { Assertions.assertEquals(z.real(), x, () -> name + " real"); Assertions.assertEquals(z.imag(), y, () -> name + " imaginary"); return null; }); return z; } Complex lnz = TestUtils.assertSame(z, Complex::log, ComplexFunctions::log, "log"); Complex ln10z = TestUtils.assertSame(z, Complex::log10, ComplexFunctions::log10, "log10"); // Test lnz ...

aherbert · 2022-07-18T08:16:53Z

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

+ * complex number \( a + ib \), \( a \) is called the <em>real part</em> and
+ * \( b \) is called the <em>imaginary part</em>.
+ *
+ * <p>This class is immutable, it is to change from an OO representation of a Complex number


The class is a utility class with only static methods so immutability is not relevant. I think just remove this paragraph.

Which paragraph do you mean? Do you mean to just leave the part of changing from OO representation and take out everything else in that paragraph?

Remove the paragraph starting within <p>This class is immutable, .... It is about 3 lines of the javadoc.

aherbert · 2022-07-18T08:23:12Z

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

+     *
+     * @param real Real part \( a \) of the complex number \(a +ib \).
+     * @param imaginary Imaginary part \( b \) of the complex number \(a +ib \).
+     * @param constructor Terminating consumer for the complex result, used to construct a result of type {@code R}.


Rename constructor to action. Move the javadoc description from the return tag to this tag. The return tag can be made more generic as you do not know what the object will represent.

@param action Consumer for the natural logarithm of the complex number @return the object returned by the supplied action ...

aherbert · 2022-07-18T08:28:56Z

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

@@ -2904,7 +2782,8 @@ private static Complex atanh(final double real, final double imaginary,
     * @param y the y value
     * @return {@code x^2 + y^2 - 1}.
     */
-    private static double x2y2m1(double x, double y) {
+    //TODO - make it private in future


Since the intent is to eventually move all these private functions to ComplexFunctions, then please start the process now. Make any function that is currently also required in Complex package-private and add the TODO note to make private in CompexFunctions in the future.

It may be convenient to leave the functions locally as pass through functions:

private static double x2y2m1(double x, double y) { return ComplexFunctions.x2y2m1(x, y); }

This will prevent having to update all the call sites in Complex and minimise the diff.

aherbert · 2022-07-18T08:31:36Z

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

+        // insensitive to the sign bit. Thus use of Math.abs(double) is only in edge-case
+        // branches.
+        //
+        // 5. The exponent different to ignore the smaller component has changed from 60 to 54.


Correct original typo to difference

aherbert

Hi @sumanth-rajkumar

I have suggested a few changes. Please contact me if you need more details.

aherbert · 2022-07-19T16:43:30Z

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

+                             String name) {
+        final Complex z = operation1.apply(c);
+        // Test operation2 produces the exact same result
+        operation2.apply(c.real(), c.imag(), (x, y) -> {


This should actually create a ComplexNumber since we wish to test the object created is returned by the function:

final ComplexNumber z2 = operation2.apply(c.real(), c.imag(), ComplexNumber::new); Assertions.assertEquals(z.real(), z2.real(), () -> "UnaryOperator " + name + " real"); Assertions.assertEquals(z.imag(), z2.imag(), () -> "UnaryOperator " + name + " imaginary");

aherbert · 2022-07-19T16:45:23Z

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

+     * complex real and imaginary parts.
+     *
+     * @param c The input complex number.
+     * @param operation1 the operation on the Complex object


Drop the the, use capitalisation and end with a period:

Operation on the Complex object.

Same with the other tags.

aherbert · 2022-07-19T16:46:30Z

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

@@ -169,13 +170,11 @@ static void assertComplex(Complex c,
        Complex expected, long maxUlps) {

        final Complex z = operation1.apply(c);


Replace with:

final Complex z = TestUtils.assertSame(c, operation1, operation2, name);

aherbert · 2022-07-19T16:47:47Z

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

@@ -18,22 +18,22 @@
 package org.apache.commons.numbers.complex;

 /**
- * Represents a Terminating consumer for the complex result, used to construct a result of type {@code R}.
- * The operation may return an object representation of the complex result.
+ * Represents a data sink for a complex number \( (a + i b) \)


Period at the end of a sentence.

aherbert · 2022-07-19T16:50:20Z

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

+     * @param real Real part \( a \) of the complex number \( (a +ib \).
+     * @param imaginary Imaginary part \( b \) of the complex number \( (a +ib \).
+     * @param action Consumer for the natural logarithm of the complex number.
+     * @param <R> the object taken by the supplied action.


Perhaps change to the return type of the supplied action

aherbert · 2022-07-19T17:12:30Z

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

+     *
+     * @param real Real part \( a \) of the complex number \( (a +ib \).
+     * @param imaginary Imaginary part \( b \) of the complex number \( (a +ib \).
+     *


aherbert · 2022-07-19T17:13:32Z

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

+
+    /**
+     * Constructor representing a complex number by its real and imaginary parts.
+     * Takes in real and imaginary and sets it to this complex number's real and imaginary


Second sentence is not required. I think it may be left over from when this was a method not a constructor.

aherbert · 2022-07-19T17:14:06Z

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

+     *
+     * @param real Real part \( a \) of the complex number \( (a +ib \).
+     * @param imaginary Imaginary part \( b \) of the complex number \( (a +ib \).
+     * @return {@code ComplexNumber} object.


@return conjugated complex number

aherbert · 2022-07-19T17:15:02Z

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

+     * @param expected the expected value
+     * @param actual the actual value
+     */
+    public static void assertSame(Complex expected, ComplexNumber actual) {


Do we need this method? If so then add a message to the assertions for "real" and "imaginary"

aherbert · 2022-07-19T17:15:22Z

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

+     * @return the resulting complex number from the given operation
+     */
+    static Complex assertSame(Complex c,
+                             UnaryOperator<Complex> operation1,


Indent 1 more space

aherbert · 2022-07-20T10:51:16Z

I have squashed all commits and merged to the feature branch.

Closed by ae7deb7

sumanth-rajkumar and others added 7 commits June 12, 2022 02:00

Merge pull request #1 from apache/master

8063da5

resync with apache master

Merge branch 'apache:master' into master

0244743

Merge branch 'apache:master' into master

fd92c25

Merge branch 'apache:master' into master

78222d9

Merge branch 'apache:master' into master

e6c7a17

Merge branch 'apache:master' into master

17ab2f4

NUMBERS-188 refactor complex log function using functional interfaces

9042811

NUMBERS-188 revert unintended change

b6ac36c

darkma773r reviewed Jul 16, 2022

View reviewed changes

aherbert requested changes Jul 16, 2022

View reviewed changes

NUMBERS-188 applied requested changes

b7374e4

aherbert requested changes Jul 18, 2022

View reviewed changes

NUMBERS-188 made javadoc changes

c9d5cb4

aherbert requested changes Jul 19, 2022

View reviewed changes

NUMBERS-188 javadoc edits

142fa7b

aherbert closed this Jul 20, 2022

		@@ -169,13 +170,11 @@ static void assertComplex(Complex c,
		Complex expected, long maxUlps) {

		final Complex z = operation1.apply(c);

Numbers 188.refactor complex log function #117

Numbers 188.refactor complex log function #117

Conversation

sumanth-rajkumar commented Jul 15, 2022

codecov-commenter commented Jul 15, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 20, 2022

codecov-commenter commented Jul 15, 2022 •

edited