NUMBERS-186 added additional complex list operations #123

sumanth-rajkumar · 2022-09-06T22:55:53Z

This PR introduces additional operations that can be done on ComplexList.

Summary of changes

ComplexListTest - unit tests for complex additional operations on ComplexList
ComplexConsumer - interface that acts as a consumer for the complex element's double real and imaginary parts

codecov-commenter · 2022-09-06T22:58:41Z

Codecov Report

Merging #123 (58e89a4) into complex-gsoc-2022 (2bd5755) will not change coverage.
The diff coverage is n/a.

@@                 Coverage Diff                  @@
##             complex-gsoc-2022     #123   +/-   ##
====================================================
  Coverage                99.13%   99.13%           
  Complexity                1699     1699           
====================================================
  Files                       65       65           
  Lines                     4274     4274           
  Branches                   836      836           
====================================================
  Hits                      4237     4237           
  Misses                      10       10           
  Partials                    27       27

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

aherbert

The forEach has been copied from replaceAll without removing the concurrent modification check or updating the javadoc. The test is also incorrectly named.

For your javadoc can you make sure you do not use the term element; here we should use complex number.

The tests should use data where each item in the list is unique. Index out of bound exceptions should be tested, especially at the upper size limit as without a range check the get methods can return stale/invalid data and the set methods may silently not error and write to an unused part of the backing array.

aherbert · 2022-09-07T10:02:55Z

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

+     * @param real Real part \( a \) of the complex number \( (a + ib) \).
+     * @param imaginary Imaginary part \( b \) of the complex number \( (a + ib) \).
+     */
+    void apply(double real, double imaginary);


java.util.function.Consumer has the method accept

aherbert · 2022-09-07T10:05:01Z

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

