You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/09/08 17:25:24 UTC
[GitHub] [commons-numbers] sumanth-rajkumar opened a new pull request, #124: Numbers 186.complex list interleaved
sumanth-rajkumar opened a new pull request, #124:
URL: https://github.com/apache/commons-numbers/pull/124
This PR refactors ComplexList to be abstract and gives a interleaved implementation as a private sub-class. This allows for multiple implementations of data storage.
Summary of changes:
ComplexList - made abstract with factory methods implemented in interleaved implementation.
ComplexInterleavedList - private sub-class with interleaved double array storage.
ComplexListTest - updates to calling ComplexList using ComplexInterleavedList.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert commented on a diff in pull request #124: Numbers 186.complex list interleaved
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #124:
URL: https://github.com/apache/commons-numbers/pull/124#discussion_r966333894
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -28,23 +28,9 @@
import java.util.Objects;
/**
- * Resizable-double array implementation of the List interface. Implements all optional list operations,
- * and permits all complex numbers. In addition to implementing the List interface,
- * this class provides methods to manipulate the size of the array that is used internally to store the list.
- *
- * <p>Each ComplexList instance has a capacity. The capacity is half the size of the double array used to store the complex numbers
- * in the list. As complex numbers are added to an ComplexList, its capacity grows automatically.
- * The complex number is stored using an interleaved format and so the maximum number of complex numbers that may be added is
- * approximately 2<sup>30</sup>. This is half the maximum capacity of java.util.ArrayList.
- * The memory usage is more efficient than using a List of Complex objects as the underlying numbers are not stored
- * using instances of Complex.</p>
- *
- * <p>An application can increase the capacity of an ComplexList instance before adding a large number of complex numbers
- * using the ensureCapacity operation. This may reduce the amount of incremental reallocation.</p>
- *
- * <p>This list does not support {@code null} Complex objects.
+ * This is an abstract class that implements all the common data and operations for a ComplexList.
*/
-public class ComplexList extends AbstractList<Complex> {
+public abstract class ComplexList extends AbstractList<Complex> {
Review Comment:
Prevent extension of this class outside the inner classes by adding a private constructor. It may have to be package-private to pass the build checks (e.g. PMD).
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -395,46 +229,322 @@ private String outOfBoundsMsg(int index) {
}
/**
- * Replaces each complex number of the list with the result of applying the operator to that complex number.
+ * Constructs an empty interleaved list which can store up to the specified capacity without a memory reallocation.
*
- * @param operator The operator to apply to each complex number.
- * @throws NullPointerException if the specified operator is null.
+ * @param capacity Capacity of interleaved list.
+ * @return ComplexList object.
+ * @throws IllegalArgumentException if the {@code capacity} is greater than {@code MAX_CAPACITY}.
*/
- public void replaceAll(ComplexUnaryOperator<Void> operator) {
- Objects.requireNonNull(operator);
- final double[] parts = this.realAndImagParts;
- final int m = size;
- final int expectedModCount = modCount;
- for (int i = 0; i < m; i++) {
- final int index = i << 1;
- operator.apply(parts[index], parts[index + 1], (x, y) -> {
- parts[index] = x;
- parts[index + 1] = y;
- return null;
- });
- }
- // check for comodification
- if (modCount != expectedModCount) {
- throw new ConcurrentModificationException();
- }
- modCount++;
+ public static ComplexList interleaved(int capacity) {
+ return new ComplexInterleavedList(capacity);
}
/**
- * Performs the given action for each complex number of the list until all complex numbers
- * have been processed or the action throws an exception. Unless otherwise specified by the
- * implementing class, actions are performed in the order of iteration.
+ * Constructs an empty interleaved list.
*
- * @param action The action to apply to each complex number.
- * @throws NullPointerException if the specified action is null.
+ * @return ComplexList object.
+ */
+ public static ComplexList interleaved() {
+ return new ComplexInterleavedList();
+ }
+
+ /**
+ * Constructs an interleaved list using the specified double array.
+ *
+ * @param data Specified backing double array.
+ * @return ComplexList object.
+ * @throws IllegalArgumentException if the specified double array doesn't have an even amount of doubles.
*/
- public void forEach(ComplexConsumer action) {
- Objects.requireNonNull(action);
- final double[] parts = this.realAndImagParts;
- final int m = size;
- for (int i = 0; i < m; i++) {
- final int index = i << 1;
- action.accept(parts[index], parts[index + 1]);
+ public static ComplexList from(double[] data) {
+ return new ComplexInterleavedList(data);
+ }
+
+ /**
+ * Resizable-double array implementation of the List interface. Implements all optional list operations,
+ * and permits all complex numbers. In addition to implementing the List interface,
+ * this class provides methods to manipulate the size of the array that is used internally to store the list.
+ *
+ * <p>Each ComplexInterleavedList instance has a capacity. The capacity is half the size of the double array used to store the complex numbers
+ * in the list. As complex numbers are added to an ComplexInterleavedList, its capacity grows automatically.
+ * The complex number is stored using an interleaved format and so the maximum number of complex numbers that may be added is
+ * approximately 2<sup>30</sup>. This is half the maximum capacity of java.util.ArrayList.
+ * The memory usage is more efficient than using a List of Complex objects as the underlying numbers are not stored
+ * using instances of Complex.</p>
+ *
+ * <p>An application can increase the capacity of an ComplexInterleavedList instance before adding a large number of complex numbers
+ * using the ensureCapacity operation. This may reduce the amount of incremental reallocation.</p>
+ *
+ * <p>This list does not support {@code null} Complex objects.
+ */
+ private static class ComplexInterleavedList extends ComplexList {
+ /**
+ * Storage for the complex numbers.
+ * Data is stored in an interleaved format using (real, imaginary) pairs.
+ */
+ private double[] realAndImagParts;
+
+ /**
+ * Constructs an empty list which can store up to the specified capacity without a memory reallocation.
+ *
+ * @param capacity Capacity of list.
+ * @throws IllegalArgumentException if the {@code capacity} is greater than {@code MAX_CAPACITY}.
+ */
+ ComplexInterleavedList(int capacity) {
+ if (capacity > MAX_CAPACITY) {
+ throw new IllegalArgumentException(String.format(OOM_ERROR_STRING, capacity));
+ }
+ final int arrayLength = Math.max(DEFAULT_CAPACITY, capacity) * 2;
+ realAndImagParts = new double[arrayLength];
+ }
+
+ /**
+ * Constructs an empty list.
+ */
+ ComplexInterleavedList() {
+ realAndImagParts = new double[DEFAULT_CAPACITY * 2];
+ }
+
+ /**
+ * Constructs an interleaved list using the specified double array.
+ *
+ * @param fromArray Specified backing double array.
+ * @throws IllegalArgumentException if the specified double array doesn't have an even amount of doubles.
+ */
+ ComplexInterleavedList(double[] fromArray) {
+ if (fromArray.length % 2 != 0) {
Review Comment:
if ((fromArray.length & 1) != 0)
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -105,13 +116,31 @@ void testAddAndAddAll() {
assertListOperation(list -> list.addAll(list1), l5, l6);
//Test for adding an empty list to an empty list
- ComplexList list = new ComplexList();
+ ComplexList list = ComplexList.interleaved();
assertListOperation(l -> {
l.addAll(list);
return l.addAll(0, list);
});
}
+ @Test
+ void testSetAddAndAddAllNullPointerException() {
+ ComplexList list1 = generateComplexList(3);
+ Assertions.assertThrows(NullPointerException.class, () -> list1.add(null));
+ Assertions.assertThrows(NullPointerException.class, () -> list1.add(0, null));
+ Assertions.assertThrows(NullPointerException.class, () -> list1.set(1, null));
+
+ List<Complex> list2 = generateList(3);
+ for (int i = 0; i < list2.size(); i++) {
+ Complex val = list2.get(i);
+ if (val != null) {
+ list2.set(i, null);
+ }
+ }
+ Assertions.assertThrows(NullPointerException.class, () -> list1.addAll(list2));
Review Comment:
Check that list1 has the same size after the NPE is thrown, i.e. it is unchanged.
You could create a copy of the list for reference and use assertEquals to compare the before and after.
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -28,23 +28,9 @@
import java.util.Objects;
/**
- * Resizable-double array implementation of the List interface. Implements all optional list operations,
Review Comment:
Some of this javadoc still applies. The list provides efficient storage of complex numbers. It does not accept null complex objects.
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -105,13 +116,31 @@ void testAddAndAddAll() {
assertListOperation(list -> list.addAll(list1), l5, l6);
//Test for adding an empty list to an empty list
- ComplexList list = new ComplexList();
+ ComplexList list = ComplexList.interleaved();
assertListOperation(l -> {
l.addAll(list);
return l.addAll(0, list);
});
}
+ @Test
+ void testSetAddAndAddAllNullPointerException() {
+ ComplexList list1 = generateComplexList(3);
+ Assertions.assertThrows(NullPointerException.class, () -> list1.add(null));
+ Assertions.assertThrows(NullPointerException.class, () -> list1.add(0, null));
+ Assertions.assertThrows(NullPointerException.class, () -> list1.set(1, null));
+
+ List<Complex> list2 = generateList(3);
+ for (int i = 0; i < list2.size(); i++) {
Review Comment:
This loop is setting all elements to null. Just set one of them:
`list2.set(1, null)`
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -37,6 +37,17 @@ public class ComplexListTest {
private static final int MAX_CAPACITY = (Integer.MAX_VALUE - 9) / 2;
+ @Test
+ void testFromArray() {
+ double[] fromArray1 = ThreadLocalRandom.current().doubles(6, -Math.PI, Math.PI).toArray();
Review Comment:
```
int size = 3;
... doubles(size * 2, ...
for (... ; i < size; ...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] codecov-commenter commented on pull request #124: Numbers 186.complex list interleaved
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #124:
URL: https://github.com/apache/commons-numbers/pull/124#issuecomment-1241347552
# [Codecov](https://codecov.io/gh/apache/commons-numbers/pull/124?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#124](https://codecov.io/gh/apache/commons-numbers/pull/124?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d1cc044) into [complex-gsoc-2022](https://codecov.io/gh/apache/commons-numbers/commit/d791b98bfce690e4f01de17297b5ab38faf0f512?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d791b98) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## complex-gsoc-2022 #124 +/- ##
====================================================
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
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert merged pull request #124: Numbers 186.complex list interleaved
Posted by GitBox <gi...@apache.org>.
aherbert merged PR #124:
URL: https://github.com/apache/commons-numbers/pull/124
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert commented on a diff in pull request #124: Numbers 186.complex list interleaved
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #124:
URL: https://github.com/apache/commons-numbers/pull/124#discussion_r966886729
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -39,9 +39,10 @@ public class ComplexListTest {
@Test
void testFromArray() {
- double[] fromArray1 = ThreadLocalRandom.current().doubles(6, -Math.PI, Math.PI).toArray();
+ int size = 3;
+ double[] fromArray1 = ThreadLocalRandom.current().doubles(size * 2, -Math.PI, Math.PI).toArray();
ComplexList list = ComplexList.from(fromArray1);
- for (int i = 0; i < fromArray1.length >> 1; i++) {
+ for (int i = 0; i < size >> 1; i++) {
Review Comment:
This should iterate over `size`, not `size >> 1`
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -130,14 +131,12 @@ void testSetAddAndAddAllNullPointerException() {
Assertions.assertThrows(NullPointerException.class, () -> list1.add(0, null));
Assertions.assertThrows(NullPointerException.class, () -> list1.set(1, null));
+ ComplexList copy = ComplexList.interleaved();
+ copy.addAll(list1);
List<Complex> list2 = generateList(3);
- for (int i = 0; i < list2.size(); i++) {
- Complex val = list2.get(i);
- if (val != null) {
- list2.set(i, null);
- }
- }
+ list2.set(1, null);
Assertions.assertThrows(NullPointerException.class, () -> list1.addAll(list2));
+ Assertions.assertEquals(copy.size(), list1.size());
Review Comment:
You should also check `addAll(int, Collection)`. The implementation, if done incorrectly, could move all elements to make space, then copy the new elements in and throw a NPE. This will leave the list in a broken state.
It may be better to check the contents of the list are the same using `Assertions.assertEquals(copy, list1);`
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -51,6 +56,13 @@ public abstract class ComplexList extends AbstractList<Complex> {
/** Size of complex numbers in the list. */
protected int size;
+ /**
+ * Constructor to prevent extension of this class outside inner clases.
+ */
+ private ComplexList() {
+
Review Comment:
Comment empty line: `// Intentionally empty`
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -311,7 +324,7 @@ private static class ComplexInterleavedList extends ComplexList {
* @throws IllegalArgumentException if the specified double array doesn't have an even amount of doubles.
*/
ComplexInterleavedList(double[] fromArray) {
- if (fromArray.length % 2 != 0) {
+ if ((fromArray.length & 1) != 0) {
throw new IllegalArgumentException("Length of array has to be even");
}
realAndImagParts = fromArray;
Review Comment:
Here the data is not defensively cloned. This is fine but the behaviour should be documented. Any public method should indicate that the argument array is used in-place; any external modifications to the array will be reflected in this List until a structural modification is made to the data storage, for example a resize.
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -130,14 +131,12 @@ void testSetAddAndAddAllNullPointerException() {
Assertions.assertThrows(NullPointerException.class, () -> list1.add(0, null));
Assertions.assertThrows(NullPointerException.class, () -> list1.set(1, null));
+ ComplexList copy = ComplexList.interleaved();
Review Comment:
Create this copy at the start of the method. When used at the end of the method this will also check the add methods do not result in modifications of the list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert commented on a diff in pull request #124: Numbers 186.complex list interleaved
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #124:
URL: https://github.com/apache/commons-numbers/pull/124#discussion_r967196235
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -126,18 +126,18 @@ void testAddAndAddAll() {
@Test
void testSetAddAndAddAllNullPointerException() {
+ ComplexList copy = ComplexList.interleaved();
ComplexList list1 = generateComplexList(3);
Assertions.assertThrows(NullPointerException.class, () -> list1.add(null));
Assertions.assertThrows(NullPointerException.class, () -> list1.add(0, null));
Assertions.assertThrows(NullPointerException.class, () -> list1.set(1, null));
- ComplexList copy = ComplexList.interleaved();
- copy.addAll(list1);
List<Complex> list2 = generateList(3);
list2.set(1, null);
Assertions.assertThrows(NullPointerException.class, () -> list1.addAll(list2));
- Assertions.assertEquals(copy.size(), list1.size());
Assertions.assertThrows(NullPointerException.class, () -> list1.addAll(0, list2));
+ copy.addAll(list1);
Review Comment:
copy.addAll(list1) should be **before** all the operations on list1. Otherwise you are not comparing before (the copy) and after (list1).
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -60,7 +60,7 @@ public abstract class ComplexList extends AbstractList<Complex> {
* Constructor to prevent extension of this class outside inner clases.
*/
private ComplexList() {
-
+ //Intentional empty
Review Comment:
`// Intentionally empty`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org