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/06 22:55:55 UTC
[GitHub] [commons-numbers] sumanth-rajkumar opened a new pull request, #123: NUMBERS-186 added additional complex list operations
sumanth-rajkumar opened a new pull request, #123:
URL: https://github.com/apache/commons-numbers/pull/123
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
--
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 #123: NUMBERS-186 added additional complex list operations
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #123:
URL: https://github.com/apache/commons-numbers/pull/123#discussion_r965359957
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -166,20 +190,76 @@ private void setNoRangeCheck(int index, double real, double imaginary) {
/**
* {@inheritDoc}
*
- * @param element Complex element to be set.
+ * @param number Complex number to be set.
*/
@Override
- public Complex set(int index, Complex element) {
+ public Complex set(int index, Complex number) {
rangeCheck(index);
final int i = index << 1;
final Complex oldValue = Complex.ofCartesian(realAndImagParts[i], realAndImagParts[i + 1]);
- setNoRangeCheck(index, element.getReal(), element.getImaginary());
+ setNoRangeCheck(index, number.getReal(), number.getImaginary());
return oldValue;
}
+ /**
+ * Replaces the complex number's real part at the specified position
+ * in the list with the specified real number.
+ *
+ * @param index Index of the complex number.
+ * @param real Real part \( a \) of the complex number \( (a +ib) \).
+ */
+ public void setReal(int index, double real) {
+ rangeCheck(index);
+ final int i = index << 1;
+ realAndImagParts[i] = real;
+ }
+
+ /**
+ * Replaces the complex number's imaginary part at the specified position
+ * in the list with the specified imaginary number.
+ *
+ * @param index Index of the complex number.
+ * @param imaginary Imaginary part \( b \) of the complex number \( (a +ib) \).
+ */
+ public void setImaginary(int index, double imaginary) {
+ rangeCheck(index);
+ final int i = index << 1;
Review Comment:
This index i should be inlined.
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -166,20 +190,76 @@ private void setNoRangeCheck(int index, double real, double imaginary) {
/**
* {@inheritDoc}
*
- * @param element Complex element to be set.
+ * @param number Complex number to be set.
*/
@Override
- public Complex set(int index, Complex element) {
+ public Complex set(int index, Complex number) {
rangeCheck(index);
final int i = index << 1;
final Complex oldValue = Complex.ofCartesian(realAndImagParts[i], realAndImagParts[i + 1]);
- setNoRangeCheck(index, element.getReal(), element.getImaginary());
+ setNoRangeCheck(index, number.getReal(), number.getImaginary());
return oldValue;
}
+ /**
+ * Replaces the complex number's real part at the specified position
+ * in the list with the specified real number.
+ *
+ * @param index Index of the complex number.
+ * @param real Real part \( a \) of the complex number \( (a +ib) \).
+ */
+ public void setReal(int index, double real) {
+ rangeCheck(index);
+ final int i = index << 1;
Review Comment:
This index i should be inlined.
##########
commons-numbers-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);
Review Comment:
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)`
--
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 #123: NUMBERS-186 added additional complex list operations
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #123:
URL: https://github.com/apache/commons-numbers/pull/123#issuecomment-1238731878
# [Codecov](https://codecov.io/gh/apache/commons-numbers/pull/123?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 [#123](https://codecov.io/gh/apache/commons-numbers/pull/123?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ed548d6) into [complex-gsoc-2022](https://codecov.io/gh/apache/commons-numbers/commit/2bd5755565470ab94a9ec2460ea2989e92021aca?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2bd5755) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ 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
```
: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 commented on a diff in pull request #123: NUMBERS-186 added additional complex list operations
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #123:
URL: https://github.com/apache/commons-numbers/pull/123#discussion_r964653374
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -348,4 +433,25 @@ public void replaceAll(ComplexUnaryOperator<Void> operator) {
}
modCount++;
}
+
+ /**
+ * Replaces each element of the list with the result of applying the action to that element.
Review Comment:
This javadoc is wrong. Look at the javadoc for `forEach(Consumer<Complex>)` and adapt as appropriate.
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
Review Comment:
Please stop creating larger lists using duplication. The list elements should be unique. Create the list using random complex numbers or a sequence.
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -149,6 +151,30 @@ public Complex get(int index) {
return Complex.ofCartesian(realAndImagParts[i], realAndImagParts[i + 1]);
}
+ /**
+ * 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.
Review Comment:
`Index of the complex number.`
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+ for (int i = 0; i < list.size(); i++) {
+ 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));
+ }
+ }
+
+ @Test
+ void testSetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.setReal(2, 200);
+ list.setImaginary(2, 1);
+ Assertions.assertEquals(Complex.ofCartesian(200, 1), list.get(2));
Review Comment:
Test set at all positions:
```Java
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).
##########
commons-numbers-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
Review Comment:
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`
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+ for (int i = 0; i < list.size(); i++) {
+ 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));
+ }
+ }
+
+ @Test
+ void testSetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.setReal(2, 200);
+ list.setImaginary(2, 1);
+ Assertions.assertEquals(Complex.ofCartesian(200, 1), list.get(2));
+ }
+
+ @Test
+ void testToArrayRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+
+ double[] expectedReal = {42, 42, 200, 42};
+ double[] actualReal = list.toArrayReal();
+ for (int i = 0; i < expectedReal.length; i++) {
Review Comment:
Use `Assertions.assertArrayEquals`
##########
commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexConsumer.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.numbers.complex;
+
+/**
+ * Represents an operation that accepts two double input arguments and returns no result.
+ *
+ * <p>This is a functional interface whose functional method is
+ * {@link #apply(double, double)}.
+ *
+ * @since 1.1
+ */
+@FunctionalInterface
+public interface ComplexConsumer {
+
+ /**
+ * Represents a function that accepts real and imaginary part of complex number and returns no result.
+ * @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);
Review Comment:
java.util.function.Consumer has the method `accept`
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+ for (int i = 0; i < list.size(); i++) {
+ 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));
Review Comment:
This can be placed in the same loop as above. Add a simple message to the assertion, e.g. "real".
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+ for (int i = 0; i < list.size(); i++) {
+ 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));
+ }
+ }
+
+ @Test
+ void testSetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.setReal(2, 200);
+ list.setImaginary(2, 1);
+ Assertions.assertEquals(Complex.ofCartesian(200, 1), list.get(2));
+ }
+
+ @Test
+ void testToArrayRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
Review Comment:
Create a better random list.
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -348,4 +433,25 @@ public void replaceAll(ComplexUnaryOperator<Void> operator) {
}
modCount++;
}
+
+ /**
+ * Replaces each element of the list with the result of applying the action to that element.
+ *
+ * @param action The action to apply to each element.
+ */
+ public void forEach(ComplexConsumer action) {
+ Objects.requireNonNull(action);
+ final double[] parts = this.realAndImagParts;
+ final int m = size;
+ final int expectedModCount = modCount;
Review Comment:
The mod count is increased when the list is structurally modified. Here no modifications are occurring so this can be removed.
##########
commons-numbers-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
+ * in this list with the specified real element.
+ *
+ * @param index Index of the element's real part to replace.
+ * @param real Real part \( a \) of the complex number \( (a +ib) \).
+ */
+ public void setReal(int index, double real) {
+ rangeCheck(index);
+ final int i = index << 1;
+ realAndImagParts[i] = real;
+ }
+
+ /**
+ * 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.
+ * @param imaginary Imaginary part \( b \) of the complex number \( (a +ib) \).
+ */
+ public void setImaginary(int index, double imaginary) {
+ rangeCheck(index);
+ final int i = index << 1;
+ realAndImagParts[i + 1] = imaginary;
+ }
+
+ /**
+ * Returns an array containing all the complex element's real parts in
+ * the list in proper sequence (from first to last element).
+ *
+ * @return Array of real parts.
+ */
+ double[] toArrayReal() {
+ final int length = size;
+ double[] real = new double[length];
+ for (int i = 0; i < length; i++) {
+ final int index = i << 1;
Review Comment:
Again this `index` can be inlined
##########
commons-numbers-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
+ * in this list with the specified real element.
+ *
+ * @param index Index of the element's real part to replace.
+ * @param real Real part \( a \) of the complex number \( (a +ib) \).
+ */
+ public void setReal(int index, double real) {
+ rangeCheck(index);
+ final int i = index << 1;
+ realAndImagParts[i] = real;
+ }
+
+ /**
+ * 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.
Review Comment:
Add an empty line
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
+ List<Complex> data1 = generateList(10);
+ ArrayList<Complex> actual1 = new ArrayList<>();
+ ComplexList expected = new ComplexList();
+ expected.addAll(data1);
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual1.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual1);
+
+ //Testing case of when list is empty
+ List<Complex> data2 = new ArrayList<>();
+ ArrayList<Complex> actual2 = new ArrayList<>();
+ ComplexList expected2 = new ComplexList();
+ expected2.addAll(data2);
+ expected2.forEach((real, imaginary) -> actual2.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected2, actual2);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+ for (int i = 0; i < list.size(); i++) {
+ 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));
+ }
+ }
+
+ @Test
+ void testSetRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.setReal(2, 200);
+ list.setImaginary(2, 1);
+ Assertions.assertEquals(Complex.ofCartesian(200, 1), list.get(2));
+ }
+
+ @Test
+ void testToArrayRealAndImaginary() {
+ ComplexList list = new ComplexList();
+ list.add(Complex.ofCartesian(42, 13));
+ list.addAll(1, list);
+ list.addAll(list);
+ list.set(2, Complex.ofCartesian(200, 1));
+
+ double[] expectedReal = {42, 42, 200, 42};
Review Comment:
double[] expected = list.stream().mapToDouble(Complex::getReal).toArray();
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -149,6 +151,30 @@ public Complex get(int index) {
return Complex.ofCartesian(realAndImagParts[i], realAndImagParts[i + 1]);
}
+ /**
+ * 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.
+ * @return The real part.
+ */
+ public double getReal(int index) {
+ rangeCheck(index);
+ final int i = index << 1;
Review Comment:
No need for this variable, just perform the index doubling inline
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,6 +224,71 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @Test
+ void testReplaceAllComplexConsumer() {
Review Comment:
`testForEach...`
This test can be parameterized for size. This will eliminate duplication:
```Java
@ParameterizedTest
@ValuesSource(ints = {0, 10})
void testForEachComplexConsumer(int size) {
List<Complex> data1 = generateList(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] aherbert commented on a diff in pull request #123: NUMBERS-186 added additional complex list operations
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #123:
URL: https://github.com/apache/commons-numbers/pull/123#discussion_r965784959
##########
commons-numbers-complex-arrays/src/test/java/org/apache/commons/numbers/complex/arrays/ComplexListTest.java:
##########
@@ -223,14 +222,87 @@ void testReplaceAllComplexScalarFunction() {
Assertions.assertEquals(objectList, actualList);
}
+ @ParameterizedTest
+ @ValueSource(ints = {0, 10})
+ void testForEachComplexConsumer(int size) {
+ ComplexList expected = generateList(size);
+ ArrayList<Complex> actual = new ArrayList<>();
+ Assertions.assertThrows(NullPointerException.class, () -> expected.forEach((ComplexConsumer) null));
+ expected.forEach((real, imaginary) -> actual.add(Complex.ofCartesian(real, imaginary)));
+ Assertions.assertEquals(expected, actual);
+ }
+
+ @Test
+ void testGetRealAndImaginary() {
+ ComplexList list = generateList(10);
+ for (int i = 0; i < list.size(); i++) {
+ Assertions.assertEquals(list.get(i).getReal(), list.getReal(i), "real");
+ Assertions.assertEquals(list.get(i).getImaginary(), list.getImaginary(i), "imaginary");
+ }
+ }
+
+ @Test
+ void testSetRealAndImaginary() {
+ ComplexList list = generateList(10);
+
+ for (int i = 0; i < list.size(); i++) {
+ final double value = Math.PI * i;
+ list.setReal(i, value);
+ list.setImaginary(i, value);
+ Assertions.assertEquals(value, list.get(i).getReal());
+ Assertions.assertEquals(value, list.get(i).getImaginary());
+ }
+ }
+
+ @Test
+ void testGetAndSetRealAndImaginaryIndexOutOfBoundsException() {
+ ComplexList list = new ComplexList();
+ // Empty list throws
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.getReal(0));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.getImaginary(0));
+
+ int size = 5;
+ IntStream.range(0, size).mapToObj(i -> Complex.ofCartesian(i, -i)).forEach(list::add);
+
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.getReal(-1));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.getReal(size));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.getReal(size + 1));
+
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.getImaginary(-1));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.getImaginary(size));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.getImaginary(size + 1));
+
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.setReal(-2, 200));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.setReal(size, 200));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.setReal(size + 1, 200));
+
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.setImaginary(-2, 200));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.setImaginary(size, 200));
+ Assertions.assertThrows(IndexOutOfBoundsException.class, () -> list.setImaginary(size + 1, 200));
+ }
+
+ @Test
+ void testToArrayRealAndImaginary() {
+ ComplexList list = generateList(10);
+ double[] expectedReal = list.stream().mapToDouble(Complex::getReal).toArray();
+ double[] actualReal = list.toArrayReal();
+ Assertions.assertArrayEquals(expectedReal, actualReal);
+ double[] expectedImaginary = list.stream().mapToDouble(Complex::getImaginary).toArray();
+ double[] actualImaginary = list.toArrayImaginary();
+ Assertions.assertArrayEquals(expectedImaginary, actualImaginary);
+ }
+
/**
- * Generates a list of random complex numbers of the given size.
+ * Generates a ComplexList of random complex numbers of the given size.
* @param size number of complex numbers in the list.
- * @return the list of random complex numbers.
+ * @return the ComplexList of random complex numbers.
*/
- private static List<Complex> generateList(int size) {
- return ThreadLocalRandom.current().doubles(size, -Math.PI, Math.PI)
+ private static ComplexList generateList(int size) {
Review Comment:
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.
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -138,48 +139,122 @@ private void rangeCheckForInsert(int index) {
/**
* Gets the complex number \( (a + i b) \) at the indexed position of the list.
- *
+ * <p>
* {@inheritDoc}
+ *
* @return the complex number.
*/
@Override
public Complex get(int index) {
rangeCheck(index);
- final int i = index << 1;
- return Complex.ofCartesian(realAndImagParts[i], realAndImagParts[i + 1]);
+ return Complex.ofCartesian(realAndImagParts[index << 1], realAndImagParts[(index << 1) + 1]);
+ }
+
+ /**
+ * Gets the real part \( a \) of the complex number \( (a + i b) \) at the indexed position of the list.
+ *
+ * @param index Index of the complex number.
+ * @return The real part.
+ */
+ public double getReal(int index) {
+ rangeCheck(index);
+ return realAndImagParts[index << 1];
+ }
+
+ /**
+ * Gets the imaginary part \( b \) of the complex number \( (a + i b) \) at the indexed position of the list.
+ *
+ * @param index Index of the complex number.
+ * @return The imaginary part.
+ */
+ public double getImaginary(int index) {
+ rangeCheck(index);
+ return realAndImagParts[(index << 1) + 1];
}
/**
- * Replaces the element at the specified position in this list with the specified element's
+ * Replaces the complex number at the specified position in the list with the specified complex number's
* real and imaginary parts. No range checks are done.
*
- * @param index Index of the element to replace.
+ * @param index Index of the complex number to replace.
* @param real Real part \( a \) of the complex number \( (a +ib) \).
* @param imaginary Imaginary part \( b \) of the complex number \( (a +ib) \).
*/
private void setNoRangeCheck(int index, double real, double imaginary) {
- final int i = index << 1;
- realAndImagParts[i] = real;
- realAndImagParts[i + 1] = imaginary;
+ realAndImagParts[index << 1] = real;
+ realAndImagParts[(index << 1) + 1] = imaginary;
}
/**
* {@inheritDoc}
*
- * @param element Complex element to be set.
+ * @param number Complex number to be set.
*/
@Override
- public Complex set(int index, Complex element) {
+ public Complex set(int index, Complex number) {
rangeCheck(index);
- final int i = index << 1;
Review Comment:
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.
##########
commons-numbers-complex-arrays/src/main/java/org/apache/commons/numbers/complex/arrays/ComplexList.java:
##########
@@ -138,48 +139,122 @@ private void rangeCheckForInsert(int index) {
/**
* Gets the complex number \( (a + i b) \) at the indexed position of the list.
- *
+ * <p>
* {@inheritDoc}
+ *
* @return the complex number.
*/
@Override
public Complex get(int index) {
rangeCheck(index);
- final int i = index << 1;
Review Comment:
The precomputed value i will be reused so it can be left as before, please revert.
--
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 #123: NUMBERS-186 added additional complex list operations
Posted by GitBox <gi...@apache.org>.
aherbert merged PR #123:
URL: https://github.com/apache/commons-numbers/pull/123
--
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