+     */
+    public double getReal(int index) {
+        rangeCheck(index);
+        final int i = index << 1;


No need for this variable, just perform the index doubling inline

aherbert · 2022-09-07T10:05:55Z

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

+    /**
+     * Gets the real part \( a \) of the complex number \( (a + i b) \) at the indexed position of the list.
+     *
+     * @param index Index of the element's real part to get.


Index of the complex number.

aherbert · 2022-09-07T10:08:03Z

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

@@ -177,6 +203,63 @@ public Complex set(int index, Complex element) {
        return oldValue;
    }

+    /**
+     * Replaces the complex element's real part at the specified position


change element to number. Element is for a generic list; since this is typed to Complex then all element occurrences should be changed to complex number

aherbert · 2022-09-07T10:09:36Z

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

+    /**
+     * Replaces the complex element's imaginary part at the specified position
+     * in this list with the specified imaginary element.
+     * @param index Index of the element's imaginary part to replace.


Add an empty line

aherbert · 2022-09-07T10:16:54Z

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

+            Assertions.assertEquals(list.get(i).getReal(), list.getReal(i));
+        }
+        for (int i = 0; i < list.size(); i++) {
+            Assertions.assertEquals(list.get(i).getImaginary(), list.getImaginary(i));


This can be placed in the same loop as above. Add a simple message to the assertion, e.g. "real".

aherbert · 2022-09-07T10:20:33Z

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

+        list.addAll(list);
+        list.setReal(2, 200);
+        list.setImaginary(2, 1);
+        Assertions.assertEquals(Complex.ofCartesian(200, 1), list.get(2));


Test set at all positions:

for (int i = 0; i < list.size(); i++) { final double value = Math.PI * i; list.setReal(i, value); Assertions.assertEquals(value, list.get(i).getReal()); }

Also test set/get with invalid index positions (-1, size, size+1).

aherbert · 2022-09-07T10:21:13Z

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

+    @Test
+    void testToArrayRealAndImaginary() {
+        ComplexList list = new ComplexList();
+        list.add(Complex.ofCartesian(42, 13));


Create a better random list.

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

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

+        list.addAll(list);
+        list.set(2, Complex.ofCartesian(200, 1));
+
+        double[] expectedReal = {42, 42, 200, 42};


double[] expected = list.stream().mapToDouble(Complex::getReal).toArray();

aherbert · 2022-09-07T10:23:11Z

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

+
+        double[] expectedReal = {42, 42, 200, 42};
+        double[] actualReal = list.toArrayReal();
+        for (int i = 0; i < expectedReal.length; i++) {


Use Assertions.assertArrayEquals

aherbert

This looks OK. Just a few minor changes to address. Thanks

aherbert · 2022-09-07T22:49:48Z

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

+     */
+    public void setImaginary(int index, double imaginary) {
+        rangeCheck(index);
+        final int i = index << 1;


This index i should be inlined.

aherbert · 2022-09-07T22:49:52Z

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

+     */
+    public void setReal(int index, double real) {
+        rangeCheck(index);
+        final int i = index << 1;


This index i should be inlined.

aherbert · 2022-09-07T22:51:21Z

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

@@ -152,9 +153,9 @@ void testAddIndexOutOfBoundExceptions() {

    @Test
    void testRemoveIndexOutOfBoundExceptions() {
+        List<Complex> objectList = generateList(2);


Since a lot of the tests now use generateList() then addAll to a new ComplexList instance this should be refactored into a method: private static ComplexList generateComplexList(int)

aherbert

I think you have removed too many uses of i = index << 1. There are some where this is useful.

Also the test class should have generateList and generateComplexList. The later to be used only when the list will not be compared to another list using assertEquals, e.g. for tests for accessor methods.

aherbert · 2022-09-08T10:26:51Z

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

     */
-    private static List<Complex> generateList(int size) {
-        return ThreadLocalRandom.current().doubles(size, -Math.PI, Math.PI)
+    private static ComplexList generateList(int size) {


There are use cases for generateList and generateComplexList. The generateList would function as before and return a List implementation from the JDK. This can be assumed to work correctly. The generateComplexList would call generateList and then addAll to a new ComplexList instance. This should be asserted to be equal to the original List. This method can be used for convenience to populate a ComplexList for testing the accessor methods (get/set).

One case where you may wish to have a List that is not a ComplexList is in testing addAll. You also require a non ComplexList for any list compared for equality with ComplexList: you cannot have a list equals comparison to itself as there may be the same error in both implementations being compared.

aherbert · 2022-09-08T10:27:41Z

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

     * @return the complex number.
     */
    @Override
    public Complex get(int index) {
        rangeCheck(index);
-        final int i = index << 1;


The precomputed value i will be reused so it can be left as before, please revert.

aherbert · 2022-09-08T10:29:13Z

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

        rangeCheck(index);
-        final int i = index << 1;


The precomputed value i will be reused so it can be left as before, please revert.

In addition the call to setNoRangeCheck can be removed. Since you already have the value i this will avoid calling it again.

NUMBERS-186 added additional complex list operations

ed548d6

aherbert requested changes Sep 7, 2022

View reviewed changes

NUMBERS-186 added more test and updated javadocs

e67d4f0

aherbert requested changes Sep 7, 2022

View reviewed changes

sumanth-rajkumar added 2 commits September 7, 2022 20:28

NUMBERS-186 minor code structure changes

58e89a4

NUMBERS-186 consistent use of index << 1

4347c4c

aherbert requested changes Sep 8, 2022

View reviewed changes

NUMBERS-186 updated test logic

022147f

aherbert merged commit ddd2c06 into apache:complex-gsoc-2022 Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NUMBERS-186 added additional complex list operations #123

NUMBERS-186 added additional complex list operations #123

sumanth-rajkumar commented Sep 6, 2022

codecov-commenter commented Sep 6, 2022 •

edited

aherbert left a comment

aherbert Sep 7, 2022

aherbert Sep 7, 2022

aherbert Sep 7, 2022

aherbert Sep 7, 2022

aherbert Sep 7, 2022

aherbert Sep 7, 2022

aherbert Sep 7, 2022

aherbert Sep 7, 2022

aherbert Sep 7, 2022

aherbert Sep 7, 2022

aherbert left a comment

aherbert Sep 7, 2022

aherbert Sep 7, 2022

aherbert Sep 7, 2022

aherbert left a comment

aherbert Sep 8, 2022

aherbert Sep 8, 2022

aherbert Sep 8, 2022

NUMBERS-186 added additional complex list operations #123

NUMBERS-186 added additional complex list operations #123

Conversation

sumanth-rajkumar commented Sep 6, 2022

codecov-commenter commented Sep 6, 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 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

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

codecov-commenter commented Sep 6, 2022 •

edited