NUMBERS-186: Lists of Complex Numbers #121

sumanth-rajkumar · 2022-08-29T06:03:12Z

This PR introduces a new complex arrays module that introduces ComplexList where the complex numbers in the complex module are stored using an interleaved format. This allow operations to be performed on lists of complex numbers.

Summary of changes

ComplexList - representation of multiple complex numbers into a list structure storing real and imaginary parts.
ComplexListTest - unit tests that tests all conditions of adding, removing, getting, and setting the list.

aherbert

Thanks for the PR. I have added some comments from an initial review. Please ask questions if anything is unclear.

aherbert · 2022-08-29T20:24:46Z

...ers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/package-info.java

+ * limitations under the License.
+ */
+/**
+ * Complex numbers.


Provides a list of complex numbers.

aherbert · 2022-08-29T20:25:35Z

commons-numbers-complex-arrays/src/site/site.xml

+      <item name="Overview" href="index.html"/>
+      <item name="Latest API docs (development)"
+            href="apidocs/index.html"/>
+      <item name="Javadoc (1.0 release)"


These are not required as this package has not been released. Only the latest API docs item is required.

aherbert · 2022-08-29T20:26:44Z

commons-numbers-complex-arrays/src/site/xdoc/index.xml

+
+  <body>
+
+    <section name="Apache Commons Numbers: Number Types and Utilities" href="summary">


This section name does not seem relevant

aherbert · 2022-08-29T20:31:05Z

...-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java

+            return list.add(Complex.ofCartesian(11, 12));
+        });
+
+        assertListOperation(list -> {


This repeats the above test but changes the final add. The assert method with the consumer does not appear to be relevant and can be dropped and replaced with the assert method taking the function.

I think here would be better tested using smaller functions:

assertListOperation(list -> list.add(Complex.ofCartesian(42, 13))); assertListOperation(list -> list.add(1, Complex.ofCartesian(11, 12))); // etc

aherbert · 2022-08-29T20:32:12Z

...-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java

+            list.add(1, Complex.ofCartesian(11, 12));
+        });
+
+        ComplexList list = new ComplexList();


Is the intension to add an empty list to an empty list? A comment would be useful.

aherbert · 2022-08-29T21:29:58Z

...bers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java

+        final int numNew = c.size();
+        expand(numNew * 2);
+        double[] realAndImgData = new double[c.size() * 2];
+        int i = 0;


I would just increment this in the loop:

for (final Complex val : c) { realAndImgData[i++] = val.getReal(); realAndImgData[i++] = val.getImaginary(); }

aherbert · 2022-08-29T21:31:48Z

...bers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java

+        rangeCheckForInsert(index);
+        final int numNew = c.size();
+        final int numNew2 = numNew << 1;
+        expand(numNew * 2);


The expand method is expecting a number of elements, not a number of doubles. So this should not be doubled.

aherbert · 2022-08-29T21:32:38Z

...bers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java

+        final int numNew = c.size();
+        final int numNew2 = numNew << 1;
+        expand(numNew * 2);
+        final double[] realAndImgData = new double[c.size() * 2];


Reuse the numNew local variable instead of calling size again. The size method may be dynamically computed by the collection.

aherbert · 2022-08-29T21:33:23Z

...bers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java

+            System.arraycopy(realAndImagParts, i + 2, realAndImagParts, i, numMoved);
+        }
+        size--;
+        realAndImagParts[s] = 0;


This has no purpose as this part of the array should never be accessed as it is out-of-range.

aherbert · 2022-08-29T21:34:03Z

...bers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java

+    private String outOfBoundsMsg(int index) {
+        return INDEX_MSG + index + SIZE_MSG + size;
+    }
+


Remove extra line.

aherbert

Hi @sumanth-rajkumar

This just needs a few small changes to be ready. Thanks.

aherbert · 2022-08-31T16:25:15Z

commons-numbers-complex-arrays/src/site/xdoc/index.xml

@@ -24,17 +24,9 @@
  </properties>

  <body>
-
-    <section name="Apache Commons Numbers: Number Types and Utilities" href="summary">


I think you still need this section.

You can render the site documentation using:

mvn site:site -DgenerateReports=false open target/site/index.html

Looking at the documentation for other modules this seems to be standard boiler plate used in all modules. So you should reinstate it. Sorry for the error.

aherbert · 2022-08-31T16:28:48Z

...-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java

+        assertListOperation(list -> list.add(Complex.ofCartesian(21, 22)), l1, l2);
+        assertListOperation(list -> list.add(Complex.ofCartesian(23, 24)), l1, l2);
+
+        List<Complex> l3 = new ArrayList<>();


It is not clear what is achieved in the l3, l4 test that has not already been done above.

aherbert · 2022-08-31T16:37:36Z

...-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java

+        List<Complex> l5 = new ArrayList<>();
+        List<Complex> l6 = new ComplexList();
+        assertListOperation(list -> list.add(Complex.ofCartesian(1, 2)), l5, l6);
+        assertListOperation(list -> {


Is this not better served by:

assertListOperation(list -> list.add(Complex.ofCartesian(1, 2)), l5, l6); // Expand the list by doubling in size until at the known minArrayCapacity while (l5.size() < 8) { assertListOperation(list -> list.addAll(list), l5, l6); } assertListOperation(list -> list.addAll(list1), l5, l6);

aherbert · 2022-08-31T17:41:39Z

commons-numbers-complex-arrays/pom.xml

+            <groupId>org.apache.commons</groupId>
+            <artifactId>commons-numbers-complex</artifactId>
+        </dependency>
+        <dependency>


We do not need these test dependencies at the moment

aherbert · 2022-08-31T17:42:20Z

...bers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java

+    /**
+     * The double array buffer into which the elements of the ComplexList are stored.
+     */
+    protected double[] realAndImagParts;


No need to be protected.

aherbert · 2022-08-31T17:47:54Z

...bers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java

+            System.arraycopy(realAndImagParts, i + 2, realAndImagParts, i, numMoved);
+        }
+        size--;
+


Remove extra line

aherbert · 2022-08-31T17:49:58Z

...bers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java

+    /**
+     * {@inheritDoc}
+     *
+     * @param index {@inheritDoc}


I don't think you need all the inheritDoc here.

In fact you do not require it for any param where you are using the inheritDoc tag. If you delete the param tag totally IIUC it should just pull in the parent javadoc for that parameter. So you should be able to delete a lot of param tags that simple use inheritDoc.

You can test with:

mvn javadoc:javadoc

And then look at the output javadoc in the target directory. If this is OK then you can delete all the plain inheritDoc param tags. Note the inheritDoc will pull in the parent documentation. Then typically you add more information to it. If you are not adding information then do not bother.

aherbert · 2022-08-31T17:59:40Z

...bers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java

+     * @throws IndexOutOfBoundsException if index isn't within the range.
+     */
+    private void rangeCheck(int index) {
+        if (index < 0 || index >= size) {


Note that ArrayList does not perform index < 0 here. This check is for access. If the index is negative then the IOOBE will be generated with the index value.

This is different from rangeCheckForInsert as there is copy operations that must be performed before the index is written to. If the index is negative then these operations may fail with unwanted runtime exception messages.

aherbert · 2022-08-31T18:00:25Z

commons-numbers-complex-arrays/src/site/resources/profile.japicmp

+# limitations under the License.
+# -----------------------------------------------------------------------------
+#
+# Empty file used to automatically trigger profile from commons parent pom


trigger JApiCmp profile

aherbert · 2022-08-31T18:01:55Z

...-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java

+        assertListOperation(operation, new ArrayList<>(), new ComplexList());
+    }
+
+    private static class SizedList extends ArrayList<Complex> {


This should be commented so its purpose is clear. It deliberately reports a fixed size (and is therefore a non-functional list) and is used to trigger capacity exceptions when adding a collection to the ComplexList.

NUMBERS-186 add complex arrays module

5a0da3b

aherbert requested changes Aug 29, 2022

View reviewed changes

NUMBERS-186 updated test class and logics in ComplexList

630d2f0

aherbert requested changes Aug 31, 2022

View reviewed changes

NUMBERS-186 more updates in test class

836f877

aherbert merged commit 2bd5755 into apache:complex-gsoc-2022 Sep 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NUMBERS-186: Lists of Complex Numbers #121

NUMBERS-186: Lists of Complex Numbers #121

sumanth-rajkumar commented Aug 29, 2022 •

edited

aherbert left a comment

aherbert Aug 29, 2022

aherbert Aug 29, 2022

aherbert Aug 29, 2022

aherbert Aug 29, 2022

aherbert Aug 29, 2022

aherbert Aug 29, 2022

aherbert Aug 29, 2022

aherbert Aug 29, 2022

aherbert Aug 29, 2022

aherbert Aug 29, 2022

aherbert left a comment

aherbert Aug 31, 2022

aherbert Aug 31, 2022

aherbert Aug 31, 2022

aherbert Aug 31, 2022

aherbert Aug 31, 2022

aherbert Aug 31, 2022

aherbert Aug 31, 2022

aherbert Aug 31, 2022

aherbert Aug 31, 2022

aherbert Aug 31, 2022


		<body>

		<section name="Apache Commons Numbers: Number Types and Utilities" href="summary">

NUMBERS-186: Lists of Complex Numbers #121

NUMBERS-186: Lists of Complex Numbers #121

Conversation

sumanth-rajkumar commented Aug 29, 2022 • edited

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 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

sumanth-rajkumar commented Aug 29, 2022 •

edited